Move most visited item state info from InstantController to InstantService.

Move most visited item state info from InstantController to InstantService.

BUG=none
TEST=none

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

[kmadhusu, 2013-05-13 23:42:41]

Please review.

Thanks.

[samarth, 2013-05-15 19:22:27]

Hey,

I think this one is probably better off reviewed by Sreeram since he knows the
most-visited code much better. Let me know if you still want me to review this.

Thanks,
Samarth

[kmadhusu, 2013-05-20 16:49:17]

On 2013/05/15 19:22:27, samarth wrote:
> Hey,
> 
> I think this one is probably better off reviewed by Sreeram since he knows the
> most-visited code much better. Let me know if you still want me to review
this.
> 
> Thanks,
> Samarth

Np Samarth. 

Thanks.

[samarth, 2013-05-21 16:21:45]

samarth -> jered

[Jered, 2013-05-21 20:18:21]

Generally looks good, just nits.

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

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:165: const GURL& most_visited_item_url)
{
"item_url" or "url" would be clear to me.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:254: Browser* browser =
chrome::FindBrowserWithProfile(profile_,
Is it correct to send the update only to one browser?

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:263:
std::vector<InstantMostVisitedItemIDPair> items_with_ids;
Instead, expose UpdateMostVisitedItems() and call that.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1634: void
InstantController::SendMostVisitedItems(
Inline this in UpdateMostVisitedItems() and expose that. That way, IC can only
send items from IS, which is a slightly less open-ended interface.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:252: void OnDeleteMostVisitedItem(const
GURL& item_url);
Just "url" is fine here and below, since the method name makes it obvious what
the url is for.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page_unittest.cc (right):

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page_unittest.cc:50:
MOCK_METHOD1(DeleteMostVisitedItem, void(const GURL& item_url));
It's a little pedantic, but you could test that OnMessageReceived() dispatches
calls to these mocks.

[kmadhusu, 2013-05-23 18:26:21]

Addressed comments. PTAL.

Thanks.

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

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:165: const GURL& most_visited_item_url)
{
On 2013/05/21 20:18:21, Jered wrote:
> "item_url" or "url" would be clear to me.

most_visited_item_url => url. Fixed.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:254: Browser* browser =
chrome::FindBrowserWithProfile(profile_,
On 2013/05/21 20:18:21, Jered wrote:
> Is it correct to send the update only to one browser?

Good catch. Fixed to send updates to all the browsers with |profile_|.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:263:
std::vector<InstantMostVisitedItemIDPair> items_with_ids;
On 2013/05/21 20:18:21, Jered wrote:
> Instead, expose UpdateMostVisitedItems() and call that.

Done.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1634: void
InstantController::SendMostVisitedItems(
On 2013/05/21 20:18:21, Jered wrote:
> Inline this in UpdateMostVisitedItems() and expose that. That way, IC can only
> send items from IS, which is a slightly less open-ended interface.

Done.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:252: void OnDeleteMostVisitedItem(const
GURL& item_url);
On 2013/05/21 20:18:21, Jered wrote:
> Just "url" is fine here and below, since the method name makes it obvious what
> the url is for.

Done.

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page_unittest.cc (right):

https://codereview.chromium.org/14660022/diff/19001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page_unittest.cc:50:
MOCK_METHOD1(DeleteMostVisitedItem, void(const GURL& item_url));
On 2013/05/21 20:18:21, Jered wrote:
> It's a little pedantic, but you could test that OnMessageReceived() dispatches
> calls to these mocks.

Done.

[Jered, 2013-05-24 15:58:21]

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

https://codereview.chromium.org/14660022/diff/67001/chrome/common/instant_res...
chrome/common/instant_restricted_id_cache.h:134: // cache_.Put() can invalidate
the iterator |last_add_start_| is pointing to.
So were you seeing last_add_start_ reset for i == 0, and then become invalidated
inside the loop above?

From reading mru_cache, I think that should only happen if items contains
duplicate restricted ids or is larger than the cache capacity. Both seem like
deeper problems. Backing up by items.size() would not be correct in those cases
because we did not add that many items.

[Jered, 2013-05-24 19:23:02]

lgtm

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

https://codereview.chromium.org/14660022/diff/67001/chrome/common/instant_res...
chrome/common/instant_restricted_id_cache.h:134: // cache_.Put() can invalidate
the iterator |last_add_start_| is pointing to.
On 2013/05/24 15:58:21, Jered wrote:
> So were you seeing last_add_start_ reset for i == 0, and then become
invalidated
> inside the loop above?
> 
> From reading mru_cache, I think that should only happen if items contains
> duplicate restricted ids or is larger than the cache capacity. Both seem like
> deeper problems. Backing up by items.size() would not be correct in those
cases
> because we did not add that many items.

Ok, got it. This is sort of nasty and subtle. :-)

To avoid actually having a dummy before begin(), reverse_iterator defines
rbegin() to be end() and rend() maps to begin(). operator* accesses i-1. So
--rend() actually points at the last element of the _previously_ added MV items,
it's just that operator* makes it look like the first added item from this
batch. Yuck.

So, in your unit test, when erase() removes item 12, i.e. the last item of the
previously added batch, that invalidates last_add_start_.

I'm not sure why we're not seeing a bazillion renderer crashes because of this.

Anyway, I am fine with this fix. I would be _even happier_ with never ever using
reverse_iterators at all, because this behavior is really confusing and
dangerous.

[kmadhusu, 2013-05-24 20:12:23]

sreeram@: Please do the OWNER's check for chrome/browser/search/* changes.

brettw@: Please do the OWNER's check for chrome/common/* 

Thanks.

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

https://codereview.chromium.org/14660022/diff/67001/chrome/common/instant_res...
chrome/common/instant_restricted_id_cache.h:134: // cache_.Put() can invalidate
the iterator |last_add_start_| is pointing to.
On 2013/05/24 19:23:02, Jered wrote:
> On 2013/05/24 15:58:21, Jered wrote:
> > So were you seeing last_add_start_ reset for i == 0, and then become
> invalidated
> > inside the loop above?
> > 
> > From reading mru_cache, I think that should only happen if items contains
> > duplicate restricted ids or is larger than the cache capacity. Both seem
like
> > deeper problems. Backing up by items.size() would not be correct in those
> cases
> > because we did not add that many items.
> 
> Ok, got it. This is sort of nasty and subtle. :-)
> 
> To avoid actually having a dummy before begin(), reverse_iterator defines
> rbegin() to be end() and rend() maps to begin(). operator* accesses i-1. So
> --rend() actually points at the last element of the _previously_ added MV
items,
> it's just that operator* makes it look like the first added item from this
> batch. Yuck.
> 
> So, in your unit test, when erase() removes item 12, i.e. the last item of the
> previously added batch, that invalidates last_add_start_.
> 
> I'm not sure why we're not seeing a bazillion renderer crashes because of
this.
> 
> Anyway, I am fine with this fix. I would be _even happier_ with never ever
using
> reverse_iterators at all, because this behavior is really confusing and
> dangerous.

Yes. I completely agree. Reverse_iterator is very confusing. I will  clean up
this file in a separate CL.

[kmadhusu, 2013-05-28 16:57:59]

palmer@: Please do the OWNER's check for chrome/common/* changes.

Thanks.

[kmadhusu, 2013-05-29 20:55:20]

Jered@: Modified InstantService::OnMostVisitedItemsReceived() to fix the android
bot compile error. PTAL.

Thanks.

[palmer, 2013-05-29 21:06:05]

LGTM

[Jered, 2013-05-29 21:39:38]

slgtm

On 2013/05/29 21:06:05, Chromium Palmer wrote:
> LGTM

[kmadhusu, 2013-05-29 22:48:24]

On 2013/05/29 21:39:38, Jered wrote:
> slgtm
> 
> On 2013/05/29 21:06:05, Chromium Palmer wrote:
> > LGTM

sreeram@: Friendly ping. PTAL.

Thanks.

[sreeram, 2013-06-03 20:40:42]

LGTM

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:52: if (top_sites) {
Nit: This works for now, because the InstantService isn't created until first
use, but is brittle. I.e., in reality this check succeeds and we get Most
Visited info because nobody currently calls
InstantServiceFactory::GetForProfile() early enough. It would be better to do
the following:
1. Declare the dependencies for InstantServiceFactory (for example, see
https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...).
2. Instantiate the factory in ChromeBrowserMainExtraPartsProfiles::
EnsureBrowserContextKeyedServiceFactoriesBuilt().

Feel free to do the above in a followup CL (add a TODO or bug).

Note: I'm not talking about the "if" check. That's certainly a good idea and you
should keep it. In fact, I'm not recommending any change to the code here. I
just mean that, in production, we want the check to succeed more reliably, than
just by accident.

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:265: }
This is okay for now. But I think it would be cleaner to have each
InstantController register itself as an InstantServiceObserver and push out
updates that way. See
https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...

Again, fine to do as a cleanup.

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
chrome/browser/search/instant_service.h:99: // Called when we get new most
visited items from the TopNav code,
TopNav? Did you mean TopSites?

[kmadhusu, 2013-06-03 21:40:48]

sreeram: Addressed comments. Can I add you as an OWNER to review
chrome/common/instant_restricted_id_cache.h && .cc changes?

Thanks.

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:52: if (top_sites) {
On 2013/06/03 20:40:42, sreeram wrote:
> Nit: This works for now, because the InstantService isn't created until first
> use, but is brittle. I.e., in reality this check succeeds and we get Most
> Visited info because nobody currently calls
> InstantServiceFactory::GetForProfile() early enough. It would be better to do
> the following:
> 1. Declare the dependencies for InstantServiceFactory (for example, see
>
https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...).
> 2. Instantiate the factory in ChromeBrowserMainExtraPartsProfiles::
> EnsureBrowserContextKeyedServiceFactoriesBuilt().
> 
> Feel free to do the above in a followup CL (add a TODO or bug).
> 
> Note: I'm not talking about the "if" check. That's certainly a good idea and
you
> should keep it. In fact, I'm not recommending any change to the code here. I
> just mean that, in production, we want the check to succeed more reliably,
than
> just by accident.

Filed crbug.com/246351.

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
chrome/browser/search/instant_service.cc:265: }
On 2013/06/03 20:40:42, sreeram wrote:
> This is okay for now. But I think it would be cleaner to have each
> InstantController register itself as an InstantServiceObserver and push out
> updates that way. See
>
https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...
> 
> Again, fine to do as a cleanup.

Filed crbug.com/246355.

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/14660022/diff/133001/chrome/browser/search/in...
chrome/browser/search/instant_service.h:99: // Called when we get new most
visited items from the TopNav code,
On 2013/06/03 20:40:42, sreeram wrote:
> TopNav? Did you mean TopSites?

Changed TopNav ->TopSites.

[commit-bot: I haz the power, 2013-06-03 22:12:47]

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

[commit-bot: I haz the power, 2013-06-03 22:44:17]

Retried try job too often on win7_aura for step(s) ash_unittests,
aura_unittests, browser_tests, compositor_unittests, content_browsertests,
content_unittests, interactive_ui_tests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-06-04 00:02:42]

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

[commit-bot: I haz the power, 2013-06-04 03:19:53]

Change committed as 203821