Popular sites on the NTP: Favicons!

Popular sites on the NTP: Favicons!

Follow-up to https://codereview.chromium.org/1265983007/

BUG=516618
Committed: https://crrev.com/7f5cf8749f6aa3a2ec9c03f363c11749a277b0be
Cr-Commit-Position: refs/heads/master@{#342814}

[Marc Treib, 2015-08-11 09:13:28]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-08-11 09:13:29]

PTAL!

[Bernhard Bauer, 2015-08-11 09:37:26]

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
File chrome/browser/android/favicon_helper.cc (right):

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/favicon_helper.cc:278: GURL page_url =
GURL(ConvertJavaStringToUTF8(env, j_page_url));
You can directly initialize the variable (as opposed to assigning it an
explicitly constructed GURL).

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/favicon_helper.cc:281: ScopedJavaGlobalRef<jobject>*
j_scoped_callback =
Where is this object destroyed?

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:465: if (!observer_.is_null()) {
Early-return if the observer is null?

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:472:
NotifyPopularURLsObserver(urls, favicon_urls);
I think you could inline this method.

[Marc Treib, 2015-08-11 10:23:33]

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
File chrome/browser/android/favicon_helper.cc (right):

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/favicon_helper.cc:278: GURL page_url =
GURL(ConvertJavaStringToUTF8(env, j_page_url));
On 2015/08/11 09:37:25, Bernhard Bauer wrote:
> You can directly initialize the variable (as opposed to assigning it an
> explicitly constructed GURL).

I'm pretty sure that amounts to the exact same thing (i.e. no assignment will be
performed anyway). But still, nicer to just initialize directly. Done.

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/favicon_helper.cc:281: ScopedJavaGlobalRef<jobject>*
j_scoped_callback =
On 2015/08/11 09:37:25, Bernhard Bauer wrote:
> Where is this object destroyed?

It wasn't :)
Now it's done in the Bind below via base::Owned.

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:465: if (!observer_.is_null()) {
On 2015/08/11 09:37:25, Bernhard Bauer wrote:
> Early-return if the observer is null?

Done.

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:472:
NotifyPopularURLsObserver(urls, favicon_urls);
On 2015/08/11 09:37:25, Bernhard Bauer wrote:
> I think you could inline this method.

Done.

[Bernhard Bauer, 2015-08-11 10:45:07]

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
File chrome/browser/android/favicon_helper.cc (right):

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/favicon_helper.cc:281: ScopedJavaGlobalRef<jobject>*
j_scoped_callback =
On 2015/08/11 10:23:33, Marc Treib wrote:
> On 2015/08/11 09:37:25, Bernhard Bauer wrote:
> > Where is this object destroyed?
> 
> It wasn't :)
> Now it's done in the Bind below via base::Owned.

Urr... except now it will be destroyed at the end of
OnFaviconImageResultAvailable(), which means you'll pass an invalid pointer to
the callback bound to OnFaviconDownloaded().

Really, you want something like base::Passed() here so there is no chance of a
leak or use-after-free, or just directly copy the ScopedJavaGlobalRef around
(it's just a wrapper around jobject, so copying isn't that expensive, and you
can use const-ref like we usually do).

[Marc Treib, 2015-08-11 11:22:42]

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
File chrome/browser/android/favicon_helper.cc (right):

https://codereview.chromium.org/1272883004/diff/20001/chrome/browser/android/...
chrome/browser/android/favicon_helper.cc:281: ScopedJavaGlobalRef<jobject>*
j_scoped_callback =
On 2015/08/11 10:45:06, Bernhard Bauer wrote:
> On 2015/08/11 10:23:33, Marc Treib wrote:
> > On 2015/08/11 09:37:25, Bernhard Bauer wrote:
> > > Where is this object destroyed?
> > 
> > It wasn't :)
> > Now it's done in the Bind below via base::Owned.
> 
> Urr... except now it will be destroyed at the end of
> OnFaviconImageResultAvailable(), which means you'll pass an invalid pointer to
> the callback bound to OnFaviconDownloaded().
> 
> Really, you want something like base::Passed() here so there is no chance of a
> leak or use-after-free, or just directly copy the ScopedJavaGlobalRef around
> (it's just a wrapper around jobject, so copying isn't that expensive, and you
> can use const-ref like we usually do).

Durr. That was silly, sorry. (And good catch!)
Now passing around copies.

[Bernhard Bauer, 2015-08-11 11:34:32]

LGTM!

[Marc Treib, 2015-08-11 11:34:59]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-08-11 11:35:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272883004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272883004/60001

[commit-bot: I haz the power, 2015-08-11 13:11:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-11 13:12:33]

Patchset 4 (id:??) landed as
https://crrev.com/7f5cf8749f6aa3a2ec9c03f363c11749a277b0be
Cr-Commit-Position: refs/heads/master@{#342814}