1035422 - Only set layout.imagevisibility.enabled to true in all.js

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andrew McCreight [:mccr8]]

This is now enabled everywhere (yay!), but it ends up getting set to true in a bunch of places:
  http://mxr.mozilla.org/mozilla-central/search?string=%22layout.imagevisibility.enabled%22&find=&findi=&filter=[Ll]ayout.imagevisibility.enabled&hitlimit=&tree=mozilla-central

I think that all of the prefs except for all.js (and the nsPresShell.cpp reference which is not setting a pref) should just get removed, so it is all controlled in a single place.  This is a pretty simple patch.

Comment 1

[Andrew McCreight [:mccr8]]

Well, calling all.js a Javascript file is a bit of a stretch.

Comment 2

[Trishul]

Hey Andrew, can I work on this bug?

Comment 3

[Andrew McCreight [:mccr8]]

Hi, sorry I was on vacation until today.  Yes, you can work on it.

The basic idea is that prefs get set in /modules/libpref/src/init/all.js, then on various specific versions, like B2G or Android, we set additional prefs, as in /b2g/app/b2g.js.  For this particular pref, it is being set to true in all.js, then set again to true in other pref files.  It used to be disabled on B2G and Android, so it was set to false there, but now it is enabled everywhere.  The list of places that this pref is set are in comment 0.

Let me know if you have any questions.

Comment 4

[Lucas Silvestre]

Hi there, could I work on it?

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Lucas Silvestre from comment #4)
> Hi there, could I work on it?

Yes, I think that is okay.  Trishul has not posted for a few months.

Comment 6

[Lucas Silvestre]

Grand!
So, all I need to do is remove the "layout.imagevisibility.enabled" pref in every file listed in comment #0 except in the /modules/libpref/src/init/all.js ?

Comment 7

[Andrew McCreight [:mccr8]]

Yup.

Comment 8

[Lucas Silvestre]

Attachment: Patch removing unnecessary prefs

Here we go.
Hopefully I did everything right.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8508123 [details] [diff] [review]
Patch removing unnecessary prefs

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

This looks good, aside from the nsPresShell change which you just need to undo.  Please fix that and upload a new version of the patch, and ask [e-mail] for review.

Also, just so you know, if you ask somebody for review, it sends an alert email similar to needinfo, so you don't need to do both.

::: layout/base/nsPresShell.cpp
@@ -5932,5 @@
>    static bool sImageVisibilityPrefCached = false;
>  
>    if (!sImageVisibilityPrefCached) {
> -    Preferences::AddBoolVarCache(&sImageVisibilityEnabled,
> -      "layout.imagevisibility.enabled", true);

You need to leave this in.  This is the code that checks if the pref is set or not, whereas the other places are the ones setting it.

Comment 10

[Andrew McCreight [:mccr8]]

Oh, and you also need to format the commit message with the bug number and a description of what you are fixing.  See https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 11

[Lucas Silvestre]

Attachment: Keeps the code that checks if the pref is set

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8508168 [details] [diff] [review]
Keeps the code that checks if the pref is set

Looks good. Thanks!

Comment 13

[Lucas Silvestre]

You're welcome.
Is there anything else that I have to do?

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Lucas Silvestre from comment #13)
> Is there anything else that I have to do?
Nope.  This patch is simple enough I think it doesn't need a try run.  I'll go ahead and land this patch when the tree is open.

Comment 15

[Andrew McCreight [:mccr8]]

Thanks for the patch.  I have landed it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4316faa5c0c

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4316faa5c0c