Issue 5977 - Set popup programmatically on Firefox

Issue 5977 - Set popup programmatically on Firefox

[Manish Jethani, 2017-11-04 11:27:18]

Patch Set 1

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:133: if (!url && info.application != "fennec")
Note that we can't check for "fennec" in the initial if condition because
info.application is "unknown" at that point.

https://codereview.adblockplus.org/29597555/diff/29597556/metadata.gecko
File metadata.gecko (right):

https://codereview.adblockplus.org/29597555/diff/29597556/metadata.gecko#newc...
metadata.gecko:8: browserAction -= popup.html
This also needs a change in buildtools. If this approach looks good, I can file
an issue and upload a separate patch for it.

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

LGTM

Please could you update the issue description to more accurately reflect the
problem and our approach to solve? Also please could you file an issue for the
buildtools change.

I don't know how wise pushing all this before the release is, but let's see what
Wladimir thinks.

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:129: if ("getPopup" in browser.browserAction)
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)..."?

https://codereview.adblockplus.org/29597555/diff/29597556/metadata.gecko
File metadata.gecko (right):

https://codereview.adblockplus.org/29597555/diff/29597556/metadata.gecko#newc...
metadata.gecko:8: browserAction -= popup.html
On 2017/11/04 11:27:18, Manish Jethani wrote:
> This also needs a change in buildtools. If this approach looks good, I can
file
> an issue and upload a separate patch for it.

I think your approach is OK.

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

For the change here, it is clearly too late for the release. Maybe the solution
I suggested, not really sure about that either however.

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:129: if ("getPopup" in browser.browserAction)
On 2017/11/04 16:17:26, kzar wrote:
> 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)..."?

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

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:133: if (!url && info.application != "fennec")
On 2017/11/04 11:27:17, Manish Jethani wrote:
> Note that we can't check for "fennec" in the initial if condition because
> info.application is "unknown" at that point.

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:32]

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:133: if (!url && info.application != "fennec")
On 2017/11/04 19:43:26, Wladimir Palant wrote:
> On 2017/11/04 11:27:17, Manish Jethani wrote:
> > Note that we can't check for "fennec" in the initial if condition because
> > info.application is "unknown" at that point.
> 
> 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: ""});
>   });

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:01]

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:129: if ("getPopup" in browser.browserAction)
On 2017/11/04 16:17:26, kzar wrote:
> 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)..."?

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).

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:129: if ("getPopup" in browser.browserAction)
On 2017/11/04 19:43:26, Wladimir Palant wrote:
> On 2017/11/04 16:17:26, kzar wrote:
> > 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)..."?
> 
> I think that the idea is rather introducing a delay that will allow
> info.application to initialize.

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.

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:133: if (!url && info.application != "fennec")
On 2017/11/04 19:43:26, Wladimir Palant wrote:
> On 2017/11/04 11:27:17, Manish Jethani wrote:
> > Note that we can't check for "fennec" in the initial if condition because
> > info.application is "unknown" at that point.
> 
> 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: ""});
>   });

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]

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:129: if ("getPopup" in browser.browserAction)
On 2017/11/05 11:35:00, Manish Jethani wrote:
> On 2017/11/04 16:17:26, kzar wrote:
> > 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)..."?
> 
> 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).

Right gotya.

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:133: if (!url && info.application != "fennec")
On 2017/11/05 11:35:00, Manish Jethani wrote:
> On 2017/11/04 19:43:26, Wladimir Palant wrote:
> > On 2017/11/04 11:27:17, Manish Jethani wrote:
> > > Note that we can't check for "fennec" in the initial if condition because
> > > info.application is "unknown" at that point.
> > 
> > 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: ""});
> >   });
> 
> This does not work unfortunately due to a bug that I've reported:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1414613

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:10]

Patch Set 2: Check for Firefox explicitly

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29597556/lib/options.js#newc...
lib/options.js:133: if (!url && info.application != "fennec")
On 2017/11/06 09:36:51, kzar wrote:
> On 2017/11/05 11:35:00, Manish Jethani wrote:
> > On 2017/11/04 19:43:26, Wladimir Palant wrote:
> > > On 2017/11/04 11:27:17, Manish Jethani wrote:
> > > > Note that we can't check for "fennec" in the initial if condition
because
> > > > info.application is "unknown" at that point.
> > > 
> > > 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: ""});
> > >   });
> > 
> > This does not work unfortunately due to a bug that I've reported:
> > 
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1414613
> 
> 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"});
>   });
> }

Done.

https://codereview.adblockplus.org/29597555/diff/29599592/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29599592/lib/options.js#newc...
lib/options.js:133: if (info.application == "firefox")
Firefox 50 doesn't have runtime.getBrowserInfo so info.application will be set
to "firefox" already.

https://codereview.adblockplus.org/29597555/diff/29599592/lib/options.js#newc...
lib/options.js:137: else if (info.platform == "gecko")
We can restrict this to Gecko-based browsers so we don't have to do any checks
(e.g. checking for runtime.getBrowserInfo, which is not available on other
platforms).

[Manish Jethani, 2017-11-06 14:17:48]

Patch Set 3: Revert changes to polyfill.js

Actually there's no need to polyfill runtime.getBrowserInfo here since we're
explicitly checking for Gecko.

[kzar, 2017-11-06 14:28:38]

LGTM

[Wladimir Palant, 2017-11-06 15:04:31]

https://codereview.adblockplus.org/29597555/diff/29599596/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29599596/lib/options.js#newc...
lib/options.js:144: }
If we are choosing that approach, I wonder whether we should still have special
logic for Gecko. It could be:

  if (typeof browser.runtime.getBrowserInfo == "function")
    set popup conditionally, not for fennec
  else
    set popup unconditionally

[Manish Jethani, 2017-11-13 10:42:14]

Patch Set 4: Check only for runtime.getBrowserInfo

https://codereview.adblockplus.org/29597555/diff/29599596/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29599596/lib/options.js#newc...
lib/options.js:144: }
On 2017/11/06 15:04:31, Wladimir Palant wrote:
> If we are choosing that approach, I wonder whether we should still have
special
> logic for Gecko. It could be:
> 
>   if (typeof browser.runtime.getBrowserInfo == "function")
>     set popup conditionally, not for fennec
>   else
>     set popup unconditionally

I was hoping to restrict this to Gecko only to avoid any unintended side
effects, but I guess it's OK to make it generic. I've made the change and tested
it on Firefox (desktop and mobile) and Chrome now and it works OK.

Done.

https://codereview.adblockplus.org/29597555/diff/29599596/metadata.gecko
File metadata.gecko (right):

https://codereview.adblockplus.org/29597555/diff/29599596/metadata.gecko#newc...
metadata.gecko:7: version = 2.99.0
This is just rebased.

https://codereview.adblockplus.org/29597555/diff/29605749/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29597555/diff/29605749/lib/options.js#newc...
lib/options.js:133: if ("getBrowserInfo" in browser.runtime)
Note that I'm doing '"getBrowserInfo" in browser.runtime' here instead of
'typeof browser.runtime.getBrowserInfo == "function"' as you suggested because
that's how we check for APIs generally, I think at some point we agreed to
maintain that consistency.

[Manish Jethani, 2017-11-13 10:45:38]

Patch Set 5: Wrap runtime.getBrowserInfo

Well since now this is no longer Gecko-only we need to wrap
runtime.getBrowserInfo, just in case Chrome or Edge implements it with a
callback instead of a promise.

[kzar, 2017-11-13 12:58:44]

LGTM, please could you update the issue description for #5977 and file the
buildtools issue?

[Manish Jethani, 2017-11-14 15:47:20]

On 2017/11/13 12:58:44, kzar wrote:
> LGTM, please could you update the issue description for #5977 and file the
> buildtools issue?

About the description for #5977, it's filed as a defect and the description
pretty much says how to reproduce the issue, the expected behavior, and so on.
What should it be changed to?

[kzar, 2017-11-14 15:56:59]

On 2017/11/14 15:47:20, Manish Jethani wrote:
> On 2017/11/13 12:58:44, kzar wrote:
> > LGTM, please could you update the issue description for #5977 and file the
> > buildtools issue?
> 
> About the description for #5977, it's filed as a defect and the description
> pretty much says how to reproduce the issue, the expected behavior, and so on.
> What should it be changed to?

Oh, Wladimir already did it[1]. Sorry I didn't realise.

[1] - https://issues.adblockplus.org/ticket/5977?action=diff&version=2

[Manish Jethani, 2017-11-17 15:09:04]

Patch Set 6: Update buildtools dependency

[kzar, 2017-11-18 12:27:51]

On 2017/11/17 15:09:04, Manish Jethani wrote:
> Patch Set 6: Update buildtools dependency

Please could you update the issue description since this is now a dependency
update too? The testers need to know what functionality to test, since it might
not be obvious what code changes are incidentally included in the update.

[Manish Jethani, 2017-11-20 16:09:02]

On 2017/11/18 12:27:51, kzar wrote:
> On 2017/11/17 15:09:04, Manish Jethani wrote:
> > Patch Set 6: Update buildtools dependency
> 
> Please could you update the issue description since this is now a dependency
> update too? The testers need to know what functionality to test, since it
might
> not be obvious what code changes are incidentally included in the update.

OK, I've added a "What to change" section now that describes the change
including the buildtools dependency update.

[kzar, 2017-11-20 16:16:50]

On 2017/11/20 16:09:02, Manish Jethani wrote:
> On 2017/11/18 12:27:51, kzar wrote:
> > On 2017/11/17 15:09:04, Manish Jethani wrote:
> > > Patch Set 6: Update buildtools dependency
> > 
> > Please could you update the issue description since this is now a dependency
> > update too? The testers need to know what functionality to test, since it
> might
> > not be obvious what code changes are incidentally included in the update.
> 
> OK, I've added a "What to change" section now that describes the change
> including the buildtools dependency update.

Thanks, but for dependency update issues you also need to look through all of
the commits included by the dependency update and check what code was changed in
those commits. Any commits which changed relevant code need to be mentioned in
the issue description, and the "Hints for Testers" section needs to list all the
functionality which those code changes touched. That way the QA team have a
chance to test any code which was incidentally changed by the dependency update,
and to view the related issues for more context where necessary. Note: If no
other relevant changes exist it's nice to mention that, so it makes it obvious
that it was at least checked.

[Manish Jethani, 2017-11-21 07:39:12]

On 2017/11/20 16:16:50, kzar wrote:
> On 2017/11/20 16:09:02, Manish Jethani wrote:
> > On 2017/11/18 12:27:51, kzar wrote:
> > > On 2017/11/17 15:09:04, Manish Jethani wrote:
> > > > Patch Set 6: Update buildtools dependency
> > > 
> > > Please could you update the issue description since this is now a
dependency
> > > update too? The testers need to know what functionality to test, since it
> > might
> > > not be obvious what code changes are incidentally included in the update.
> > 
> > OK, I've added a "What to change" section now that describes the change
> > including the buildtools dependency update.
> 
> Thanks, but for dependency update issues you also need to look through all of
> the commits included by the dependency update and check what code was changed
in
> those commits. Any commits which changed relevant code need to be mentioned in
> the issue description, and the "Hints for Testers" section needs to list all
the
> functionality which those code changes touched. That way the QA team have a
> chance to test any code which was incidentally changed by the dependency
update,
> and to view the related issues for more context where necessary. Note: If no
> other relevant changes exist it's nice to mention that, so it makes it obvious
> that it was at least checked.

Thanks, I've added a "Hints for testers" section now. Let me know if it looks
OK.

[kzar, 2017-11-21 12:39:40]

On 2017/11/21 07:39:12, Manish Jethani wrote:
> Thanks, I've added a "Hints for testers" section now. Let me know if it looks
> OK.

Well I've tweaked it a little but it looks good now. Sorry to be pedantic about
it, just trying to avoid regressions sneaking into the next release... we have
had it happen before this way! By the way Tristan is working on a tool called
depup which helps with the admin involved doing dependency updates, you might
find it handy https://codereview.adblockplus.org/29599579/
https://github.com/TrLucas/depup

LGTM

[Manish Jethani, 2017-11-21 12:47:24]

On 2017/11/21 12:39:40, kzar wrote:
> On 2017/11/21 07:39:12, Manish Jethani wrote:
> > Thanks, I've added a "Hints for testers" section now. Let me know if it
looks
> > OK.
> 
> Well I've tweaked it a little but it looks good now. Sorry to be pedantic
about
> it, just trying to avoid regressions sneaking into the next release... we have
> had it happen before this way! By the way Tristan is working on a tool called
> depup which helps with the admin involved doing dependency updates, you might
> find it handy https://codereview.adblockplus.org/29599579/
> https://github.com/TrLucas/depup

Cool, thanks!

> LGTM

https://hg.adblockplus.org/adblockpluschrome/rev/e9df991b7542