Fixed the bug that caused extension options not to fit their overlays properly when zoomed.

Fixed a bug that caused extension options not to fit their overlays properly when zoomed.

The issue involved a discrepancy between sizes specified in logical versus physical pixels. See https://codereview.chromium.org/939443002/ for an explanation on a very similar bugfix, which this CL utilizes.

BUG=459266
Committed: https://crrev.com/f018aaad33c80348dd6e935d573d558b83f17936
Cr-Commit-Position: refs/heads/master@{#317113}

[paulmeyer, 2015-02-18 17:52:47]

paulmeyer@chromium.org changed reviewers:
+ fsamuel@chromium.org, kalman@chromium.org

[paulmeyer, 2015-02-18 17:52:48]

+fsamuel@ for guest_view/

+kalman@ for extension_options_overlay.js

[not at google - send to devlin, 2015-02-18 18:00:12]

https://codereview.chromium.org/934303003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/934303003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:132: var
newWidth = Math.max(evt.width + 2, minWidth);
Could you explain where the value of 2 comes from, e.g. why not 1?

[paulmeyer, 2015-02-18 18:14:11]

https://codereview.chromium.org/934303003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/934303003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:132: var
newWidth = Math.max(evt.width + 2, minWidth);
On 2015/02/18 18:00:12, kalman wrote:
> Could you explain where the value of 2 comes from, e.g. why not 1?

2 pixels allows the overlay a 1 pixel border around the extension options (i.e.
1 per top, bottom, left, right). Adding only 1 pixel for width/height still
results in scrollbars appearing for some zoom levels.

[Fady Samuel, 2015-02-18 18:27:31]

https://codereview.chromium.org/934303003/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/934303003/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:522: return
static_cast<int>(physical_pixels / zoom_factor + 0.5);
Why are you casting to int here and then returning double? Also please avoid
static_cast<int> and +0.5 and use ceil/floor/round instead as appropriate.
Thanks!

[not at google - send to devlin, 2015-02-18 19:19:10]

https://codereview.chromium.org/934303003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/934303003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:132: var
newWidth = Math.max(evt.width + 2, minWidth);
On 2015/02/18 18:14:11, Paul Meyer wrote:
> On 2015/02/18 18:00:12, kalman wrote:
> > Could you explain where the value of 2 comes from, e.g. why not 1?
> 
> 2 pixels allows the overlay a 1 pixel border around the extension options
(i.e.
> 1 per top, bottom, left, right). Adding only 1 pixel for width/height still
> results in scrollbars appearing for some zoom levels.

Ok, thanks for explaining.

Why is the pixel needed at all? The scrollbar thing sounds pretty unfortunate
but adding a pixel seems... well, a hack. I'm just wondering if this could be
some rounding issue, e.g. if you rounded up evt.height then it would work
(though it does seem like you're doing this already in the event).

Another option would be to force there to be no scrollbars with overflow:hidden
on the overlay, but that's also a bit hacky.

[paulmeyer, 2015-02-19 18:42:58]

ptal

https://codereview.chromium.org/934303003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/934303003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:132: var
newWidth = Math.max(evt.width + 2, minWidth);
On 2015/02/18 19:19:10, kalman wrote:
> On 2015/02/18 18:14:11, Paul Meyer wrote:
> > On 2015/02/18 18:00:12, kalman wrote:
> > > Could you explain where the value of 2 comes from, e.g. why not 1?
> > 
> > 2 pixels allows the overlay a 1 pixel border around the extension options
> (i.e.
> > 1 per top, bottom, left, right). Adding only 1 pixel for width/height still
> > results in scrollbars appearing for some zoom levels.
> 
> Ok, thanks for explaining.
> 
> Why is the pixel needed at all? The scrollbar thing sounds pretty unfortunate
> but adding a pixel seems... well, a hack. I'm just wondering if this could be
> some rounding issue, e.g. if you rounded up evt.height then it would work
> (though it does seem like you're doing this already in the event).
> 
> Another option would be to force there to be no scrollbars with
overflow:hidden
> on the overlay, but that's also a bit hacky.

It looks like there is a problem with the preferred size coming in from blink
where for some zoom levels the size is too small, and so scrollbars appear. I'll
put a TODO here for now and then investigate the problem with the size in Blink.

https://codereview.chromium.org/934303003/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/934303003/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/guest_view_base.cc:522: return
static_cast<int>(physical_pixels / zoom_factor + 0.5);
On 2015/02/18 18:27:31, Fady Samuel wrote:
> Why are you casting to int here and then returning double? Also please avoid
> static_cast<int> and +0.5 and use ceil/floor/round instead as appropriate.
> Thanks!

Done.

[Fady Samuel, 2015-02-19 18:48:42]

lgtm

[not at google - send to devlin, 2015-02-19 18:53:11]

lgtm

[paulmeyer, 2015-02-19 18:53:49]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2015-02-19 18:54:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/934303003/20001

[commit-bot: I haz the power, 2015-02-19 19:55:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-19 19:56:30]

Patchset 2 (id:??) landed as
https://crrev.com/f018aaad33c80348dd6e935d573d558b83f17936
Cr-Commit-Position: refs/heads/master@{#317113}