Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271263002:
Allows the PDF engine return the page index it is scrolling to (Closed)
DescriptionAllows the PDF engine return the page index it is scrolling to
Chromium/PDFium behave differently from IE/Acrobat, when it
comes to the way some Javascript that scroll to a given
page index is handled.
For instance, lets assume a PDF file is showing its page
'0' and the following JS runs:
this.pageNum = 1;
app.alert(this.pageNum);
The output of the alert in IE/Acrobat is '1', whereas it is
'0' in Chromium/PDFium.
This happens because of the asynchronous way Chromium's PDF
plugin handles the "scroll to page X" request.
Also, a similar behavior difference is seen on other
Acrobat JS APIs, including Document::gotoNamedDest, where
the same code path is taken to scroll to a given page index.
CL adds an "optional" class member variable that
caches the page index the PDF is going to be scrolled
to, and allows the PDF plugin to return the target page
index even before the it has finished handling the scroll
request.
BUG=pdfium:492
Committed: https://crrev.com/3e0099107a8a8e3f6d685f220530b047c9c90105
Cr-Commit-Position: refs/heads/master@{#414921}
Patch Set 1 : Allow PDFium to return the page index it is scrolling to #
Total comments: 2
Patch Set 2 : Dropped DCHECK, as per tsepez comment. #
Total comments: 1
Patch Set 3 : removed an if-branch as per tsepezs advice #
Total comments: 2
Patch Set 4 : addressed thestig's review #Messages
Total messages: 20 (8 generated)
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org, tsepez@chromium.org, weili@chromium.org
Adding some people to the R= line so it'll show up in their queue.
https://codereview.chromium.org/2271263002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2271263002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:3192: DCHECK(*in_flight_visible_page_ == index); This dcheck may be too strong -- what if we get a sequence of scrollToPage() calls before the first one is ack'd? Having dropped the dcheck, we can just reset. If there are a bunch in-flight, we might still get the wrong answer, but its not likely to be a case we care about.
https://codereview.chromium.org/2271263002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2271263002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:3192: DCHECK(*in_flight_visible_page_ == index); On 2016/08/25 19:19:22, Tom Sepez wrote: > This dcheck may be too strong -- what if we get a sequence of scrollToPage() > calls before the first one is ack'd? Having dropped the dcheck, we can just > reset. > > If there are a bunch in-flight, we might still get the wrong answer, but its not > likely to be a case we care about. Done.
Looks Good, but get Lei to sign-off on this as well. https://codereview.chromium.org/2271263002/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2271263002/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3191: if (in_flight_visible_page_) nit: branch probably not needed.
On 2016/08/25 20:36:28, Tom Sepez wrote: > Looks Good, but get Lei to sign-off on this as well. > > https://codereview.chromium.org/2271263002/diff/20001/pdf/pdfium/pdfium_engin... > File pdf/pdfium/pdfium_engine.cc (right): > > https://codereview.chromium.org/2271263002/diff/20001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.cc:3191: if (in_flight_visible_page_) > nit: branch probably not needed. Done in patchset #3.
Description was changed from ========== Allow PDFium to return the page index it is scrolling to PDFium behaves differently from IE/acrobat, when it comes to the way some Acrobat Javascript that scroll to a given page index X are handled. For instance, assume a PDF is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); As a result, it is expected that PDFium shows '1' as the result of the alert, but '0' is shown instead. This happens because of the asynchrnous way PDFium handles the "scroll to page X" request. Notice that this interfers also how other Acrobat JS that use the same code path to scroll to a given page index behave, including Document::gotoNamedDest. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to and returns it even before the Browser has returned the control flow back to the PDFium engine. In practice, this behavior difference affects a huge base of PDF samples (250,000+). BUG=pdfium:492 ========== to ========== Allow PDFium to return the page index it is scrolling to PDFium behaves differently from IE/acrobat, when it comes to the way some Acrobat Javascript that scroll to a given page index X are handled. For instance, assume a PDF is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); As a result, it is expected that PDFium shows '1' as the result of the alert, but '0' is shown instead. This happens because of the asynchronous way PDFium handles the "scroll to page X" request. Notice that this interfers also how other Acrobat JS that use the same code path to scroll to a given page index behave, including Document::gotoNamedDest. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to and returns it even before the Browser has returned the control flow back to the PDFium engine. In practice, this behavior difference affects a huge base of PDF samples (250,000+). BUG=pdfium:492 ==========
Just reading the CL description: - "interfers" is not a word? - I think by PDFium, in some cases, you really mean the PDF plugin. - The Browser is not involved here. It's PDF plugin and PDFium interacting with one another.
This looks like it can work. https://codereview.chromium.org/2271263002/diff/40001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2271263002/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:255: // to be used during the interval the client has not finished handling it. nit: funny line break. https://codereview.chromium.org/2271263002/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:688: // messages being sent by the Browser <-> PDFium engine. The other side is not the Browser. It's Chromium's "pdf.js" which is still part of the PDF Plugin. We wouldn't run JS in the browser process. The JS and C++ portions of the PDF plugin indeed sends each other messages asynchronously.
Description was changed from ========== Allow PDFium to return the page index it is scrolling to PDFium behaves differently from IE/acrobat, when it comes to the way some Acrobat Javascript that scroll to a given page index X are handled. For instance, assume a PDF is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); As a result, it is expected that PDFium shows '1' as the result of the alert, but '0' is shown instead. This happens because of the asynchronous way PDFium handles the "scroll to page X" request. Notice that this interfers also how other Acrobat JS that use the same code path to scroll to a given page index behave, including Document::gotoNamedDest. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to and returns it even before the Browser has returned the control flow back to the PDFium engine. In practice, this behavior difference affects a huge base of PDF samples (250,000+). BUG=pdfium:492 ========== to ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchrnous way Chromium's PDF engine handles the "scroll to page X" request. Notice that this behavior difference is also seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to and returns it even before the PDF plugin has returned the control flow back to the PDF engine. In practice, this behavior difference affects a huge base of PDF samples (250,000+). BUG=pdfium:492 ==========
On 2016/08/27 01:20:54, Lei Zhang wrote: > This looks like it can work. > > https://codereview.chromium.org/2271263002/diff/40001/pdf/pdfium/pdfium_engine.h > File pdf/pdfium/pdfium_engine.h (right): > > https://codereview.chromium.org/2271263002/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.h:255: // to be used during the interval the client has > not finished handling it. > nit: funny line break. > > https://codereview.chromium.org/2271263002/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.h:688: // messages being sent by the Browser <-> PDFium > engine. > The other side is not the Browser. It's Chromium's "pdf.js" which is still part > of the PDF Plugin. We wouldn't run JS in the browser process. The JS and C++ > portions of the PDF plugin indeed sends each other messages asynchronously. That is a great feedback Lei. I uploaded patchset #3 taking your comments into account. Please notice that the commit message/title is also updated.
Description was changed from ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchrnous way Chromium's PDF engine handles the "scroll to page X" request. Notice that this behavior difference is also seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to and returns it even before the PDF plugin has returned the control flow back to the PDF engine. In practice, this behavior difference affects a huge base of PDF samples (250,000+). BUG=pdfium:492 ========== to ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchronous way Chromium's PDF engine handles the "scroll to page X" request. Notice that this behavior difference is also seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to and returns it even before the PDF plugin has returned the control flow back to the PDF engine. In practice, this behavior difference affects a huge base of PDF samples (250,000+). BUG=pdfium:492 ==========
lgtm
Description was changed from ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchronous way Chromium's PDF engine handles the "scroll to page X" request. Notice that this behavior difference is also seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to and returns it even before the PDF plugin has returned the control flow back to the PDF engine. In practice, this behavior difference affects a huge base of PDF samples (250,000+). BUG=pdfium:492 ========== to ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchronous way Chromium's PDF plugin handles the "scroll to page X" request. Also, a similar behavior difference is seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to, and allows the PDF plugin to return the target page index even before the it has finished handling the scroll request. BUG=pdfium:492 ==========
The CQ bit was checked by tonikitoo@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchronous way Chromium's PDF plugin handles the "scroll to page X" request. Also, a similar behavior difference is seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to, and allows the PDF plugin to return the target page index even before the it has finished handling the scroll request. BUG=pdfium:492 ========== to ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchronous way Chromium's PDF plugin handles the "scroll to page X" request. Also, a similar behavior difference is seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to, and allows the PDF plugin to return the target page index even before the it has finished handling the scroll request. BUG=pdfium:492 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchronous way Chromium's PDF plugin handles the "scroll to page X" request. Also, a similar behavior difference is seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to, and allows the PDF plugin to return the target page index even before the it has finished handling the scroll request. BUG=pdfium:492 ========== to ========== Allows the PDF engine return the page index it is scrolling to Chromium/PDFium behave differently from IE/Acrobat, when it comes to the way some Javascript that scroll to a given page index is handled. For instance, lets assume a PDF file is showing its page '0' and the following JS runs: this.pageNum = 1; app.alert(this.pageNum); The output of the alert in IE/Acrobat is '1', whereas it is '0' in Chromium/PDFium. This happens because of the asynchronous way Chromium's PDF plugin handles the "scroll to page X" request. Also, a similar behavior difference is seen on other Acrobat JS APIs, including Document::gotoNamedDest, where the same code path is taken to scroll to a given page index. CL adds an "optional" class member variable that caches the page index the PDF is going to be scrolled to, and allows the PDF plugin to return the target page index even before the it has finished handling the scroll request. BUG=pdfium:492 Committed: https://crrev.com/3e0099107a8a8e3f6d685f220530b047c9c90105 Cr-Commit-Position: refs/heads/master@{#414921} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3e0099107a8a8e3f6d685f220530b047c9c90105 Cr-Commit-Position: refs/heads/master@{#414921} |
