950546 - mozIColorAnalyzer sometimes returns different colors for the same icon

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Matt Brubeck (:mbrubeck)]

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.

Comment 1

[Peiyong Lin[:lpy](UTC+8)]

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 :)

Comment 2

[Matt Brubeck (:mbrubeck)]

(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.

Comment 3

[Tomasz Kołodziejski [:tomasz]]

I'd like to work on this!

Comment 4

[Tomasz Kołodziejski [:tomasz]]

Attachment: color-analyzer.patch

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.

Comment 5

[Matt Brubeck (: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.

Comment 6

[Matthew N. [:MattN]]

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

Comment 7

[Tomasz Kołodziejski [:tomasz]]

Attachment: color-analyzer.patch

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 :-(

Comment 8

[Matthew N. [:MattN]]

Note that the commit message is missing the bug # and you should update the reviewers.

Comment 9

[Tomasz Kołodziejski [:tomasz]]

Attachment: color-analyzer.patch

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.

Comment 10

[Matthew N. [:MattN]]

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.

Comment 11

[Tomasz Kołodziejski [:tomasz]]

Ok, thanks. Is reviewer always the one that's adding checkin-needed?

Comment 12

[Matthew N. [:MattN]]

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.

Comment 13

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=94ca39b74e02

Comment 14

[Tomasz Kołodziejski [:tomasz]]

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?

Comment 15

[Tomasz Kołodziejski [:tomasz]]

Attachment: color-analyzer.patch

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?)

Comment 16

[Tomasz Kołodziejski [:tomasz]]

Attachment: color-analyzer.patch

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!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/dc6bb1c3bfbc

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc6bb1c3bfbc