Closed Bug 1035422 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mccr8, Assigned: lukas.silvestre, Mentored)

Details

Attachments

(1 file, 1 obsolete file)

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.
Well, calling all.js a Javascript file is a bit of a stretch.
Whiteboard: [lang=js]
Hey Andrew, can I work on this bug?
Flags: needinfo?(continuation)
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.
Flags: needinfo?(continuation)
Hi there, could I work on it?
Flags: needinfo?(continuation)
(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.
Flags: needinfo?(continuation)
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 ?
Flags: needinfo?(continuation)
Yup.
Flags: needinfo?(continuation)
Attached patch Patch removing unnecessary prefs (obsolete) — Splinter Review
Here we go.
Hopefully I did everything right.
Flags: needinfo?(continuation)
Attachment #8508123 - Flags: review?(continuation)
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 tnikkel@gmail.com 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.
Attachment #8508123 - Flags: review?(continuation) → feedback+
Assignee: nobody → lukas.silvestre
Flags: needinfo?(continuation)
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
Attachment #8508123 - Attachment is obsolete: true
Attachment #8508168 - Flags: review?(tnikkel)
Comment on attachment 8508168 [details] [diff] [review]
Keeps the code that checks if the pref is set

Looks good. Thanks!
Attachment #8508168 - Flags: review?(tnikkel) → review+
You're welcome.
Is there anything else that I have to do?
Flags: needinfo?(continuation)
(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.
Thanks for the patch.  I have landed it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4316faa5c0c
Flags: needinfo?(continuation)
https://hg.mozilla.org/mozilla-central/rev/f4316faa5c0c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.