Issue 5977 - Set popup programmatically on Firefox

lib/options.js

Index: lib/options.js
===================================================================
--- a/lib/options.js
+++ b/lib/options.js
@@ -119,16 +119,27 @@
       browser.tabs.create({url: optionsUrl}, () =>
       {
         returnShowOptionsCall(optionsTab, callback);
       });
     }
   });
 };
 
+// On Firefox we set the popup programmatically here rather than via
+// manifest.json so we can open the options page directly on Android.
+if ("getPopup" in browser.browserAction)

[kzar, 2017/11/04 16:17:26]

The idea is that browser.browserAction.getPopup doesn't exist on Firefox mobile
right? Maybe improve the comment to hint at that if so, like "On Firefox
(desktop)..."?

[Wladimir Palant, 2017/11/04 19:43:26]

I think that the idea is rather introducing a delay that will allow
info.application to initialize.

[Manish Jethani, 2017/11/05 11:35:00]

The idea was that popup.html is set in manifest.json for all platforms except
Firefox. Instead of checking for Firefox, we check that getPopup gives us an
empty URL.

[Manish Jethani, 2017/11/05 11:35:00]

getPopup does exist on Firefox for Android v57, but it doesn't exist on Edge for
example. I was hoping to restrict this programmatic setting of the popup only to
Firefox (desktop).

[kzar, 2017/11/06 09:36:51]

Right gotya.

+{
+  browser.browserAction.getPopup({}).then(url =>
+  {
+    if (!url && info.application != "fennec")

[Manish Jethani, 2017/11/04 11:27:17]

Note that we can't check for "fennec" in the initial if condition because
info.application is "unknown" at that point.

[Wladimir Palant, 2017/11/04 19:43:26]

Can't we call getBrowserInfo explicitly here then? Using getPopup seems to be a
hack to me, also setting pop-up programmatically in all the various
applications. Something like:

  browser.runtime.getBrowserInfo().then(info =>
  {
    if (fennec)
      browser.browserAction.setPopup({popup: ""});
  });

[kzar, 2017/11/04 21:50:31]

Oh yea, cool idea. By clearing the popup URL for Firefox mobile, instead of
setting it for other platforms, we'd avoid having to mess with buildtools.

[Manish Jethani, 2017/11/05 11:35:00]

This does not work unfortunately due to a bug that I've reported:

https://bugzilla.mozilla.org/show_bug.cgi?id=1414613

[kzar, 2017/11/06 09:36:51]

Damn, that sucks. I guess we could at least get rid of some of the checks. If
the platform's Firefox we already know the URL won't have been set (untested):

// We need to clear the popup URL for Firefox Android in order for the options
// page to open instead of the bubble. Unfortunately there's a bug[1] which
// prevents us from doing that, so we must avoid setting the URL on Firefox from
// the manifest at all, instead setting it here only for non-mobile.
// [1] - https://bugzilla.mozilla.org/show_bug.cgi?id=1414613
if ("getBrowserInfo" in browser.runtime)
{
  browser.runtime.getBrowserInfo().then(info =>
  {
    if (info.name.toLowerCase() != "fennec")
      browser.browserAction.setPopup({popup: "popup.html"});
  });
}

[Manish Jethani, 2017/11/06 14:15:09]

Done.

+      browser.browserAction.setPopup({popup: "popup.html"});
+  });
+}
+
 // On Firefox for Android, open the options page directly when the browser
 // action is clicked.
 browser.browserAction.onClicked.addListener(() =>
 {
   browser.tabs.query({active: true, lastFocusedWindow: true}, ([tab]) =>
   {
     let currentPage = new ext.Page(tab);