Add an example of extracting theme colors from an image

RESOLVED FIXED

Status

WebExtensions
General
P5
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged[theming])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8809438 - Flags: review?(jaws)
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
(Assignee)

Comment 3

2 years ago
I kinda did, but also didn't - it hadn't registered fully yet and it does exactly what we'd need here.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8809438 - Flags: review?(jaws)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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?
Attachment #8809438 - Flags: review?(jaws) → review+

Comment 7

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

Comment 8

2 years ago
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

Updated

10 months ago
Priority: -- → P5
Whiteboard: triaged → triaged[theming]

Updated

9 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED

Updated

a month ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.