Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503633002:
Propagate browser zoom changes to embedded PDFs. (Closed)
|
Created:
4 years, 1 month ago by Kevin McNee Modified:
4 years ago CC:
chromium-reviews, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPropagate browser zoom changes to embedded PDFs.
Currently, changes to the browser zoom are ignored by embedded PDF
viewers. This patch propagates browser zoom changes so that the zoom
level of the viewer may be updated. For embedded viewers, we apply
the internal change in zoom to the new browser zoom, rather than
overwriting the viewer's zoom level.
This patch also initializes an embedded viewer's zoom level to its
parent's zoom level.
BUG=88802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/2999413a1038cfd7a2020edf431671a17db0e8d8
Cr-Commit-Position: refs/heads/master@{#436695}
Patch Set 1 #Patch Set 2 : Apply internal pdf zoom to browser zoom. #
Total comments: 11
Patch Set 3 : Apply browser zoom separately. #Patch Set 4 : Rebase only. #Patch Set 5 : Use getDefaultZoom for clarity. #
Total comments: 9
Patch Set 6 : Rebase only. #Patch Set 7 : Code review changes. #Patch Set 8 : Use American spelling of behaviour. #
Messages
Total messages: 41 (14 generated)
Description was changed from ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we interpret the change as a zoom in or zoom out on the viewer, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 ========== to ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we interpret the change as a zoom in or zoom out on the viewer, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we interpret the change as a zoom in or zoom out on the viewer, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we interpret the change as a zoom in or zoom out on the viewer, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mcnee@chromium.org changed reviewers: + wjmaclean@chromium.org
mcnee@chromium.org changed reviewers: + thestig@chromium.org
Hello thestig. I have another pdf JS change. I'm not sure who I should be sending this to. dpapad reviewed my pdf pinch zoom CL, but it sounded like he wasn't taking over responsibility for pdf JS changes. Could you add a suitable reviewer? Thanks.
I don't feel qualified to review this because my JS-fu is too weak. You can try asking dpapad / raymes and see who has time.
mcnee@chromium.org changed reviewers: + raymes@chromium.org
Hello, raymes. Could you take a look at this pdf JS change? Thanks.
Thanks for taking a look at this Kevin! I'm not actually sure if we want to do this though, because: 1) It's not clear to me that the current behavior is problematic? The PDF won't zoom when the browser zoom changes but the user still has control over the PDF zoom level through the PDF controls. 2) It's not clear how the browser zoom level and PDF zoom level should interact if we do incorporate the browser zoom. It's not clear to me that this approach is strictly better than the current behavior. 3) The zoom code is already somewhat complicated and it's unfortunate to complicate it further with extra logic. What are your thoughts? Thanks
On 2016/11/14 23:34:32, raymes wrote: > Thanks for taking a look at this Kevin! I'm not actually sure if we want to do > this though, because: > 1) It's not clear to me that the current behavior is problematic? The PDF won't > zoom when the browser zoom changes but the user still has control over the PDF > zoom level through the PDF controls. I think it's *extremely* jarring when the page zoom changes, but part of the page stays put at the old zoom level. Apparently I'm not the only one, given that a user filed the related bug. > 2) It's not clear how the browser zoom level and PDF zoom level should interact > if we do incorporate the browser zoom. It's not clear to me that this approach > is strictly better than the current behavior. There are multiple choices to be sure, but one important guiding principle is that page scale applied to the main tab should affect the PDF in a manner that keeps the PDF content content the same relative size as other parts of the page, which is what this CL is doing. As to what happens after that, it seems easy enough for (i) the PDF content to know it's current zoom state, and (ii) go to the next level up/down when the user zooms directly via the PDF's zoom control. This is again fairly easy to implement. But we can consider alternative schemes, if you have any in mind. Of course, for sanity's sake, PDF zoom should respect the current page zoom limits, so if the user adjusts the PDF to maximum zoom, then starts zooming the main tab, the PDF should probably not keep increasing it's zoom, though again this is open for discussion. > 3) The zoom code is already somewhat complicated and it's unfortunate to > complicate it further with extra logic. The additional complication in this case is entirely encapsulated within the PDF JavaScript code. I assume when you say "already somewhat complicated" you're talking about browser zoom? ... if not, can you explain what you're thinking? > What are your thoughts? Thanks That's my $0.02 :-)
On 2016/11/15 01:54:17, wjmaclean wrote: > On 2016/11/14 23:34:32, raymes wrote: > > Thanks for taking a look at this Kevin! I'm not actually sure if we want to do > > this though, because: > > 1) It's not clear to me that the current behavior is problematic? The PDF > won't > > zoom when the browser zoom changes but the user still has control over the PDF > > zoom level through the PDF controls. > > I think it's *extremely* jarring when the page zoom changes, but part of the > page stays put at the old zoom level. Apparently I'm not the only one, given > that a user filed the related bug. > > > 2) It's not clear how the browser zoom level and PDF zoom level should > interact > > if we do incorporate the browser zoom. It's not clear to me that this approach > > is strictly better than the current behavior. > > There are multiple choices to be sure, but one important guiding principle is > that page scale applied to the main tab should affect the PDF in a manner that > keeps the PDF content content the same relative size as other parts of the page, > which is what this CL is doing. As to what happens after that, it seems easy > enough for (i) the PDF content to know it's current zoom state, and (ii) go to > the next level up/down when the user zooms directly via the PDF's zoom control. > This is again fairly easy to implement. But we can consider alternative schemes, > if you have any in mind. Hmm, would a better option be to keep the internal zoom value the same in the PDF and instead scale it according to the browser zoom - so that the two zoom factors apply separately and the user is only ever exposed to the PDF zoom level? That might be slightly harder to implement though. We do already know the browser zoom inside of the plugin. > > Of course, for sanity's sake, PDF zoom should respect the current page zoom > limits, so if the user adjusts the PDF to maximum zoom, then starts zooming the > main tab, the PDF should probably not keep increasing it's zoom, though again > this is open for discussion. > > > 3) The zoom code is already somewhat complicated and it's unfortunate to > > complicate it further with extra logic. > > The additional complication in this case is entirely encapsulated within the PDF > JavaScript code. I assume when you say "already somewhat complicated" you're > talking about browser zoom? ... if not, can you explain what you're thinking? > Yeah - that's the main complication. There can be race conditions between the browser setting the zoom and the plugin, along with needing to take care that when we change the browser's zoom, we don't respond to the zoom-change notification and get into an infinite loop. > > What are your thoughts? Thanks > > That's my $0.02 :-)
On 2016/11/15 02:11:56, raymes wrote: > > Hmm, would a better option be to keep the internal zoom value the same in the > PDF and instead scale it according to the browser zoom - so that the two zoom > factors apply separately and the user is only ever exposed to the PDF zoom > level? That might be slightly harder to implement though. We do already know the > browser zoom inside of the plugin. That's not terribly unlike what we were thinking; the main difference seems to be instead of having the combined zooms adhere to the preset levels, you'd keep the PDF zoom separately adhere to the preset levels, is that correct? Would you then allow combined zoom to go to max^2 / min^2? > > Yeah - that's the main complication. There can be race conditions between the > browser setting the zoom and the plugin, along with needing to take care that > when we change the browser's zoom, we don't respond to the zoom-change > notification and get into an infinite loop. Certainly we'd need to avoid any loops/races. But ultimately the PDF JS code has complete control over what PDFium sees, right? So it seems possible to control it properly.
On 2016/11/15 02:19:03, wjmaclean wrote: > On 2016/11/15 02:11:56, raymes wrote: > > > > Hmm, would a better option be to keep the internal zoom value the same in the > > PDF and instead scale it according to the browser zoom - so that the two zoom > > factors apply separately and the user is only ever exposed to the PDF zoom > > level? That might be slightly harder to implement though. We do already know > the > > browser zoom inside of the plugin. > > That's not terribly unlike what we were thinking; the main difference seems to > be instead of having the combined zooms adhere to the preset levels, you'd keep > the PDF zoom separately adhere to the preset levels, is that correct? Would you > then allow combined zoom to go to max^2 / min^2? I guess the idea I was thinking is that we scale the whole PDF according to the browser zoom and then we apply the PDF zoom on top of that. It's a good question about the min/max. I would probably say we would allow them to go high or low assuming that there was no technical complication. I tested out the patch on http://plugindoc.mozdev.org/testpages/pdf.html and it seems to have some weird behavior. If I zoom out using the PDF toolbar and then zoom in using the browser zoom, the zoom of the PDF seems to snap in a weird way. Also if I refresh the page the width of the PDF fits to the viewport, but zooming in/out using the browser zoom makes the page no longer fit in the viewport, which looks strange. > > > > > Yeah - that's the main complication. There can be race conditions between the > > browser setting the zoom and the plugin, along with needing to take care that > > when we change the browser's zoom, we don't respond to the zoom-change > > notification and get into an infinite loop. > > Certainly we'd need to avoid any loops/races. But ultimately the PDF JS code has > complete control over what PDFium sees, right? So it seems possible to control > it properly. Right - I was just weighing up the value of this change vs the added complexity. I guess I don't see this bug as too significant - it hasn't had much activity in 5 years and I personally don't see this as a strict improvement. But that's just my personal take.
> > That's not terribly unlike what we were thinking; the main difference seems to > > be instead of having the combined zooms adhere to the preset levels, you'd > keep > > the PDF zoom separately adhere to the preset levels, is that correct? Sorry I don't think I explained myself well above :( But the main difference in my mind is that if we always scale the PDF by the browser zoom and apply the PDF zoom on top then we will always scale the PDF perfectly with the rest of the page. This is as opposed to trying to scale the PDF zoom by the change in browser zoom which is prone to rounding error and possible some edge cases. Please let me know if this idea still isn't clear.
raymes: How's this? Also, in the interest of clarity, I went ahead and factored the ZoomManager into subclasses depending on what it controls. For instance, the race you describe is only relevant to the case where we control the browser zoom, so that's now in a class separate from the embedded case.
Description was changed from ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we interpret the change as a zoom in or zoom out on the viewer, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we reapply the internal change in zoom to the new browser zoom rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we reapply the internal change in zoom to the new browser zoom rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we reapply the internal change in zoom to the new browser zoom, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
raymes@chromium.org changed reviewers: + sammc@chromium.org
Thanks Kevin. I tested the patch again on http://plugindoc.mozdev.org/testpages/pdf.html and zooming all the way out (with browser zoom) and then in results in things not scaling properly - I'm not sure why that is. +sammc who has some familiarity with zooming in PDF. https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:140: this.browserApi_.getInitialZoom(), Hmm, how come you're changing this to InitialZoom rather than DefaultZoom? +sammc who fixed some bugs around this. https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/zoom_manager.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/zoom_manager.js:155: let internalZoomRatio = this.viewport_.zoom / this.browserZoom_; I'm still not a big fan of the fact that we're working backward to determine the internal zoom - which means that in essence we're just scaling relatively with each zoom which is prone to rounding error and will affect the behavior of zooming in the plugin. It seems like it would be better to keep track of browser zoom separately from the internal zoom and apply them on top of each other.
I'm deferring to sammc@ on this - hopefully he will be able to help! He had the wise suggestion of potentially getting UX input on what the desired behavior is. Then we can implement what they suggest. It may be that they are ok with an approach like this (though it doesn't feel quite right to me :) What are your thoughts on doing that? I think the main practical concerns to discuss are: 1) Is it ok if the PDF doesn't scale exactly proportionally to the change in browser zoom, e.g. because of rounding errors accumulating or edge cases (I think this approach will have those). 2) Is it ok if changing the browser zoom results in other subtle behavioral changes to the plugin. For example, if the plugin was in "fit to page" zoom mode, and the browser is zoomed, it will no longer be in that zoom mode (with this approach). Practically this means that if the PDF iframe is resized, the PDF won't resize with it, whereas it would have it it was in "fit to page mode". These are quite subtle things but I think they can add up to something noticeable. Anyhow I'll leave it to sammc from here :)
https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:140: this.browserApi_.getInitialZoom(), On 2016/11/17 00:59:57, raymes wrote: > Hmm, how come you're changing this to InitialZoom rather than DefaultZoom? > +sammc who fixed some bugs around this. The reason for the default zoom was for PDFs we want to zoom the PDF to fit the page, but not zoom in more than the default zoom level. As the comment for lookupInitialZoom() says, this isn't always the same as the zoom level of the tab. This should still be the default zoom for top-level PDFs, and either the default zoom or default zoom * initial zoom for embedded PDFs. https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/zoom_manager.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/zoom_manager.js:155: let internalZoomRatio = this.viewport_.zoom / this.browserZoom_; On 2016/11/17 00:59:57, raymes wrote: > I'm still not a big fan of the fact that we're working backward to determine the > internal zoom - which means that in essence we're just scaling relatively with > each zoom which is prone to rounding error and will affect the behavior of > zooming in the plugin. It seems like it would be better to keep track of browser > zoom separately from the internal zoom and apply them on top of each other. +1 https://codereview.chromium.org/2503633002/diff/20001/chrome/test/data/pdf/zo... File chrome/test/data/pdf/zoom_manager_test.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/test/data/pdf/zo... chrome/test/data/pdf/zoom_manager_test.js:56: let zoomManager = ZoomManager.create(BrowserApi.ZoomBehaviour.MANAGE, Wrap these either functionName(arg1, arg2, arg3, ...) or functionName( arg1, arg2, arg3, ...)
Here's an implementation where the browser zoom level is applied on a separate internal zoom level for the embedded pdf. I agree, this method feels nicer from a user perspective. https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:140: this.browserApi_.getInitialZoom(), On 2016/11/18 04:59:23, Sam McNally wrote: > On 2016/11/17 00:59:57, raymes wrote: > > Hmm, how come you're changing this to InitialZoom rather than DefaultZoom? > > +sammc who fixed some bugs around this. > > The reason for the default zoom was for PDFs we want to zoom the PDF to fit the > page, but not zoom in more than the default zoom level. As the comment for > lookupInitialZoom() says, this isn't always the same as the zoom level of the > tab. > > This should still be the default zoom for top-level PDFs, and either the default > zoom or default zoom * initial zoom for embedded PDFs. Right. If the tab the pdf is in has been zoomed, we want the pdf to start off zoomed as well. https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/zoom_manager.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/zoom_manager.js:155: let internalZoomRatio = this.viewport_.zoom / this.browserZoom_; On 2016/11/18 04:59:23, Sam McNally wrote: > On 2016/11/17 00:59:57, raymes wrote: > > I'm still not a big fan of the fact that we're working backward to determine > the > > internal zoom - which means that in essence we're just scaling relatively with > > each zoom which is prone to rounding error and will affect the behavior of > > zooming in the plugin. It seems like it would be better to keep track of > browser > > zoom separately from the internal zoom and apply them on top of each other. > > +1 Done. https://codereview.chromium.org/2503633002/diff/20001/chrome/test/data/pdf/zo... File chrome/test/data/pdf/zoom_manager_test.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/test/data/pdf/zo... chrome/test/data/pdf/zoom_manager_test.js:56: let zoomManager = ZoomManager.create(BrowserApi.ZoomBehaviour.MANAGE, On 2016/11/18 04:59:23, Sam McNally wrote: > Wrap these either > functionName(arg1, arg2, > arg3, ...) > or > functionName( > arg1, arg2, > arg3, ...) Done.
https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:140: this.browserApi_.getInitialZoom(), On 2016/11/22 23:17:32, Kevin McNee wrote: > On 2016/11/18 04:59:23, Sam McNally wrote: > > On 2016/11/17 00:59:57, raymes wrote: > > > Hmm, how come you're changing this to InitialZoom rather than DefaultZoom? > > > +sammc who fixed some bugs around this. > > > > The reason for the default zoom was for PDFs we want to zoom the PDF to fit > the > > page, but not zoom in more than the default zoom level. As the comment for > > lookupInitialZoom() says, this isn't always the same as the zoom level of the > > tab. > > > > This should still be the default zoom for top-level PDFs, and either the > default > > zoom or default zoom * initial zoom for embedded PDFs. > > Right. If the tab the pdf is in has been zoomed, we want the pdf to start off > zoomed as well. For non-embedded PDFs we still want to use the default zoom here.
https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:140: this.browserApi_.getInitialZoom(), On 2016/11/28 07:25:13, Sam McNally wrote: > On 2016/11/22 23:17:32, Kevin McNee wrote: > > On 2016/11/18 04:59:23, Sam McNally wrote: > > > On 2016/11/17 00:59:57, raymes wrote: > > > > Hmm, how come you're changing this to InitialZoom rather than DefaultZoom? > > > > +sammc who fixed some bugs around this. > > > > > > The reason for the default zoom was for PDFs we want to zoom the PDF to fit > > the > > > page, but not zoom in more than the default zoom level. As the comment for > > > lookupInitialZoom() says, this isn't always the same as the zoom level of > the > > > tab. > > > > > > This should still be the default zoom for top-level PDFs, and either the > > default > > > zoom or default zoom * initial zoom for embedded PDFs. > > > > Right. If the tab the pdf is in has been zoomed, we want the pdf to start off > > zoomed as well. > > For non-embedded PDFs we still want to use the default zoom here. Yes, if it's not embedded, we set the zoom mode to manual, which resets the zoom level to the default. So the initial zoom is equal to the default zoom. But for clarity, I can change it back.
https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2503633002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:140: this.browserApi_.getInitialZoom(), On 2016/11/28 16:18:44, Kevin McNee wrote: > On 2016/11/28 07:25:13, Sam McNally wrote: > > On 2016/11/22 23:17:32, Kevin McNee wrote: > > > On 2016/11/18 04:59:23, Sam McNally wrote: > > > > On 2016/11/17 00:59:57, raymes wrote: > > > > > Hmm, how come you're changing this to InitialZoom rather than > DefaultZoom? > > > > > +sammc who fixed some bugs around this. > > > > > > > > The reason for the default zoom was for PDFs we want to zoom the PDF to > fit > > > the > > > > page, but not zoom in more than the default zoom level. As the comment for > > > > lookupInitialZoom() says, this isn't always the same as the zoom level of > > the > > > > tab. > > > > > > > > This should still be the default zoom for top-level PDFs, and either the > > > default > > > > zoom or default zoom * initial zoom for embedded PDFs. > > > > > > Right. If the tab the pdf is in has been zoomed, we want the pdf to start > off > > > zoomed as well. > > > > For non-embedded PDFs we still want to use the default zoom here. > > Yes, if it's not embedded, we set the zoom mode to manual, which resets the zoom > level to the default. So the initial zoom is equal to the default zoom. But for > clarity, I can change it back. Done.
Description was changed from ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. If the zoom level of the browser and the zoom level of the viewer differ, we reapply the internal change in zoom to the new browser zoom, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. For embedded viewers, we apply the internal change in zoom to the new browser zoom, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:159: BrowserApi.ZoomBehaviour = { I believe we're meant to use US spelling. https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:57: * @param {number} initialZoom The zoom level to initialize the viewer to. Let's leave this as it was. It's used for deciding how zoomed we want to be rather than how zoomed we are initially. The reason for getting both zoom values is that the initial zoom and default zoom can be different by the time we actually check. https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/zoom_manager.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/zoom_manager.js:158: this.changingBrowserZoom_ = null; Is this expected to occur?
https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:159: BrowserApi.ZoomBehaviour = { On 2016/12/04 23:56:45, Sam McNally wrote: > I believe we're meant to use US spelling. Really? In code search, "behaviour" pops up quite a bit. https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/viewport.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/viewport.js:57: * @param {number} initialZoom The zoom level to initialize the viewer to. On 2016/12/04 23:56:45, Sam McNally wrote: > Let's leave this as it was. It's used for deciding how zoomed we want to be > rather than how zoomed we are initially. > > The reason for getting both zoom values is that the initial zoom and default > zoom can be different by the time we actually check. Done. https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/zoom_manager.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/zoom_manager.js:158: this.changingBrowserZoom_ = null; On 2016/12/04 23:56:45, Sam McNally wrote: > Is this expected to occur? Ah, this only affected a previous patch. Since the promise is currently resolved when the browser zoom is not managed, |this.browserZoom_| was being set incorrectly. While it's currently harmless, it led to a bug in my first implementation, hence why I switched it to reject and added this onRejected function for the embedded case. Now that the embedded case doesn't call the |setBrowserZoomFunction_|, this won't occur. Removed.
LGTM https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:159: BrowserApi.ZoomBehaviour = { On 2016/12/05 22:21:00, Kevin McNee wrote: > On 2016/12/04 23:56:45, Sam McNally wrote: > > I believe we're meant to use US spelling. > > Really? In code search, "behaviour" pops up quite a bit. Behavior pops up a lot more though and consistency is nice.
rs lgtm
https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:159: BrowserApi.ZoomBehaviour = { On 2016/12/05 23:16:47, Sam McNally wrote: > On 2016/12/05 22:21:00, Kevin McNee wrote: > > On 2016/12/04 23:56:45, Sam McNally wrote: > > > I believe we're meant to use US spelling. > > > > Really? In code search, "behaviour" pops up quite a bit. > > Behavior pops up a lot more though and consistency is nice. Sure. I can change it before committing.
https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2503633002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:159: BrowserApi.ZoomBehaviour = { On 2016/12/06 18:07:16, Kevin McNee wrote: > On 2016/12/05 23:16:47, Sam McNally wrote: > > On 2016/12/05 22:21:00, Kevin McNee wrote: > > > On 2016/12/04 23:56:45, Sam McNally wrote: > > > > I believe we're meant to use US spelling. > > > > > > Really? In code search, "behaviour" pops up quite a bit. > > > > Behavior pops up a lot more though and consistency is nice. > > Sure. I can change it before committing. Done.
The CQ bit was checked by mcnee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2503633002/#ps140001 (title: "Use American spelling of behaviour.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481049931901210,
"parent_rev": "0dc389102f2f9e64604ec11aeb46cae0b1430874", "commit_rev":
"6dad5d222bb61e0fc1139c25409ca64b2862d57f"}
Message was sent while issue was closed.
Description was changed from ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. For embedded viewers, we apply the internal change in zoom to the new browser zoom, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. For embedded viewers, we apply the internal change in zoom to the new browser zoom, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. For embedded viewers, we apply the internal change in zoom to the new browser zoom, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Propagate browser zoom changes to embedded PDFs. Currently, changes to the browser zoom are ignored by embedded PDF viewers. This patch propagates browser zoom changes so that the zoom level of the viewer may be updated. For embedded viewers, we apply the internal change in zoom to the new browser zoom, rather than overwriting the viewer's zoom level. This patch also initializes an embedded viewer's zoom level to its parent's zoom level. BUG=88802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2999413a1038cfd7a2020edf431671a17db0e8d8 Cr-Commit-Position: refs/heads/master@{#436695} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2999413a1038cfd7a2020edf431671a17db0e8d8 Cr-Commit-Position: refs/heads/master@{#436695} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
