Closed Bug 915526 Opened 6 years ago Closed 6 years ago

make image visibility usable on android and b2g when prefed on

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
This seems reasonable and should let us start testing it by flipping the main pref.
Attachment #803520 - Flags: review?(matspal)
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.
Attachment #803520 - Flags: feedback?(bugmail.mozilla)
Attachment #803520 - Flags: feedback?(bugmail.mozilla) → feedback+
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 on attachment 803520 [details] [diff] [review]
patch

I think roc would be a better reviewer for this.
Attachment #803520 - Flags: review?(matspal) → review?(roc)
(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.
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.
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 on attachment 803520 [details] [diff] [review]
patch

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

Please add default values for these prefs to all.js.
Attachment #803520 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/72f8d310a723
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(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?
(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.
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.