Conversation
This slightly shortens the code, in various `destroy`-methods, which cannot hurt. Also, use pre-processor checks to simplify `PDFDocumentLoadingTask.destroy` in the Firefox PDF Viewer since the `PDFWorker.fromPort`-method isn't used there.
- Use `this` in all scopes where that's possible, to avoid having to spell out `WorkerMessageHandler` everywhere. - Inline the `isMessagePort` helper function, since there's only a single call-site. - Use a static initialization block to move more code into the `WorkerMessageHandler` class itself.
This allows us to remove an ESLint disable-statement for `arrow-body-style`, without affecting readability of the code, and fetching the metadata and the page in parallel should be a *tiny* bit more efficient as well.
…api.js` The `Array` type takes one parameter that describes the possible types of the inner elements. This parameter may exist of pipes to indicate multiple possible types. However, the current type definition provides multiple parameters; this is incorrect syntax, and TypeScript 5.7+ now fails on this due to stricter syntax validation. This commit fixes the issue by changing the type definition to the proper syntax, which together with the accompanying comment about the contents of the fingerprints array should document it sufficiently.
Given that `browsertest` repeatedly timeout in Google Chrome, and considering that Firefox is the primary development target, we stop running them on the bots to avoid having to repeatedly deal with this. Note that we already disabled these tests *on Windows* almost three years ago, because of stability issues; see PR 14392.
Some tests rely on the presence of a server that serves PDF files. When tests are run from a web browser, the test files and PDF files are served by the same server (WebServer), but in Node.js that server is not around. Currently, the tests that depend on it start a minimal Node.js server that re-implements part of the functionality from WebServer. To avoid code duplication when tests depend on more complex behaviors, this patch replaces createTemporaryNodeServer with the existing WebServer, wrapped in a new test utility that has the same interface in Node.js and non-Node.js environments (=TestPdfsServer). This patch has been tested by running the refactored tests in the following three configurations: 1. From the browser: - http://localhost:8888/test/unit/unit_test.html?spec=api - http://localhost:8888/test/unit/unit_test.html?spec=fetch_stream 2. Run specific tests directly with jasmine without legacy bundling: `JASMINE_CONFIG_PATH=test/unit/clitests.json ./node_modules/.bin/jasmine --filter='^api|^fetch_stream'` 3. `gulp unittestcli`
Regression tests for issue mozilla#12744 and PR mozilla#19028
Move .messageBar out of .dialog into its own standalone class in order to reuse as much of it for the upcoming feature for an undo message for annotations.
When a user deletes any number of annotations, they are notified of the action by a popup message with an undo button. Besides that, this change reuses the existing messageBar CSS class from the new alt-text dialog as much as possible.
…e telemetry (bug 1929311)
This simplifies the creation of Promises in some cases; see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/try
…ion in integration tests
…visible hover/focus state (issue 19165) Similar to the regular toolbarButtons that can be toggled, this ensure that it's always possible to tell when the findbar "buttons" are hovered/focused.
We were trying to update the drawing on the current page but if the drawing is an another page then it wasn't updated.
…ilding (issue 19145) This fixes a Node.js-specific regression from PR 18959.
…endering is finished It fixes mozilla#18622. It avoids to recreate a canvasWrapper element in order minimize the DOM operations.
…DFNetworkStream` (issue 19075) In the event that the "Content-Range" header is missing/invalid, loading will now be aborted rather than hanging indefinitely.
This helps ensure that loading errors are always handled correctly, and note that both `PDFNetworkStreamFullRequestReader` and `PDFNetworkStreamRangeRequestReader` already provided such a callback.
Converting errors to string drops their stack trace, making it more difficult to debug their actual reason. We can instead pass the error objects as-is to console.warn/error, so that Firefox/Chrome devtools will show both the stack trace of the console.warn/error call, and the original stack trace of the error. This commit also enables the `unicorn/no-console-spaces` ESLint rule, which avoids accidental extra spaces when passing multiple parameters to `console.*` methods.
…icode map It fixes mozilla#19182.
…ndocumented but matches Acrobat behavior (issue mozilla#19484)
…w-up) This restores the behaviour that existed prior to PR 19128, see e.g. https://github.com/mozilla/pdf.js/blob/8727a04ae588545c8e954c143b9fc64384712461/web/pdf_page_view.js#L908-L911, since `RenderingCancelledException` should still overwrite any previously seen Error.
…ted font-data These options are needed in the `FontFaceObject` class, and indirectly in `FontLoader` as well, which means that we currently need to pass them around manually in the API. Given that the options are (obviously) available on the worker-thread, it's very easy to just provide them when creating `Font`-instances and then send them as part of the exported font-data. This way we're able to simplify the code (primarily on the main-thread), and note that `Font`-instances even had a `disableFontFace`-field already (but it wasn't properly initialized).
Remove the `Catalog.prototype.fontFallback` method, and move its code into `PDFDocument.prototype.fontFallback` instead, to reduce the indirection a little bit. Pass the `evaluatorOptions` directly to the `TranslatedFont.prototype.fallback` method, since nothing else in the `TranslatedFont`-class needs it now.
…ies (PR 11777 follow-up) None of the "composite", "subtype", or "type" properties are normally used on the main-thread and/or in the API, hence there's no need to include them in the exported font-data by default. Given that these properties may still be useful when debugging, and that `debugger.mjs` actually relies on the "type" property, they will instead only be sent to the main-thread when the `fontExtraProperties` API-option is used.
…mponents` This, ever so slightly, shortens the code which can never hurt.
… 19128 follow-up) Looking at recent integration-test logs there's occasionally warnings about trying to invoke `PDFViewer.prototype.update` too late, which probably started with PR 19128 (see log excerpt below). Given that we already had another timeout before that PR the problem was pre-existing, but it seems to trigger more easily now. ``` JavaScript warning: http://127.0.0.1:42333/build/generic/web/viewer.mjs, line 12668: Script terminated by timeout at: update@http://127.0.0.1:42333/build/generic/web/viewer.mjs:12668:9 @http://127.0.0.1:42333/build/generic/web/viewer.mjs:12373:12 setTimeout handler*_scrollUpdate@http://127.0.0.1:42333/build/generic/web/viewer.mjs:12371:29 viewAreaElementScrolled@http://127.0.0.1:42333/build/generic/web/viewer.mjs:154:15 FrameRequestCallback*debounceScroll@http://127.0.0.1:42333/build/generic/web/viewer.mjs:140:18 EventListener.handleEvent*watchScroll@http://127.0.0.1:42333/build/generic/web/viewer.mjs:165:19 PDFViewer@http://127.0.0.1:42333/build/generic/web/viewer.mjs:11777:19 _initializeViewerComponents@http://127.0.0.1:42333/build/generic/web/viewer.mjs:15081:23 initialize@http://127.0.0.1:42333/build/generic/web/viewer.mjs:14931:16 async*run@http://127.0.0.1:42333/build/generic/web/viewer.mjs:15227:16 webViewerLoad@http://127.0.0.1:42333/build/generic/web/viewer.mjs:17078:24 @http://127.0.0.1:42333/build/generic/web/viewer.mjs:17082:3 ```
Currently we're first loading the font, and then for Type3 fonts we're invoking `loadType3Data` every time that the font is encountered. That seems completely unnecessary, and it's probably connected to the age of this code, since the `loadType3Data`-method will only run once anyway (note the caching).
Auto-linking requires a normal textLayer which XFA documents obviously don't have, and currently XFA documents cause "pointless" error messages to be logged in the console.
The `viewerCssTheme` was removed in mozilla#17222 and subsequently reenabled in mozilla#17293, but only for Chromium and generic builds. This commit reenables the function using the new method introduced in mozilla#17293.
…ild of the openjpeg library (bug 1951128)
This complements the existing `LocalColorSpaceCache`, which is unique to each `getOperatorList`-invocation since it also caches by `Name`, which should help reduce unnecessary re-parsing especially for e.g. `ICCBased` ColorSpaces once we properly support those.
Update the test to wait for the `pagerendered`` event of a specific page (1), so that the `pagerendered`` event of other pages from a previously running render doesn't resolve the `waitForPageRendered` promise.
Given that this functionality is only used in the development viewer and in TESTING builds, there's no reason to include this in the regular builds.
…ream` class This is a tiny bit shorter, which cannot hurt.
…fest links (issue 19579) This appears to have broken in PR 17431, since prior to that the `downloadFile` function returned a string (via a callback) on failure and now we're instead rejecting with an Error.
…es (issue 19580) As mentioned in https://nodejs.org/docs/latest-v20.x/api/http.html#httpgeturl-options-callback if a request results in an error, you still need to consume the data, or call `response.resume()`, or your code will hang waiting for the request to close.
Currently we re-implement the same helper function twice, which in hindsight seems like the wrong decision since that way it's quite easy for the implementations to accidentally diverge. The reason for doing it this way was because the code in the worker-thread is able to check for `Ref`- and `Name`-instances directly, which obviously isn't possible in the viewer but can be solved by passing validation-functions to the helper.
A lot of this is quite old code, which we can shorten slightly by using arrow functions instead of "regular" functions.
The current logic assumes that all spans in the text layer contain only one text node, and thus that the position information returned by `highlighter._convertMatches` can be directly used on the element's only child. This is not true in case of highlighted search results: they will be injected in the DOM as `<span>` elements, causing the `<span>`s in the text layer to have more than one child. This patch fixes the problem by properly converting the (span, offset) pair in a (textNode, offset) pair that points to the right text node.
A lot of this is fairly old code, which we can shorten slightly by using arrow functions instead of "regular" functions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Main use case (User story)
How? (Comments on implementation)
What? (How do we test it?)
PR checklist: