Rip out browser-side RID caching for most visited items.

Rip out browser-side RID caching for most visited items.

(1) Most visited thumbnails and favicons no longer require id-based urls. Updated ThumbnailSource and FaviconSource to serve requests of the form:
   chrome-search://favicon/<most_visited_item_favicon_url>
      chrome-search://thumb/<most_visited_item_thumbnail_url>

(2) Removed |most_visited_items_cache_| from InstantIOContext and InstantService.
(3) SearchTabHelper will track the last sent most visited items. This prevents duplicate most visited item related IPCs while switching tabs.

BUG=239253, 225760, 242667
TEST=none

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

[kmadhusu, 2013-05-30 18:27:47]

Jered: CL is ready for review. Still working on interactive ui tests. PTAL.

Thanks.

[Jered, 2013-05-31 17:34:33]

https://codereview.chromium.org/15907006/diff/22/chrome/browser/search/instan...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/15907006/diff/22/chrome/browser/search/instan...
chrome/browser/search/instant_service.cc:175:
std::vector<InstantMostVisitedItem> most_visited_items;
Call this new_most_visited_items to make the distinction between it and
most_visited_items_ clearer.

https://codereview.chromium.org/15907006/diff/22/chrome/browser/search/search.h
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/15907006/diff/22/chrome/browser/search/search...
chrome/browser/search/search.h:226: // Returns true if |new_items| and
|old_items| are equal.
Instead of "new" and "old", you can just call these items_a and items_b or
something (the names should reflect that the test is symmetric.)

https://codereview.chromium.org/15907006/diff/22/chrome/browser/ui/webui/favi...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/15907006/diff/22/chrome/browser/ui/webui/favi...
chrome/browser/ui/webui/favicon_source.cc:165: std::string path =
request->url().path().substr(1);
Why do the !path.empty() check here? I think we can omit this, since we're not
actually checking that the path is valid and it seems odd to just check this one
way in which it could be bad.

https://codereview.chromium.org/15907006/diff/22/chrome/browser/ui/webui/favi...
chrome/browser/ui/webui/favicon_source.cc:177: bool
FaviconSource::ParsePath(const std::string& raw_path,
Here and elsewhere, change "raw_path" to "path".

https://codereview.chromium.org/15907006/diff/22/chrome/browser/ui/webui/favi...
File chrome/browser/ui/webui/favicon_source_unittest.cc (right):

https://codereview.chromium.org/15907006/diff/22/chrome/browser/ui/webui/favi...
chrome/browser/ui/webui/favicon_source_unittest.cc:57: const std::string
path("chrome-search/favicon/http://www.google.com");
What is up with the "chrome-search" prefixed to this path?

https://codereview.chromium.org/15907006/diff/22/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/thumbnail_source.cc (right):

https://codereview.chromium.org/15907006/diff/22/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:38: const std::string& raw_path,
raw_path -> path

https://codereview.chromium.org/15907006/diff/22/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:68: std::string raw_path =
request->url().path().substr(1);
Same comment here about the check for an empty path.

https://codereview.chromium.org/15907006/diff/22/chrome/renderer/searchbox/se...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/15907006/diff/22/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:94: v8::Handle<v8::String>
GenerateThumbnailURL(const GURL& url) {
Instead of overloading based on types, give these functions different names like
GenerateThumbnailURLFromRID() and GenerateThumbnailURL().

https://codereview.chromium.org/15907006/diff/22/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:175: // Set |privileged_mode|
to true to set the most visited item url in thumbnail
Instead of doing this, how about always specifying the URLs in terms of RIDs,
but rewriting them in ChromeContentRendererClient?

That way the page never deals with URLs explicitly, but the requests from
renderer -> browser are still always framed in terms of URLs.

https://codereview.chromium.org/15907006/diff/22/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:211:
obj->Set(v8::String::New("faviconUrl"), GenerateFaviconURL(mv_item.url));
This is priveleged.

[kmadhusu, 2013-06-04 02:36:01]

Addressed comments. Initialized InstantRestictedIDCache::last_restricted_id_ to
a random value. This fixes the MVI iframe issues. Once I get your initial
comments, I will update InstantRestrictedIDCacheTest and InstantExtendedTest.

PTAL. Thanks.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/search...
File chrome/browser/search/instant_service.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/search...
chrome/browser/search/instant_service.cc:175:
std::vector<InstantMostVisitedItem> most_visited_items;
On 2013/05/31 17:34:33, Jered wrote:
> Call this new_most_visited_items to make the distinction between it and
> most_visited_items_ clearer.

Done.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/search...
File chrome/browser/search/search.h (right):

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/search...
chrome/browser/search/search.h:226: // Returns true if |new_items| and
|old_items| are equal.
On 2013/05/31 17:34:33, Jered wrote:
> Instead of "new" and "old", you can just call these items_a and items_b or
> something (the names should reflect that the test is symmetric.)

Done.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/ui/web...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/ui/web...
chrome/browser/ui/webui/favicon_source.cc:165: std::string path =
request->url().path().substr(1);
On 2013/05/31 17:34:33, Jered wrote:
> Why do the !path.empty() check here? I think we can omit this, since we're not
> actually checking that the path is valid and it seems odd to just check this
one
> way in which it could be bad.

It was just a safety check. Removed.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/ui/web...
chrome/browser/ui/webui/favicon_source.cc:177: bool
FaviconSource::ParsePath(const std::string& raw_path,
On 2013/05/31 17:34:33, Jered wrote:
> Here and elsewhere, change "raw_path" to "path".

Done.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/ui/web...
File chrome/browser/ui/webui/favicon_source_unittest.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/ui/web...
chrome/browser/ui/webui/favicon_source_unittest.cc:57: const std::string
path("chrome-search/favicon/http://www.google.com");
On 2013/05/31 17:34:33, Jered wrote:
> What is up with the "chrome-search" prefixed to this path?

I missed ":/" in the path. Fixed.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/ui/web...
File chrome/browser/ui/webui/ntp/thumbnail_source.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/ui/web...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:38: const std::string& raw_path,
On 2013/05/31 17:34:33, Jered wrote:
> raw_path -> path

Done.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/browser/ui/web...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:68: std::string raw_path =
request->url().path().substr(1);
On 2013/05/31 17:34:33, Jered wrote:
> Same comment here about the check for an empty path.

Fixed.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/renderer/searc...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/renderer/searc...
chrome/renderer/searchbox/searchbox_extension.cc:94: v8::Handle<v8::String>
GenerateThumbnailURL(const GURL& url) {
On 2013/05/31 17:34:33, Jered wrote:
> Instead of overloading based on types, give these functions different names
like
> GenerateThumbnailURLFromRID() and GenerateThumbnailURL().

Done. As per the next comment, I moved GenerateThumbnailURLFromRID to
SearchBox.cc.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/renderer/searc...
chrome/renderer/searchbox/searchbox_extension.cc:175: // Set |privileged_mode|
to true to set the most visited item url in thumbnail
On 2013/05/31 17:34:33, Jered wrote:
> Instead of doing this, how about always specifying the URLs in terms of RIDs,
> but rewriting them in ChromeContentRendererClient?
> 
> That way the page never deals with URLs explicitly, but the requests from
> renderer -> browser are still always framed in terms of URLs.

Done.

https://chromiumcodereview.appspot.com/15907006/diff/22/chrome/renderer/searc...
chrome/renderer/searchbox/searchbox_extension.cc:211:
obj->Set(v8::String::New("faviconUrl"), GenerateFaviconURL(mv_item.url));
On 2013/05/31 17:34:33, Jered wrote:
> This is priveleged.

Fixed.

[Jered, 2013-06-04 16:09:48]

https://codereview.chromium.org/15907006/diff/39002/chrome/common/instant_res...
File chrome/common/instant_restricted_id_cache.h (right):

https://codereview.chromium.org/15907006/diff/39002/chrome/common/instant_res...
chrome/common/instant_restricted_id_cache.h:90:
last_restricted_id_(base::RandGenerator(kint32max)) {
Let's try to fix this in a deterministic way.

Currently thumbnails and favicons are cacheable because
URLDataSource::AllowCaching() returns true. That's correct if they're identified
by c-s://f/URL, but not if they're identified by RID since RIDs are transient.
Based on the behavior we're seeing, I suspect that's the case.

Option #1: Have ::AllowCaching() return false for Thumbnail/Favicon sources.
That'd be unfortunate for other users with non-RID based URLs, however, if there
aren't any (say if Instant Extended is enabled), we should just do that.

Option #2: Override the cache policy for RID-based requests. This is done in
RenderViewImpl::willSendRequest() but plumbing the cache policy override in
looks like it may be a little tricky.

[kmadhusu, 2013-06-04 21:55:48]

On 2013/06/04 16:09:48, Jered wrote:
>
https://codereview.chromium.org/15907006/diff/39002/chrome/common/instant_res...
> File chrome/common/instant_restricted_id_cache.h (right):
> 
>
https://codereview.chromium.org/15907006/diff/39002/chrome/common/instant_res...
> chrome/common/instant_restricted_id_cache.h:90:
> last_restricted_id_(base::RandGenerator(kint32max)) {
> Let's try to fix this in a deterministic way.
> 
> Currently thumbnails and favicons are cacheable because
> URLDataSource::AllowCaching() returns true. That's correct if they're
identified
> by c-s://f/URL, but not if they're identified by RID since RIDs are transient.
> Based on the behavior we're seeing, I suspect that's the case.
> 
> Option #1: Have ::AllowCaching() return false for Thumbnail/Favicon sources.
> That'd be unfortunate for other users with non-RID based URLs, however, if
there
> aren't any (say if Instant Extended is enabled), we should just do that.
> 
> Option #2: Override the cache policy for RID-based requests. This is done in
> RenderViewImpl::willSendRequest() but plumbing the cache policy override in
> looks like it may be a little tricky.

As we discussed, I changed the thumbnailURL and faviconURL format as follows:

chrome-search://favicon/<render_view_id>:<restricted_id> 
chrome-search://thumb/<render_view_id>:<restricted_id> 

PTAL.

Thanks.

[sreeram, 2013-06-04 21:59:17]

On 2013/06/04 21:55:48, kmadhusu wrote:
> As we discussed, I changed the thumbnailURL and faviconURL format as follows:
> 
> chrome-search://favicon/<render_view_id>:<restricted_id> 
> chrome-search://thumb/<render_view_id>:<restricted_id> 

Pedro is trying to support complex favicon URL formats in
https://codereview.chromium.org/15388002. To make life easier, how about
".../restricted_id/render_view_id" instead?

[kmadhusu, 2013-06-04 22:06:09]

On 2013/06/04 21:59:17, sreeram wrote:
> On 2013/06/04 21:55:48, kmadhusu wrote:
> > As we discussed, I changed the thumbnailURL and faviconURL format as
follows:
> > 
> > chrome-search://favicon/<render_view_id>:<restricted_id> 
> > chrome-search://thumb/<render_view_id>:<restricted_id> 
> 
> Pedro is trying to support complex favicon URL formats in
> https://codereview.chromium.org/15388002. To make life easier, how about
> ".../restricted_id/render_view_id" instead?

Sure. Fixed. PTAL.

[Jered, 2013-06-04 23:19:19]

lgtm

This looks much better. I am pretty happy with all the Instant stuff and think
it's ready for broader review. I spent a long time convincing myself the rvid
comes from RPH and is unique per-process which has the right granularity for
what you're using it for here.

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1038: return
GetInstantRestrictedIDFromURL(render_view->GetRoutingID(), url,
If the rewrite fails, we'll send a bad URL to chrome-search://{favicon,thumb}.
Have you checked that they don't fall over in that case?

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:178: InstantRestrictedID
restricted_id,
Can we also order these as render_view_id, restricted_id?

[Jered, 2013-06-04 23:27:22]

slgtm

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:215: bool
GetInstantRestrictedIDFromURL(int render_view_id,
I realized this function might make more sense in searchbox or in instant_types
than here. What do you think?

[kmadhusu, 2013-06-05 00:48:29]

jered@: Addressed comments. 

Adding reviewers to do OWNER's check.

estade@: chrome/browser/ui/webui/* changes.
samarth@: chrome/browser/search/* changes.
palmer@: chrome/common/* changes.
jam@: chrome/renderer/chrome_content_renderer_client.cc

OWNER's PTAL.

Thanks.

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:215: bool
GetInstantRestrictedIDFromURL(int render_view_id,
On 2013/06/04 23:27:23, Jered wrote:
> I realized this function might make more sense in searchbox or in
instant_types
> than here. What do you think?

Moved to SearchBox.

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1038: return
GetInstantRestrictedIDFromURL(render_view->GetRoutingID(), url,
On 2013/06/04 23:19:20, Jered wrote:
> If the rewrite fails, we'll send a bad URL to chrome-search://{favicon,thumb}.
> Have you checked that they don't fall over in that case?

If rewrite fails, we will send a bad URL to FaviconSource/ThumbnailSource. 

FaviconSource/ThumbnailSource::StartDataRequest will handle the situation
gracefully and return default favicon/thumbnail.

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/15907006/diff/50003/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:178: InstantRestrictedID
restricted_id,
On 2013/06/04 23:19:20, Jered wrote:
> Can we also order these as render_view_id, restricted_id?

Done.

[jam, 2013-06-05 16:36:01]

lgtm

https://codereview.chromium.org/15907006/diff/74001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/15907006/diff/74001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:205: content::RenderView*
GetRenderViewFromWebFrame(WebKit::WebFrame* frame) {
nit: no need for this method. we know that frame is not null. we also know that
view is not null. so just inline content::RenderView::FromWebView in line 1006.

[palmer, 2013-06-05 17:50:54]

IPC security LGTM

https://codereview.chromium.org/15907006/diff/74001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/15907006/diff/74001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:188: most_visited_items_))
Nit: I think the style guide wants you to use curly braces here.

I kind of do, too. :)

https://codereview.chromium.org/15907006/diff/74001/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/15907006/diff/74001/chrome/browser/search/sea...
chrome/browser/search/search.cc:740: items_b[i].title != items_a[i].title)
Nit: Might need curly braces here, too.

I could be wrong, though; I am not a style guide expert and I am too lazy to
look it up. :)

https://codereview.chromium.org/15907006/diff/74001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15907006/diff/74001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:188: transient_url, &rid))
Nit: curly braces, here and on line 202

[kmadhusu, 2013-06-05 18:40:38]

jam@, palmer@: Addressed comments. PTAL.

Thanks.

https://chromiumcodereview.appspot.com/15907006/diff/74001/chrome/browser/sea...
File chrome/browser/search/instant_service.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/74001/chrome/browser/sea...
chrome/browser/search/instant_service.cc:188: most_visited_items_))
On 2013/06/05 17:50:54, Chromium Palmer wrote:
> Nit: I think the style guide wants you to use curly braces here.
> 
> I kind of do, too. :)

Done.

https://chromiumcodereview.appspot.com/15907006/diff/74001/chrome/browser/sea...
File chrome/browser/search/search.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/74001/chrome/browser/sea...
chrome/browser/search/search.cc:740: items_b[i].title != items_a[i].title)
On 2013/06/05 17:50:54, Chromium Palmer wrote:
> Nit: Might need curly braces here, too.
> 
> I could be wrong, though; I am not a style guide expert and I am too lazy to
> look it up. :)

Done.

https://chromiumcodereview.appspot.com/15907006/diff/74001/chrome/renderer/ch...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/74001/chrome/renderer/ch...
chrome/renderer/chrome_content_renderer_client.cc:205: content::RenderView*
GetRenderViewFromWebFrame(WebKit::WebFrame* frame) {
On 2013/06/05 16:36:01, jam wrote:
> nit: no need for this method. we know that frame is not null. we also know
that
> view is not null. so just inline content::RenderView::FromWebView in line
1006.

Done.

https://chromiumcodereview.appspot.com/15907006/diff/74001/chrome/renderer/se...
File chrome/renderer/searchbox/searchbox.cc (right):

https://chromiumcodereview.appspot.com/15907006/diff/74001/chrome/renderer/se...
chrome/renderer/searchbox/searchbox.cc:188: transient_url, &rid))
On 2013/06/05 17:50:54, Chromium Palmer wrote:
> Nit: curly braces, here and on line 202

Done.

[samarth, 2013-06-05 22:59:04]

Awesome to see no more flashing!

https://codereview.chromium.org/15907006/diff/87001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:187: if
(chrome::AreMostVisitedItemsEqual(new_most_visited_items,
Is this check necessary here? We already check for diffs in SearchTabHelper and
in general, do we get spurious updates from the most visited service?

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:998: if (SearchBox* search_box
= SearchBox::Get(render_view)) {
nit: I prefer keeping initialization outside of conditionals

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:36: bool
GetInstantRestrictedIDFromURL(int render_view_id,
This deserves a unit test.

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:44: if (Tokenize(path, "/", &tokens) !=
2)
No include needed for this?

[Evan Stade, 2013-06-05 23:57:31]

webui lgtm

[kmadhusu, 2013-06-06 02:27:38]

samarth@: Addressed comments. PTAL.

Thanks.

https://codereview.chromium.org/15907006/diff/87001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:187: if
(chrome::AreMostVisitedItemsEqual(new_most_visited_items,
On 2013/06/05 22:59:04, samarth wrote:
> Is this check necessary here? We already check for diffs in SearchTabHelper
and
> in general, do we get spurious updates from the most visited service?

Occasionally, InstantService receives spurious updates from most visited
service. We don't want to notify InstantController during those situations.

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:998: if (SearchBox* search_box
= SearchBox::Get(render_view)) {
On 2013/06/05 22:59:04, samarth wrote:
> nit: I prefer keeping initialization outside of conditionals

Done.

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:36: bool
GetInstantRestrictedIDFromURL(int render_view_id,
On 2013/06/05 22:59:04, samarth wrote:
> This deserves a unit test.

Done. Moved the helper function from anonymous namespace to an util file for
testing.

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:44: if (Tokenize(path, "/", &tokens) !=
2)
On 2013/06/05 22:59:04, samarth wrote:
> No include needed for this?

Included header file.

[samarth, 2013-06-06 16:38:44]

LGTM

It would also be nice to add an interactive UI test that did the following:
1) Open NTP in tab A
2) Open NTP in tab B
3) switch from B to A
4) Confirm that tab A got no onmostvisitedchanged event

Thanks!
Samarth

https://codereview.chromium.org/15907006/diff/87001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:187: if
(chrome::AreMostVisitedItemsEqual(new_most_visited_items,
On 2013/06/06 02:27:38, kmadhusu wrote:
> On 2013/06/05 22:59:04, samarth wrote:
> > Is this check necessary here? We already check for diffs in SearchTabHelper
> and
> > in general, do we get spurious updates from the most visited service?
> 
> Occasionally, InstantService receives spurious updates from most visited
> service. We don't want to notify InstantController during those situations.

The spurious update would still get caught and suppressed downstream in
SearchTabHelper, no?

I don't feel too strongly about this so it's OK to leave this in if you really
prefer. But I think it's cleaner to do this check only in one place that matters
(not to mention that will allow you to not expose the AreMostVisitedItemsEqual
in search.h which feels a little strange).

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:36: bool
GetInstantRestrictedIDFromURL(int render_view_id,
On 2013/06/06 02:27:38, kmadhusu wrote:
> On 2013/06/05 22:59:04, samarth wrote:
> > This deserves a unit test.
> 
> Done. Moved the helper function from anonymous namespace to an util file for
> testing.

Instead of making a new file for this, how about just changing the anonymous
namespace to a "namespace internal".  You can then still access it in a test.
See for example searchbox_extension.cc and its corresponding tests.

[kmadhusu, 2013-06-07 01:34:07]

samarth@: Addressed comments. I will write an interactive ui test in a follow up
CL (filed crbug.com/247554). PTAL.

Thanks.

https://codereview.chromium.org/15907006/diff/87001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:187: if
(chrome::AreMostVisitedItemsEqual(new_most_visited_items,
On 2013/06/06 16:38:45, samarth wrote:
> On 2013/06/06 02:27:38, kmadhusu wrote:
> > On 2013/06/05 22:59:04, samarth wrote:
> > > Is this check necessary here? We already check for diffs in
SearchTabHelper
> > and
> > > in general, do we get spurious updates from the most visited service?
> > 
> > Occasionally, InstantService receives spurious updates from most visited
> > service. We don't want to notify InstantController during those situations.
> 
> The spurious update would still get caught and suppressed downstream in
> SearchTabHelper, no?
> 
> I don't feel too strongly about this so it's OK to leave this in if you really
> prefer. But I think it's cleaner to do this check only in one place that
matters
> (not to mention that will allow you to not expose the AreMostVisitedItemsEqual
> in search.h which feels a little strange).

I will leave it here for now. Will remove it once I fix crbug.com/246355.

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/15907006/diff/87001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:36: bool
GetInstantRestrictedIDFromURL(int render_view_id,
On 2013/06/06 16:38:45, samarth wrote:
> On 2013/06/06 02:27:38, kmadhusu wrote:
> > On 2013/06/05 22:59:04, samarth wrote:
> > > This deserves a unit test.
> > 
> > Done. Moved the helper function from anonymous namespace to an util file for
> > testing.
> 
> Instead of making a new file for this, how about just changing the anonymous
> namespace to a "namespace internal".  You can then still access it in a test.
> See for example searchbox_extension.cc and its corresponding tests.

Done.

[samarth, 2013-06-07 01:43:10]

(CC Pedro FYI since he'll need to update some of the URL parsing code for hi-dpi
favicons)

SLGTM

[commit-bot: I haz the power, 2013-06-07 18:16:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/15907006/135001

[commit-bot: I haz the power, 2013-06-07 21:58:58]

Failed to apply patch for chrome/browser/ui/search/search_tab_helper.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/search/search_tab_helper.h
  Hunk #1 FAILED at 5.
  Hunk #2 succeeded at 42 with fuzz 2 (offset -6 lines).
  Hunk #3 succeeded at 82 (offset -27 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/search/search_tab_helper.h.rej

Patch:       chrome/browser/ui/search/search_tab_helper.h
Index: chrome/browser/ui/search/search_tab_helper.h
diff --git a/chrome/browser/ui/search/search_tab_helper.h
b/chrome/browser/ui/search/search_tab_helper.h
index
d7f260f5a49a9b63b4b99b87c92ee553b6ec1b32..8dab85cb68dd4a4a4bce65aa3bdf8364ad716a0f
100644
--- a/chrome/browser/ui/search/search_tab_helper.h
+++ b/chrome/browser/ui/search/search_tab_helper.h
@@ -5,10 +5,13 @@
 #ifndef CHROME_BROWSER_UI_SEARCH_SEARCH_TAB_HELPER_H_
 #define CHROME_BROWSER_UI_SEARCH_SEARCH_TAB_HELPER_H_
 
+#include <vector>
+
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "chrome/browser/ui/search/search_model.h"
+#include "chrome/common/instant_types.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -48,6 +51,11 @@ class SearchTabHelper : public content::NotificationObserver,
   // Invoked to update the Instant support state.
   void InstantSupportChanged(bool supports_instant);
 
+  // Updates |last_known_most_visited_items_| with |items|.
+  // Returns false if |items| matches the |last_known_most_visited_items_|.
+  bool UpdateLastKnownMostVisitedItems(
+      const std::vector<InstantMostVisitedItem>& items);
+
  private:
   friend class content::WebContentsUserData<SearchTabHelper>;
   friend class InstantPageTest;
@@ -104,6 +112,10 @@ class SearchTabHelper : public
content::NotificationObserver,
 
   content::WebContents* web_contents_;
 
+  // Tracks the last set of most visited items sent to the InstantPage
renderer.
+  // Used to prevent sending duplicate IPC messages to the renderer.
+  std::vector<InstantMostVisitedItem> last_known_most_visited_items_;
+
   DISALLOW_COPY_AND_ASSIGN(SearchTabHelper);
 };

[commit-bot: I haz the power, 2013-06-07 23:15:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/15907006/153001

[commit-bot: I haz the power, 2013-06-08 00:59:39]

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

[commit-bot: I haz the power, 2013-06-08 01:01:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/15907006/153001

[commit-bot: I haz the power, 2013-06-08 05:13:38]

Change committed as 205026