Unused icons for Chrome/Safari builds

This fix is related to current ticket:
https://issues.adblockplus.org/ticket/3

Chrome and Safari build contain some unused icons:

Chrome:
icons/abp-16.png
icons/abp-16-notification-critical.png
icons/abp-16-notification-information.png

Safari: 
icons/abp-19.png
icons/abp-19-notification-critical.png
icons/abp-19-notification-information.png
icons/abp-48.png (while it's only loaded in notificaition.html which only used in chrome)

[saroyanm, 2014-03-13 09:39:14]

Had no chance to test under safari, but should work, I've compared the icons
folder for both and they contain only icons that are used for each build, 
Also added icons/abp-48.png while It's only used in notifications.html as
discussed in current issue:
http://codereview.adblockplus.org/5767063142400000/

[Sebastian Noack, 2014-03-13 10:04:31]

On 2014/03/13 09:39:14, saroyanm wrote:
> Had no chance to test under safari, but should work, I've compared the icons
> folder for both and they contain only icons that are used for each build, 
> Also added icons/abp-48.png while It's only used in notifications.html as
> discussed in current issue:
> http://codereview.adblockplus.org/5767063142400000/

abp-32.png is also not used on Safari. And abp-128.png is mapped to Icon.png on
Safari, but the original file under icons/abp-128.png is not used. So please
move abp-32.png and abp-128.png also to chrome/icons.

[saroyanm, 2014-03-13 15:30:39]

> abp-32.png is also not used on Safari. And abp-128.png is mapped to Icon.png
on
> Safari, but the original file under icons/abp-128.png is not used. So please
> move abp-32.png and abp-128.png also to chrome/icons.

Thanks for pointing that Sebastian, I've updated background.js to be able to
move abp-128.png into chrome folder while it was still been used by
notifications.
Just having a little doubt regarding abp-32 while I've noticed that it is used
in options.html and block.html, Just wanted to align with you while can't test
under safari, but seams like abp-32.png is used by both builds.

[Sebastian Noack, 2014-03-13 15:55:08]

On 2014/03/13 15:30:39, saroyanm wrote:
> > abp-32.png is also not used on Safari. And abp-128.png is mapped to Icon.png
> on
> > Safari, but the original file under icons/abp-128.png is not used. So please
> > move abp-32.png and abp-128.png also to chrome/icons.
> 
> Thanks for pointing that Sebastian, I've updated background.js to be able to
> move abp-128.png into chrome folder while it was still been used by
> notifications.
> Just having a little doubt regarding abp-32 while I've noticed that it is used
> in options.html and block.html, Just wanted to align with you while can't test
> under safari, but seams like abp-32.png is used by both builds.

You are right, abp-32.png is actually used on Safari. So just leave where it
was.

[Sebastian Noack, 2014-03-13 15:55:14]

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
File background.js (right):

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
background.js:353: iconUrl = ext.getURL("icon.png");
This shouldn't be necessary. The code below that uses iconUrl, is only used on
Chrome.

[saroyanm, 2014-03-13 16:01:18]

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
File background.js (right):

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
background.js:353: iconUrl = ext.getURL("icon.png");
On 2014/03/13 15:55:14, Sebastian Noack wrote:
> This shouldn't be necessary. The code below that uses iconUrl, is only used on
> Chrome.

iconUrl is also used to generate webkit notification on line 384:
var notification = webkitNotifications.createNotification(iconUrl, title,
message);
Seams like this part of code is also used in safari build.

[Sebastian Noack, 2014-03-13 16:20:00]

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
File background.js (right):

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
background.js:353: iconUrl = ext.getURL("icon.png");
On 2014/03/13 16:01:19, saroyanm wrote:
> On 2014/03/13 15:55:14, Sebastian Noack wrote:
> > This shouldn't be necessary. The code below that uses iconUrl, is only used
on
> > Chrome.
> 
> iconUrl is also used to generate webkit notification on line 384:
> var notification = webkitNotifications.createNotification(iconUrl, title,
> message);
> Seams like this part of code is also used in safari build.

In that case I would prefer to keep that image redundant rather than requiring
this special case here.

[saroyanm, 2014-03-13 16:31:19]

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
File background.js (right):

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
background.js:353: iconUrl = ext.getURL("icon.png");
On 2014/03/13 16:20:00, Sebastian Noack wrote:
> On 2014/03/13 16:01:19, saroyanm wrote:
> > On 2014/03/13 15:55:14, Sebastian Noack wrote:
> > > This shouldn't be necessary. The code below that uses iconUrl, is only
used
> on
> > > Chrome.
> > 
> > iconUrl is also used to generate webkit notification on line 384:
> > var notification = webkitNotifications.createNotification(iconUrl, title,
> > message);
> > Seams like this part of code is also used in safari build.
> 
> In that case I would prefer to keep that image redundant rather than requiring
> this special case here.

I see your point, in that case if you are okey with other changes then we can go
with patch #1.
Or we can think about better solution, like having icon.png in chrome as well in
root directory ?

[Sebastian Noack, 2014-03-13 16:36:43]

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
File background.js (right):

http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
background.js:353: iconUrl = ext.getURL("icon.png");
On 2014/03/13 16:31:19, saroyanm wrote:
> On 2014/03/13 16:20:00, Sebastian Noack wrote:
> > On 2014/03/13 16:01:19, saroyanm wrote:
> > > On 2014/03/13 15:55:14, Sebastian Noack wrote:
> > > > This shouldn't be necessary. The code below that uses iconUrl, is only
> used
> > on
> > > > Chrome.
> > > 
> > > iconUrl is also used to generate webkit notification on line 384:
> > > var notification = webkitNotifications.createNotification(iconUrl, title,
> > > message);
> > > Seams like this part of code is also used in safari build.
> > 
> > In that case I would prefer to keep that image redundant rather than
requiring
> > this special case here.
> 
> I see your point, in that case if you are okey with other changes then we can
go
> with patch #1.
> Or we can think about better solution, like having icon.png in chrome as well
in
> root directory ?

Yep, considering my assumptions were wrong, the patch set #1 LGTM.

[saroyanm, 2014-03-13 17:14:07]

On 2014/03/13 16:36:43, Sebastian Noack wrote:
>
http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
> File background.js (right):
> 
>
http://codereview.adblockplus.org/5709671373471744/diff/5717271485874176/back...
> background.js:353: iconUrl = ext.getURL("icon.png");
> On 2014/03/13 16:31:19, saroyanm wrote:
> > On 2014/03/13 16:20:00, Sebastian Noack wrote:
> > > On 2014/03/13 16:01:19, saroyanm wrote:
> > > > On 2014/03/13 15:55:14, Sebastian Noack wrote:
> > > > > This shouldn't be necessary. The code below that uses iconUrl, is only
> > used
> > > on
> > > > > Chrome.
> > > > 
> > > > iconUrl is also used to generate webkit notification on line 384:
> > > > var notification = webkitNotifications.createNotification(iconUrl,
title,
> > > > message);
> > > > Seams like this part of code is also used in safari build.
> > > 
> > > In that case I would prefer to keep that image redundant rather than
> requiring
> > > this special case here.
> > 
> > I see your point, in that case if you are okey with other changes then we
can
> go
> > with patch #1.
> > Or we can think about better solution, like having icon.png in chrome as
well
> in
> > root directory ?
> 
> Yep, considering my assumptions were wrong, the patch set #1 LGTM.

Pushed, thanks.

[saroyanm, 2014-03-17 11:05:24]

@Sebastian I've noticed an issue when building the chrome after pushing current
changes in chrome build, actually the chrome used files in "icons" directory to
generate "icons" parameter in manifest.json obj. 
So here is the object for icons parameter that is generated now:
"icons": {
    "128": "icons/abp-128.png", 
    "32": "icons/abp-32.png"
  }, 

Which actually should be:
"icons": {
    "128": "icons/abp-128.png", 
    "48": "icons/abp-48.png",
    "16": "icons/abp-16.png"
  }, 
according to chrome manifest documentation:
http://developer.chrome.com/extensions/manifest/icons

So I'll reopen current issue and update the build code also to generate exact
JSON which is mentioned in official documentation.

What you think ?

P.S. beforehand we were generating current JSON which again is different from
official one:
  "icons": {
    "128": "icons/abp-128.png", 
    "16": "icons/abp-16.png", 
    "19": "icons/abp-19.png", 
    "32": "icons/abp-32.png", 
    "48": "icons/abp-48.png"
  },

[Sebastian Noack, 2014-03-17 11:15:58]

If I understand the problem correctly, you just have to move 

  files['manifest.json'] = createManifest(params)

below

  if metadata.has_section('mapping'):
    files.readMappedFiles(metadata.items('mapping')

in buildtools/packagerChrome.py. Please open a new review for the new patch.

[saroyanm, 2014-03-17 11:43:44]

On 2014/03/17 11:15:58, Sebastian Noack wrote:
> If I understand the problem correctly, you just have to move 
> 
>   files['manifest.json'] = createManifest(params)
> 
> below
> 
>   if metadata.has_section('mapping'):
>     files.readMappedFiles(metadata.items('mapping')
> 
> in buildtools/packagerChrome.py. Please open a new review for the new patch.

Not sure actually, Will have a look,
Will experiment with mappings also,
I was thinking to add each image manually like it's done for "webAccessible"
param in metadata.chrome.
Anyway Will open new review with results of investigations and more info. 
Thanks.