Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2271263002: Allows the PDF engine return the page index it is scrolling to (Closed)

Created:
1 year, 2 months ago by tonikitoo
Modified:
1 year, 2 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

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
Adding some people to the R= line so it'll show up in their queue.
1 year, 2 months ago (2016-08-25 13:45:34 UTC) #2
Tom Sepez
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#newcode3192 pdf/pdfium/pdfium_engine.cc:3192: DCHECK(*in_flight_visible_page_ == index); This dcheck may be too strong ...
1 year, 2 months ago (2016-08-25 19:19:23 UTC) #3
tonikitoo
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#newcode3192 pdf/pdfium/pdfium_engine.cc:3192: DCHECK(*in_flight_visible_page_ == index); On 2016/08/25 19:19:22, Tom Sepez wrote: ...
1 year, 2 months ago (2016-08-25 20:17:51 UTC) #4
Tom Sepez
Looks Good, but get Lei to sign-off on this as well. https://codereview.chromium.org/2271263002/diff/20001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): ...
1 year, 2 months ago (2016-08-25 20:36:28 UTC) #5
tonikitoo
On 2016/08/25 20:36:28, Tom Sepez wrote: > Looks Good, but get Lei to sign-off on ...
1 year, 2 months ago (2016-08-26 13:44:03 UTC) #6
Lei Zhang
Just reading the CL description: - "interfers" is not a word? - I think by ...
1 year, 2 months ago (2016-08-27 01:15:32 UTC) #8
Lei Zhang
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_engine.h#newcode255 pdf/pdfium/pdfium_engine.h:255: // to be ...
1 year, 2 months ago (2016-08-27 01:20:54 UTC) #9
tonikitoo
On 2016/08/27 01:20:54, Lei Zhang wrote: > This looks like it can work. > > ...
1 year, 2 months ago (2016-08-27 04:46:51 UTC) #11
Lei Zhang
lgtm
1 year, 2 months ago (2016-08-27 04:51:58 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2271263002/60001
1 year, 2 months ago (2016-08-27 04:54:25 UTC) #16
commit-bot: I haz the power
Committed patchset #4 (id:60001)
1 year, 2 months ago (2016-08-27 11:06:10 UTC) #18
commit-bot: I haz the power
1 year, 2 months ago (2016-08-27 11:08:41 UTC) #20
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/3e0099107a8a8e3f6d685f220530b047c9c90105
Cr-Commit-Position: refs/heads/master@{#414921}

Powered by Google App Engine
This is Rietveld efc10ee0f