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

Issue 29825555: Issue 6291 - add ManifoldJS packaging for Edge (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by tlucas
Modified:
1 month, 1 week 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: 14

Patch Set 7 : #

Total comments: 2

Patch Set 8 : #

Total comments: 4

Patch Set 9 : #

Total comments: 2

Patch Set 10 : Final patch set #

Unified diffs Side-by-side diffs Delta from patch set Stats (+217 lines, -493 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 6 7 8 9 3 chunks +109 lines, -129 lines 0 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
A tests/abp-150.png View 1 2 3 4 5 6 Binary file 0 comments Download
A tests/abp-44.png View 1 2 3 4 5 6 Binary file 0 comments Download
A tests/abp-50.png View 1 2 3 4 5 6 Binary file 0 comments Download
M tests/expecteddata/AppxManifest_edge_development_build.xml View 1 2 3 4 5 6 1 chunk +36 lines, -57 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 6 1 chunk +36 lines, -57 lines 0 comments Download
M tests/metadata.edge View 1 2 3 4 5 6 1 chunk +3 lines, -3 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 7 8 7 chunks +24 lines, -18 lines 0 comments Download
M tox.ini View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 20
tlucas
Patch Set 1 * Introduce manifoldjs, for explicitly packaging adblockplus for Edge * Remove obsolete ...
2 months, 1 week ago (2018-07-09 07:51:40 UTC) #1
tlucas
Patch Set 2 * Also remove obsolete templates.
2 months, 1 week 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, ...
2 months, 1 week 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 ...
2 months, 1 week 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, ...
2 months, 1 week 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: ...
2 months, 1 week 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: ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 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: > ...
2 months, 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, ...
2 months, 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 ...
2 months, 1 week ago (2018-07-12 09:31:56 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#newcode44 packagerEdge.py:44: namespaces = register_xml_namespaces(manifest_path) I wonder if this is necessary? ...
2 months, 1 week ago (2018-07-12 14:43:11 UTC) #13
tlucas
Patch Set 7: * Let manifoldjs localize the DisplayName. * Use packagerChrome's makeIcons for listed ...
2 months ago (2018-07-19 12:30:28 UTC) #14
Sebastian Noack
https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py#newcode63 packagerEdge.py:63: overwrite = { On 2018/07/19 12:30:28, tlucas wrote: > ...
1 month, 3 weeks ago (2018-07-25 19:18:42 UTC) #15
tlucas
Patch Set 8: * Simplified the xml traversal according to comments https://codereview.adblockplus.org/29825555/diff/29828574/packagerEdge.py File packagerEdge.py (right): ...
1 month, 1 week ago (2018-08-08 09:35:56 UTC) #16
Sebastian Noack
https://codereview.adblockplus.org/29825555/diff/29850555/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29825555/diff/29850555/packagerEdge.py#newcode73 packagerEdge.py:73: ) I didn't notice before that sometimes you update ...
1 month, 1 week ago (2018-08-08 19:19:05 UTC) #17
tlucas
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#newcode73 packagerEdge.py:73: ) On 2018/08/08 ...
1 month, 1 week ago (2018-08-08 22:10:08 UTC) #18
Sebastian Noack
Almost LGTM. To not further delay this (due to time zone difference), I'm fine if ...
1 month, 1 week ago (2018-08-08 22:27:33 UTC) #19
tlucas
1 month, 1 week ago (2018-08-09 07:09:25 UTC) #20
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.
Sign in to reply to this message.

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