Fix crash when icons for popular sites are loaded before the suggestions.

Fix crash when icons for popular sites are loaded before the suggestions.

BUG=579712
Committed: https://crrev.com/12a62b08d77fb5cdac1ddd754b1a6a81875e92fd
Cr-Commit-Position: refs/heads/master@{#370737}

[knn, 2016-01-21 13:51:06]

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

[knn, 2016-01-21 13:51:07]

PTAL. Thanks!

[Bernhard Bauer, 2016-01-21 16:16:54]

lgtm

https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:983:
public void onIconUpdated(final String url) {
Alternatively, you could return here if |mMostVisitedItems| is null, which would
make the contract a little simpler.

[knn, 2016-01-21 16:56:02]

Thanks for the review!

https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:983:
public void onIconUpdated(final String url) {
On 2016/01/21 16:16:54, Bernhard Bauer wrote:
> Alternatively, you could return here if |mMostVisitedItems| is null, which
would
> make the contract a little simpler.

Yeah it is slightly better, I chose this as it makes it easier to cherry-pick to
m48 where there are multiple onIconUpdated calls.

[knn, 2016-01-21 16:56:14]

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

[commit-bot: I haz the power, 2016-01-21 16:56:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1613933002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1613933002/1

[commit-bot: I haz the power, 2016-01-21 17:36:36]

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

[commit-bot: I haz the power, 2016-01-21 17:36:37]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-01-21 17:43:28]

https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:983:
public void onIconUpdated(final String url) {
On 2016/01/21 16:56:01, knn wrote:
> On 2016/01/21 16:16:54, Bernhard Bauer wrote:
> > Alternatively, you could return here if |mMostVisitedItems| is null, which
> would
> > make the contract a little simpler.
> 
> Yeah it is slightly better, I chose this as it makes it easier to cherry-pick
to
> m48 where there are multiple onIconUpdated calls.

…and we only want to do this for one of them? Or are you agreeing with my
suggestion, and I'm just missing an additional patch set?

[knn, 2016-01-21 18:01:09]

On 2016/01/21 17:43:28, Bernhard Bauer wrote:
>
https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
> File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
> (right):
> 
>
https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:983:
> public void onIconUpdated(final String url) {
> On 2016/01/21 16:56:01, knn wrote:
> > On 2016/01/21 16:16:54, Bernhard Bauer wrote:
> > > Alternatively, you could return here if |mMostVisitedItems| is null, which
> > would
> > > make the contract a little simpler.
> > 
> > Yeah it is slightly better, I chose this as it makes it easier to
cherry-pick
> to
> > m48 where there are multiple onIconUpdated calls.
> 
> …and we only want to do this for one of them? Or are you agreeing with my
> suggestion, and I'm just missing an additional patch set?

We want to do it for both of them (Icon + Thumbnail NTP even though one is
deprecated).
I was just being lazy with the cherry-pick, let me upload a fresh patch.

[knn, 2016-01-21 18:02:59]

The CQ bit was checked by knn@chromium.org

[knn, 2016-01-21 18:02:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1613933002/#ps20001
(title: " ")

[Bernhard Bauer, 2016-01-21 18:03:48]

On 2016/01/21 18:01:09, knn wrote:
> On 2016/01/21 17:43:28, Bernhard Bauer wrote:
> >
>
https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
> > File
> chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1613933002/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:983:
> > public void onIconUpdated(final String url) {
> > On 2016/01/21 16:56:01, knn wrote:
> > > On 2016/01/21 16:16:54, Bernhard Bauer wrote:
> > > > Alternatively, you could return here if |mMostVisitedItems| is null,
which
> > > would
> > > > make the contract a little simpler.
> > > 
> > > Yeah it is slightly better, I chose this as it makes it easier to
> cherry-pick
> > to
> > > m48 where there are multiple onIconUpdated calls.
> > 
> > …and we only want to do this for one of them? Or are you agreeing with my
> > suggestion, and I'm just missing an additional patch set?
> 
> We want to do it for both of them (Icon + Thumbnail NTP even though one is
> deprecated).
> I was just being lazy with the cherry-pick, let me upload a fresh patch.

Gotcha, no worries.

[commit-bot: I haz the power, 2016-01-21 18:04:08]

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

[commit-bot: I haz the power, 2016-01-21 18:43:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-21 18:44:18]

Description was changed from

==========
Fix crash when icons for popular sites are loaded before the suggestions.

BUG=579712
==========

to

==========
Fix crash when icons for popular sites are loaded before the suggestions.

BUG=579712

Committed: https://crrev.com/12a62b08d77fb5cdac1ddd754b1a6a81875e92fd
Cr-Commit-Position: refs/heads/master@{#370737}
==========

[commit-bot: I haz the power, 2016-01-21 18:44:19]

Patchset 2 (id:??) landed as
https://crrev.com/12a62b08d77fb5cdac1ddd754b1a6a81875e92fd
Cr-Commit-Position: refs/heads/master@{#370737}