Issue 1538 - Replace ABP icon with new one (v.1.1) - for Internet Explorer

Use new icons with 32x32 dimensions.

[sergei, 2015-01-26 13:10:33]

Here it's not vertically centered. There is a small gap at the bottom and zero
top padding.

[Sebastian Noack, 2015-01-28 09:25:30]

What's about html/static/img/abp-128.png and installer/src/setup-exe/abp-64.png?

[Eric, 2015-02-02 15:13:00]

On 2015/01/28 09:25:30, Sebastian Noack wrote:
> What's about html/static/img/abp-128.png and
installer/src/setup-exe/abp-64.png?

The one in setup-exe/ is in dead code. setup-exe/ is the old MSI selector based
on 'burn' that we've ditched. The directory for it hasn't been deleted yet.

The icon specification in the installer uses the blocking icon at present, but
that was only a quick hack to get the Wix code in place. The proper icon file
for the installer needs two resolutions: a small one for the line-item entry in
the control panel, and a large one for the detail information shown when that
line is selected. Given that the present UI code relies on a particular image
size (as I recall), it would probably be better to supply an installer-specfic
icon file with multiple resolutions.

That said, it probably works to continue to use the blocking icon, but Windows
will apply its own rescaling operation as it needs to and it won't look as good.

[Eric, 2015-02-03 16:07:48]

Right now the code is using API call 'LoadIcon'. If we were to switch to
'LoadImage', which has arguments for preferred size, we could use .ico files
with multiple resolutions in them.

We need a multi-resolution icon file for the installer still. Issue #1538 should
not be closed until we have one. Whether we introduce it here or in another
change set matters little to me.

http://codereview.adblockplus.org/6210609305616384/diff/5717271485874176/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/6210609305616384/diff/5717271485874176/src/...
src/plugin/PluginClass.cpp:48: uint32_t iconWidth = 32;
At the very least, it's time to introduce an anonymous namespace here for new
file-scope variables.

If you need to introduce new such variables here, it's also the right time to
remove 'h_Icons' and 'h_IconTypes' from CPluginClass and declare them in
parallel, since they're part of the same icon facility.

Even better yet would be to take this opportunity and add the first files to a
new 'ui/' directory under 'src/'. This was one directory that was not at all in
question in the code quality meeting.

[Wladimir Palant, 2015-02-09 11:13:48]

There is also installer/src/setup-exe/abp-64.png, it should be replaced as well.

[Sebastian Noack, 2015-02-09 11:22:48]

On 2015/02/09 11:13:48, Wladimir Palant wrote:
> There is also installer/src/setup-exe/abp-64.png, it should be replaced as
well.

I already pointed that out above. However, according to Eric this is dead code.

[Oleksandr, 2015-02-09 11:37:43]

Yes. Also, I am working on another codereview which would use multiresolution
icon, as Eric suggested. So I am no 100% sure which resolutions will be needed
yet.


On 2015/02/09 11:22:48, Sebastian Noack wrote:
> On 2015/02/09 11:13:48, Wladimir Palant wrote:
> > There is also installer/src/setup-exe/abp-64.png, it should be replaced as
> well.
> 
> I already pointed that out above. However, according to Eric this is dead
code.

[Oleksandr, 2015-02-10 06:09:00]

Suggested multiresoltuion icons are made from the currently available
resolutions: 16x16, 19x19, 32x32, 38x38. These will be updated as soon as more
resolutions are available.

Since the unused abp-64.png is introducing confusion, I am removing it.

[Eric, 2015-03-02 13:47:33]

LGTM

[sergei, 2015-03-06 10:41:33]

Could you rebase it, as I understand the code is already in the repo.

Description says that there are new icons with 32x32 dimensions but are the
icons 48x48, 128x128 and 256x256 replaced by icon 38x38 and it's not mentioned?

[Oleksandr, 2015-03-20 22:25:13]

Hm, somehow I have missed your comment, Sergei, sorry for the delay. I have
rebased, though there are no code changes.

I don't completely understand what you're asking, but the blocking.ico is a
single file that contains multiple resolutions. LoadImage loads appropriate
resolution. 48x48, 64x64, 128x128 and 256x256 are only used in the Windows UI
(in a list of Installed Applications, for example)

[sergei, 2015-03-30 09:24:12]

On 2015/03/20 22:25:13, Oleksandr wrote:
> Hm, somehow I have missed your comment, Sergei, sorry for the delay. I have
> rebased, though there are no code changes.

I guess there is something wrong with the rebasing because there are indeed no
code cghanges, all changes from this patch are already in the repository, so
they should not be here.
https://hg.adblockplus.org/adblockplusie/file/tip/src/plugin/PluginClass.cpp
 
> I don't completely understand what you're asking, but the blocking.ico is a
> single file that contains multiple resolutions. LoadImage loads appropriate
> resolution. 48x48, 64x64, 128x128 and 256x256 are only used in the Windows UI
> (in a list of Installed Applications, for example)

Just want to clarify
In my currently checked out version blocking.ico contains: 16x16, 19x19, 32x32,
48x48, 128x128, 256x256, icons.
In "Patch Set 3 : Use multiresolution icons" blocking.ico contains: 16x16,
19x19, 32x32, 38x38 icons.
It seems "Patch Set 4 : Rebased" blocking.ico is reverted, so now there is no
38x38 icon and 32x32 is not updated.

I cared about 48x48, 128x128, 256x256 icons.

[sergei, 2015-03-30 09:47:00]

In addition, when I open current blocking.ico in Visual Studio it shows that
there are two 32x32 icons and the first one seems to be correct, but there is
something with colors, may be Visual Studio displays them incorrectly.

[Oleksandr, 2015-03-30 11:15:50]

Rebased to how it was committed.