mozIColorAnalyzer sometimes returns different colors for the same icon

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mbrubeck, Assigned: tomasz, Mentored)

Tracking

({polish})

Trunk
mozilla34
polish
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][defect])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Blocks: 861680

Updated

5 years ago
No longer blocks: 842686
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 :)
(Reporter)

Comment 2

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

Updated

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

Comment 3

4 years ago
I'd like to work on this!
Assignee: nobody → tkolodziejski
(Assignee)

Comment 4

4 years ago
Created attachment 8478423 [details] [diff] [review]
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.
Attachment #8478423 - Flags: review?(mbrubeck)
(Reporter)

Comment 5

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

Comment 7

4 years ago
Created attachment 8478454 [details] [diff] [review]
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 :-(
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8478508 [details] [diff] [review]
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.
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+
(Assignee)

Comment 11

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

Comment 14

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

Comment 15

4 years ago
Created attachment 8479241 [details] [diff] [review]
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?)
Attachment #8478508 - Attachment is obsolete: true
Attachment #8479241 - Flags: review?(MattN+bmo)
Attachment #8479241 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8479460 [details] [diff] [review]
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!
Attachment #8479241 - Attachment is obsolete: true
Attachment #8479460 - Flags: review+
(Assignee)

Updated

4 years ago
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
Last Resolved: 4 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.