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

RESOLVED FIXED in mozilla36

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mccr8, Assigned: Lucas Silvestre, Mentored)

Tracking

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Well, calling all.js a Javascript file is a bit of a stretch.
Whiteboard: [lang=js]

Comment 2

4 years ago
Hey Andrew, can I work on this bug?
Flags: needinfo?(continuation)
(Reporter)

Comment 3

4 years ago
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)
(Assignee)

Comment 4

4 years ago
Hi there, could I work on it?
(Assignee)

Updated

4 years ago
Flags: needinfo?(continuation)
(Reporter)

Comment 5

4 years ago
(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)
(Assignee)

Comment 6

4 years ago
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)
(Reporter)

Comment 7

4 years ago
Yup.
Flags: needinfo?(continuation)
(Assignee)

Comment 8

4 years ago
Created attachment 8508123 [details] [diff] [review]
Patch removing unnecessary prefs

Here we go.
Hopefully I did everything right.
Flags: needinfo?(continuation)
Attachment #8508123 - Flags: review?(continuation)
(Reporter)

Comment 9

4 years ago
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+
(Reporter)

Updated

4 years ago
Assignee: nobody → lukas.silvestre
Flags: needinfo?(continuation)
(Reporter)

Comment 10

4 years ago
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
(Assignee)

Comment 11

4 years ago
Created attachment 8508168 [details] [diff] [review]
Keeps the code that checks if the pref is set
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+
(Assignee)

Comment 13

4 years ago
You're welcome.
Is there anything else that I have to do?
Flags: needinfo?(continuation)
(Reporter)

Comment 14

4 years ago
(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.
(Reporter)

Comment 15

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.