1316603 - Add an example of extracting theme colors from an image

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P5)

Description

[Mike de Boer [:mikedeboer]]

Use an image quantizer to read all the color values from an image and use the most distinctive ones ('dominant' colors) to use in the UI.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1316603 - add a findDominantColors() method to the ColorAnalyzer class to extract dominant colors from an image to use as theme colors.

Review commit: https://reviewboard.mozilla.org/r/92032/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92032/

Comment 2

[Matthew N. [:MattN]]

In case you didn't know, we already have ColorAnalyzer(_worker).js which does some similar things e.g. https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/ColorAnalyzer_worker.js#56-66

Comment 3

[Mike de Boer [:mikedeboer]]

I kinda did, but also didn't - it hadn't registered fully yet and it does exactly what we'd need here.

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8809438 [details]
Bug 1316603 - add a findDominantColors() method to the ColorAnalyzer class to extract dominant colors from an image to use as theme colors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92032/diff/1-2/

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8809438 [details]
Bug 1316603 - add a findDominantColors() method to the ColorAnalyzer class to extract dominant colors from an image to use as theme colors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92032/diff/2-3/

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8809438 [details]
Bug 1316603 - add a findDominantColors() method to the ColorAnalyzer class to extract dominant colors from an image to use as theme colors.

https://reviewboard.mozilla.org/r/92032/#review94744

::: browser/components/extensions/ext-theme.js:513
(Diff revision 3)
>      // Use a temporary element to filter the CSS values that themes can provide.
>      if (!WindowManager.topWindow) {
>        return;
>      }
>  
> +    let deferred = false;
> +
>      let temp = WindowManager.topWindow.document.createElement("temp");

Please keep the temp variable declaration closer to the comment about using a temporary element.

::: browser/components/extensions/ext-theme.js:543
(Diff revision 3)
>        } else if (image == "theme_frame" || image == "headerURL") {
>          this.LWTStyles.headerURL = val;
> +        if (!fullTheme.colors || !Object.keys(fullTheme.colors).length) {
> +          deferred = true;
> +          const CA = Cc["@mozilla.org/places/colorAnalyzer;1"].getService(Ci.mozIColorAnalyzer);
> +          CA.findDominantColors(Services.io.newURI(val, "", null), function(success, color, pal) {

please use 'palette' instead of 'pal' here and elsewhere in the patch.

::: toolkit/components/places/ColorAnalyzer_worker.js:173
(Diff revision 3)
>        }
>      }
>    }
>  
>    let colors = [];
>    for (var color in colorFrequency) {

'color' here is a redeclaration of the variable declared at line 161, which will mean the one at line 161 will shadow this instance.

Also, please use 'let' instead of 'var'.

::: toolkit/components/places/mozIColorAnalyzer.idl:28
(Diff revision 3)
>     *        If success is false, color is not provided.
>     */
> -  void onComplete(in boolean success, [optional] in unsigned long color);
> +  void onComplete(in boolean success, [optional] in unsigned long color, [optional] in jsval colors);
>  };
>  
> -[scriptable, uuid(d056186c-28a0-494e-aacc-9e433772b143)]
> +[scriptable, uuid(0ce30b36-3f67-452b-a91f-0f52b04c7720)]

uuids don't need to be updated anymore, per https://groups.google.com/forum/#!searchin/mozilla.dev.platform/uuid%7Csort:relevance/mozilla.dev.platform/n6qBpyxoI6I/4qxwFvBOAgAJ

::: toolkit/modules/Color.jsm:17
(Diff revision 3)
> + * @param {Number} population
>   */
> -function Color(r, g, b) {
> +function Color(r, g, b, population) {

Can this comment explain what 'population' is?

Comment 7

[Tim Nguyen :ntim]

Thanks for adding me into this bug!

As far as I know there's no need to provide a new theme API for this? Just a XHR/webRequest to the favicon and a background page should be enough. I have a colour-utils module here that does the job for me, it just needs an image element: https://github.com/nt1m/vivaldi-fox/blob/master/utils/colour.js . So as long as I can access the icon, it should be fine :)

Maybe it would be more helpful to have an alternative to google chrome's chrome://favicon/ uris.

Comment 8

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/cedar/rev/e411a5414a9d2685574dd5cd5b7843e629cf21b8
Bug 1316603 - add a findDominantColors() method to the ColorAnalyzer class to extract dominant colors from an image to use as theme colors. r=jaws