Issue 2072 - Don`t duplicate 128x128 icon in Chrome builds

Issue 2072 - Don`t duplicate 128x128 icon in Chrome builds

[Sebastian Noack, 2015-03-02 14:25:01]

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/firs...
File firstRun.js (right):

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/firs...
firstRun.js:62: logo.src = "skin/abp-128.png";
Can't you specify that URL in firstRun.html? DOMContentLoaded should occur
before the error event, won't it?

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/firs...
firstRun.js:63: var errorCallback = function()
Why an anonymous function?

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/skin...
File skin/firstRun.css (left):

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/skin...
skin/firstRun.css:155: height: 128px;
Any reason to still specify the size for the <img> element? IMO this will just
increase the chance that either the CCS or the image will be forgotten to be
updated, in case the size will be changed in the future.

[Wladimir Palant, 2015-03-02 14:28:51]

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/firs...
File firstRun.js (right):

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/firs...
firstRun.js:62: logo.src = "skin/abp-128.png";
On 2015/03/02 14:25:02, Sebastian Noack wrote:
> Can't you specify that URL in firstRun.html? DOMContentLoaded should occur
> before the error event, won't it?

No, that's not guaranteed. Over the web the browser will load and process the
document in chunks, in theory it could do the same when loading from the file
system.

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/firs...
firstRun.js:63: var errorCallback = function()
On 2015/03/02 14:25:02, Sebastian Noack wrote:
> Why an anonymous function?

What exactly are you suggesting? A global function? Seems to hurt code locality
here and not really worth it. Naming the function? The browsers are capable of
doing that themselves. Proper nested function? I consider that ugly, given that
the logo variable is being used here - the function would be declared before
that variable.

[Sebastian Noack, 2015-03-02 14:32:16]

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/firs...
File firstRun.js (right):

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/firs...
firstRun.js:63: var errorCallback = function()
On 2015/03/02 14:28:51, Wladimir Palant wrote:
> On 2015/03/02 14:25:02, Sebastian Noack wrote:
> > Why an anonymous function?
> 
> What exactly are you suggesting? A global function? Seems to hurt code
locality
> here and not really worth it. Naming the function? The browsers are capable of
> doing that themselves. Proper nested function? I consider that ugly, given
that
> the logo variable is being used here - the function would be declared before
> that variable.

Yes, I'm suggesting a nested named function. But if you prefer an anonymous
function instead, never mind.

[Wladimir Palant, 2015-03-02 14:34:17]

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/skin...
File skin/firstRun.css (left):

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/skin...
skin/firstRun.css:155: height: 128px;
On 2015/03/02 14:25:02, Sebastian Noack wrote:
> Any reason to still specify the size for the <img> element?

As the image is loaded with a delay, not specifying the size here will make the
content jump.

[Sebastian Noack, 2015-03-02 14:35:08]

LGTM

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/skin...
File skin/firstRun.css (left):

http://codereview.adblockplus.org/5244235691327488/diff/5629499534213120/skin...
skin/firstRun.css:155: height: 128px;
On 2015/03/02 14:34:17, Wladimir Palant wrote:
> On 2015/03/02 14:25:02, Sebastian Noack wrote:
> > Any reason to still specify the size for the <img> element?
> 
> As the image is loaded with a delay, not specifying the size here will make
the
> content jump.

Alright.

[Thomas Greiner, 2015-03-03 12:29:39]

LGTM