1373238 - browser_ext_browserAction_popup_resize.js complains when run on hidpi machines

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[:Gijs (he/him)]

Spinning this off from bug 1372309, given the concerns Andrew raised that the changes might be masking a real bug where the fact that there's a scrollMax of 1 might be indicating that the contents of the panel are indeed scrollable.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1373238 - make popup resize test not care about rounding errors so it works on local hidpi machines

This lets me successfully run the tests on my hidpi machines.

Review commit: https://reviewboard.mozilla.org/r/149444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149444/

Comment 2

[:Gijs (he/him)]

(In reply to Andrew Swan [:aswan] from bug 1372309 comment #21)
> (In reply to :Gijs from comment #19)
> > (In reply to Andrew Swan [:aswan] from comment #16)
> > > > -  is(win.scrollMaxY, 0, "Document should not be vertically scrollable");
> > > > +  Assert.lessOrEqual(win.scrollMaxY, 1, "Document should not be vertically scrollable");
> > > 
> > > nit: should this (and the others below) be `Assert.less()`?  If scrollMaxY
> > > is exactly 1, it will be scrollable yes?
> > 
> > That would be better, but AFAICT scrolMaxY is rounded to ints. And the item
> > is sometimes scrollable, IIRC, but I guess that's a rounding error
> > somewhere, if it's literally exactly 1px? We can file a followup for this if
> > you prefer, it doesn't happen in automation but it happens for me both pre-
> > and post-photon when running locally on a hidpi machine... Let me know what
> > you want me to do...
> 
> I'm happy to keep it as-is but I haven't worked much with this test, I'm
> going to redirect to Kris who has...

Transferring this ni to this bug, with the patch now attached here.

Comment 3

[Andy McKay]

Mark, could you take a look at this one? I think its a matter of trying and reviewing Gijs patch at this point.

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 8878027 [details]
Bug 1373238 - make popup resize test not care about rounding errors so it works on local hidpi machines

https://reviewboard.mozilla.org/r/149444/#review161684

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:32
(Diff revision 1)
>    let browser = await openPanel(extension, undefined, true);
>  
>    async function checkSize(expected) {
>      let dims = await promiseContentDimensions(browser);
>  
> -    is(dims.window.innerHeight, expected, `Panel window should be ${expected}px tall`);
> +    Assert.lessOrEqual(Math.abs(dims.window.innerHeight - expected), 1,

Line 38 is basically the same but for width and uses `ok(Math.abs(...) <= 1, ...)`. Either version seems fine to me but it would be nice to be consistent here.

Comment 5

[Mark Striemer [:mstriemer]]

So this fixes the test for me on my MBP, that seems reasonable.

I cannot comment on if this is hiding a bug though, I've never looked at this code before. It seems like we don't want it to scroll, so scrolling 1 pixel would be a bug.

Comment 6

[Mark Striemer [:mstriemer]]

Comment on attachment 8878027 [details]
Bug 1373238 - make popup resize test not care about rounding errors so it works on local hidpi machines

https://reviewboard.mozilla.org/r/149444/#review162094

I can't reproduce this with an add-on using what seems similar to this test case that fails. Seems like it should be fine to merge to me.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4434cdfcb3
make popup resize test not care about rounding errors so it works on local hidpi machines, r=mstriemer

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7c4434cdfcb3