Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove pageAction.show and .hide from API metadata #13

Closed
wants to merge 1 commit into from

Conversation

m4tx
Copy link

@m4tx m4tx commented Dec 27, 2016

Their presence there caused them to be wrapped as asynchronous functions. It caused Promise's resolve function to be passed to them, which generated error (Invocation of form pageAction.show(function) doesn't match definition pageAction.show(integer tabId)).

This is a response to PR #9.

Their presence there caused them to be wrapped as asynchronous functions. It caused Promise's resolve function to be passed to them, which generated error (Invocation of form pageAction.show(function) doesn't match definition pageAction.show(integer tabId)).

This is a response to PR mozilla#9.
@Rob--W
Copy link
Member

Rob--W commented Dec 27, 2016

FYI: The fact that some pageAction/browserAction methods in Chrome do not take callbacks is a bug in Chrome. I expect Chrome to eventually support callbacks for these APIs:

@kmaglione
Copy link
Contributor

Sorry for the delay.

These methods need to stay in the polyfill, but in Chrome they should just return an immediately resolved Promise rather than passing a callback.

@Rob--W
Copy link
Member

Rob--W commented Feb 3, 2017

These methods need to stay in the polyfill, but in Chrome they should just return an immediately resolved Promise rather than passing a callback.

As said before, I expect the callback to be implemented eventually, so doing something like if (browser is Chrome) ... wouldn't fly well. The logic should be something like this:

new Promise((resolve, reject) => {
    try {
        chrome.pageAction.show(tabId, function() {
            if (chrome.runtime.lastError) reject(chrome.runtime.lastError);
            else resolve();
        });
    } catch (e) { // Error. Callback argument is probably not supported.
        chrome.pageAction.show(tabId);
        resolve();
    }
});

@rpl
Copy link
Member

rpl commented Apr 24, 2017

Closing as state: invalid based on comment #13 (comment) (we are going to fix this issue by returning a resolved promise based on an updated "api-metadata.json" file)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants