Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(30)

Issue 29825555: Issue 6291 - add ManifoldJS packaging for Edge

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by tlucas
Modified:
6 days, 17 hours ago
CC:
geo_adblockplus.org
Base URL:
https://hg.adblockplus.org/buildtools/file/9a56d76cd951
Visibility:
Public.

Description

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!

Patch Set 1 #

Patch Set 2 : #

Total comments: 11

Patch Set 3 : #

Total comments: 5

Patch Set 4 : #

Patch Set 5 : #

Total comments: 12

Patch Set 6 : #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+217 lines, -485 lines) Patch
M package.json View 1 2 3 4 5 1 chunk +4 lines, -1 line 0 comments Download
M packagerEdge.py View 1 2 3 4 5 3 chunks +117 lines, -129 lines 5 comments Download
D templates/edge/AppxBlockMap.xml.tmpl View 1 1 chunk +0 lines, -10 lines 0 comments Download
D templates/edge/AppxManifest.xml.tmpl View 1 chunk +0 lines, -61 lines 0 comments Download
D templates/edge/[Content_Types].xml.tmpl View 1 1 chunk +0 lines, -9 lines 0 comments Download
M tests/expecteddata/AppxManifest_edge_development_build.xml View 1 2 3 4 5 1 chunk +35 lines, -56 lines 0 comments Download
D tests/expecteddata/AppxManifest_edge_devenv.xml View 1 2 3 4 5 1 chunk +0 lines, -61 lines 0 comments Download
M tests/expecteddata/AppxManifest_edge_release_build.xml View 1 2 3 4 5 1 chunk +35 lines, -56 lines 0 comments Download
D tests/test_packagerEdge.py View 1 chunk +0 lines, -87 lines 0 comments Download
M tests/test_packagerWebExt.py View 1 2 3 4 5 6 chunks +21 lines, -15 lines 0 comments Download
M tox.ini View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 13
tlucas
Patch Set 1 * Introduce manifoldjs, for explicitly packaging adblockplus for Edge * Remove obsolete ...
1 week, 3 days ago (2018-07-09 07:51:40 UTC) #1
tlucas
Patch Set 2 * Also remove obsolete templates.
1 week, 3 days ago (2018-07-09 07:52:40 UTC) #2
Sebastian Noack
> * Packaging the extension relies on a http service This isn't great, but well, ...
1 week, 2 days ago (2018-07-09 12:35:20 UTC) #3
tlucas
Hey Sebastian, thanks for your comments. Patch Set 2: * Use zip.extract_all() instead of custom ...
1 week, 2 days ago (2018-07-09 13:23:57 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py File packagerEdge.py (left): https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py#oldcode201 packagerEdge.py:201: files[MANIFEST] = create_appx_manifest(params, files, How does that even work, ...
1 week, 2 days ago (2018-07-09 14:30:56 UTC) #5
tlucas
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#oldcode201 packagerEdge.py:201: ...
1 week, 2 days ago (2018-07-09 14:56:07 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py File packagerEdge.py (left): https://codereview.adblockplus.org/29825555/diff/29825632/packagerEdge.py#oldcode201 packagerEdge.py:201: files[MANIFEST] = create_appx_manifest(params, files, On 2018/07/09 14:56:07, tlucas wrote: ...
1 week, 2 days ago (2018-07-09 15:36:57 UTC) #7
Sebastian Noack
One more thing, it seems we would also use ManifoldJS (and consequently hit Microsoft's web ...
1 week, 2 days ago (2018-07-09 16:17:27 UTC) #8
tlucas
Patch Set 5: * Use manifoldjs also to create a source-folder-structure * Add missing values ...
1 week ago (2018-07-11 12:18:03 UTC) #9
Oleksandr
https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#newcode144 packagerEdge.py:144: if not devenv: On 2018/07/11 12:18:03, tlucas wrote: > ...
1 week ago (2018-07-11 14:23:47 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29825555/diff/29827608/packagerEdge.py#newcode137 packagerEdge.py:137: move_files_to_extension(files) Since we skip the packaging for devenv now, ...
1 week ago (2018-07-11 16:26:47 UTC) #11
tlucas
Patch Set 6: * Completely rely on manifoldjs to create the package (and it's source ...
6 days, 23 hours ago (2018-07-12 09:31:56 UTC) #12
Sebastian Noack
6 days, 18 hours ago (2018-07-12 14:43:11 UTC) #13
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],
              },
          },
      },
  }
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5