915526 - make image visibility usable on android and b2g when prefed on

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

This seems reasonable and should let us start testing it by flipping the main pref.

Comment 1

[Timothy Nikkel (:tnikkel)]

Comment on attachment 803520 [details] [diff] [review]
patch

This is just something that makes sense so we can start testing. We can change it later. No hurry.

Comment 2

[Mats Palmgren (inactive)]

Just to be clear, you want B2G and Mobile to have:
pref("layout.imagevisibility.enabled", false);
pref("layout.imagevisibility.numscrollportwidths", 1);
pref("layout.imagevisibility.numscrollportheights", 1);

modules/libpref/src/init/all.js doesn't declare any of those, so it will
get the defaults from the C++ code (true, 0, 1, respectively):
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5362
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2228

Is that your intention? (it seems a little odd to leave numscrollportwidths = 0
for desktop)

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 803520 [details] [diff] [review]
patch

I think roc would be a better reviewer for this.

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #2)
> Is that your intention? (it seems a little odd to leave numscrollportwidths
> = 0
> for desktop)

Yeah, that was my intention. My reasoning was that horizontal scrolling is much more rare on desktop. It happens frequently on mobile because users have to zoom in and pan around the page. I can give us some horizontal lookahead on desktop if you think it's a good idea.

Comment 5

[Mats Palmgren (inactive)]

OK.  I think it's fine as is, it's probably better for testing purposes to
to test the values we intend to ship.  OTOH, our installed base on desktop
is orders of magnitude larger than b2g/mobile (afaik) so using 1 there
(for a while) might give us earlier feedback in case there's a problem.

Comment 6

[Timothy Nikkel (:tnikkel)]

I'm not opposed to shipping 1 for horizontal on desktop either, that also seems reasonable.

I just wanted to provide good initial values (just based on a good guess by myself), and I'm open to any suggestions (based on instinct or data) to change the values.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 803520 [details] [diff] [review]
patch

Review of attachment 803520 [details] [diff] [review]:
-----------------------------------------------------------------

Please add default values for these prefs to all.js.

Comment 8

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72f8d310a723

Comment 9

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/72f8d310a723

Comment 10

[Jim Mathies [:jimm]]

(In reply to Timothy Nikkel (:tn) from comment #4)
> (In reply to Mats Palmgren (:mats) from comment #2)
> > Is that your intention? (it seems a little odd to leave numscrollportwidths
> > = 0
> > for desktop)
> 
> Yeah, that was my intention. My reasoning was that horizontal scrolling is
> much more rare on desktop. It happens frequently on mobile because users
> have to zoom in and pan around the page. I can give us some horizontal
> lookahead on desktop if you think it's a good idea.

We use horizontal scrolling in the start tab for metrofx and we also support panning and zooming. Should we add these prefs for metrofx?

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Jim Mathies [:jimm] from comment #10)
> We use horizontal scrolling in the start tab for metrofx and we also support
> panning and zooming. Should we add these prefs for metrofx?

Yes, definitely. Thanks for noticing this. I filed bug 916904.