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

Issue 29575633: Issue 4720 - enable devenv target for Edge packager (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by tlucas
Modified:
2 years ago
Reviewers:
Sebastian Noack, kzar
CC:
Vasily Kuznetsov, Oleksandr
Visibility:
Public.

Description

Issue 4720 - enable devenv target for Edge packager

Patch Set 1 #

Patch Set 2 : Outsourcing devenv creation #

Total comments: 4

Patch Set 3 : NO CHANGE rebasing against master #

Patch Set 4 : Addressing Sebastian's comments #

Total comments: 3

Patch Set 5 : Addressing nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -11 lines) Patch
M build.py View 2 chunks +5 lines, -2 lines 0 comments Download
M packagerChrome.py View 1 2 3 4 3 chunks +15 lines, -9 lines 0 comments Download
M packagerEdge.py View 1 2 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 12
tlucas
Patch Set 1 Taking https://codereview.adblockplus.org/29367148/ and rebasing it against current master (499:4d26be1c438e)
2 years, 1 month ago (2017-10-13 15:00:06 UTC) #1
tlucas
Patch Set 2 * Outsourcing devenv creation in packagerChrome.py
2 years, 1 month ago (2017-10-13 15:02:09 UTC) #2
kzar
LGTM except you didn't address Sebastian's comment about the qunit tests in the other review.
2 years, 1 month ago (2017-10-13 15:15:03 UTC) #3
tlucas
On 2017/10/13 15:15:03, kzar wrote: > LGTM except you didn't address Sebastian's comment about the ...
2 years, 1 month ago (2017-10-13 15:23:47 UTC) #4
tlucas
Adding Ollie to CC
2 years, 1 month ago (2017-10-13 15:24:22 UTC) #5
tlucas
On 2017/10/13 15:24:22, tlucas wrote: > Adding Ollie to CC Taken from the previous review: ...
2 years, 1 month ago (2017-10-13 16:08:35 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29575633/diff/29575637/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29575633/diff/29575637/packagerChrome.py#newcode309 packagerChrome.py:309: import buildtools If we determine the path relative to ...
2 years, 1 month ago (2017-10-17 21:29:52 UTC) #7
tlucas
Patch Set 4 * Moving an import to where it should be * Simplifying a ...
2 years ago (2017-10-18 09:27:50 UTC) #8
kzar
https://codereview.adblockplus.org/29575633/diff/29582559/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29575633/diff/29582559/packagerChrome.py#newcode324 packagerChrome.py:324: files.read(os.path.join(os.path.dirname(__file__), Nit: IMO this would look nicer indented differently: ...
2 years ago (2017-10-18 13:50:26 UTC) #9
tlucas
Patch Set 5 * Addressing Dave's comment https://codereview.adblockplus.org/29575633/diff/29582559/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29575633/diff/29582559/packagerChrome.py#newcode324 packagerChrome.py:324: files.read(os.path.join(os.path.dirname(__file__), On ...
2 years ago (2017-10-18 14:29:08 UTC) #10
kzar
LGTM
2 years ago (2017-10-18 14:33:33 UTC) #11
Sebastian Noack
2 years ago (2017-10-18 22:00:44 UTC) #12
LGTM

https://codereview.adblockplus.org/29575633/diff/29582559/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29575633/diff/29582559/packagerChrome.py#n...
packagerChrome.py:324: files.read(os.path.join(os.path.dirname(__file__),
On 2017/10/18 14:29:07, tlucas wrote:
> On 2017/10/18 13:50:26, kzar wrote:
> > Nit: IMO this would look nicer indented differently:
> > 
> > files.read(
> >     os.path.join(os.path.dirname(__file__), 'chromeDevenvPoller__.js'),
> >     relpath='devenvPoller__.js'
> > ),
> 
> Done.

FWIW, both look good to me.
Sign in to reply to this message.

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