Noissue - Fixed browserAction in Firefox on Android

Noissue - Fixed browserAction in Firefox on Android

[Sebastian Noack, 2018-05-23 15:07:01]

This is a regression introduced by
https://hg.adblockplus.org/adblockpluschrome/rev/8a5705415bdb. Apparently,
setPopup({popup: ""}) doesn't have any effect in Firefox on Android.

So there isn't a way around hard-coding "popup.html". However, the way I changed
the code now, the hard-coded popup filename is only picked up on Firefox, hence
won't affect AdBlock.

[kzar, 2018-05-24 12:09:01]

https://codereview.adblockplus.org/29788558/diff/29788559/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29788558/diff/29788559/lib/options.js#newc...
lib/options.js:180: // We need to clear the popup URL on Firefox for Android in
order for the
This comment is pretty good at explaining how come we have to avoid setting the
popup URL for Android Firefox, but so far it doesn't mention why we take care to
set it in the metadata for other platforms like Chrome (so that AdBlock can
override the setting in its metadata). Perhaps you should mention that here.

[kzar, 2018-05-24 12:15:27]

Forgot to say, otherwise this LGTM!

[Sebastian Noack, 2018-05-24 12:37:27]

https://codereview.adblockplus.org/29788558/diff/29788559/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29788558/diff/29788559/lib/options.js#newc...
lib/options.js:180: // We need to clear the popup URL on Firefox for Android in
order for the
On 2018/05/24 12:09:01, kzar wrote:
> This comment is pretty good at explaining how come we have to avoid setting
the
> popup URL for Android Firefox, but so far it doesn't mention why we take care
to
> set it in the metadata for other platforms like Chrome (so that AdBlock can
> override the setting in its metadata). Perhaps you should mention that here.

Well, setting it in the metadata.*/manifest.json file is the canonical way. Not
sure, if its worth to explicitly mention AdBlock here (or even appropriate, as
we never mention AdBlock anywhere else in our code base).

[kzar, 2018-05-24 12:58:07]

https://codereview.adblockplus.org/29788558/diff/29788559/lib/options.js
File lib/options.js (right):

https://codereview.adblockplus.org/29788558/diff/29788559/lib/options.js#newc...
lib/options.js:180: // We need to clear the popup URL on Firefox for Android in
order for the
On 2018/05/24 12:37:27, Sebastian Noack wrote:
> On 2018/05/24 12:09:01, kzar wrote:
> > This comment is pretty good at explaining how come we have to avoid setting
> the
> > popup URL for Android Firefox, but so far it doesn't mention why we take
care
> to
> > set it in the metadata for other platforms like Chrome (so that AdBlock can
> > override the setting in its metadata). Perhaps you should mention that here.
> 
> Well, setting it in the metadata.*/manifest.json file is the canonical way.
Not
> sure, if its worth to explicitly mention AdBlock here (or even appropriate, as
> we never mention AdBlock anywhere else in our code base).

Yea fair enough. I drafted a possible improvement but deleted it again, in
fairness the existing comment is pretty good.