[buildtools] Issue 4578 - Make uap3:AppExtension.Id configurable for Microsoft Edge builds

[buildtools] Issue 4578 - Make uap3:AppExtension.Id configurable for Microsoft Edge builds

[Wladimir Palant, 2016-12-16 10:39:25]

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py#new...
packagerEdge.py:87: params['app_extension_id'] = '1.0' if release_build else
'EdgeExtension'
Given that this logic isn't something that is inherent to Edge extensions but
rather a mess that we (and Microsoft) created, this should be a configurable
parameter IMHO - so we don't have this hardcoded in buildtools but rather
configured in the particular project. Maybe [general] app_extension_id for
release builds and [general] app_extension_id_devbuild for development builds.

[Sebastian Noack, 2016-12-16 11:20:11]

I added Vasily as reviewer, who wrote the intial code for the Edge packager, and
is also a peer of the buildtools module.

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py#new...
packagerEdge.py:87: params['app_extension_id'] = '1.0' if release_build else
'EdgeExtension'
On 2016/12/16 10:39:25, Wladimir Palant wrote:
> Given that this logic isn't something that is inherent to Edge extensions but
> rather a mess that we (and Microsoft) created, this should be a configurable
> parameter IMHO - so we don't have this hardcoded in buildtools but rather
> configured in the particular project. Maybe [general] app_extension_id for
> release builds and [general] app_extension_id_devbuild for development builds.

I already had the same thought. But on the other hand, this way the
implementation is much simpler, and it is unlikely that we will build another
Edge extension any time soon, and if we do this can be made configurable easily
later on. No strong opinion from my side.

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
File tests/AppManifest.xml.expect (right):

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
tests/AppManifest.xml.expect:44: Id="1.0"
I wonder whether we should extend the tests to create, both a release and
development build. Vasily, what do you think?

[Sebastian Noack, 2016-12-19 15:30:20]

I just noticed, today, that Vasily is on vacation. So removing him again as
reviewer. Sorry for the confusion. So Winsley, what do you think about the
outstanding comment? As I said, I don't have a strong opinion either way.

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
File tests/AppManifest.xml.expect (right):

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
tests/AppManifest.xml.expect:44: Id="1.0"
On 2016/12/16 11:20:11, Sebastian Noack wrote:
> I wonder whether we should extend the tests to create, both a release and
> development build. Vasily, what do you think?

As discussed offline, the tests would be much less trivial when testing both
scenarios, release and development builds, as we can no longer just compare the
file contents, or if we'd do we would need two almost identical sample output
files, with a lot of duplication. On the other other hand, parsing the XML might
be preferable anyway, as the tests will be tolerant of whitespaces for example
and as another side effect we'd automatically make sure that the XML is actually
valid. However, this would arguably be out of the scope of this change.

[wspee, 2016-12-19 16:07:05]

On 2016/12/16 10:39:25, Wladimir Palant wrote:
> Given that this logic isn't something that is inherent to Edge extensions but
> rather a mess that we (and Microsoft) created, this should be a configurable
> parameter IMHO - so we don't have this hardcoded in buildtools but rather
> configured in the particular project. Maybe [general] app_extension_id for
> release builds and [general] app_extension_id_devbuild for development builds.
As far as I have understood #4578 this is a rather permanent change?! And if we
wanted to ever change it we should do it now as long as the EdgeExtension hasn't
been released?! Not really in the position to have an opinion on this, so should
I change it or not?

On 2016/12/19 15:30:20, Sebastian Noack wrote:
> As discussed offline, the tests would be much less trivial when testing both
> scenarios, release and development builds, as we can no longer just compare
the
> file contents, or if we'd do we would need two almost identical sample output
> files, with a lot of duplication. On the other other hand, parsing the XML
might
> be preferable anyway, as the tests will be tolerant of whitespaces for example
> and as another side effect we'd automatically make sure that the XML is
actually
> valid. However, this would arguably be out of the scope of this change.
IMHO sooner or later the string comparison approach will stop working anyways
(it does most of the time with things like this) so we should just change it
while
we are at it?! So I would say I go and extend the test case for development
builds
and use and xml parser instead?!

[Sebastian Noack, 2016-12-19 17:19:52]

Please always reply inline to inline comments, i.e. go to the patch the comment
you are replying on was created for, and use the "Reply" link in the existing
thread, embedded in the diff. Otherwise, it is hard to keep an overview of all
simultaneous discussions and to associate them with their related code.

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py#new...
packagerEdge.py:87: params['app_extension_id'] = '1.0' if release_build else
'EdgeExtension'
On 2016/12/19 16:07:05, wspee wrote:
> On 2016/12/16 10:39:25, Wladimir Palant wrote:
> > Given that this logic isn't something that is inherent to Edge extensions
but
> > rather a mess that we (and Microsoft) created, this should be a configurable
> > parameter IMHO - so we don't have this hardcoded in buildtools but rather
> > configured in the particular project. Maybe [general] app_extension_id for
> > release builds and [general] app_extension_id_devbuild for development
builds.
> As far as I have understood #4578 this is a rather permanent change?! And if
we
> wanted to ever change it we should do it now as long as the EdgeExtension
hasn't
> been released?! Not really in the position to have an opinion on this, so
should
> I change it or not?

Yes, it is permanent for Adblock Plus for Microsoft Edge, which has already been
released, i.e. published on the Windows Store, though labelled as beta, for now.
But once we leave beta it will still be the same item. So there is no way to get
rid of this inconsistency anymore.

However, this issue is specific to the Adblock Plus release version (and the few
other Microsoft Edge extensions that have been on the Windows Store from the
beginning). And if we should ever release another extension for Microsoft Edge,
separate from Adblock Plus, we might use the same code for packaging it, but
then the Id must be set to "EdgeExtension" also for release builds. At that
point it would have to be configurable per extension.

But this would add quite a bit unnecessary complexity and configuration
boilerplate for the time being, if this code is even ever used to build any
other extension for Microsoft Edge. I don't have a strong opinion here.

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
File tests/AppManifest.xml.expect (right):

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
tests/AppManifest.xml.expect:44: Id="1.0"
> On 2016/12/19 15:30:20, Sebastian Noack wrote:
> > As discussed offline, the tests would be much less trivial when testing both
> > scenarios, release and development builds, as we can no longer just compare
> the
> > file contents, or if we'd do we would need two almost identical sample
output
> > files, with a lot of duplication. On the other other hand, parsing the XML
> might
> > be preferable anyway, as the tests will be tolerant of whitespaces for
example
> > and as another side effect we'd automatically make sure that the XML is
> actually
> > valid. However, this would arguably be out of the scope of this change.
> IMHO sooner or later the string comparison approach will stop working anyways
> (it does most of the time with things like this) so we should just change it
> while
> we are at it?! So I would say I go and extend the test case for development
> builds
> and use and xml parser instead?!

Fine with me. I just got the impression, when we talked in person, that you were
not convinced of this approach (neither was I). So I wanted to avoid blocking
this change just because of a controversial and slightly unrelated refactoring.
But I won't hold you back either. ;)

[wspee, 2016-12-21 14:15:52]

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
File tests/AppManifest.xml.expect (right):

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
tests/AppManifest.xml.expect:44: Id="1.0"
On 2016/12/19 17:19:52, Sebastian Noack wrote:
> > On 2016/12/19 15:30:20, Sebastian Noack wrote:
> > > As discussed offline, the tests would be much less trivial when testing
both
> > > scenarios, release and development builds, as we can no longer just
compare
> > the
> > > file contents, or if we'd do we would need two almost identical sample
> output
> > > files, with a lot of duplication. On the other other hand, parsing the XML
> > might
> > > be preferable anyway, as the tests will be tolerant of whitespaces for
> example
> > > and as another side effect we'd automatically make sure that the XML is
> > actually
> > > valid. However, this would arguably be out of the scope of this change.
> > IMHO sooner or later the string comparison approach will stop working
anyways
> > (it does most of the time with things like this) so we should just change it
> > while
> > we are at it?! So I would say I go and extend the test case for development
> > builds
> > and use and xml parser instead?!
> 
> Fine with me. I just got the impression, when we talked in person, that you
were
> not convinced of this approach (neither was I). So I wanted to avoid blocking
> this change just because of a controversial and slightly unrelated
refactoring.
> But I won't hold you back either. ;)

The xml file is generated by rendering a template. Which is a simple and
effective way to do it, but prone to xml validity errors. So it makes sense the
very the validity of the xml file (IMHO).

Unfortunately it's not that simple to verify a dynamically generated xml file. I
have used an approach that works well for testing websites, verifying various
aspects of the document via xpath.

This way you don't have to write a completely separate second implementation to
generate the file but still can test for key aspects of the document while at
the same time ensuring xml validity.

Unfortunately this cannot be done with the build in ElementTree package. Instead
I had to depend on lxml which depends on c libxml2 which (afaik) cannot be
installed with tox.

Is this acceptable? Is there a way to install lxml via tox? How should i
document the depency?

[Sebastian Noack, 2016-12-21 15:54:16]

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
File tests/AppManifest.xml.expect (right):

https://codereview.adblockplus.org/29368690/diff/29368691/tests/AppManifest.x...
tests/AppManifest.xml.expect:44: Id="1.0"
On 2016/12/21 14:15:51, wspee wrote:
> On 2016/12/19 17:19:52, Sebastian Noack wrote:
> > > On 2016/12/19 15:30:20, Sebastian Noack wrote:
> > > > As discussed offline, the tests would be much less trivial when testing
> both
> > > > scenarios, release and development builds, as we can no longer just
> compare
> > > the
> > > > file contents, or if we'd do we would need two almost identical sample
> > output
> > > > files, with a lot of duplication. On the other other hand, parsing the
XML
> > > might
> > > > be preferable anyway, as the tests will be tolerant of whitespaces for
> > example
> > > > and as another side effect we'd automatically make sure that the XML is
> > > actually
> > > > valid. However, this would arguably be out of the scope of this change.
> > > IMHO sooner or later the string comparison approach will stop working
> anyways
> > > (it does most of the time with things like this) so we should just change
it
> > > while
> > > we are at it?! So I would say I go and extend the test case for
development
> > > builds
> > > and use and xml parser instead?!
> > 
> > Fine with me. I just got the impression, when we talked in person, that you
> were
> > not convinced of this approach (neither was I). So I wanted to avoid
blocking
> > this change just because of a controversial and slightly unrelated
> refactoring.
> > But I won't hold you back either. ;)
> 
> The xml file is generated by rendering a template. Which is a simple and
> effective way to do it, but prone to xml validity errors. So it makes sense
the
> very the validity of the xml file (IMHO).
> 
> Unfortunately it's not that simple to verify a dynamically generated xml file.
I
> have used an approach that works well for testing websites, verifying various
> aspects of the document via xpath.
> 
> This way you don't have to write a completely separate second implementation
to
> generate the file but still can test for key aspects of the document while at
> the same time ensuring xml validity.
> 
> Unfortunately this cannot be done with the build in ElementTree package.
Instead
> I had to depend on lxml which depends on c libxml2 which (afaik) cannot be
> installed with tox.
> 
> Is this acceptable? Is there a way to install lxml via tox? How should i
> document the depency?

lxml can be installed using pip, and so also via tox. However, you'd need to
have the build dependencies installed (globally) before you run tox. This is
something we should avoid, as it partially defeats the purpose of tox, which is
that you only need one tool which then automatically installs and runs
everything else in an ad-hoc environment.

Also IIRC, libxml2/lxml is a rather error-tolerant parser, which comes handy in
most situations, but not so much when writing tests where you want to strictly
validate the XML.

I wonder why ElementTree from the corelib isn't sufficient. It supports XPath,
but I suppose you are using some more advanced syntax, that isn't supported in
the corelib? I suppose with a little bit of boilerplate it might be possible to
workaround that limitations. Or if really necessary, there are a couple XPath
implementations in pure Python, we could use without any native build
requirements.

But I'm undecided, whether XPath is much better than using the DOM directly. You
would need more code when using the DOM, but on the other hand across your XPath
expressions you have quite a bit of duplication, which you wouldn't have if
using the DOM an putting all intermediate nodes into variables. Though, it's
hard to judge which would be more preferable before seeing the resulting
implementation.

https://codereview.adblockplus.org/29368690/diff/29369464/tests/test_packager...
File tests/test_packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29369464/tests/test_packager...
tests/test_packagerEdge.py:190: for release_build, pairs in [[False, devbuild],
[True, release]]:
Please use tuples for data that have structure (as opposed to order). There is a
great post on Stack Overflow, which explains that concept:
https://stackoverflow.com/questions/626759/whats-the-difference-between-lists...

It is also stated in our coding style guide:
https://adblockplus.org/coding-style#python

So according to this rule, the loop here should look like that:

  for release_build, pairs in [(False, devbuild), (True, release)]:
    ...

The outer container is a list because it has order as you iterate over it. You
could add, move or remove an item, and the code below processing that list would
still work as expected without any adaption.

However, the inner container should be a tuple as it has structure. It is
essential to the code processing it that there are exactly two items, while the
first one is processed entirely different from the second.

[wspee, 2017-01-02 14:16:19]

I have updated the code to no longer depend on lxml but instead use the build in
xpath functionality plus a few expansions (mainly a function to retrieve
attributes).

It's not as bad as I had feared ;).

https://codereview.adblockplus.org/29368690/diff/29369464/tests/test_packager...
File tests/test_packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29369464/tests/test_packager...
tests/test_packagerEdge.py:190: for release_build, pairs in [[False, devbuild],
[True, release]]:
On 2016/12/21 15:54:15, Sebastian Noack wrote:
> Please use tuples for data that have structure (as opposed to order). There is
a
> great post on Stack Overflow, which explains that concept:
>
https://stackoverflow.com/questions/626759/whats-the-difference-between-lists...
> 
> It is also stated in our coding style guide:
> https://adblockplus.org/coding-style#python
> 
> So according to this rule, the loop here should look like that:
> 
>   for release_build, pairs in [(False, devbuild), (True, release)]:
>     ...
> 
> The outer container is a list because it has order as you iterate over it. You
> could add, move or remove an item, and the code below processing that list
would
> still work as expected without any adaption.
> 
> However, the inner container should be a tuple as it has structure. It is
> essential to the code processing it that there are exactly two items, while
the
> first one is processed entirely different from the second.

Done.

[Wladimir Palant, 2017-01-05 12:20:49]

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py#new...
packagerEdge.py:87: params['app_extension_id'] = '1.0' if release_build else
'EdgeExtension'
On 2016/12/19 17:19:52, Sebastian Noack wrote:
> But this would add quite a bit unnecessary complexity and configuration
> boilerplate for the time being, if this code is even ever used to build any
> other extension for Microsoft Edge. I don't have a strong opinion here.

I don't really see what this discussion is about. Having this hardcoded might do
the job but this is still ugly and not really self-explaining. I had to read
through the issue myself to understand what this is supposed to be. And I don't
see much complexity being added by solving this properly:

    id_setting = 'id' if release_build else 'id_devbuild'
    if metadata.has_option('general', id_setting):
      params['app_extension_id'] = metadata.get('general', id_setting)
    else:
      params['app_extension_id'] = 'EdgeExtension'

And we can comment in metadate.edge that we keep that id=1.0 setting for legacy
reasons, because we can no longer change it for the already published extension.

[Sebastian Noack, 2017-01-05 12:27:57]

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py#new...
packagerEdge.py:87: params['app_extension_id'] = '1.0' if release_build else
'EdgeExtension'
On 2017/01/05 12:20:49, Wladimir Palant wrote:
> On 2016/12/19 17:19:52, Sebastian Noack wrote:
> > But this would add quite a bit unnecessary complexity and configuration
> > boilerplate for the time being, if this code is even ever used to build any
> > other extension for Microsoft Edge. I don't have a strong opinion here.
> 
> I don't really see what this discussion is about. Having this hardcoded might
do
> the job but this is still ugly and not really self-explaining. I had to read
> through the issue myself to understand what this is supposed to be. And I
don't
> see much complexity being added by solving this properly:
> 
>     id_setting = 'id' if release_build else 'id_devbuild'
>     if metadata.has_option('general', id_setting):
>       params['app_extension_id'] = metadata.get('general', id_setting)
>     else:
>       params['app_extension_id'] = 'EdgeExtension'
> 
> And we can comment in metadate.edge that we keep that id=1.0 setting for
legacy
> reasons, because we can no longer change it for the already published
extension.

Thanks for getting back to us. As I said above, I don't have a strong opinion
either way. I just offered an alternative point of view. However, since you seem
to feel strong about it, lets make it configurable, I guess.

[wspee, 2017-01-06 16:18:11]

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py#new...
packagerEdge.py:87: params['app_extension_id'] = '1.0' if release_build else
'EdgeExtension'
On 2017/01/05 12:20:49, Wladimir Palant wrote:
> On 2016/12/19 17:19:52, Sebastian Noack wrote:
> > But this would add quite a bit unnecessary complexity and configuration
> > boilerplate for the time being, if this code is even ever used to build any
> > other extension for Microsoft Edge. I don't have a strong opinion here.
> 
> I don't really see what this discussion is about. Having this hardcoded might
do
> the job but this is still ugly and not really self-explaining. I had to read
> through the issue myself to understand what this is supposed to be. And I
don't
> see much complexity being added by solving this properly:
> 
>     id_setting = 'id' if release_build else 'id_devbuild'
>     if metadata.has_option('general', id_setting):
>       params['app_extension_id'] = metadata.get('general', id_setting)
>     else:
>       params['app_extension_id'] = 'EdgeExtension'
> 
> And we can comment in metadate.edge that we keep that id=1.0 setting for
legacy
> reasons, because we can no longer change it for the already published
extension.

I have made it configure able similar to how you suggested it. The fallback if
id hasn't been configured isn't really necessary, so I omitted it, right?

[wspee, 2017-01-06 17:21:46]

Removed Wladimir as a reviewer as he will be afk for the next two weeks.

[Sebastian Noack, 2017-01-07 10:18:46]

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py#new...
packagerEdge.py:87: params['app_extension_id'] = '1.0' if release_build else
'EdgeExtension'
On 2017/01/06 16:18:10, wspee wrote:
> On 2017/01/05 12:20:49, Wladimir Palant wrote:
> > On 2016/12/19 17:19:52, Sebastian Noack wrote:
> > > But this would add quite a bit unnecessary complexity and configuration
> > > boilerplate for the time being, if this code is even ever used to build
any
> > > other extension for Microsoft Edge. I don't have a strong opinion here.
> > 
> > I don't really see what this discussion is about. Having this hardcoded
might
> do
> > the job but this is still ugly and not really self-explaining. I had to read
> > through the issue myself to understand what this is supposed to be. And I
> don't
> > see much complexity being added by solving this properly:
> > 
> >     id_setting = 'id' if release_build else 'id_devbuild'
> >     if metadata.has_option('general', id_setting):
> >       params['app_extension_id'] = metadata.get('general', id_setting)
> >     else:
> >       params['app_extension_id'] = 'EdgeExtension'
> > 
> > And we can comment in metadate.edge that we keep that id=1.0 setting for
> legacy
> > reasons, because we can no longer change it for the already published
> extension.
> 
> I have made it configure able similar to how you suggested it. The fallback if
> id hasn't been configured isn't really necessary, so I omitted it, right?

I think it makes sense to use 'EdgeExtension' as default, and only specify the
id in the metadata file if it differs. Note that new extensions must use the
value 'EdgeExtension' anyway. So if we should ever use this code to bundle any
other extension for Microsfot Edge in the future, we don't need to bother, if we
use the appropriate default here.

https://codereview.adblockplus.org/29368690/diff/29370844/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29370844/packagerEdge.py#new...
packagerEdge.py:89: metadata_id = 'id_{}'.format(metadata_id_suffix)
As per our coding style guide (https://adblockplus.org/coding-style#python) we
agreed to use the + operator when concatenating exactly two strings, while only
using the format() method for more complex string formatting. We think the code
is more readable that way.

Also I think the key should be "extension_id_*" rather than just "id_*", for
consistence and distinction from "app_id" and "publisher_id".

[wspee, 2017-01-09 09:15:53]

https://codereview.adblockplus.org/29368690/diff/29370844/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29370844/packagerEdge.py#new...
packagerEdge.py:89: metadata_id = 'id_{}'.format(metadata_id_suffix)
On 2017/01/07 10:18:46, Sebastian Noack wrote:
> As per our coding style guide (https://adblockplus.org/coding-style#python) we
> agreed to use the + operator when concatenating exactly two strings, while
only
> using the format() method for more complex string formatting. We think the
code
> is more readable that way.
> 
> Also I think the key should be "extension_id_*" rather than just "id_*", for
> consistence and distinction from "app_id" and "publisher_id".

Done.

[wspee, 2017-01-09 13:32:29]

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29368690/diff/29368691/packagerEdge.py#new...
packagerEdge.py:87: params['app_extension_id'] = '1.0' if release_build else
'EdgeExtension'
On 2017/01/07 10:18:46, Sebastian Noack wrote:
> On 2017/01/06 16:18:10, wspee wrote:
> > On 2017/01/05 12:20:49, Wladimir Palant wrote:
> > > On 2016/12/19 17:19:52, Sebastian Noack wrote:
> > > > But this would add quite a bit unnecessary complexity and configuration
> > > > boilerplate for the time being, if this code is even ever used to build
> any
> > > > other extension for Microsoft Edge. I don't have a strong opinion here.
> > > 
> > > I don't really see what this discussion is about. Having this hardcoded
> might
> > do
> > > the job but this is still ugly and not really self-explaining. I had to
read
> > > through the issue myself to understand what this is supposed to be. And I
> > don't
> > > see much complexity being added by solving this properly:
> > > 
> > >     id_setting = 'id' if release_build else 'id_devbuild'
> > >     if metadata.has_option('general', id_setting):
> > >       params['app_extension_id'] = metadata.get('general', id_setting)
> > >     else:
> > >       params['app_extension_id'] = 'EdgeExtension'
> > > 
> > > And we can comment in metadate.edge that we keep that id=1.0 setting for
> > legacy
> > > reasons, because we can no longer change it for the already published
> > extension.
> > 
> > I have made it configure able similar to how you suggested it. The fallback
if
> > id hasn't been configured isn't really necessary, so I omitted it, right?
> 
> I think it makes sense to use 'EdgeExtension' as default, and only specify the
> id in the metadata file if it differs. Note that new extensions must use the
> value 'EdgeExtension' anyway. So if we should ever use this code to bundle any
> other extension for Microsfot Edge in the future, we don't need to bother, if
we
> use the appropriate default here.

Done.

[Sebastian Noack, 2017-01-09 14:52:19]

https://codereview.adblockplus.org/29368690/diff/29370870/tests/metadata.edge
File tests/metadata.edge (right):

https://codereview.adblockplus.org/29368690/diff/29370870/tests/metadata.edge...
tests/metadata.edge:21: extension_id_devbuild = EdgeExtension
This seems redundant now, where we fallback to "EdgeExtension" as default.

[wspee, 2017-01-10 08:09:00]

https://codereview.adblockplus.org/29368690/diff/29370870/tests/metadata.edge
File tests/metadata.edge (right):

https://codereview.adblockplus.org/29368690/diff/29370870/tests/metadata.edge...
tests/metadata.edge:21: extension_id_devbuild = EdgeExtension
On 2017/01/09 14:52:19, Sebastian Noack wrote:
> This seems redundant now, where we fallback to "EdgeExtension" as default.

Done.

[Sebastian Noack, 2017-01-10 12:54:42]

LGTM

As usual, can you send me the patch (including the metadata,  etc.)?