Ensure using chrome://thumb2/... adds the URL as a forced URL in TopSites so that a thumbnail is ca…

Ensure using chrome://thumb2/... adds the URL as a forced URL in TopSites so that a thumbnail is captured next time it is visited.

Following up on patches:
- https://codereview.chromium.org/53283004/
- https://codereview.chromium.org/39053005/

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236639

[beaudoin, 2013-11-13 18:20:59]

Hi Brett,

The third (and last) CL in my series to add forced URL to TopSites. This one
links the chrome://thumb2/... URL in a way that adds the requested URL to forced
URLs in TopSites.

Thanks for the review!

[beaudoin, 2013-11-15 04:07:51]

On 2013/11/13 18:20:59, beaudoin wrote:
> Hi Brett,
> 
> The third (and last) CL in my series to add forced URL to TopSites. This one
> links the chrome://thumb2/... URL in a way that adds the requested URL to
forced
> URLs in TopSites.
> 
> Thanks for the review!

Hi Brett, friendly ping on this. Last one, let's wrap this up! :)

[beaudoin, 2013-11-18 15:58:39]

On 2013/11/15 04:07:51, beaudoin wrote:
> On 2013/11/13 18:20:59, beaudoin wrote:
> > Hi Brett,
> > 
> > The third (and last) CL in my series to add forced URL to TopSites. This one
> > links the chrome://thumb2/... URL in a way that adds the requested URL to
> forced
> > URLs in TopSites.
> > 
> > Thanks for the review!
> 
> Hi Brett, friendly ping on this. Last one, let's wrap this up! :)

Brett: Think you can get around to this today?

[brettw, 2013-11-19 06:38:09]

https://codereview.chromium.org/59903010/diff/90001/chrome/browser/history/to...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/59903010/diff/90001/chrome/browser/history/to...
chrome/browser/history/top_sites_impl.cc:619: if
(new_list[index].last_forced_time.is_null())
Non-forced URLs might move around a lot. Will this mean that such URLs could get
automatically dropped off the list even though we're intending to force them?

[beaudoin, 2013-11-19 11:59:45]

https://codereview.chromium.org/59903010/diff/90001/chrome/browser/history/to...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/59903010/diff/90001/chrome/browser/history/to...
chrome/browser/history/top_sites_impl.cc:619: if
(new_list[index].last_forced_time.is_null())
On 2013/11/19 06:38:09, brettw wrote:
> Non-forced URLs might move around a lot. Will this mean that such URLs could
get
> automatically dropped off the list even though we're intending to force them?

I've thought about that. I don't think it will be a problem in practice:
- Forced URLs are nice-to-have but they don't offer a guarantee (we don't know
when the user will visit the website next, so we don't know when we'll have a
thumbnail for it)
- If a URL is dropped from the non-forced list chances are it's not really that
popular on this machine so it's probably not needed in the forced list. (The
forced list is mostly useful when there are small differences in the URLs
generated by Most Likely and those generated by Most Visited.)

I could keep a "bool forced" field with each MostVisitedURL so when we drop a
non-forced URL with a non-zero timestamp we'd move it to the forced list. This
is quite involved though so I strongly suggest we only do it if we notice a
problem with the current approach.

[brettw, 2013-11-19 23:21:52]

Sounds OK, just wanted to make sure we thought about that.

LGTM

[commit-bot: I haz the power, 2013-11-20 01:10:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/59903010/90001

[commit-bot: I haz the power, 2013-11-20 02:39:56]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[beaudoin, 2013-11-20 03:54:23]

Hi Scott, Evan,

I'll need your review on this, I missed Brett wasn't owner on:
  chrome/browser/thumbnails/**
  chrome/browser/ui/webui/ntp/**

[sky, 2013-11-20 15:10:49]

LGTM

[Evan Stade, 2013-11-20 22:32:36]

https://codereview.chromium.org/59903010/diff/90001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/thumbnail_source.h (right):

https://codereview.chromium.org/59903010/diff/90001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_source.h:56: // Indicate that this source
will force the eventual capture of requested URLs
I can't understand this comment. Can you rewrite it with more context?

"forced" doesn't seem like the best choice of adjective. Maybe "eager" or
"aggressive"?

[beaudoin, 2013-11-21 00:06:45]

https://codereview.chromium.org/59903010/diff/90001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/thumbnail_source.h (right):

https://codereview.chromium.org/59903010/diff/90001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_source.h:56: // Indicate that this source
will force the eventual capture of requested URLs
On 2013/11/20 22:32:36, Evan Stade wrote:
> I can't understand this comment. Can you rewrite it with more context?
> 
> "forced" doesn't seem like the best choice of adjective. Maybe "eager" or
> "aggressive"?

You're right, it's a bit opaque. Is the new one better?

[Evan Stade, 2013-11-21 19:06:05]

new comment is great, thanks! lgtm

[commit-bot: I haz the power, 2013-11-21 19:40:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/59903010/470001

[commit-bot: I haz the power, 2013-11-22 00:01:52]

Change committed as 236639