Issue 5083 - Merges chrome subdirectory to top level directory

Issue 5083 - Merges chrome subdirectory to top level directory

[Sebastian Noack, 2017-04-05 11:05:49]

Thanks for the patch. Looks mostly good on first glance. Can you send me the
patch after addressing my comments? I'd like to try it out before we land it,
and make sure we don't have any unwanted changes in the generated builds.

https://codereview.adblockplus.org/29403564/diff/29403565/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29403564/diff/29403565/ext/background.js#n...
ext/background.js:73: (function()
No need to create a new IFEE. Just add the code to the bottom of the IFEE above,
like the issue description says.

https://codereview.adblockplus.org/29403564/diff/29403565/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29403564/diff/29403565/ext/common.js#newco...
ext/common.js:53: (function()
No need to create a new IFEE. Just add the code to the bottom of the IFEE above,
like the issue description says.

https://codereview.adblockplus.org/29403564/diff/29403565/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29403564/diff/29403565/metadata.chrome#new...
metadata.chrome:39: author = eyeo GmbH
I would move general metadata like this above the different files. I suggest
following order:

basename
version
author
permissions
<everything that refers to filenames>

This would also be consistent with metadata.gecko in the "adblockplus" repo for
the old Firefox extension.

[Jon Sonesen, 2017-04-05 11:39:01]

Hey,

Thanks for comments and quick response!

https://codereview.adblockplus.org/29403564/diff/29403565/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29403564/diff/29403565/ext/background.js#n...
ext/background.js:73: (function()
On 2017/04/05 11:05:49, Sebastian Noack wrote:
> No need to create a new IFEE. Just add the code to the bottom of the IFEE
above,
> like the issue description says.

Done.

https://codereview.adblockplus.org/29403564/diff/29403565/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29403564/diff/29403565/ext/common.js#newco...
ext/common.js:53: (function()
On 2017/04/05 11:05:49, Sebastian Noack wrote:
> No need to create a new IFEE. Just add the code to the bottom of the IFEE
above,
> like the issue description says.

Done.

https://codereview.adblockplus.org/29403564/diff/29403565/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29403564/diff/29403565/metadata.chrome#new...
metadata.chrome:39: author = eyeo GmbH
On 2017/04/05 11:05:49, Sebastian Noack wrote:
> I would move general metadata like this above the different files. I suggest
> following order:
> 
> basename
> version
> author
> permissions
> <everything that refers to filenames>
> 
> This would also be consistent with metadata.gecko in the "adblockplus" repo
for
> the old Firefox extension.

Done.

[Sebastian Noack, 2017-04-05 12:29:42]

https://codereview.adblockplus.org/29403564/diff/29403641/metadata.chrome
File metadata.chrome (left):

https://codereview.adblockplus.org/29403564/diff/29403641/metadata.chrome#old...
metadata.chrome:6: permissions = tabs
Almost, but as per my previous comment, I think the permissions should be listed
above and filename, like before.

https://codereview.adblockplus.org/29403564/diff/29403641/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29403564/diff/29403641/metadata.chrome#new...
metadata.chrome:75: managed-storage-schema.json =
lib/managed-storage-schema.json
Any reason managed-storage-schema.json went into lib? It's not a background
script bundled as module, so it doesn't belong into lib. Please just put it
under the top-level.

[Jon Sonesen, 2017-04-05 13:02:06]

Sorry for the oversight there. Thanks again

https://codereview.adblockplus.org/29403564/diff/29403641/metadata.chrome
File metadata.chrome (left):

https://codereview.adblockplus.org/29403564/diff/29403641/metadata.chrome#old...
metadata.chrome:6: permissions = tabs
On 2017/04/05 12:29:41, Sebastian Noack wrote:
> Almost, but as per my previous comment, I think the permissions should be
listed
> above and filename, like before.

Done.

https://codereview.adblockplus.org/29403564/diff/29403641/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29403564/diff/29403641/metadata.chrome#new...
metadata.chrome:75: managed-storage-schema.json =
lib/managed-storage-schema.json
On 2017/04/05 12:29:41, Sebastian Noack wrote:
> Any reason managed-storage-schema.json went into lib? It's not a background
> script bundled as module, so it doesn't belong into lib. Please just put it
> under the top-level.

oh, that was a mistake, will fix.

[Jon Sonesen, 2017-04-05 13:02:12]

Sorry for the oversight there. Thanks again

[Jon Sonesen, 2017-04-05 13:02:16]

Sorry for the oversight there. Thanks again

[Sebastian Noack, 2017-04-05 14:08:56]

I just tried out your patch, and with these changes, the
managed-storage-schema.json file is no longer included in the build. Reason for
that is following code in buildtools/packagerChrome.py:

  def getPackageFiles(params):
      result = set(('_locales', 'icons', 'jquery-ui', 'lib', 'skin', 'ui',
'ext'))

      if params['devenv']:
          result.add('qunit')

      baseDir = params['baseDir']
      for file in os.listdir(baseDir):
          if file.endswith('.js') or file.endswith('.html') or
file.endswith('.xml'):
              result.add(file)
      return result

So without explicit mapping (which we removed), only files, from the top-level
directors, with a .js, ,.html or .xml file extension, get included in the
builds. We have to also add .json there. Mind creating a separate issue for
buildtools, in order to do so?

[Jon Sonesen, 2017-04-06 10:24:22]

On 2017/04/05 14:08:56, Sebastian Noack wrote:
> I just tried out your patch, and with these changes, the
> managed-storage-schema.json file is no longer included in the build. Reason
for
> that is following code in buildtools/packagerChrome.py:
> 
>   def getPackageFiles(params):
>       result = set(('_locales', 'icons', 'jquery-ui', 'lib', 'skin', 'ui',
> 'ext'))
> 
>       if params['devenv']:
>           result.add('qunit')
> 
>       baseDir = params['baseDir']
>       for file in os.listdir(baseDir):
>           if file.endswith('.js') or file.endswith('.html') or
> file.endswith('.xml'):
>               result.add(file)
>       return result
> 
> So without explicit mapping (which we removed), only files, from the top-level
> directors, with a .js, ,.html or .xml file extension, get included in the
> builds. We have to also add .json there. Mind creating a separate issue for
> buildtools, in order to do so?

Hey, yeah I am happy to do so. I thought that may be the case. I will create the
issue and I don't mind doing the change itself as well.

[Jon Sonesen, 2017-04-10 06:00:47]

On 2017/04/06 10:24:22, Jon Sonesen wrote:
> On 2017/04/05 14:08:56, Sebastian Noack wrote:
> > I just tried out your patch, and with these changes, the
> > managed-storage-schema.json file is no longer included in the build. Reason
> for
> > that is following code in buildtools/packagerChrome.py:
> > 
> >   def getPackageFiles(params):
> >       result = set(('_locales', 'icons', 'jquery-ui', 'lib', 'skin', 'ui',
> > 'ext'))
> > 
> >       if params['devenv']:
> >           result.add('qunit')
> > 
> >       baseDir = params['baseDir']
> >       for file in os.listdir(baseDir):
> >           if file.endswith('.js') or file.endswith('.html') or
> > file.endswith('.xml'):
> >               result.add(file)
> >       return result
> > 
> > So without explicit mapping (which we removed), only files, from the
top-level
> > directors, with a .js, ,.html or .xml file extension, get included in the
> > builds. We have to also add .json there. Mind creating a separate issue for
> > buildtools, in order to do so?
> 
> Hey, yeah I am happy to do so. I thought that may be the case. I will create
the
> issue and I don't mind doing the change itself as well.

Since the patch for buildtool has landed I have ensured the json file ends up in
the build

[kzar, 2017-04-10 06:18:04]

It makes me nervous how a bunch of unchanged files are showing up in this diff.
Not all of them are binary either. Any ideas why that's happening?

[Jon Sonesen, 2017-04-10 06:26:53]

On 2017/04/10 06:18:04, kzar wrote:
> It makes me nervous how a bunch of unchanged files are showing up in this
diff.
> Not all of them are binary either. Any ideas why that's happening?

Hm I just noticed some files from the ext dir which are not changed showing up
on the diff. That is odd. However, I do know that Mercurial has a different way
of handling the merging of files into directories than git does. However, the
fact that all the files in ext appear there are likely because I moved them into
the chrome subdir then merged everything up into the top level dir.

[kzar, 2017-04-10 06:55:03]

On 2017/04/10 06:26:53, Jon Sonesen wrote:
> On 2017/04/10 06:18:04, kzar wrote:
> > It makes me nervous how a bunch of unchanged files are showing up in this
> diff.
> > Not all of them are binary either. Any ideas why that's happening?
> 
> Hm I just noticed some files from the ext dir which are not changed showing up
> on the diff. That is odd. However, I do know that Mercurial has a different
way
> of handling the merging of files into directories than git does. However, the
> fact that all the files in ext appear there are likely because I moved them
into
> the chrome subdir then merged everything up into the top level dir.

Do you have your branch / bookmark in GitHub or BitBucket or somewhere else
where we can take a look at it?

[Jon Sonesen, 2017-04-10 06:57:04]

On 2017/04/10 06:55:03, kzar wrote:
> On 2017/04/10 06:26:53, Jon Sonesen wrote:
> > On 2017/04/10 06:18:04, kzar wrote:
> > > It makes me nervous how a bunch of unchanged files are showing up in this
> > diff.
> > > Not all of them are binary either. Any ideas why that's happening?
> > 
> > Hm I just noticed some files from the ext dir which are not changed showing
up
> > on the diff. That is odd. However, I do know that Mercurial has a different
> way
> > of handling the merging of files into directories than git does. However,
the
> > fact that all the files in ext appear there are likely because I moved them
> into
> > the chrome subdir then merged everything up into the top level dir.
> 
> Do you have your branch / bookmark in GitHub or BitBucket or somewhere else
> where we can take a look at it?

No, however I uploaded a separate patch set where I merged it differently.

[Sebastian Noack, 2017-04-10 07:10:33]

Something seems to be wrong with the latest patchset. In the previous patch sets
Rietveld were showing files that were moved as modified. Now they show up as
removed/added.

[Jon Sonesen, 2017-04-10 08:10:56]

On 2017/04/10 07:10:33, Sebastian Noack wrote:
> Something seems to be wrong with the latest patchset. In the previous patch
sets
> Rietveld were showing files that were moved as modified. Now they show up as
> removed/added.

Sorry about that, I updated the buildtools dep and uploaded a proper patch set.
I was just trying to exemplify that we just moved the files into the respective
top level dirs, but that rietveld was just not representing this well and
showing them as modified rather than moved, and that if i merged the chrome dir
differently it would show them as removed then added. It was not necessary
though to I am sorry.

[Sebastian Noack, 2017-04-10 08:14:59]

LGTM

[kzar, 2017-04-10 08:17:00]

(I don't have anything else to add, you can move me back to Cc again if you like
Jon.)

[Jon Sonesen, 2017-04-10 08:38:11]

On 2017/04/10 08:17:00, kzar wrote:
> (I don't have anything else to add, you can move me back to Cc again if you
like
> Jon.)

No problem, thanks for bringing that up :)