Issue 4670 - Add a new build type for Gecko-based WebExtensions

Issue 4670 - Add a new build type for Gecko-based WebExtensions

[Wladimir Palant, 2016-11-30 12:23:18]

Uploaded Patch Set 2, now also setting application data in the info module
correctly for Firefox.

[Wladimir Palant, 2016-11-30 13:25:10]

Uploaded Patch Set 3 - Firefox doesn't support options_page in manifest.json, it
requires the newer options_ui setting. Since options_ui is supported starting
with Chrome 40 we don't need a backwards compatibility fallback.

[Sebastian Noack, 2016-12-01 16:24:54]

https://codereview.adblockplus.org/29365583/diff/29365796/build.py
File build.py (right):

https://codereview.adblockplus.org/29365583/diff/29365796/build.py#newcode198
build.py:198: elif type == 'chrome' or type == 'gecko-webext':
How about |type in {'chrome', 'gecko-webext'}|?

https://codereview.adblockplus.org/29365583/diff/29365796/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29365583/diff/29365796/packagerChrome.py#n...
packagerChrome.py:339: file_extension = 'xpi'
So do I understand correctly, that WebExtensions for Firefox use the exact same
package format as Chrome, but merely with a different file extension?

https://codereview.adblockplus.org/29365583/diff/29365796/templates/chromeInf...
File templates/chromeInfo.js.tmpl (right):

https://codereview.adblockplus.org/29365583/diff/29365796/templates/chromeInf...
templates/chromeInfo.js.tmpl:38:
browser.runtime.getBrowserInfo().then(function(info)
This will error out when running on Chrome with tweaked user agent string.

https://codereview.adblockplus.org/29365583/diff/29365796/templates/manifest....
File templates/manifest.json.tmpl (right):

https://codereview.adblockplus.org/29365583/diff/29365796/templates/manifest....
templates/manifest.json.tmpl:33: {%- if metadata.has_section('compat') and
metadata.has_option('compat', 'gecko') %}
If I understand correctly this will result into a blank line above the comma
which could be easily avoided by using -%} when closing the jinja tag.

https://codereview.adblockplus.org/29365583/diff/29365796/templates/manifest....
templates/manifest.json.tmpl:81: "options_ui": {
Did you test whether this is supported on Microsoft Edge? It seems it is not:
https://developer.microsoft.com/en-us/microsoft-edge/platform/documentation/e...

[Wladimir Palant, 2016-12-01 21:39:32]

https://codereview.adblockplus.org/29365583/diff/29365796/build.py
File build.py (right):

https://codereview.adblockplus.org/29365583/diff/29365796/build.py#newcode198
build.py:198: elif type == 'chrome' or type == 'gecko-webext':
On 2016/12/01 16:24:54, Sebastian Noack wrote:
> How about |type in {'chrome', 'gecko-webext'}|?

Done.

https://codereview.adblockplus.org/29365583/diff/29365796/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29365583/diff/29365796/packagerChrome.py#n...
packagerChrome.py:339: file_extension = 'xpi'
On 2016/12/01 16:24:54, Sebastian Noack wrote:
> So do I understand correctly, that WebExtensions for Firefox use the exact
same
> package format as Chrome, but merely with a different file extension?

Yes, there is only a difference for signed packages - but we no longer support
signing for Firefox extensions.

https://codereview.adblockplus.org/29365583/diff/29365796/templates/chromeInf...
File templates/chromeInfo.js.tmpl (right):

https://codereview.adblockplus.org/29365583/diff/29365796/templates/chromeInf...
templates/chromeInfo.js.tmpl:38:
browser.runtime.getBrowserInfo().then(function(info)
On 2016/12/01 16:24:54, Sebastian Noack wrote:
> This will error out when running on Chrome with tweaked user agent string.

Ok, let's add some more failsafes.

https://codereview.adblockplus.org/29365583/diff/29365796/templates/manifest....
File templates/manifest.json.tmpl (right):

https://codereview.adblockplus.org/29365583/diff/29365796/templates/manifest....
templates/manifest.json.tmpl:33: {%- if metadata.has_section('compat') and
metadata.has_option('compat', 'gecko') %}
On 2016/12/01 16:24:54, Sebastian Noack wrote:
> If I understand correctly this will result into a blank line above the comma
> which could be easily avoided by using -%} when closing the jinja tag.

It doesn't matter because this JSON file is being rewritten by the packager
anyway - none of the whitespace will make it into the build.

https://codereview.adblockplus.org/29365583/diff/29365796/templates/manifest....
templates/manifest.json.tmpl:81: "options_ui": {
On 2016/12/01 16:24:54, Sebastian Noack wrote:
> Did you test whether this is supported on Microsoft Edge? It seems it is not:
>
https://developer.microsoft.com/en-us/microsoft-edge/platform/documentation/e...

You are right, it doesn't. So fallback it is.

[Sebastian Noack, 2016-12-02 11:47:51]

https://codereview.adblockplus.org/29365583/diff/29366716/templates/chromeInf...
File templates/chromeInfo.js.tmpl (right):

https://codereview.adblockplus.org/29365583/diff/29366716/templates/chromeInf...
templates/chromeInfo.js.tmpl:32: else if (app == "Gecko" && typeof browser ==
"object" &&
Sorry for the confusion, but I just realized, that since we have separate builds
for Gecko anyway, It might make sense to create a separate template without the
Chromium-specific logic.

Same goes for Edge. The reason I didn't do it in the first place there, was that
we didn't knew back then that they are going to use a different package format
anyway.

https://codereview.adblockplus.org/29365583/diff/29366716/templates/manifest....
File templates/manifest.json.tmpl (right):

https://codereview.adblockplus.org/29365583/diff/29366716/templates/manifest....
templates/manifest.json.tmpl:85: {# Edge doesn't support options_ui so we
specify options_page as well #}
Since the target platform is known here, perhaps it makes more sense to include
the corresponding key based on the platform:

  {%- if type == 'edge' %}
    "options_page": ...
  {%- else %}
    "options_ui": ...
  {%- endif %}

[Wladimir Palant, 2016-12-03 19:02:56]

https://codereview.adblockplus.org/29365583/diff/29366716/templates/chromeInf...
File templates/chromeInfo.js.tmpl (right):

https://codereview.adblockplus.org/29365583/diff/29366716/templates/chromeInf...
templates/chromeInfo.js.tmpl:32: else if (app == "Gecko" && typeof browser ==
"object" &&
On 2016/12/02 11:47:50, Sebastian Noack wrote:
> Sorry for the confusion, but I just realized, that since we have separate
builds
> for Gecko anyway, It might make sense to create a separate template without
the
> Chromium-specific logic.
> 
> Same goes for Edge. The reason I didn't do it in the first place there, was
that
> we didn't knew back then that they are going to use a different package format
> anyway.

Done.

https://codereview.adblockplus.org/29365583/diff/29366716/templates/manifest....
File templates/manifest.json.tmpl (right):

https://codereview.adblockplus.org/29365583/diff/29366716/templates/manifest....
templates/manifest.json.tmpl:85: {# Edge doesn't support options_ui so we
specify options_page as well #}
On 2016/12/02 11:47:50, Sebastian Noack wrote:
> Since the target platform is known here, perhaps it makes more sense to
include
> the corresponding key based on the platform:
> 
>   {%- if type == 'edge' %}
>     "options_page": ...
>   {%- else %}
>     "options_ui": ...
>   {%- endif %}

Done.

[Sebastian Noack, 2016-12-03 21:03:26]

LGTM