extension: fix issues with context menus

extension/flow-typed/webextension-polyfill.js

@@ -170,6 +170,13 @@ type browser$webNavigation = {
   onDOMContentLoaded       : browser$WebNavigationX,
 }
 
+type browser$contextMenus = {
+  removeAll(callback?: () => void): void,
+  onClicked: {
+    ...chrome$Event,
+    addListener(callback: $contextMenus$OnClick): void
+  }
+}
 
 declare var browser: {
   bookmarks    : browser$bookmarks,
@@ -180,6 +187,7 @@ declare var browser: {
   scripting    : browser$scripting,
   runtime      : browser$runtime,
   webNavigation: browser$webNavigation,
+  contextMenus : browser$contextMenus,
 }
 
 
@@ -192,4 +200,5 @@ declare module "webextension-polyfill" {
   declare var scripting    : browser$scripting;
   declare var runtime      : browser$runtime;
   declare var webNavigation: browser$webNavigation;
+  declare var contextMenus : browser$contextMenus;
 }

extension/src/background.js

@@ -635,6 +635,8 @@ const onMessageCallback = async (msg: any) => { // TODO not sure if should defen
         await handleOpenSearch({
             utc_timestamp_s: utc_timestamp_s.toString()
         })
+    } else if (method == Methods.OPTIONS_UPDATED) {
+        updateContextMenus(msg.options)
     } else if (method == Methods.MARK_VISITED) {
         await defensify(handleToggleMarkVisited, 'handleToggleMarkVisited')()
     } else if (method == Methods.OPEN_SEARCH) {
@@ -932,8 +934,91 @@ const TOGGLES: Array<MenuToggle> = [
     },
 ]
 
+function hasContextMenus(): boolean {
+    // need to be defensive since contextMenus API isn't available under mobile browser
+    if (browser.contextMenus == undefined) {
+        isMobile().then(mobile => {
+            if (!mobile) {
+                notifyError("error: chrome.contextMenus should be available")
+            }
+        })
+        return false
+    }
+    return true
+}
+
+
+function initContextMenus(): void {
+    if (!hasContextMenus()) {
+        return
+    }
+
+    /*
+    Normally, you'd create context menu in chrome.runtime.onInstalled, since browser remembers context menu items in between installs.
+    see https://stackoverflow.com/a/19578984/706389
+    So if you create context menus outside of onInstalled, it would try to create same menus twice and cause error.
+    However then we can't change its states (checked/unchecked in this case) in response to changing options.
+    If we add the call to update context menu outside of onInstalled, then it might have a race condition on first init, 
+    and will fail to set checks on non yet existing menu itmes.
+    So seems the easiest to just remove all menus and init them again :shrug:
+    */
+    browser.contextMenus.removeAll(() => {
+        for (const {id: id, title: title, parentId: parentId, contexts: contexts} of MENUS) {
+            chrome.contextMenus.create({
+                id: id,
+                parentId: parentId,
+                title: title,
+                contexts: contexts || DEFAULT_CONTEXTS,
+            })
+        }
+
+        for (const {id: id, title: title, parentId: parentId, contexts: contexts} of TOGGLES) {
+            chrome.contextMenus.create({
+                id: id,
+                parentId: parentId,
+                title: title,
+                contexts: contexts || DEFAULT_CONTEXTS,
+                type: 'checkbox',
+            })
+        }
+        getOptions().then((opts) => updateContextMenus(opts))
+    })
+
+    // need to keep these callbacks here, since onInstalled above isn't called when background page resumes
+    const onMenuClickedCallback = defensify(async (info: MenuInfo, tab: chrome$Tab) => {
+        const mid = info.menuItemId
+        for (const m of [...MENUS, ...TOGGLES]) {
+            if (mid == m.id) {
+                const cb = m.callback
+                if (cb != null) {
+                    await cb(info, tab)
+                }
+                break
+            }
+        }
+    }, 'onMenuClicked')
 
-function initBackground() {
+    // seems that it's best to keep onClicked listenere here (instead of inside removeAll)
+    // otherwise it's not working in firefox
+    // $FlowFixMe[incompatible-call] flow complains presumably because of defensify
+    browser.contextMenus.onClicked.addListener(onMenuClickedCallback)
+}
+
+
+function updateContextMenus(opts: Options): void {
+    if (!hasContextMenus()) {
+        return
+    }
+    for (const {id: id, checker: checker} of TOGGLES) {
+        chrome.contextMenus.update(
+            id,
+            {checked: checker(opts)},
+        )
+    }
+}
+
+
+function initBackground(): void {
     // NOTE: callback registering needs to be synchronous
     // otherwise doesn't work well with background page suspension
 
@@ -954,58 +1039,7 @@ function initBackground() {
         })
     }
 
-    // not sure why but context menus need to be created in onInstalled?
-    // https://stackoverflow.com/a/19578984/706389
-    chrome.runtime.onInstalled.addListener(() => {
-        // need to be defensive since contextMenus API isn't available under mobile browser
-        if (chrome.contextMenus == undefined) {
-            isMobile().then(mobile => {
-                if (!mobile) {
-                    notifyError("error: chrome.contextMenus should be available")
-                }
-            })
-            return
-        }
-        for (const {id: id, title: title, parentId: parentId, contexts: contexts} of MENUS) {
-            chrome.contextMenus.create({
-                id: id,
-                parentId: parentId,
-                title: title,
-                contexts: contexts || DEFAULT_CONTEXTS,
-            })
-        }
-        // TODO crap -- we need to refresh these menus when options update??
-        // it's broken in prod though so can live without it for a bit
-        // also cover with a test
-        getOptions().then((opts) => {
-            for (const {id: id, title: title, parentId: parentId, checker: checker, contexts: contexts} of TOGGLES) {
-                chrome.contextMenus.create({
-                    id: id,
-                    parentId: parentId,
-                    title: title,
-                    contexts: contexts || DEFAULT_CONTEXTS,
-                    type: 'checkbox',
-                    checked: checker(opts),
-                })
-            }
-        })
-
-        const onMenuClickedCallback = defensify(async (info: MenuInfo, tab: chrome$Tab) => {
-            const mid = info.menuItemId
-            for (const m of [...MENUS, ...TOGGLES]) {
-                if (mid == m.id) {
-                    const cb = m.callback
-                    if (cb != null) {
-                        await cb(info, tab)
-                    }
-                    break
-                }
-            }
-        }, 'onMenuClicked');
-
-        //  $FlowFixMe // err, complains at Promise but nevertheless works
-        chrome.contextMenus.onClicked.addListener(onMenuClickedCallback)
-    })
+    initContextMenus()
 }
 
 
@@ -1026,9 +1060,6 @@ initBackground()
 
 
 // for debugging
-/*
-browser.runtime.onSuspend.addListener(() => {
-    console.error("SUSPENDING BACKGROUND PAGE!!")
-    notifyError("SUSPENDING BACKGROUND!!")
-})
-*/
+// browser.runtime.onSuspend.addListener(() => {
+//     notifyError("SUSPENDING BACKGROUND!!")
+// })

extension/src/common.js

@@ -197,6 +197,7 @@ export const Methods = {
     MARK_VISITED        : 'markVisited',
     OPEN_SEARCH         : 'openSearch',
     SIDEBAR_TOGGLE      : 'sidebarToggle',
+    OPTIONS_UPDATED     : 'optionsUpdated',
     // TODO not used
     SIDEBAR_SHOW        : 'sidebarShow',
     ZAPPER_EXCLUDELIST  : 'zapperExcludelist',

extension/src/options.js

@@ -1,5 +1,5 @@
 /* @flow */
-import {getBrowser} from './common'
+import {getBrowser, Methods} from './common'
 
 /* NOTE: options can only be renamed in-between store releases */
 /* maybe later will bother with migrations for consistent naming, but that would require tests first */
@@ -284,20 +284,29 @@ export async function getOptions(): Promise<Options> {
 }
 
 
+async function notifyOptionsUpdated(): Promise<void> {
+    const options = await getOptions() // meh
+    browser.runtime.sendMessage({method: Methods.OPTIONS_UPDATED, options: options})
+}
+
+
 // TODO would be nice to accept a substructure of Options??
 export async function setOptions(opts: StoredOptions) {
     const os = await optSync()
     await os.set(opts)
+    notifyOptionsUpdated()
 }
 
 export async function setOption(opt: Opt1 | Opt2): Promise<void> {
     const os = await optSync()
     await os.set(opt)
+    notifyOptionsUpdated()
 }
 
 export async function resetOptions(): Promise<void> {
     const os = await optSync()
     await os.setAll({})
+    notifyOptionsUpdated()
 }
 
 type ToggleOptionRes = () => Promise<void>