Issue 2049 - Wrong logo being displayed in Options and Block Element dialog

Issue 2049 - Wrong logo being displayed in Options and Block Element dialog

[Sebastian Noack, 2015-02-27 12:49:42]

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
Nit: redundant space before "srcset".

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
Nit: You don't need to redundantly specify the orginal image in the "srcset". If
there is no image for "1x", it falls back to the "src" attribute anyway.

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
How about specifying the abp-48.png for 1.5x?

[Wladimir Palant, 2015-02-27 18:46:20]

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
On 2015/02/27 12:49:42, Sebastian Noack wrote:
> Nit: You don't need to redundantly specify the orginal image in the "srcset".
If
> there is no image for "1x", it falls back to the "src" attribute anyway.

I verified that this indeed matches the current implementation - MDN
documentation contradicted the spec here. Apparently, the MDN documentation was
wrong so I fixed it.

> How about specifying the abp-48.png for 1.5x?

For what purpose? There are no screens with 1.5x pixel density, and we
definitely won't provide images for this intermediate size.

[Sebastian Noack, 2015-02-27 18:53:12]

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
On 2015/02/27 18:46:20, Wladimir Palant wrote:
> On 2015/02/27 12:49:42, Sebastian Noack wrote:
> > Nit: You don't need to redundantly specify the orginal image in the
"srcset".
> If
> > there is no image for "1x", it falls back to the "src" attribute anyway.
> 
> I verified that this indeed matches the current implementation - MDN
> documentation contradicted the spec here. Apparently, the MDN documentation
was
> wrong so I fixed it.
> 
> > How about specifying the abp-48.png for 1.5x?
> 
> For what purpose? There are no screens with 1.5x pixel density, and we
> definitely won't provide images for this intermediate size.

This isn't about screens, but about operating system configuration. It's merely
coincidentally that OS X automatically set's pixel density to 2x on devices with
Retina display.

However, on Windows 8 for instance, you have more fine-grained options, and a
pixel density of 1.5x isn't entirely uncommon there.

I checked how Wikipedia is doing it.You might want to inspect the example image
on https://en.wikipedia.org/wiki/Portable_Network_Graphics to get an idea.

[Wladimir Palant, 2015-02-27 19:04:30]

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
On 2015/02/27 18:53:12, Sebastian Noack wrote:
> This isn't about screens, but about operating system configuration. It's
merely
> coincidentally that OS X automatically set's pixel density to 2x on devices
with
> Retina display.
> 
> However, on Windows 8 for instance, you have more fine-grained options, and a
> pixel density of 1.5x isn't entirely uncommon there.

The point is: 1.5x is an uncommon setting, there are no devices that ship with
it. Only users who set it manually get that, and that is rare. So we will
definitely accept the browser scaling down the larger image here instead of
providing yet another set of images.

> I checked how Wikipedia is doing it.You might want to inspect the example
image
> on https://en.wikipedia.org/wiki/Portable_Network_Graphics to get an idea.

Please tell me what I am seeing there. From where I am standing, Wikipedia took
a 560px image and specified it as both the 1x and 2x URL. The width of the <img>
tag is 280px. So Retina users will get the expected result, whereas for users
with normal pixel density the browser will scale down the image - like it would
do if only the src tag were specified. Frankly, I don't get why Wikipedia is
doing such nonsense for an image that they only have in one size.

[Wladimir Palant, 2015-02-27 19:11:16]

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
Ok, it seems that Wikipedia will produce a different srcset attribute for
Chrome, one that makes more sense. However, in their case it's all the same base
image, it has merely been scaled down automatically on the server. The browser
could do the scaling just as well - so the only reason they must be doing that
is giving people smaller downloads. In our case, providing separate images for
1.5x pixel density would actually produce the exact opposite result and increase
the download size.

[Sebastian Noack, 2015-02-27 19:16:01]

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
On 2015/02/27 19:04:30, Wladimir Palant wrote:
> On 2015/02/27 18:53:12, Sebastian Noack wrote:
> > This isn't about screens, but about operating system configuration. It's
> merely
> > coincidentally that OS X automatically set's pixel density to 2x on devices
> with
> > Retina display.
> > 
> > However, on Windows 8 for instance, you have more fine-grained options, and
a
> > pixel density of 1.5x isn't entirely uncommon there.
> 
> The point is: 1.5x is an uncommon setting, there are no devices that ship with
> it. Only users who set it manually get that, and that is rare. So we will
> definitely accept the browser scaling down the larger image here instead of
> providing yet another set of images.

Sure 2x is way more common. But if we have a matching icon for 1.5x in our build
anyway why not using it?

> > I checked how Wikipedia is doing it.You might want to inspect the example
> image
> > on https://en.wikipedia.org/wiki/Portable_Network_Graphics to get an idea.
> 
> Please tell me what I am seeing there. From where I am standing, Wikipedia
took
> a 560px image and specified it as both the 1x and 2x URL. The width of the
<img>
> tag is 280px. So Retina users will get the expected result, whereas for users
> with normal pixel density the browser will scale down the image - like it
would
> do if only the src tag were specified. Frankly, I don't get why Wikipedia is
> doing such nonsense for an image that they only have in one size.

This were common practice to support Retina displays, before srcset were
introduced. However, apparently Wikipedia is using a different set of images
depending on the browser. On Chrome they specify a "src" attribute with the
original image size and a srcset with one image for 1.5x and another one for 2x,
just as I suggested to do here.

[Sebastian Noack, 2015-02-27 19:18:37]

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
On 2015/02/27 19:11:17, Wladimir Palant wrote:
> Ok, it seems that Wikipedia will produce a different srcset attribute for
> Chrome, one that makes more sense. However, in their case it's all the same
base
> image, it has merely been scaled down automatically on the server. The browser
> could do the scaling just as well - so the only reason they must be doing that
> is giving people smaller downloads. In our case, providing separate images for
> 1.5x pixel density would actually produce the exact opposite result and
increase
> the download size.

Are you talking about the few bytes we would add by configuring an additional
image here? As I said, I'm not talking about adding a new image to the build,
but merely about using those we already have in the build.

[Wladimir Palant, 2015-02-27 19:26:59]

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
On 2015/02/27 19:18:37, Sebastian Noack wrote:
> Are you talking about the few bytes we would add by configuring an additional
> image here? As I said, I'm not talking about adding a new image to the build,
> but merely about using those we already have in the build.

If we support a particular pixel density, we should do it consistently. Right
now we are starting to support 2x pixel density properly. We still need to add
additional versions of all images, e.g. of the social icons. It isn't going to
end with our logo.

And - no, just because we happen to have one image of the right size already for
1.5x pixel density (on Chrome only btw., it's not part of the Safari build right
now) we shouldn't use it. It should be consistent.

[Sebastian Noack, 2015-02-27 19:29:56]

LGTM

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
File block.html (right):

http://codereview.adblockplus.org/5121634776121344/diff/5629499534213120/bloc...
block.html:75: <img src="icons/detailed/abp-32.png" 
srcset="icons/detailed/abp-32.png 1x, icons/detailed/abp-64.png 2x">
On 2015/02/27 19:26:59, Wladimir Palant wrote:
> On 2015/02/27 19:18:37, Sebastian Noack wrote:
> > Are you talking about the few bytes we would add by configuring an
additional
> > image here? As I said, I'm not talking about adding a new image to the
build,
> > but merely about using those we already have in the build.
> 
> If we support a particular pixel density, we should do it consistently. Right
> now we are starting to support 2x pixel density properly. We still need to add
> additional versions of all images, e.g. of the social icons. It isn't going to
> end with our logo.
> 
> And - no, just because we happen to have one image of the right size already
for
> 1.5x pixel density (on Chrome only btw., it's not part of the Safari build
right
> now) we shouldn't use it. It should be consistent.

Fair enough.

[Sebastian Noack, 2015-02-28 09:19:04]

I missed one more thing. For consistency, the chrome/icons/abp-48.png should be
moved to chrome/icons/detailed/abp-48.png.

[Sebastian Noack, 2015-02-28 09:19:44]

On 2015/02/28 09:19:04, Sebastian Noack wrote:
> I missed one more thing. For consistency, the chrome/icons/abp-48.png should
be
> moved to chrome/icons/detailed/abp-48.png.

Argh, you actually did. I'm an idiot. Never mind. Still LGTM.

[Thomas Greiner, 2015-03-02 11:07:56]

LGTM