Issue 6291 - add ManifoldJS packaging for Edge

Issue 6291 - add ManifoldJS packaging for Edge

Non-obvious changes:

* The run-time of ensure_dependencies.py in a clean environment has doubled (manifoldjs introduces a lot of large dependencies).

* Packaging the extension relies on a http service (see https://github.com/MicrosoftEdge/hwa-cli/blob/master/src/cloudAppx.ts#L9 ) and also doubles to triples the run-time of build.py for Edge.

* The above also implies a much longer run-time of tox for the buildtools.

@Ollie, i was not able to find (or access) the endpoint, where i could manually upload new devbuilds for Edge, could you please give me a link - or try uploading a devbuild created with this change yourself? Thank you!

[tlucas, 2018-07-09 07:51:40]

Patch Set 1

 * Introduce manifoldjs, for explicitly packaging adblockplus for Edge
 * Remove obsolete code

[tlucas, 2018-07-09 07:52:40]

Patch Set 2

* Also remove obsolete templates.

[Sebastian Noack, 2018-07-09 12:35:20]

> * Packaging the extension relies on a http service

This isn't great, but well, I don't have a better idea. Rolling our own packager
form scratch turned out to be kinda unreliable and high on maintenance. So I
guess that this is the best option we got.

https://codereview.adblockplus.org/29825555/diff/29825565/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29825555/diff/29825565/packager.py#newcode171
packager.py:171: def to_tmp_folder(self, subfolder=None):
See the devenv() function in the "build" module. There we use
ZipFile.extractall(), which seems to be simpler than the boilerplate here.

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py
File packagerEdge.py (left):

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#old...
packagerEdge.py:204: files[CONTENT_TYPES] =
create_content_types_map(files.keys() + [BLOCKMAP])
Any reason you didn't remove the BLOCKMAP and CONTENT_TYPES variables as well as
the create_appx_blockmap() and create_content_types_map() functions?

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#new...
packagerEdge.py:15: MANIFEST = 'appxmanifest.xml'
How come that this changed?

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#new...
packagerEdge.py:148: # mock a generationInfo.json, as i would be created by
manifoldjs
Typo: i -> it

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#new...
packagerEdge.py:148: # mock a generationInfo.json, as i would be created by
manifoldjs
Why do we have to do that manually, now where we use manifoldjs?

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#new...
packagerEdge.py:169: with open(package, 'rb') as fp:
Where is this code path coming from? It seems previously we were assuming
outfile to always be a path to write the build to.

[tlucas, 2018-07-09 13:23:57]

Hey Sebastian, thanks for your comments.

Patch Set 2:

* Use zip.extract_all() instead of custom machinery
* Clean up obsolete variables

https://codereview.adblockplus.org/29825555/diff/29825565/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29825555/diff/29825565/packager.py#newcode171
packager.py:171: def to_tmp_folder(self, subfolder=None):
On 2018/07/09 12:35:19, Sebastian Noack wrote:
> See the devenv() function in the "build" module. There we use
> ZipFile.extractall(), which seems to be simpler than the boilerplate here.

Done.

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py
File packagerEdge.py (left):

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#old...
packagerEdge.py:204: files[CONTENT_TYPES] =
create_content_types_map(files.keys() + [BLOCKMAP])
On 2018/07/09 12:35:20, Sebastian Noack wrote:
> Any reason you didn't remove the BLOCKMAP and CONTENT_TYPES variables as well
as
> the create_appx_blockmap() and create_content_types_map() functions?

Done.

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#new...
packagerEdge.py:15: MANIFEST = 'appxmanifest.xml'
On 2018/07/09 12:35:20, Sebastian Noack wrote:
> How come that this changed?

manifoldjs only accepts "AppxManifest.xml" to be the results of it's packaging
process; but not to read from it. (I.e. it raises:

[error] manifoldjs  : Failed to package the Microsoft Edge Extension Platform
app.
                      The specified path does not contain a valid app manifest
file.
                      ENOENT: no such file or directory, open
'/tmp/tmpKYWE1Madblockplus_package/edgeextension/manifest/appxmanifest.xml')

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#new...
packagerEdge.py:148: # mock a generationInfo.json, as i would be created by
manifoldjs
On 2018/07/09 12:35:19, Sebastian Noack wrote:
> Why do we have to do that manually, now where we use manifoldjs?

We don't have to.

For packaging the extension (lines 157 / 161), manifoldjs requires the
generationInfo.json to be present.

In order to generate this, we could a) invoke manifoldjs 2 times, first for
building, a.k.a copying the extension's source files around, along with some
placeholder images and then overwrite the resulting manifest, additionally copy
the icons we actually want - this is basically what we already have in our
files-object; finally start the actual packaging, or b) what i implemented here.

for that typo: done.

https://codereview.adblockplus.org/29825555/diff/29825565/packagerEdge.py#new...
packagerEdge.py:169: with open(package, 'rb') as fp:
On 2018/07/09 12:35:20, Sebastian Noack wrote:
> Where is this code path coming from? It seems previously we were assuming
> outfile to always be a path to write the build to.

The tests, or creating a devenv may pass a StringIO-object to this function (for
reference, the implementation in packagerChrome is similar:

https://hg.adblockplus.org/buildtools/file/tip/packagerChrome.py#l338 )

[Sebastian Noack, 2018-07-09 14:30:56]

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py
File packagerEdge.py (left):

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py#old...
packagerEdge.py:201: files[MANIFEST] = create_appx_manifest(params, files,
How does that even work, since you removed templates/edge/AppxManifest.xml.tmpl?
Also do we even have to touch the AppxManifest.xml at all, or can't we just
leave it up to manifoldJS and take whatever AppxManifest.xml it generates.

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py#new...
packagerEdge.py:181: outfile.write(fp.read())
Since you already use shutil, you might want to use shutil.copyfileobj() here.

[tlucas, 2018-07-09 14:56:07]

Patch Set 4:

* use shutil.copyfileobj instead of file.write

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py
File packagerEdge.py (left):

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py#old...
packagerEdge.py:201: files[MANIFEST] = create_appx_manifest(params, files,
On 2018/07/09 14:30:55, Sebastian Noack wrote:
> How does that even work, since you removed
templates/edge/AppxManifest.xml.tmpl?
> Also do we even have to touch the AppxManifest.xml at all, or can't we just
> leave it up to manifoldJS and take whatever AppxManifest.xml it generates.

I merely renamed it to the lower-case version - and no, we can't just use
whatever manifoldjs generates (that would happen in the first step i'm
omitting), since it requires us manually add data like the app-it etc (see
https://docs.microsoft.com/en-us/microsoft-edge/extensions/guides/packaging/u...
(search for "In the AppXManifest file, you'll need to")

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py#new...
packagerEdge.py:181: outfile.write(fp.read())
On 2018/07/09 14:30:55, Sebastian Noack wrote:
> Since you already use shutil, you might want to use shutil.copyfileobj() here.

Done.

[Sebastian Noack, 2018-07-09 15:36:57]

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py
File packagerEdge.py (left):

https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py#old...
packagerEdge.py:201: files[MANIFEST] = create_appx_manifest(params, files,
On 2018/07/09 14:56:07, tlucas wrote:
> On 2018/07/09 14:30:55, Sebastian Noack wrote:
> > How does that even work, since you removed
> templates/edge/AppxManifest.xml.tmpl?
> > Also do we even have to touch the AppxManifest.xml at all, or can't we just
> > leave it up to manifoldJS and take whatever AppxManifest.xml it generates.
> 
> I merely renamed it to the lower-case version - and no, we can't just use
> whatever manifoldjs generates (that would happen in the first step i'm
> omitting), since it requires us manually add data like the app-it etc (see
>
https://docs.microsoft.com/en-us/microsoft-edge/extensions/guides/packaging/u...
> (search for "In the AppXManifest file, you'll need to") 

Gotya. Though, I wonder whether generating a complete new manifest, overwriting
whatever ManifoldJS put in there is the way to go. What if in the future some
contents change (and ManifoldJS accounts for that)? From the documentation, it
seems you are only supposed to replace specific parts of the generated manifest.

[Sebastian Noack, 2018-07-09 16:17:27]

One more thing, it seems we would also use ManifoldJS (and consequently hit
Microsoft's web services) when running "build.py devenv", essentially making it
impossible to work on Adblock Plus for Microsoft Edge without a reliable
internet connection (or when their server is down). However, as far as I
understand packaging (done by ManifoldJS) isn't necessary to load an unpacked
extension during development (it seems the same files you'd load in development
mode on Chrome or Firefox would be sufficient).

[tlucas, 2018-07-11 12:18:03]

Patch Set 5:

 * Use manifoldjs also to create a source-folder-structure
 * Add missing values / files to the structure created by manifoldjs
 * Clean up the devenv for Edge (where possible) and don't use manifoldjs to
create it

I tried to verify the equality of the packages, pre- and post- manifoldjs usage.
There are changes in AppxBlockmap.xml and [ContentTypes].xml, which are
seemingly ony formatting changes, as well as a change in
Extension/lib/adblockplus.js.map - which seems to only be a missing newline at
the end of the file.

Additionally, there are two files "priconfig.xml" and "resources.pri" created
additionally (along with a folder "Resources", containing descriptive messages
for the windows store in every language we support), which *seem* to solely
exist for controlling the extensions appearance in the windows store. However, i
could not verify that.

The rest of the extension itself is untouched.

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:144: if not devenv:
It turned out that devenvs for Edge don't need the AppxManifest.xml file to be
present, same applies to AppxBlockMap.xml and [Content_Types].xml (see changes
in test_packagerWebExt.py) @Ollie, i verified that on my own Windows-PC, i'd
appreciate another verification though.

[Oleksandr, 2018-07-11 14:23:47]

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:144: if not devenv:
On 2018/07/11 12:18:03, tlucas wrote:
> It turned out that devenvs for Edge don't need the AppxManifest.xml file to be
> present, same applies to AppxBlockMap.xml and [Content_Types].xml (see changes
> in test_packagerWebExt.py) @Ollie, i verified that on my own Windows-PC, i'd
> appreciate another verification though.
I can confirm that for devenv only the `Extension` part is used. All the other
stuff is redundant.

[Sebastian Noack, 2018-07-11 16:26:47]

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:137: move_files_to_extension(files)
Since we skip the packaging for devenv now, there is no reason to move files to
the "Extension" sub-folder anymore in devenv mode. Neither do we have to create
the "Assets" folder (below) for devenv.

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:145: files[MANIFEST] = create_appx_manifest(params, files,
As discussed before, I would not override the APPX manifest, but rather parse
the one generated by ManifoldJS, and merely add the fields it cannot generate
automatically. This seems to be the workflow suggested in the documentation, and
will be more robust against future changes by Microsoft.

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:154: shutil.copyfileobj(zipped, outfile)
Don't we have to extract the contents for the devenv?

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:164: try:
This try-block should probably start right after the call to mkdtemp().

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:201: with open(package, 'rb') as fp:
This is essentially dead code now, since only in case of devenv, a file-like
object is passed in. So I wonder whether we should just assert that assumption,
rather than having dead/untested code in here?

  assert isinstance(outfile, basestring)
  shutil.copyfile(package, outfile)

[tlucas, 2018-07-12 09:31:56]

Patch Set 6:
 * Completely rely on manifoldjs to create the package (and it's source
structure)
 * Clean up unused code
 * Clean up unused files
 * Let tests adhere to the "new" structure
 * Apply further comments from Sebastian

Please note, as pointed out in #adblockplus, the current configuration 'out of
the box' localizes the package for the Windows Store.

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:137: move_files_to_extension(files)
On 2018/07/11 16:26:47, Sebastian Noack wrote:
> Since we skip the packaging for devenv now, there is no reason to move files
to
> the "Extension" sub-folder anymore in devenv mode. Neither do we have to
create
> the "Assets" folder (below) for devenv.

Turns out this is not necessary anymore at all - removed this alltogether.

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:145: files[MANIFEST] = create_appx_manifest(params, files,
On 2018/07/11 16:26:47, Sebastian Noack wrote:
> As discussed before, I would not override the APPX manifest, but rather parse
> the one generated by ManifoldJS, and merely add the fields it cannot generate
> automatically. This seems to be the workflow suggested in the documentation,
and
> will be more robust against future changes by Microsoft.

Done (new function update_appx_manifest)

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:154: shutil.copyfileobj(zipped, outfile)
On 2018/07/11 16:26:46, Sebastian Noack wrote:
> Don't we have to extract the contents for the devenv?

Yes - but that's done in build.py, with all 3 extensions packed before.

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:164: try:
On 2018/07/11 16:26:46, Sebastian Noack wrote:
> This try-block should probably start right after the call to mkdtemp().

Done.

https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#new...
packagerEdge.py:201: with open(package, 'rb') as fp:
On 2018/07/11 16:26:46, Sebastian Noack wrote:
> This is essentially dead code now, since only in case of devenv, a file-like
> object is passed in. So I wonder whether we should just assert that
assumption,
> rather than having dead/untested code in here?
> 
>   assert isinstance(outfile, basestring)
>   shutil.copyfile(package, outfile)

That's not an assumption, at this point 'outfile' will definitely be a string.

Removed dead code accordingly.

[Sebastian Noack, 2018-07-12 14:43:11]

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:44: namespaces = register_xml_namespaces(manifest_path)
I wonder if this is necessary? What happens if you don't register the namespace?
Also does this cause the schema to be downloaded? (It would be great to avoid
that.)

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:63: overwrite = {
Instead of a dictionaries you might want to use lists:

1. Since you don't do lookups (but merely iterate over the items) anyway.
2. If new nodes are added, they will be in predictable order.

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:69: ('', 'DisplayName'): translation[name_key]['message'],
It seems we can just leave the DisplayName alone. ManifoldJS will set it to
ms-resource:DisplayName, and creates the respective resource files. While we
don't need to bother about translating the product name, if we get it for free,
there is no reason to explicitly bypass that mechanism.

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:81: ('uap', 'VisualElements'): {
Perhaps this structure is more readable if keys are always strings (with "{uap}"
prefix if necessary). That way the logic in traverse() can also be simplified.

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:83: 'Square44x44Logo': 'Assets/logo_44.png',
Instead of hard-coding the assets, we could identify them based on the
[appx_assets] section in metadata.edge, reusing the makeIcons() function from
packagerChrome:


  assets = makeIcons(files, filenames)
  overwrite = {
      ...
      ('', 'Properties'): {
          ...
          ('', 'Logo'): assets[50],
      },
      ...
      ('', 'Applications'): {
          ('', 'Application'): {
              ('uap', 'VisualElements'): {
                  'Square150x150Logo': assets[150],
                  'Square44x44Logo': assets[44],
              },
          },
      },
  }

[tlucas, 2018-07-19 12:30:28]

Patch Set 7:
 * Let manifoldjs localize the DisplayName.
 * Use packagerChrome's makeIcons for listed appx_assets.
 * Adjusted tests according to the above.

 * Commented on further comments / suggestions.

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:44: namespaces = register_xml_namespaces(manifest_path)
On 2018/07/12 14:43:10, Sebastian Noack wrote:
> I wonder if this is necessary? What happens if you don't register the
namespace?
> Also does this cause the schema to be downloaded? (It would be great to avoid
> that.)

This is necessary so that xml.etree.ElementTree.write() preserves the file's
actual namespace configuration (otherwise it would rewrite the configured
namespaces to have auto-generated prefixed, e.g. ns0, ns1 and so forth)

To my knowing (and from what i observed), nothing is downloaded.

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:63: overwrite = {
On 2018/07/12 14:43:10, Sebastian Noack wrote:
> Instead of a dictionaries you might want to use lists:
> 
> 1. Since you don't do lookups (but merely iterate over the items) anyway.
> 2. If new nodes are added, they will be in predictable order.

Since order doesn't matter in the resulting XML (or anywhere in between), i
don't see a real benefit from switching to lists - could you please clarify?

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:69: ('', 'DisplayName'): translation[name_key]['message'],
On 2018/07/12 14:43:10, Sebastian Noack wrote:
> It seems we can just leave the DisplayName alone. ManifoldJS will set it to
> ms-resource:DisplayName, and creates the respective resource files. While we
> don't need to bother about translating the product name, if we get it for
free,
> there is no reason to explicitly bypass that mechanism.

True indeed, removed this.

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:81: ('uap', 'VisualElements'): {
On 2018/07/12 14:43:10, Sebastian Noack wrote:
> Perhaps this structure is more readable if keys are always strings (with
"{uap}"
> prefix if necessary). That way the logic in traverse() can also be simplified.

You may be right about it being (only imho) a bit more readable, but i
personally wouldn't want to trade explicitly being able to distinguish between
elements/children and attributes for this.

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:83: 'Square44x44Logo': 'Assets/logo_44.png',
On 2018/07/12 14:43:10, Sebastian Noack wrote:
> Instead of hard-coding the assets, we could identify them based on the
> [appx_assets] section in metadata.edge, reusing the makeIcons() function from
> packagerChrome:
> 
> 
>   assets = makeIcons(files, filenames)
>   overwrite = {
>       ...
>       ('', 'Properties'): {
>           ...
>           ('', 'Logo'): assets[50],
>       },
>       ...
>       ('', 'Applications'): {
>           ('', 'Application'): {
>               ('uap', 'VisualElements'): {
>                   'Square150x150Logo': assets[150],
>                   'Square44x44Logo': assets[44],
>               },
>           },
>       },
>   }

Done.

[Sebastian Noack, 2018-07-25 19:18:42]

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:63: overwrite = {
On 2018/07/19 12:30:28, tlucas wrote:
> On 2018/07/12 14:43:10, Sebastian Noack wrote:
> > Instead of a dictionaries you might want to use lists:
> > 
> > 1. Since you don't do lookups (but merely iterate over the items) anyway.
> > 2. If new nodes are added, they will be in predictable order.
> 
> Since order doesn't matter in the resulting XML (or anywhere in between), i
> don't see a real benefit from switching to lists - could you please clarify?
> 

Originally I thought that nodes may be created for the overrides here, but in
fact those nodes and attributes already exist in the output of manifoldJS and
you merely override them without adding new nodes/attributes.

Though, in theory, it could happen in the future that we'll have attributes
listed here that don't exist in the template, and then the order of these
attributes in the resulting XML will be arbitrary like the order of dict keys.

Regardless of this (theoretical) issue, I don't see a point in using a
dictionary, if you never do a single lookup, but always turn it back into a list
in order to iterate over it. Why not using a list in the first place?

Anyway, I am wondering whether we cannot use a fully-qualified XPath expression.
In fact the find() method, you already use in traverse() is expecting an XPath
expression (but you currently merely call it with an element name). Furthermore,
you can pass in the namespaces as second argument to find() so that you don't
have to explicitly look up the namespace prefix, like that:

  overrides = [
      ("ns:name/ns:name", [
          ("attributeName", "attributeValue"),
          ...
      ]),
      ...
  ]

  ...

  for xpath, attributes in overrides:
      for element in doc.findall(xpath, namespaces):
          for attr, value in attributes:
              element.set(attr, value)

The only problem appears to be that the default namespace doesn't seem to get
resolved, so that instead of "Identity" you'd have to write
"{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Identity", or
give a non-empty prefix to the default namespace so that you can write
"x:Identity" for example which isn't great either, but perhaps I overlooked
something, or there might be a workaround.

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:81: ('uap', 'VisualElements'): {
On 2018/07/19 12:30:27, tlucas wrote:
> On 2018/07/12 14:43:10, Sebastian Noack wrote:
> > Perhaps this structure is more readable if keys are always strings (with
> "{uap}"
> > prefix if necessary). That way the logic in traverse() can also be
simplified.
> 
> You may be right about it being (only imho) a bit more readable, but i
> personally wouldn't want to trade explicitly being able to distinguish between
> elements/children and attributes for this.

What I meant is using "uap:VisualElements" as key (not
"{http://schemas.microsoft.com/appx/manifest/uap/windows10}VisualElements"),
which would be more readable, IMO.

https://codereview.adblockplus.org/29825555/diff/29834555/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29834555/packagerEdge.py#new...
packagerEdge.py:57: if metadata.has_section('appx_assets'):
This check seems redundant as we fail anyway below where we try to access
assets[...] if the appx_assets section doesn't exist.

[tlucas, 2018-08-08 09:35:56]

Patch Set 8:

 * Simplified the xml traversal according to comments

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:63: overwrite = {
On 2018/07/25 19:18:41, Sebastian Noack wrote:
> On 2018/07/19 12:30:28, tlucas wrote:
> > On 2018/07/12 14:43:10, Sebastian Noack wrote:
> > > Instead of a dictionaries you might want to use lists:
> > > 
> > > 1. Since you don't do lookups (but merely iterate over the items) anyway.
> > > 2. If new nodes are added, they will be in predictable order.
> > 
> > Since order doesn't matter in the resulting XML (or anywhere in between), i
> > don't see a real benefit from switching to lists - could you please clarify?
> > 
> 
> Originally I thought that nodes may be created for the overrides here, but in
> fact those nodes and attributes already exist in the output of manifoldJS and
> you merely override them without adding new nodes/attributes.
> 
> Though, in theory, it could happen in the future that we'll have attributes
> listed here that don't exist in the template, and then the order of these
> attributes in the resulting XML will be arbitrary like the order of dict keys.
> 
> Regardless of this (theoretical) issue, I don't see a point in using a
> dictionary, if you never do a single lookup, but always turn it back into a
list
> in order to iterate over it. Why not using a list in the first place?
> 
> Anyway, I am wondering whether we cannot use a fully-qualified XPath
expression.
> In fact the find() method, you already use in traverse() is expecting an XPath
> expression (but you currently merely call it with an element name).
Furthermore,
> you can pass in the namespaces as second argument to find() so that you don't
> have to explicitly look up the namespace prefix, like that:
> 
>   overrides = [
>       ("ns:name/ns:name", [
>           ("attributeName", "attributeValue"),
>           ...
>       ]),
>       ...
>   ]
> 
>   ...
> 
>   for xpath, attributes in overrides:
>       for element in doc.findall(xpath, namespaces):
>           for attr, value in attributes:
>               element.set(attr, value)
> 
> The only problem appears to be that the default namespace doesn't seem to get
> resolved, so that instead of "Identity" you'd have to write
> "{http://schemas.microsoft.com/appx/manifest/foundation/windows10}Identity",
or
> give a non-empty prefix to the default namespace so that you can write
> "x:Identity" for example which isn't great either, but perhaps I overlooked
> something, or there might be a workaround.

It looks like adding the default namespaces with an arbitrary identifier is
common practice, (see e.g.
https://stackoverflow.com/questions/34009992/python-elementtree-default-names...
)


Additionally, i decided to use "find" instead of "findall" so that we will
notice in case some of our defined attributes or elements are not supported
anymore (the loop you suggested would just gracefully ignore missing elements)

https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#new...
packagerEdge.py:81: ('uap', 'VisualElements'): {
On 2018/07/25 19:18:41, Sebastian Noack wrote:
> On 2018/07/19 12:30:27, tlucas wrote:
> > On 2018/07/12 14:43:10, Sebastian Noack wrote:
> > > Perhaps this structure is more readable if keys are always strings (with
> > "{uap}"
> > > prefix if necessary). That way the logic in traverse() can also be
> simplified.
> > 
> > You may be right about it being (only imho) a bit more readable, but i
> > personally wouldn't want to trade explicitly being able to distinguish
between
> > elements/children and attributes for this.
> 
> What I meant is using "uap:VisualElements" as key (not
> "{http://schemas.microsoft.com/appx/manifest/uap/windows10}VisualElements"),
> which would be more readable, IMO.

Done.

https://codereview.adblockplus.org/29825555/diff/29834555/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29834555/packagerEdge.py#new...
packagerEdge.py:57: if metadata.has_section('appx_assets'):
On 2018/07/25 19:18:41, Sebastian Noack wrote:
> This check seems redundant as we fail anyway below where we try to access
> assets[...] if the appx_assets section doesn't exist.

Done.

[Sebastian Noack, 2018-08-08 19:19:05]

https://codereview.adblockplus.org/29825555/diff/29850555/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29850555/packagerEdge.py#new...
packagerEdge.py:73: )
I didn't notice before that sometimes you update the text content instead of the
attributes. In that case I'd like to suggest a more homogeneous data structure:

  overrides = [
      (xpath, text_or_None, attributes),
      ...
  ]

This makes the logic below slightly simpler and cleaner since you can get rid of
the type check, but also it is more flexible (e.g. in case we have to update
both the text content and attributes for some element in the future).

Also note that the outer container should be a list (instead of a tuple), and
that I used "overrides" (instead of "overwrite"), which seems to be more
appropriate: https://stackoverflow.com/questions/8651562/overwrite-or-override

https://codereview.adblockplus.org/29825555/diff/29850555/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29825555/diff/29850555/tests/test_packager...
tests/test_packagerWebExt.py:437: ('Extension', 'manifest', 'json')]
Nit: The closing square bracket go on a new line, and there should be a comma
after the last item (according to our coding style guide).

[tlucas, 2018-08-08 22:10:08]

Patch Set 9
 * applied comments

https://codereview.adblockplus.org/29825555/diff/29850555/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29850555/packagerEdge.py#new...
packagerEdge.py:73: )
On 2018/08/08 19:19:04, Sebastian Noack wrote:
> I didn't notice before that sometimes you update the text content instead of
the
> attributes. In that case I'd like to suggest a more homogeneous data
structure:
> 
>   overrides = [
>       (xpath, text_or_None, attributes),
>       ...
>   ]
> 
> This makes the logic below slightly simpler and cleaner since you can get rid
of
> the type check, but also it is more flexible (e.g. in case we have to update
> both the text content and attributes for some element in the future).
> 
> Also note that the outer container should be a list (instead of a tuple), and
> that I used "overrides" (instead of "overwrite"), which seems to be more
> appropriate: https://stackoverflow.com/questions/8651562/overwrite-or-override

Done.

https://codereview.adblockplus.org/29825555/diff/29850555/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29825555/diff/29850555/tests/test_packager...
tests/test_packagerWebExt.py:437: ('Extension', 'manifest', 'json')]
On 2018/08/08 19:19:04, Sebastian Noack wrote:
> Nit: The closing square bracket go on a new line, and there should be a comma
> after the last item (according to our coding style guide).

Done.

[Sebastian Noack, 2018-08-08 22:27:33]

Almost LGTM. To not further delay this (due to time zone difference), I'm fine
if you go ahead and push the change, once that last comment has been addressed,
without waiting for me to do another round.

https://codereview.adblockplus.org/29825555/diff/29850587/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29850587/packagerEdge.py#new...
packagerEdge.py:81: element.text = text
We should check if text is not None. It may not be a problem in practice at the
moment if all elements we get None for here don't have a text. But I think we
should not attempt to clear the text if we merely care about the attributes.

[tlucas, 2018-08-09 07:09:25]

Thank you!

Done and pushed ;)

https://codereview.adblockplus.org/29825555/diff/29850587/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29825555/diff/29850587/packagerEdge.py#new...
packagerEdge.py:81: element.text = text
On 2018/08/08 22:27:32, Sebastian Noack wrote:
> We should check if text is not None. It may not be a problem in practice at
the
> moment if all elements we get None for here don't have a text. But I think we
> should not attempt to clear the text if we merely care about the attributes.

Done.