Open
Bug 972099
Opened 11 years ago
Updated 2 years ago
A preference gfx.color_management.force_srgb needs to move from .js to .py
Categories
(Testing :: Reftest, defect)
Tracking
(Not tracked)
REOPENED
People
(Reporter: milan, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
1.40 KB,
patch
|
milan
:
review+
milan
:
feedback+
|
Details | Diff | Splinter Review |
Even though it isn't a default, we want to run the tests with gfx.color_management.force_srgb set to true. Right now, this is accomplished by setting this after the startup. However, changing that preference after the startup is not safe in general, so we are going to make it "read only on startup" type of pref (see bug 971126). In order to continue testing with gfx.color_management.force_srgb set to true, even though the default is false, my understanding is that we need to make a change to runreftest.py or some such.
Reporter | ||
Comment 1•11 years ago
|
||
Andrew, does the above description make sense, and is runreftest.py the right place for this change? If not, can you suggest the right approach, or point us to somebody else? Thanks.
Blocks: 971126
Flags: needinfo?(ahalberstadt)
I'd have hoped that the pref setting in reftest-cmdline.js happens before the pref gets read in the first place. Are you sure this is needed?
Comment 3•11 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #1)
> Andrew, does the above description make sense, and is runreftest.py the
> right place for this change? If not, can you suggest the right approach, or
> point us to somebody else? Thanks.
Yeah, that makes sense. You can add the pref here, where it will be set as a user_pref in the profile:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftestb2g.py#412
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)
> I'd have hoped that the pref setting in reftest-cmdline.js happens before
> the pref gets read in the first place. Are you sure this is needed?
We can't use reftest-cmdline in b2g because we are starting the emulator binary instead of a gecko binary. The emulator is what starts the gecko process and we don't have control over the command line we can pass it. So instead we kill the b2g process after it has been started, swap in a new profile and restart it manually. There's a bug somewhere to abstract the prefs out of reftest-cmdline and share them with b2g/android, but as of now they are just copied :(
Flags: needinfo?(ahalberstadt)
Reporter | ||
Comment 4•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)
> I'd have hoped that the pref setting in reftest-cmdline.js happens before
> the pref gets read in the first place. Are you sure this is needed?
Sadly, it seems to be. If I register this pref with a callback for a "modified value", the callback happens and the value is modified.
OS: Mac OS X → Windows 7
Reporter | ||
Comment 5•11 years ago
|
||
Note that, in this particular case, the issue is only relevant on Windows.
Reporter | ||
Comment 6•11 years ago
|
||
Andrew, I can't tell from the conversation if this is what you're suggesting needs to be done.
Attachment #8375558 -
Flags: review?(ahalberstadt)
Comment 7•11 years ago
|
||
Oh sorry, I didn't realize this was Windows only. I agree with :dbaron then, I'm confused as to why the pref in reftest-cmdline.js isn't being set at startup.
In this case I don't think you want to touch any of the b2g/android files (i.e runtestsb2g, b2g_start_script, and bootstrap). Did you mean to do that (I assume I just confused you with comment 3)?
Flags: needinfo?(milan)
Reporter | ||
Comment 8•11 years ago
|
||
Right - I made the change in there because that preference was being set on b2g - even though it is basically ignored there. So, for consistency.
I definitely don't know this code well enough to understand why it isn't being set at startup. I can investigate, but that'll take a while.
Does it make sense to just make the change to runreftest.py (as above) and land that now to unblock another bug and open a bug to figure out why this is happening?
Flags: needinfo?(milan) → needinfo?(ahalberstadt)
Comment 9•11 years ago
|
||
Comment on attachment 8375558 [details] [diff] [review]
Move gfx.color_management.force_srgb from .js to .py
Review of attachment 8375558 [details] [diff] [review]:
-----------------------------------------------------------------
If this fixes a problem for you, I don't mind landing this. Might want to run it passed :dbaron though..
Attachment #8375558 -
Flags: review?(ahalberstadt) → review+
Updated•11 years ago
|
Flags: needinfo?(ahalberstadt)
Reporter | ||
Comment 10•11 years ago
|
||
David, thoughts?
Attachment #8375558 -
Attachment is obsolete: true
Attachment #8375777 -
Flags: review+
Attachment #8375777 -
Flags: feedback?(dbaron)
Flags: needinfo?(dbaron)
Comment on attachment 8375777 [details] [diff] [review]
Set gfx.color_management.force_srgb preference value in runreftest.py. Carry r=ahalberstadt
Maybe put this below the extraPrefs handling, but otherwise seems fine.
Attachment #8375777 -
Flags: feedback?(dbaron) → feedback+
Flags: needinfo?(dbaron)
Reporter | ||
Comment 12•11 years ago
|
||
Attachment #8375777 -
Attachment is obsolete: true
Attachment #8376377 -
Flags: review+
Attachment #8376377 -
Flags: feedback+
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → milan
Keywords: checkin-needed
Reporter | ||
Comment 13•11 years ago
|
||
Keywords: checkin-needed
Comment 14•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 15•11 years ago
|
||
Backed out for causing bug 951139 to spike. CCing Ben to the bug to explain the reasoning behind it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42874936c10
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla30 → ---
Comment 16•11 years ago
|
||
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/e42874936c10
Reporter | ||
Updated•7 years ago
|
Assignee: milaninbugzilla → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•