Add static method WebApkIconHasher::DownloadAndComputeMurmur2Hash

Add static method WebApkIconHasher::DownloadAndComputeMurmur2Hash

Static method WebApkIconHasher::DownloadAndCompauteMurmur2Hash creates
a WebApkIconHasher and lets it download and hash the image. This change
would give WebApkIconHasher control of its life cycle and simplifies
usage.

BUG=649771
Review-Url: https://codereview.chromium.org/2782313002
Cr-Commit-Position: refs/heads/master@{#461427}
Committed: https://chromium.googlesource.com/chromium/src/+/efb812e9274d2f1e0c3761d7307921f0e44f9a8d

[F, 2017-03-29 21:59:51]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 22:00:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-03-29 22:01:51]

Description was changed from

==========
Add static method WebApkIconHasher::DownloadAndComputeMurmur2Hash

BUG=649771
==========

to

==========
Add static method WebApkIconHasher::DownloadAndComputeMurmur2Hash

Static method WebApkIconHasher::DownloadAndCompauteMurmur2Hash creates
a WebApkIconHasher and lets it download and hash the image. This change
would give WebApkIconHasher control of its life cycle and simplifies
usage.

BUG=649771
==========

[F, 2017-03-29 22:03:38]

zpeng@chromium.org changed reviewers:
+ dominickn@chromium.org

[F, 2017-03-29 22:03:38]

Hi Dominick, PTAL. Thanks!

[F, 2017-03-29 22:23:41]

Patchset #1 (id:1) has been deleted

[F, 2017-03-29 22:23:45]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 22:24:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-03-29 23:27:00]

Thanks for this!

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_icon_hasher.h (right):

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_icon_hasher.h:28: ~WebApkIconHasher()
override;
Can this be declared private? Or is it called by the URLFetcherDelegate?

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_icon_hasher.h:50: void
DownloadAndComputeMurmur2Hash();
You could remove this method and do all of its work in the constructor. Then you
don't need to store either icon_url_ or the net::URLRequestContextGetter

[F, 2017-03-30 00:18:27]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 00:18:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-03-30 00:20:13]

Thanks Dominick! PTAL

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_icon_hasher.h (right):

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_icon_hasher.h:28: ~WebApkIconHasher()
override;
On 2017/03/29 23:27:00, dominickn wrote:
> Can this be declared private? Or is it called by the URLFetcherDelegate?

Done. This can be declared private.

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_icon_hasher.h:50: void
DownloadAndComputeMurmur2Hash();
On 2017/03/29 23:27:00, dominickn wrote:
> You could remove this method and do all of its work in the constructor. Then
you
> don't need to store either icon_url_ or the net::URLRequestContextGetter

After experimenting with it, I think maybe we should keep this function. In
order to "delete" itself in the constructor, WebApkIconHasher needs to call a
new static function that calls the callback and then deletes a WebApkIconHasher
instance. Additionally,  the unused WebApkIconHasher instance would trigger
"unused variable" error during compilation.

WDUT?

[dominickn, 2017-03-30 00:26:46]

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_icon_hasher.h (right):

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_icon_hasher.h:50: void
DownloadAndComputeMurmur2Hash();
That's a good point. You could get around it by moving the icon_url_ validity
and data URL checks into the static
WebApkIconHasher::DownloadAndComputeMurmur2Hash(). Then you never have to
allocate a WebApkIconHasher for those cases. How does that sound?

[commit-bot: I haz the power, 2017-03-30 02:21:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 02:21:05]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[pkotwicz, 2017-03-30 14:47:25]

pkotwicz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[pkotwicz, 2017-03-30 14:47:25]

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_icon_hasher.h (right):

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_icon_hasher.h:50: void
DownloadAndComputeMurmur2Hash();
In my opinion, we should try to avoid having constructors doing a a lot of work.
I like the way the code is currently structured.

We could move some of the processing from
WebApkIconHasher::DownloadAndComputeMurmur2Hash() to the static
WebApkIconHasher::DownloadAndComputeMurmur2Hash() but I don't see much point

We should have a new method RunCallback() which is responsible for both running
the callback and deleting the WebApkIconHasher (so that none of the callsites
forget to delete the WebApkIconHasher)

[F, 2017-03-30 15:19:09]

Patchset #3 (id:60001) has been deleted

[F, 2017-03-30 15:44:04]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 15:44:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-03-30 15:52:46]

Thanks Dominick & Peter! PTAL

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_icon_hasher.h (right):

https://codereview.chromium.org/2782313002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_icon_hasher.h:50: void
DownloadAndComputeMurmur2Hash();
On 2017/03/30 14:47:25, pkotwicz wrote:
> In my opinion, we should try to avoid having constructors doing a a lot of
work.
> I like the way the code is currently structured.
> 
> We could move some of the processing from
> WebApkIconHasher::DownloadAndComputeMurmur2Hash() to the static
> WebApkIconHasher::DownloadAndComputeMurmur2Hash() but I don't see much point
> 
> We should have a new method RunCallback() which is responsible for both
running
> the callback and deleting the WebApkIconHasher (so that none of the callsites
> forget to delete the WebApkIconHasher)

For now I kept the current code structure and added RunCallback. WDUT?

If we move everything into the constructor, we'll split the logic into static
function and constructor. Also we'll "new" a WebApkIconHasher without assigning
it to any variable. I'd prefer keeping the logic together and call a member
function, similar to what WebApkInstaller does now.

[commit-bot: I haz the power, 2017-03-30 16:36:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 16:36:26]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-03-30 16:57:34]

Looks good to me but deferring to dominickn@ for the final review

[dominickn, 2017-03-31 00:21:49]

On 2017/03/30 14:47:25, pkotwicz wrote:
> We could move some of the processing from
> WebApkIconHasher::DownloadAndComputeMurmur2Hash() to the static
> WebApkIconHasher::DownloadAndComputeMurmur2Hash() but I don't see much point

The main wins are:

 - we only new-allocate an IconHasher when we actually have to fetch something
(otherwise we work entirely on the stack, which is faster)
 - we can store two fewer instance variables on IconHasher by creating the
url_fetcher_ in the constructor (which seems like an appropriate use of the
constructor), saving some memory and copying

There are currently only two callsites where we have to remember to free, so I
think the advantages of having RunCallback are outweighed by being slightly more
efficient and not storing things that we don't actually need to store.

I don't want to block, so if feel very strongly, I'll let it through. The above
is how I weigh the pros and cons here.

[F, 2017-03-31 17:15:41]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 17:16:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-03-31 17:17:40]

Thanks Dominick & Peter! PTAL

I removed non-static DownloadAndComputeMurmur2Hash and kept RunCallback. WDUT?

[pkotwicz, 2017-03-31 17:26:22]

Still looks good to me

[commit-bot: I haz the power, 2017-03-31 19:29:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 19:29:28]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-04-02 23:48:33]

lgtm, thanks for bearing with me. :)

[F, 2017-04-03 13:50:50]

The CQ bit was checked by zpeng@chromium.org

[commit-bot: I haz the power, 2017-04-03 13:51:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 14:29:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491227450106350,
"parent_rev": "71e3604b522136b8fe4226941b67b72f56c7d921", "commit_rev":
"efb812e9274d2f1e0c3761d7307921f0e44f9a8d"}

[commit-bot: I haz the power, 2017-04-03 14:30:30]

Description was changed from

==========
Add static method WebApkIconHasher::DownloadAndComputeMurmur2Hash

Static method WebApkIconHasher::DownloadAndCompauteMurmur2Hash creates
a WebApkIconHasher and lets it download and hash the image. This change
would give WebApkIconHasher control of its life cycle and simplifies
usage.

BUG=649771
==========

to

==========
Add static method WebApkIconHasher::DownloadAndComputeMurmur2Hash

Static method WebApkIconHasher::DownloadAndCompauteMurmur2Hash creates
a WebApkIconHasher and lets it download and hash the image. This change
would give WebApkIconHasher control of its life cycle and simplifies
usage.

BUG=649771

Review-Url: https://codereview.chromium.org/2782313002
Cr-Commit-Position: refs/heads/master@{#461427}
Committed:
https://chromium.googlesource.com/chromium/src/+/efb812e9274d2f1e0c3761d73079...
==========

[commit-bot: I haz the power, 2017-04-03 14:30:33]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/efb812e9274d2f1e0c3761d73079...