Fix: Support pageAction show/hide wrapping #59

rpl · 2017-09-26T15:08:52Z

This PR contains the changes to the api-metadata.json from #9 and it also applies the changes needed into the polyfill wrappers to be able to customize the wrapper behavior for the API methods that returns a Promise on Firefox but do not accept a callback on Chrome (which is the case for pageAction.show and hide).

rpl · 2017-09-26T15:12:16Z

@Rob--W do you mind to take a look at this PR?

similarly to #57 it is going to change the api-metadata.json but it also needs some additional changes in the polyfill wrappers, to be able to customize the behavior for the kind on chrome incompatibility that pageAction.show/hide presents.

Rob--W · 2017-09-28T09:40:25Z

This doesn't look like a forward-compatible change to me.

Yes, it would fix the current error.
However, if Chrome were to support the callback parameter (which I expect to happen - #13 (comment), I've wanted to submit a patch myself, but that's near the bottom of my priority list), then the polyfill will cause an error again.

How about trying to call with callback, and if a synchronous (schema) error about unsupported callbacks occurs, invoke the method without callback?

try { chrome.browserAction.setTitle(1, function(){}); } catch (e) { console.log(e.message); }

gives the following result (tested Chrome 34 - 61:

Invocation of form browserAction.setTitle(integer, function) doesn't match definition browserAction.setTitle(object details)

and when --native-crx-bindings=1 is set (tested with Chrome 61) (Chrome is working on replacing JS-implemented extension bindings with C++):

Error in invocation of browserAction.setTitle(object details): Too many arguments.

PS. Why does browserAction.setTitle not appear in api-metadata.json?

Rob--W · 2017-09-28T14:39:37Z

src/browser-polyfill.js

+              console.warn(`${name} API method doesn't seem to support the callback parameter, ` +
+                           "falling back to call it without a callback: ", cbError);
+
+              // Update the API method metadata, so that the next API call will not try to


Move this after line 170, to make sure that the API call is actually valid before considering the API callback-less.

👍 great call!

Rob--W · 2017-09-28T14:42:32Z

The browserAction API map is missing entries like browser.runtime.enable.

I guess that the JSON file is manually edited. Maybe it helps if you write a quick script to load the JSON files from mozilla-central and compare it with the existing definitions to see if anything is missing.

rpl · 2017-09-29T16:59:19Z

@Rob--W I've updated this PR based on your last round of review comments and updated the test cases accordingly, let me know how it looks to you.

Thanks again for you help!

Rob--W · 2017-09-29T18:09:01Z

src/browser-polyfill.js

+          function callAPIWithNoCallback() {
+            try {
+              target[name](...args);
+              if (chrome.runtime.lastError) {


This chrome.runtime.lastError check seems misplaced. The target[name](...args) call is asynchronous, so the error will never be set to that.

Worse, with the current code, if someone tries to use call e.g. browser.pageAction.show inside a function where lastError is set, then the promise will always be rejected.

👍 it was already supposed to be removed and I forgot, thanks for pointing it out.

Rob--W · 2017-09-29T18:12:16Z

src/browser-polyfill.js

+            try {
+              target[name](...args, makeCallback({resolve, reject}, metadata));
+            } catch (cbError) {
+              console.warn(`${name} API method doesn't seem to support the callback parameter, ` +


Under normal circumstances, this message will be displayed once.

This message will be displayed more than once if the extension passes the wrong arguments. Do you consider that to be OK? (personally I think that it is fine, since the code is simpler in this way; if you want to force the message to be displayed once, then it needs to be moved inside the else block above.

Why do we need log spam in the first place?

I thought to emit at least a warning log mostly because it is not completely impossible that this runtime discovery can be fooled by some chrome behaviors (maybe a future chrome behavior) that the polyfill is not expecting, and it could completely hide the real error.

Emitting the warning every time we are actually falling back between the "with callback" to the "no callback" mode is fine from my current point of view, in the common scenarios it should only be produced once, and if it is produced more than once, then it could be a signal of something different between the behavior that it expect and the actual one.

Rob--W · 2017-09-29T18:17:17Z

src/browser-polyfill.js

+              target[name](...args);
+              if (chrome.runtime.lastError) {
+                // NOTE: Ideally this should rejects if there are runtime issues with
+                // the API call on Chrome, unfortunately currently when pageAction.show/hide


This whole comment block will go probably go away because of my above comment. But if you decide to re-instate the comments somewhere, generalize the affected APIs, e.g. "APIs such as pageAction.show" because pageAction.show/hide are not the only affected APIs.

rpl · 2017-09-29T19:42:30Z

@Rob--W suggested changes applied in the updated patch

Rob--W

Looks good. I have posted one optional suggestion. The PR is fine either way.

Rob--W · 2017-09-30T08:55:44Z

src/browser-polyfill.js

+              // Catch API parameters validation errors and rejects the promise.
+              reject(error);
+            }
+          }


Now the callAPIWithNoCallback logic is very simple, it can be inlined below, without try-catch (because you are in the closure of a Promise).

The second callAPIWithNoCallback call does not require the metadata to be updated, so it can simply be:

target[name](...args); resolve();

… Mozilla's webextension-polyfill. Had to change polyfill code with a temporary fix for a PageAction show bug: mozilla/webextension-polyfill#59.

ermik · 2018-02-06T15:22:23Z

Expected at most 0 arguments for show(), got 1 is kind of killing me.

Treora · 2018-03-13T10:40:17Z

Would be great if this can be merged indeed, I stumbled upon #24 as well. Is this PR still held back by anything?

I notice one unprocessed review comment; I made a PR to this branch that inlines the function as indicated.

…ir metadata

…e pageAction.show/hide wrappers behavior

According to review comment https://github.com/mozilla/webextension-polyfill/pull/59/files#r142001949

… chrome.

rpl · 2018-03-13T13:26:14Z

@Treora Thanks a lot for your PR! I've rebased this branch, added your changes on top of the commit already part of this PR and added pageAction.show to the smoke tests that runs on Chrome.

I'm going to land this PR as soon as the Travis job run successfully.

… Mozilla's webextension-polyfill. Had to change polyfill code with a temporary fix for a PageAction show bug: mozilla/webextension-polyfill#59.

rpl added component: api metadata component: polyfill wrappers platform: chrome priority: P1 type: bug labels Sep 26, 2017

rpl mentioned this pull request Sep 26, 2017

pageAction.show has 1 argument #9

Closed

Rob--W reviewed Sep 28, 2017

View reviewed changes

rpl mentioned this pull request Sep 29, 2017

feat: Add support for wrapping the devtools APIs #57

Merged

rpl force-pushed the fix/pageAction-show-hide-wrappers branch from 65b8475 to 5882834 Compare September 29, 2017 16:46

Rob--W reviewed Sep 29, 2017

View reviewed changes

rpl force-pushed the fix/pageAction-show-hide-wrappers branch from 5882834 to dbc376b Compare September 29, 2017 19:13

Rob--W approved these changes Sep 30, 2017

View reviewed changes

Megaman and others added 4 commits March 13, 2018 13:49

fix: pageAction.show/hide should have 1 non-optional parameter in the…

d54ef9d

…ir metadata

fix: Add a new fallbackToNoCallback metadata property to customize th…

8929af3

…e pageAction.show/hide wrappers behavior

chore: Improve "fallback to no callback" wrapper.

d60baae

According to review comment https://github.com/mozilla/webextension-polyfill/pull/59/files#r142001949

test: Added smoke test for pageAction.show fallback to no callback on…

80a2fd5

… chrome.

rpl force-pushed the fix/pageAction-show-hide-wrappers branch from dbc376b to 80a2fd5 Compare March 13, 2018 13:22

rpl merged commit 0fd5086 into mozilla:master Mar 13, 2018

rpl mentioned this pull request Mar 26, 2018

All browserAction / pageAction methods support callbacks in Chrome 67+ #99

Closed

Rob--W mentioned this pull request May 31, 2018

Chrome: Error in event handler for tabs.onUpdated: Error: Expected at most 0 arguments for show(), got 1 #24

Closed

rpl deleted the fix/pageAction-show-hide-wrappers branch March 15, 2021 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Support pageAction show/hide wrapping #59

Fix: Support pageAction show/hide wrapping #59

rpl commented Sep 26, 2017

rpl commented Sep 26, 2017

Rob--W commented Sep 28, 2017

Rob--W Sep 28, 2017

rpl Sep 28, 2017

Rob--W commented Sep 28, 2017

rpl commented Sep 29, 2017

Rob--W Sep 29, 2017

rpl Sep 29, 2017

Rob--W Sep 29, 2017

rpl Sep 29, 2017

Rob--W Sep 29, 2017

rpl commented Sep 29, 2017

Rob--W left a comment

Rob--W Sep 30, 2017

ermik commented Feb 6, 2018

Treora commented Mar 13, 2018

rpl commented Mar 13, 2018 •

edited

Loading

Fix: Support pageAction show/hide wrapping #59

Fix: Support pageAction show/hide wrapping #59

Conversation

rpl commented Sep 26, 2017

rpl commented Sep 26, 2017

Rob--W commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W commented Sep 28, 2017

rpl commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpl commented Sep 29, 2017

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ermik commented Feb 6, 2018

Treora commented Mar 13, 2018

rpl commented Mar 13, 2018 • edited Loading

rpl commented Mar 13, 2018 •

edited

Loading