ntp_tiles: Avoid redundant calls to TopSites

ntp_tiles: Avoid redundant calls to TopSites

The redundant calls cause propagating redundant updates to client
observers, as reflected by tests. This should be harmless, but let's
avoid possible UI flickering.

BUG=674909
Review-Url: https://codereview.chromium.org/2577253003
Cr-Commit-Position: refs/heads/master@{#442234}
Committed: https://chromium.googlesource.com/chromium/src/+/d0da75e5f451ce330c3cc3c1448c624a88e5af1b

[mastiz, 2016-12-16 16:00:24]

mastiz@chromium.org changed reviewers:
+ sfiera@chromium.org

[sfiera, 2016-12-16 16:15:03]

https://codereview.chromium.org/2577253003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2577253003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:172: if
(weak_ptr_factory_.HasWeakPtrs())
This seems brittle—it assumes we won't ever use weak pointers for anything but
top sites queries.

Can we add a bool outstanding_top_sites_query_ instead?

[mastiz, 2017-01-05 11:50:45]

https://codereview.chromium.org/2577253003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2577253003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:172: if
(weak_ptr_factory_.HasWeakPtrs())
On 2016/12/16 16:15:03, sfiera wrote:
> This seems brittle—it assumes we won't ever use weak pointers for anything but
> top sites queries.
> 
> Can we add a bool outstanding_top_sites_query_ instead?

You have a point, but adding yet another state variable looks as fragile to me
if not more.

What would you think of renaming to top_sites_weak_ptr_factor_? Not sure if this
violates a popular pattern.

[sfiera, 2017-01-05 13:22:09]

On 2017/01/05 11:50:45, mastiz wrote:
>
https://codereview.chromium.org/2577253003/diff/1/components/ntp_tiles/most_v...
> File components/ntp_tiles/most_visited_sites.cc (right):
> 
>
https://codereview.chromium.org/2577253003/diff/1/components/ntp_tiles/most_v...
> components/ntp_tiles/most_visited_sites.cc:172: if
> (weak_ptr_factory_.HasWeakPtrs())
> On 2016/12/16 16:15:03, sfiera wrote:
> > This seems brittle—it assumes we won't ever use weak pointers for anything
but
> > top sites queries.
> > 
> > Can we add a bool outstanding_top_sites_query_ instead?
> 
> You have a point, but adding yet another state variable looks as fragile to me
> if not more.
> 
> What would you think of renaming to top_sites_weak_ptr_factor_? Not sure if
this
> violates a popular pattern.

It looks like weak ptr factories sometimes have more specific names, so I guess
that's OK.

[mastiz, 2017-01-09 08:50:49]

On 2017/01/05 13:22:09, sfiera wrote:
> On 2017/01/05 11:50:45, mastiz wrote:
> >
>
https://codereview.chromium.org/2577253003/diff/1/components/ntp_tiles/most_v...
> > File components/ntp_tiles/most_visited_sites.cc (right):
> > 
> >
>
https://codereview.chromium.org/2577253003/diff/1/components/ntp_tiles/most_v...
> > components/ntp_tiles/most_visited_sites.cc:172: if
> > (weak_ptr_factory_.HasWeakPtrs())
> > On 2016/12/16 16:15:03, sfiera wrote:
> > > This seems brittle—it assumes we won't ever use weak pointers for anything
> but
> > > top sites queries.
> > > 
> > > Can we add a bool outstanding_top_sites_query_ instead?
> > 
> > You have a point, but adding yet another state variable looks as fragile to
me
> > if not more.
> > 
> > What would you think of renaming to top_sites_weak_ptr_factor_? Not sure if
> this
> > violates a popular pattern.
> 
> It looks like weak ptr factories sometimes have more specific names, so I
guess
> that's OK.

Done.

[sfiera, 2017-01-09 09:57:42]

LGTM

[mastiz, 2017-01-09 10:20:48]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-01-09 10:21:03]

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-01-09 11:36:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483957248305040,
"parent_rev": "cee5fb4a5456777f64bc8a2359b3431f713f6448", "commit_rev":
"d0da75e5f451ce330c3cc3c1448c624a88e5af1b"}

[commit-bot: I haz the power, 2017-01-09 11:37:18]

Description was changed from

==========
ntp_tiles: Avoid redundant calls to TopSites

The redundant calls cause propagating redundant updates to client
observers, as reflected by tests. This should be harmless, but let's
avoid possible UI flickering.

BUG=674909
==========

to

==========
ntp_tiles: Avoid redundant calls to TopSites

The redundant calls cause propagating redundant updates to client
observers, as reflected by tests. This should be harmless, but let's
avoid possible UI flickering.

BUG=674909

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

[commit-bot: I haz the power, 2017-01-09 11:37:19]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d0da75e5f451ce330c3cc3c1448c...