Closed
Bug 1270742
Opened 8 years ago
Closed 8 years ago
[PageAction] Add support for chrome.pageAction icons on Android
Categories
(WebExtensions :: Untriaged, defect)
Tracking
(firefox50 fixed)
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: mattw, Assigned: mattw)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [pageAction]triaged)
Attachments
(1 file)
This bug is meant to track the implementation and testing of default_icon in the chrome.pageAction manifest on android.
Assignee | ||
Updated•8 years ago
|
Iteration: 49.1 - May 9 → 49.2 - May 23
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54074/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54074/
Attachment #8754595 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/1-2/
Comment 3•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s https://reviewboard.mozilla.org/r/54074/#review51076 This is a good start, but it needs a bit more work to handle corner cases better. ::: mobile/android/components/extensions/ext-pageAction.js:55 (Diff revision 2) > + IconDetails.convertImageURLToDataURI(imageURL, contentWindow).then(dataURI => { > + this.options.icon = dataURI; > + this.id = PageActions.add(this.options); > + resolve(); > + }) > + }); This would usually be phrased as: let imageURL = IconDetails.getURL(this.icons, contentWindow, this.extension); return IconDetails.convertImageURLToDataURI(imageURL, contentWindow).then(dataURI => { this.options.icon = dataURI; this.id = PageActions.add(this.options); }); We should try to handle it correctly when people do things like this, though: pageAction.show(tabId); pageAction.hide(tabId); The easies thing would probably be to just keep a counter that we bump after every show/hide call, and only take any action in the callback if the counter still has the same value by the time it gets called. ::: mobile/android/components/extensions/ext-pageAction.js:102 (Diff revision 2) > show(tabId) { > - pageActionMap.get(extension).show(tabId); > + return new Promise(resolve => { > + pageActionMap.get(extension).show(tabId, context.contentWindow).then(() => { > + resolve(); > + }); > + }) return pageActionMap.get(extension) .show(tabId, context.contentWindow) .then(() => {}); ::: mobile/android/components/extensions/schemas/page_action.json:60 (Diff revision 2) > "functions": [ > { > "name": "show", > "type": "function", > "description": "Shows the page action. The page action is shown whenever the tab is selected.", > + "async": "callback", Let's just make this `"async": true`, and leave out the callback argument, since this isn't supported on Chrome. The same goes for `hide()`. Let me know if you need help adding Schema.jsm support for this. Also, we need to make the same changes to the browser/ schema. ::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:55 (Diff revision 2) > manifest: { > "name": "PageAction Extension", > "page_action": { > "default_title": "Page Action", > + "default_icon": { > + "19": "extension.png", We use 18px and 36px icons, not 19 and 38. ::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:62 (Diff revision 2) > + }, > + }, > - }, > + }, > + files: { > + "extension.png": IMAGE_ARRAYBUFFER, > + "a.png": IMAGE_ARRAYBUFFER, Is this used? ::: toolkit/components/extensions/ExtensionUtils.jsm:451 (Diff revision 2) > + const DEFAULT = "chrome://browser/content/extension.svg"; > + > + return AddonManager.getPreferredIconURL({icons: icons}, size, window) || DEFAULT; > + }, > + > + convertImageURLToDataURI(imageURL, contentWindow) { Can we make this `context` rather than `contentWindow` for parity with `convertImageDataToPNG`? ::: toolkit/components/extensions/ExtensionUtils.jsm:457 (Diff revision 2) > + return new Promise(resolve => { > + var image = new contentWindow.Image(); > + image.onload = function () { > + var canvas = contentWindow.document.createElement('canvas'); > + canvas.width = this.width; > + canvas.height = this.height; We should pass in the expected dimensions, and scale the image to match, so that this works correctly with PNGs. ::: toolkit/components/extensions/ExtensionUtils.jsm:460 (Diff revision 2) > + var canvas = contentWindow.document.createElement('canvas'); > + canvas.width = this.width; > + canvas.height = this.height; > + canvas.getContext('2d').drawImage(this, 0, 0); > + resolve(canvas.toDataURL('image/png')); > + }; This also needs an `onerror` handler, which should probably reject the promise.
Attachment #8754595 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/54074/#review51076 > We use 18px and 36px icons, not 19 and 38. Do you know why IconDetails.normalize defaults to 19? Should that be switched to 18? > We should pass in the expected dimensions, and scale the image to match, so that this works correctly with PNGs. I think to handle dimensions other than 18x18 I'd need access to the `bestSize` variable calculated in `AddonManager.getPreferredIconURL`. Do you know of a different way I could get the expected dimensions?
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/2-3/
Attachment #8754595 -
Flags: review?(kmaglione+bmo)
Comment 6•8 years ago
|
||
https://reviewboard.mozilla.org/r/54074/#review51076 > Do you know why IconDetails.normalize defaults to 19? Should that be switched to 18? 19px is the default size on Chrome, so that's the size that we use when creating the object. In practice, it doesn't really make a difference. We still choose the icon that's closest to the correct size. > I think to handle dimensions other than 18x18 I'd need access to the `bestSize` variable calculated in `AddonManager.getPreferredIconURL`. Do you know of a different way I could get the expected dimensions? We want to scale it to the final image size, regardless of the size that was specified in the object, so that's not really necessary.
Assignee | ||
Comment 7•8 years ago
|
||
https://reviewboard.mozilla.org/r/54074/#review51076 > We want to scale it to the final image size, regardless of the size that was specified in the object, so that's not really necessary. But how do I know whether to size it to 18, or 36?
Comment 8•8 years ago
|
||
https://reviewboard.mozilla.org/r/54074/#review51076 > But how do I know whether to size it to 18, or 36? Multiply 18 by `window.devicePixelRatio`
Assignee | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/54074/#review51076 > Multiply 18 by `window.devicePixelRatio` Ah, thanks
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/3-4/
Assignee | ||
Updated•8 years ago
|
Summary: [PageAction] Add support for chrome.pageAction default_icon on Android → [PageAction] Add support for chrome.pageAction icons on Android
Comment 11•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s https://reviewboard.mozilla.org/r/54074/#review52624 ::: toolkit/components/extensions/ExtensionUtils.jsm:457 (Diff revisions 3 - 4) > return new Promise((resolve, reject) => { > let win = context.contentWindow; > - var image = new win.Image(); > + let image = new win.Image(); > image.onload = function () { > - var canvas = win.document.createElement('canvas'); > - canvas.width = size; > + let canvas = win.document.createElement('canvas'); > + let scaledSize = size * win.devicePixelRatio Hm. Unfortunately, this won't work, since background pages don't actually have the same device pixel ratio as browser windows. I think you're just going to have to grab the most recent browser window and take the pixel ratio from that. ::: toolkit/components/extensions/ExtensionUtils.jsm:460 (Diff revisions 3 - 4) > image.onload = function () { > - var canvas = win.document.createElement('canvas'); > - canvas.width = size; > - canvas.height = size; > + let canvas = win.document.createElement('canvas'); > + let scaledSize = size * win.devicePixelRatio > + canvas.width = scaledSize; > + canvas.height = scaledSize; > canvas.getContext('2d').drawImage(this, 0, 0); You need to set the scaling on the on the context so that the final scaled size of the image matches the size of the canvas before you draw the image. https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/scale It would probably be good to make sure we're scaling the x and y by the same ratio, too, and center the image on the one axis if one ratio is bigger than the other. ::: toolkit/components/extensions/ExtensionUtils.jsm:461 (Diff revisions 3 - 4) > - var canvas = win.document.createElement('canvas'); > - canvas.width = size; > - canvas.height = size; > + let canvas = win.document.createElement('canvas'); > + let scaledSize = size * win.devicePixelRatio > + canvas.width = scaledSize; > + canvas.height = scaledSize; > canvas.getContext('2d').drawImage(this, 0, 0); > resolve(canvas.toDataURL('image/png')); Double quotes for all strings, please. ::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:55 (Diff revision 4) > manifest: { > "name": "PageAction Extension", > "page_action": { > "default_title": "Page Action", > + "default_icon": { > + "18": "extension.png", }, Add newline after comma.
Attachment #8754595 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/4-5/
Attachment #8754595 -
Attachment description: MozReview Request: Bug 1270742 - Add support for default_icon in chrome.pageAction r?kmag → Bug 1270742 - Add support for default_icon in chrome.pageAction
Attachment #8754595 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/5-6/
Updated•8 years ago
|
Attachment #8754595 -
Flags: review?(kmaglione+bmo) → review+
Comment 14•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s https://reviewboard.mozilla.org/r/54074/#review54602 ::: mobile/android/components/extensions/ext-pageAction.js:61 (Diff revisions 4 - 6) > if (this.shouldShow) { > this.options.icon = dataURI; > this.id = PageActions.add(this.options); > } > }).catch(() => { > return Promise.reject("Failed to load PageAction icon"); This should be `Promise.reject({message: ...})`
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/6-7/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 16•8 years ago
|
||
has problems to apply: patching file mobile/android/components/extensions/ext-pageAction.js Hunk #1 FAILED at 3 Hunk #2 FAILED at 77 2 out of 2 hunks FAILED -- saving rejects to file mobile/android/components/extensions/ext-pageAction.js.rej patching file mobile/android/components/extensions/schemas/page_action.json Hunk #1 FAILED at 13 1 out of 2 hunks FAILED -- saving rejects to file mobile/android/components/extensions/schemas/page_action.json.rej patch failed to apply abort: fix up the working directory and run hg transplant --continue Tomcats-MacBook-Pro-2:fx-team Tomcat$
Flags: needinfo?(mwein)
Keywords: checkin-needed
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/7-8/
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment 18•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/5e6356e7a99e Add support for default_icon in chrome.pageAction. r=kmag
Keywords: checkin-needed
Comment 19•8 years ago
|
||
It looks like there are eslint issues with this, Matt.
Flags: needinfo?(mwein)
Comment 20•8 years ago
|
||
yes :) sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9974463&repo=fx-team
Comment 21•8 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/071e9554e3e7 Backed out changeset 5e6356e7a99e for eslint test failures
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 22•8 years ago
|
||
Sorry about that! I ran eslint for all of `mobile/` but I needed to also run it on `browser/`.
Flags: needinfo?(mwein)
Assignee | ||
Comment 23•8 years ago
|
||
And also `toolkit/` for ExtensionUtils.jsm
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/8-9/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 25•8 years ago
|
||
Some additional notes for MDN documentation: - Since the Tabs API isn't supported yet, anything involving a tab ID should be ignored. For example, PageActions will be shown for the active tab regardless of the tab ID provided. - The API methods setTitle and getTitle aren't supported on Android since Android doesn't support tooltips. - Since there is no UI yet for displaying popups as overlays, popups will be opened in new tabs.
Comment 26•8 years ago
|
||
Hey Matt, could you take a look at this, seems this has conflicts now: applying f00cfc21b57d patching file browser/components/extensions/ext-utils.js Hunk #1 FAILED at 2 1 out of 1 hunks FAILED -- saving rejects to file browser/components/extensions/ext-utils.js.rej patch failed to apply abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(mwein)
Keywords: checkin-needed
Assignee | ||
Comment 27•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/9-10/
Assignee | ||
Comment 28•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ee5541d1c7
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment 29•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/1008f5b88e6b Add support for default_icon in chrome.pageAction. r=kmag
Keywords: checkin-needed
Comment 30•8 years ago
|
||
Backed out for failing browser_ext_browserAction_context.js: https://hg.mozilla.org/integration/fx-team/rev/9dac1358aaad Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b3a0f1a9ede7f50e2da02d7efd8741da9a5c5937 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=10076488&repo=fx-team
Flags: needinfo?(mwein)
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/10-11/
Assignee | ||
Comment 32•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7c5519d7eaa
Assignee | ||
Comment 33•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6480e12294df
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/11-12/
Assignee | ||
Comment 35•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab574a23e13
Assignee | ||
Comment 36•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91f2e092944a
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/12-13/
Attachment #8754595 -
Attachment description: Bug 1270742 - Add support for default_icon in chrome.pageAction → Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8754595 [details] Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/13-14/
Assignee | ||
Comment 39•8 years ago
|
||
This should be ready to check in now
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment 40•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/32bb090d7e62 Add support for default_icon in chrome.pageAction r=kmag
Keywords: checkin-needed
Comment 42•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/32bb090d7e62
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 43•8 years ago
|
||
I've updated the data for Firefox for Android to note which pageAction APIs are supported in version 50. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction#Browser_compatibility I've also noted that the APIs that are supported operate globally on all tabs and ignore the `tabId` parameter. For example: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/show#Browser_compatibility https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/hide#Browser_compatibility Please let me know if I've missed anything...
Flags: needinfo?(mwein)
Keywords: dev-doc-needed
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•