Closed Bug 950546 Opened 10 years ago Closed 10 years ago

mozIColorAnalyzer sometimes returns different colors for the same icon

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: mbrubeck, Assigned: tomasz, Mentored)

References

Details

(Keywords: polish, Whiteboard: [lang=js][defect])

Attachments

(1 file, 4 obsolete files)

mozIColorAnalyzer.findRepresentativeColor does not always return the same color for a given icon.  For example, for http://www.metafilter.com/favicon.ico it sometimes returns a blue color and sometimes a yellowish one.  This is bad since we want the color (like the icon) to enhance the recognizability of the site.

The code to determine the color is here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/ColorAnalyzer_worker.js

I haven't tested it to make sure, but I think the nondeterminism comes from the sort() calls with custom comparison functions that can return 0 when comparing items that are not identical.  We change these so any pair of non-identical items is always ordered the same way, to make the sorts deterministic.
No longer blocks: metrov2planning
Hi Matt. I am interested in this bug. Could you please tell me how to test it to make sure? I want to have a try.
Thank you :)
(In reply to Peiyong Lin[:lpy](UTC-8) from comment #1)
> Hi Matt. I am interested in this bug. Could you please tell me how to test
> it to make sure? I want to have a try.

If you have Windows 8 then you can test changes by building and running Firefox for Metro.  The color analyzer is used to color parts of the "tiles" on the New Tab page in Metro, for example here:
http://hg.mozilla.org/mozilla-central/file/1ad9af3a2ab8/browser/metro/modules/View.jsm#l92

Or you can add some code to this automated test:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/browser/browser_colorAnalyzer.js

To run that test, first build Firefox and then run "./mach mochitest-browser toolkit/components/places/tests/browser/browser_colorAnalyzer.js".  For more information, see:
https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

Of course this bug will be hard to verify for sure, because of the randomness of the current behavior.  But for development purposes you could write a test page or script that uses the icon in comment 0 and analyzes it hundreds of times, and check whether it always returns the same result.
Whiteboard: [mentor=mbrubeck@mozilla.com][lang=js] → [mentor=mbrubeck@mozilla.com][lang=js][defect]
Mentor: mbrubeck
Whiteboard: [mentor=mbrubeck@mozilla.com][lang=js][defect] → [lang=js][defect]
Blocks: 634139
I'd like to work on this!
Assignee: nobody → tkolodziejski
Attached patch color-analyzer.patch (obsolete) — Splinter Review
No tests since I couldn't reproduce it since this code is no longer run anywhere. But as discussed or IRC I was asked to still send this patch. So here it is, please have a look.
Attachment #8478423 - Flags: review?(mbrubeck)
Comment on attachment 8478423 [details] [diff] [review]
color-analyzer.patch

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

This looks good to me. Requesting review from a toolkit peer.
Attachment #8478423 - Flags: review?(mbrubeck)
Attachment #8478423 - Flags: review?(MattN+bmo)
Attachment #8478423 - Flags: feedback+
Comment on attachment 8478423 [details] [diff] [review]
color-analyzer.patch

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

It would have been nice to have a test in browser_colorAnalyzer.js like was mentioned in comment 2 but I guess it's fine without.

::: toolkit/components/places/ColorAnalyzer_worker.js
@@ +118,5 @@
>    });
>  
>    // only send back the most desirable colors
>    mergedColors.sort(function(a, b) {
> +    return b.desirability !== a.desirability ? b.desirability - a.desirability : b.color - a.color;

Nit: Mozilla coding style is to use == by default[1] unless you need === because you're comparing different types.
[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
Attachment #8478423 - Flags: review?(MattN+bmo) → review+
Attached patch color-analyzer.patch (obsolete) — Splinter Review
Changing to ==. Although I'm very surprised that's the default code-style.

I tried to produce some tests but they didn't work. Asked mbrubeck but still couldn't reproduce it. So this is not a great patch :-(
Attachment #8478423 - Attachment is obsolete: true
Attachment #8478454 - Flags: review?(MattN+bmo)
Attachment #8478454 - Flags: review?(MattN+bmo) → review+
Note that the commit message is missing the bug # and you should update the reviewers.
Attached patch color-analyzer.patch (obsolete) — Splinter Review
Updated reviewer, added bug number.

I was asked in some other patch not to add bug number but I've seen that most of the commit messages have bug numbers.
Attachment #8478454 - Attachment is obsolete: true
Attachment #8478508 - Flags: review?(MattN+bmo)
Comment on attachment 8478508 [details] [diff] [review]
color-analyzer.patch

Note that if I give r+ and you only address the comments given, you don't need to request r+ again: you can upload a new patch and mark r+ yourself and mark r=MattN in the patch description.

> I was asked in some other patch not to add bug number but I've seen that
> most of the commit messages have bug numbers.

I've heard of people not wanting the reviewers in the commit message before the r+ but the bug number can always be there.
Attachment #8478508 - Flags: review?(MattN+bmo) → review+
Ok, thanks. Is reviewer always the one that's adding checkin-needed?
No, you can as well since the reviewer doesn't always know what commit access you have. You may also need to do a try run first.
Status: NEW → ASSIGNED
Interesting. Tests are failing with this change. I think that this means that tests are broken, that is they were generated via running this code and so they worked. So they were sort dependent but our sort is deterministic and so it worked all the time. Am I missing something?
Attached patch color-analyzer.patch (obsolete) — Splinter Review
I updated the patch with tests update. I'm sorry to bother you MattN but do you agree with my reasoning behind why I modify those tests? (hence r?)
Attachment #8478508 - Attachment is obsolete: true
Attachment #8479241 - Flags: review?(MattN+bmo)
Attachment #8479241 - Flags: review?(MattN+bmo) → review+
Just adding bug number to last patch since it disappeared in my last change. 

I also submitted it https://tbpl.mozilla.org/?tree=Try&rev=f974b95cb9b7. Will add checkin-needed once they pass.

Thanks Matt and Matt!
Attachment #8479241 - Attachment is obsolete: true
Attachment #8479460 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/dc6bb1c3bfbc
Keywords: checkin-needed
Whiteboard: [lang=js][defect] → [lang=js][defect][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dc6bb1c3bfbc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][defect][fixed-in-fx-team] → [lang=js][defect]
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.