Move .ico hashing to icon_util

Move .ico hashing to icon_util

This patch moves the icon hashing code from web_app_win to icon_util. This will
let other callers of icon creation code check if a file write is actually
necessary.

This is a precursor for https://chromiumcodereview.appspot.com/14137032/.

BUG=167984

[benwells, 2013-05-09 01:55:40]

lgtm

[gab, 2013-05-09 12:35:19]

Haven't looked at the code in details, but I'd say add BUG=167984 and say that
this is a precursor CL for https://chromiumcodereview.appspot.com/14137032/
(makes it clearer if anyone ever wonders why this was done).

(same comment for https://chromiumcodereview.appspot.com/15080002/ )

Cheers!
Gab

[Alexei Svitkine (slow), 2013-05-09 15:34:57]

Hmm, please ignore my comments about the actual code until you answer my top
level comment.

Can you talk a little bit about the motivation for this?

Since this is all file i/o code, it will be done on the FILE thread anyway, so
what's the difference between the file being overwritten or not? Especially if
you now have to read another file anyway (more IO)...

Can you point me to the bug/codereview that discussed the motivation for this
code when it was added for apps?

I'm trying to understand what we're saving here. It seems we're replacing an
over-write of the file with an additional file on the filesystem that we now
have to read. This creates more complexity on the file system, which I don't
like.

If we just want to avoid an extra write (e.g. to keep SSDs more happy), then we
could just read the existing .ico file and compare it with the bytes we're about
to write, no?

Also, I think from the profile shortcut side, we don't need to do this hashing
stuff at all - because that code should be able to tell when the icon actually
changes (i.e. only on an avatar change).

https://chromiumcodereview.appspot.com/14839008/diff/4001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://chromiumcodereview.appspot.com/14839008/diff/4001/ui/gfx/icon_util.cc...
ui/gfx/icon_util.cc:742: base::FilePath
cheksum_file(icon_file.ReplaceExtension(kIconChecksumFileExt));
Nit: cheksum -> checksum

https://chromiumcodereview.appspot.com/14839008/diff/4001/ui/gfx/icon_util.cc...
ui/gfx/icon_util.cc:749: const gfx::ImageFamily& image) {
Nit: Align.

https://chromiumcodereview.appspot.com/14839008/diff/4001/ui/gfx/icon_util.h
File ui/gfx/icon_util.h (right):

https://chromiumcodereview.appspot.com/14839008/diff/4001/ui/gfx/icon_util.h#...
ui/gfx/icon_util.h:143: base::MD5Digest* digest);
Can this be private to the implementation, in an anonymous namespace?

Or is there a use case for other code using? (In which case, perhaps it
shouldn't be in this file but in a more general gfx util location.)

However, I'd say unless some other code will soon use this from the outside,
move it to the impl.

https://chromiumcodereview.appspot.com/14839008/diff/4001/ui/gfx/icon_util.h#...
ui/gfx/icon_util.h:145: // Saves |image| as an |icon_file| with the checksum.
Please expand the comment to mention how this actually works.

https://chromiumcodereview.appspot.com/14839008/diff/4001/ui/gfx/icon_util.h#...
ui/gfx/icon_util.h:150: static bool ShouldUpdateIcon(const base::FilePath&
icon_file,
Could you write a simple unit test for this?

[calamity, 2013-05-10 08:01:22]

+sail, original author, for context

Code was added in http://codereview.chromium.org/9346013

I guess we can just read the file and compare bytes.

Yeah, we can probably just figure out if the profile icon requires a
SHChangeNotify.

[sail, 2013-05-13 17:52:12]

removing my self since I don't know this code ver well

[calamity, 2013-08-28 03:09:24]

Closing.