Issue 4720 - enable devenv target for Edge packager

Issue 4720 - enable devenv target for Edge packager

[tlucas, 2017-10-13 15:00:06]

Patch Set 1

Taking https://codereview.adblockplus.org/29367148/ and rebasing it against
current master (499:4d26be1c438e)

[tlucas, 2017-10-13 15:02:09]

Patch Set 2

 * Outsourcing devenv creation in packagerChrome.py

[kzar, 2017-10-13 15:15:03]

LGTM except you didn't address Sebastian's comment about the qunit tests in the
other review.

[tlucas, 2017-10-13 15:23:47]

On 2017/10/13 15:15:03, kzar wrote:
> LGTM except you didn't address Sebastian's comment about the qunit tests in
the
> other review.

I asked Ollie in irc about it - did not get an answer yet (but i changed it to
include the qunit stuff. I figured it at least wouldn't hurt and a quick search
yielded, that qunit can be used with MS Edge)

[tlucas, 2017-10-13 15:24:22]

Adding Ollie to CC

[tlucas, 2017-10-13 16:08:35]

On 2017/10/13 15:24:22, tlucas wrote:
> Adding Ollie to CC

Taken from the previous review:

On 2017/10/13 14:33:47, Oleksandr wrote:
> On 2017/09/25 15:58:53, Sebastian Noack wrote:
> > https://codereview.adblockplus.org/29367148/diff/29555628/packagerEdge.py
> > File packagerEdge.py (right):
> > 
> >
>
https://codereview.adblockplus.org/29367148/diff/29555628/packagerEdge.py#new...
> > packagerEdge.py:191: if devenv:
> > This logic is duplicated in packagerChrome.py. Can you moveit into a
function
> > and reuse that function here?
> > 
> > Also is it intentional that you left out the qunit test suite?
> 
> No, it was unintentional that I left out qunit test suite.

-> Done.

[Sebastian Noack, 2017-10-17 21:29:52]

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

https://codereview.adblockplus.org/29575633/diff/29575637/packagerChrome.py#n...
packagerChrome.py:309: import buildtools
If we determine the path relative to this module (provided by __file__), we
don't need to import the package.

https://codereview.adblockplus.org/29575633/diff/29575637/packagerChrome.py#n...
packagerChrome.py:310: import random
This import can go to the top of the module.

[tlucas, 2017-10-18 09:27:50]

Patch Set 4

 * Moving an import to where it should be
 * Simplifying a path determination

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

https://codereview.adblockplus.org/29575633/diff/29575637/packagerChrome.py#n...
packagerChrome.py:309: import buildtools
On 2017/10/17 21:29:52, Sebastian Noack wrote:
> If we determine the path relative to this module (provided by __file__), we
> don't need to import the package.

Done.

https://codereview.adblockplus.org/29575633/diff/29575637/packagerChrome.py#n...
packagerChrome.py:310: import random
On 2017/10/17 21:29:52, Sebastian Noack wrote:
> This import can go to the top of the module.

Done.

[kzar, 2017-10-18 13:50:26]

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__),
Nit: IMO this would look nicer indented differently:

files.read(
    os.path.join(os.path.dirname(__file__), 'chromeDevenvPoller__.js'),
    relpath='devenvPoller__.js'
),

[tlucas, 2017-10-18 14:29:08]

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#n...
packagerChrome.py:324: files.read(os.path.join(os.path.dirname(__file__),
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.

[kzar, 2017-10-18 14:33:33]

LGTM

[Sebastian Noack, 2017-10-18 22:00:44]

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.