Skip to content
This repository has been archived by the owner on Jan 4, 2019. It is now read-only.

Add chrome.contextMenus.create and chrome.contextMenus.removeAll #82

Merged
merged 1 commit into from
Oct 28, 2016

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Oct 26, 2016

darkdh added a commit to darkdh/browser-laptop that referenced this pull request Oct 26, 2016
darkdh added a commit to darkdh/browser-laptop that referenced this pull request Oct 27, 2016
@bbondy
Copy link
Member

bbondy commented Oct 28, 2016

@bridiver pls review

@@ -30,6 +30,7 @@ process.on('EXTENSION_READY_INTERNAL', (installInfo) => {
}
browserActions.setDefaultBrowserAction(installInfo.id, details)
addBackgroundPageEvent(installInfo.id, 'chrome-browser-action-clicked')
addBackgroundPageEvent(installInfo.id, 'chrome-context-menus-clicked')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will only add the context menu events if the extension also has a browser action. Just moving it out of the if should be fine

}
},
removeAll: function (cb) {
var responseId = ++id
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add the extension id to this as well? I know none of the others have it, but we should probably add for all of them because there is a chance of conflict now that we have more than one extension running at the same time

},
create: function (properties, cb) {
console.warn('chrome.contentMenus.create is not supported yet')
cb && cb()
var responseId = ++id
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@darkdh
Copy link
Member Author

darkdh commented Oct 28, 2016

feedback addressed in 765646166e1f2e2fa50b7ea90b890b6c2efc0d0a

@bridiver bridiver merged commit 749e427 into brave:master Oct 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants