449681 - switch color_management pref from off/on bool to off/on/taggedonly int

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Bobby Holley (:bholley)]

We want the option to color correct tagged images only. If we switch the current bool to an int and do 0=off, 1=on, 2=taggedonly, we should maintain backward pref compatibility.

Marking as blocking bug 390697, even though work on the UI stuff isn't all that tightly coupled with this. I'll get it done soon.

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch to switch to a 3-mode color pref

patch added - flagging vlad for review

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 333035 [details] [diff] [review]
patch to switch to a 3-mode color pref

Man, I wonder who started that silly "AllCount" convention :)

Can you call the enums more descriptive names, and put in comments what each one days?

Maybe eCMSMode_All, eCMSMode_TaggedImagesOnly?  Looks great otherwise.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 333035 [details] [diff] [review]
patch to switch to a 3-mode color pref

can you just double check with.. hrm, any of gavin/dolske/deitrich to make sure that switching that pref from a boolean to an int won't cause problems if the 0/1 values match up?

Comment 4

[Bobby Holley (:bholley)]

Attachment: updated patch with new enum names, new pref, and migration code

dolske was pretty sure the auto-migration wouldn't happen. Given that color_management.enabled is a pretty crappy name for an int pref, he suggested using a new pref name and adding code to migrate and then delete the old pref. Added that here.

Flagging vlad for review

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 333184 [details] [diff] [review]
updated patch with new enum names, new pref, and migration code

Looks good

Comment 6

[Bobby Holley (:bholley)]

thanks vlad. marking as checkin-needed.

Comment 7

[Bobby Holley (:bholley)]

crap - not checkin-needed!

Comment 8

[Bobby Holley (:bholley)]

Attachment: same as above, with a missing semicolon added

oops - I had a tiny change (an added semicolon that fixed compile bustage) in my local tree but I hadn't qrefreshed it into the patch. This patch should be checked in, not the above. Aside from that small fix it's the same.

Comment 9

[Bobby Holley (:bholley)]

reflagging checkin-needed

Comment 10

[Bobby Holley (:bholley)]

guh - bitrot. fixing...

Comment 11

[Bobby Holley (:bholley)]

Attachment: fixed a bit of bitrot

I thought there was more, but I guess hg import works better than patch

Comment 12

[Bobby Holley (:bholley)]

pushed in dfc43ed110cb

Comment 13

[Glenn Randers-Pehrson]

A comment in the source code listing the meanings of mode 0,1, and 2 would have been helpful.

Comment 14

[Glenn Randers-Pehrson]

(In reply to comment #13)
> A comment in the source code listing the meanings of mode 0,1, and 2 would have
> been helpful.

It would be sufficient to replace
pref("gfx.color_management.mode", 0);
with
pref("gfx.color_management.mode", eCMSMode_Off);

which would lead a person to the definitions of eCMSMode_* which are commented.

Comment 15

[Bobby Holley (:bholley)]

Attachment: patch to comment the modes in all.js

Glenn - I don't think we can use the enum in all.js, since it's a javascript file.

Attached a patch to comment the modes in all.js instead. Flagging vlad for review.

Comment 16

[Glenn Randers-Pehrson]

Looks fine, thanks.

Comment 17

[Bobby Holley (:bholley)]

pushed in 0916bd66cf67