Separate checking the user identity and checking if the user is syncing his history in two differen…

Separate checking the user identity and checking if the user is syncing his history in two different Search API javascript calls.

The check for history sync is noticeably slower and hangs until sync is up. As a result we want to do it at a different location in the NTP Javascript code. This requires splitting the two checks that were conflated together in CheckIsUserSignedInToChromeAs before.

Matching server-side cl: cr/80408232

BUG=

Committed: https://crrev.com/709ff0205573855bf5087fbc82303dd96feff13d
Cr-Commit-Position: refs/heads/master@{#305295}

[beaudoin, 2014-11-05 01:47:13]

beaudoin@chromium.org changed reviewers:
+ mathp@chromium.org

[beaudoin, 2014-11-05 01:47:14]

Hi Mathieu,

Can you sanity check that CL before I send it to the various owners for review?

Thanks!

[Mathieu, 2014-11-05 13:38:27]

On 2014/11/05 01:47:14, beaudoin wrote:
> Hi Mathieu,
> 
> Can you sanity check that CL before I send it to the various owners for
review?
> 
> Thanks!

Looks great! My main comment (which is repeated but not everywhere) is that the
names should be consistent with the identity check, and remove the "User" part
in the name because it's implicit.

CheckHistorySync, OnHistorySyncCheckResult, etc.

[Mathieu, 2014-11-05 13:38:43]

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
chrome/browser/ui/search/search_ipc_router.h:86: // Calls
SendCheckUserSyncHistoryResult with true if the user syncs their
to be consistent with name above, I would call it SendHistorySyncCheckResult.

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
chrome/browser/ui/search/search_ipc_router.h:219: void
OnCheckUserSyncHistory(int page_seq_no) const;
nit: OnHistorySyncCheck

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
chrome/browser/ui/search/search_tab_helper.cc:571: void
SearchTabHelper::OnCheckUserSyncHistory() {
OnHistorySyncCheck

https://codereview.chromium.org/701973002/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/701973002/diff/1/chrome/common/render_message...
chrome/common/render_messages.h:276:
IPC_MESSAGE_ROUTED1(ChromeViewMsg_CheckUserSyncHistoryResult,
HistorySyncCheckResult

https://codereview.chromium.org/701973002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/701973002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/searchbox_api.js:182:
this.onusersynchistorydone = null;
onsynchistorycheckdone

https://codereview.chromium.org/701973002/diff/1/chrome/renderer/searchbox/se...
File chrome/renderer/searchbox/searchbox_extension.h (right):

https://codereview.chromium.org/701973002/diff/1/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.h:48: bool sync_history);
indent 1 more

[beaudoin, 2014-11-05 21:28:21]

Thanks for the review! PTAL?

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
chrome/browser/ui/search/search_ipc_router.h:86: // Calls
SendCheckUserSyncHistoryResult with true if the user syncs their
On 2014/11/05 13:38:42, Mathieu Perreault wrote:
> to be consistent with name above, I would call it SendHistorySyncCheckResult.

Done.

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
chrome/browser/ui/search/search_ipc_router.h:219: void
OnCheckUserSyncHistory(int page_seq_no) const;
On 2014/11/05 13:38:42, Mathieu Perreault wrote:
> nit: OnHistorySyncCheck

Done.

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/701973002/diff/1/chrome/browser/ui/search/sea...
chrome/browser/ui/search/search_tab_helper.cc:571: void
SearchTabHelper::OnCheckUserSyncHistory() {
On 2014/11/05 13:38:42, Mathieu Perreault wrote:
> OnHistorySyncCheck

Done.

https://codereview.chromium.org/701973002/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/701973002/diff/1/chrome/common/render_message...
chrome/common/render_messages.h:276:
IPC_MESSAGE_ROUTED1(ChromeViewMsg_CheckUserSyncHistoryResult,
On 2014/11/05 13:38:42, Mathieu Perreault wrote:
> HistorySyncCheckResult

Done.

https://codereview.chromium.org/701973002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/701973002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/searchbox_api.js:182:
this.onusersynchistorydone = null;
On 2014/11/05 13:38:42, Mathieu Perreault wrote:
> onsynchistorycheckdone

Used onhistorysynccheckdone for consistency.

https://codereview.chromium.org/701973002/diff/1/chrome/renderer/searchbox/se...
File chrome/renderer/searchbox/searchbox_extension.h (right):

https://codereview.chromium.org/701973002/diff/1/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.h:48: bool sync_history);
On 2014/11/05 13:38:42, Mathieu Perreault wrote:
> indent 1 more

Done.

[Mathieu, 2014-11-06 14:12:53]

lgtm, couple nits

https://codereview.chromium.org/701973002/diff/20001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/701973002/diff/20001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router_unittest.cc:61:
MOCK_METHOD0(OnHistorySyncCheck, void());
sorry to be nit-picky, but sometimes you call it HistorySync and sometimes
SyncHistory! I prefer HistorySync.

https://codereview.chromium.org/701973002/diff/20001/chrome/common/render_mes...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/701973002/diff/20001/chrome/common/render_mes...
chrome/common/render_messages.h:603: // The Instant page asks whether the user
sync its history.
*syncs

[beaudoin, 2014-11-07 16:16:24]

https://codereview.chromium.org/701973002/diff/20001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/701973002/diff/20001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router_unittest.cc:61:
MOCK_METHOD0(OnHistorySyncCheck, void());
On 2014/11/06 14:12:52, Mathieu Perreault wrote:
> sorry to be nit-picky, but sometimes you call it HistorySync and sometimes
> SyncHistory! I prefer HistorySync.

Thanks for catching it. Search & Replace mishap. Hopefully I got them all this
time.

https://codereview.chromium.org/701973002/diff/20001/chrome/common/render_mes...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/701973002/diff/20001/chrome/common/render_mes...
chrome/common/render_messages.h:603: // The Instant page asks whether the user
sync its history.
On 2014/11/06 14:12:52, Mathieu Perreault wrote:
> *syncs

Done.

[beaudoin, 2014-11-07 16:22:01]

beaudoin@chromium.org changed reviewers:
+ dcheng@chromium.org, jered@chromium.org

[beaudoin, 2014-11-07 16:22:02]

R+jered, dcheng

Owners please review:
jered :
   chrome/browser/ui/search/*
   chrome/renderer/resources/extensions/searchbox_api.js
   chrome/renderer/searchbox/*
dcheng : chrome/common/render_messages.h

Thanks!

[Mathieu, 2014-11-12 15:03:08]

On 2014/11/07 16:22:02, beaudoin wrote:
> R+jered, dcheng
> 
> Owners please review:
> jered :
>    chrome/browser/ui/search/*
>    chrome/renderer/resources/extensions/searchbox_api.js
>    chrome/renderer/searchbox/*
> dcheng : chrome/common/render_messages.h
> 
> Thanks!

Ping to Jered and dcheng

[dcheng, 2014-11-12 21:40:47]

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.cc:349:
delegate_->OnInstantSupportDetermined(true);
I don't really understand how this works. Why does receiving this message imply
that this page must support instant?

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.h:138: // Tells the renderer whether
the user sync his history.
syncs

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router_unittest.cc:461:
scoped_ptr<IPC::Message> message(new ChromeViewHostMsg_HistorySyncCheck(
Nit: Can't this (and line 477) just be stack allocated?

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper_unittest.cc (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper_unittest.cc:279:
ASSERT_NE(static_cast<SearchTabHelper*>(NULL), search_tab_helper);
ASSERT_NE(nullptr, search_tab_helper);

though it's even more concise (and preferred) to just say:

ASSERT(search_tab_helper);

Similar comment applies to line 285.

[Mathieu, 2014-11-18 13:51:40]

mathp@chromium.org changed reviewers:
+ samarth@chromium.org
- jered@chromium.org

[Mathieu, 2014-11-18 13:52:02]

-Jered (ooo), +samarth

Hi Samarth, can you look at Philippe's CL?

[Mathieu, 2014-11-18 20:43:22]

mathp@chromium.org changed reviewers:
+ kmadhusu@chromium.org

[Mathieu, 2014-11-18 20:43:31]

+kmadhusu

[kmadhusu, 2014-11-19 19:56:19]

I assume there is a server side change corresponding to this CL. Please add the
server side CL link in the description for future references.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.cc:69: sync_history));
style nit: sync_history can go to the previous line.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.cc:349:
delegate_->OnInstantSupportDetermined(true);
On 2014/11/12 21:40:47, dcheng wrote:
> I don't really understand how this works. Why does receiving this message
imply
> that this page must support instant?

We handle this message only when the web contents are rendered in the Instant
Process (Ref: SearchIPCRouter::OnMessageReceived). NewTab contents are rendered
in the Instant process. Hence we know that the page supports Instant.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.h:138: // Tells the renderer whether
the user sync his history.
nit: Please avoid using "his/her" in the comment.

https://codereview.chromium.org/701973002/diff/40001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/extensions/searchbox_api.js:182:
this.onhistorysynccheckdone = null;
Please make sure you update http://www.chromium.org/embeddedsearch document
about this new function.

[beaudoin, 2014-11-20 22:58:26]

Answered all comments, PTAL.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router.cc (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.cc:69: sync_history));
On 2014/11/19 19:56:19, kmadhusu wrote:
> style nit: sync_history can go to the previous line.

Done.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.cc:349:
delegate_->OnInstantSupportDetermined(true);
On 2014/11/12 21:40:47, dcheng wrote:
> I don't really understand how this works. Why does receiving this message
imply
> that this page must support instant?

IIUC, all the API calls here are only performed when the NTP supports instant.
Whenever one of these is called we flip a bit to remember that we have an
instant-enabled NTP.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.h:138: // Tells the renderer whether
the user sync his history.
On 2014/11/12 21:40:47, dcheng wrote:
> syncs

Done.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router.h:138: // Tells the renderer whether
the user sync his history.
On 2014/11/19 19:56:19, kmadhusu wrote:
> nit: Please avoid using "his/her" in the comment.

Done.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router_unittest.cc:461:
scoped_ptr<IPC::Message> message(new ChromeViewHostMsg_HistorySyncCheck(
On 2014/11/12 21:40:47, dcheng wrote:
> Nit: Can't this (and line 477) just be stack allocated?

Just copied what was everywhere else. I've updated the entire file.

https://codereview.chromium.org/701973002/diff/40001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/extensions/searchbox_api.js:182:
this.onhistorysynccheckdone = null;
On 2014/11/19 19:56:19, kmadhusu wrote:
> Please make sure you update http://www.chromium.org/embeddedsearch document
> about this new function.

I will do as soon as this is landed. I'll also document
checkIsUserSyncingHistory

Done.

[kmadhusu, 2014-11-21 17:41:13]

lgtm

[dcheng, 2014-11-21 20:50:32]

lgtm

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router_unittest.cc:461:
scoped_ptr<IPC::Message> message(new ChromeViewHostMsg_HistorySyncCheck(
On 2014/11/20 22:58:26, beaudoin wrote:
> On 2014/11/12 21:40:47, dcheng wrote:
> > Nit: Can't this (and line 477) just be stack allocated?
> 
> Just copied what was everywhere else. I've updated the entire file.

Sorry, you should have just pushed back on this. I didn't notice it was the
pattern in the entire file.

https://codereview.chromium.org/701973002/diff/80001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router_policy_impl.cc (right):

https://codereview.chromium.org/701973002/diff/80001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router_policy_impl.cc:62: bool
SearchIPCRouterPolicyImpl::ShouldProcessHistorySyncCheck() {
This shouldn't block this patch, but it's a bit strange to have so many policy
checks, most of which have the same body. Perhaps there's a better way to
express these policy checks that wouldn't require so many identical method
bodies...

[beaudoin, 2014-11-21 21:16:37]

Committing.

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router_unittest.cc (right):

https://codereview.chromium.org/701973002/diff/40001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router_unittest.cc:461:
scoped_ptr<IPC::Message> message(new ChromeViewHostMsg_HistorySyncCheck(
On 2014/11/21 20:50:32, dcheng wrote:
> On 2014/11/20 22:58:26, beaudoin wrote:
> > On 2014/11/12 21:40:47, dcheng wrote:
> > > Nit: Can't this (and line 477) just be stack allocated?
> > 
> > Just copied what was everywhere else. I've updated the entire file.
> 
> Sorry, you should have just pushed back on this. I didn't notice it was the
> pattern in the entire file.

I agreed with you and figured if no one ever did it we'd keep on using the bad
pattern. :)

https://codereview.chromium.org/701973002/diff/80001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_ipc_router_policy_impl.cc (right):

https://codereview.chromium.org/701973002/diff/80001/chrome/browser/ui/search...
chrome/browser/ui/search/search_ipc_router_policy_impl.cc:62: bool
SearchIPCRouterPolicyImpl::ShouldProcessHistorySyncCheck() {
On 2014/11/21 20:50:32, dcheng wrote:
> This shouldn't block this patch, but it's a bit strange to have so many policy
> checks, most of which have the same body. Perhaps there's a better way to
> express these policy checks that wouldn't require so many identical method
> bodies...

Agreed. I'm not familiar with policy stuff though...

[beaudoin, 2014-11-21 21:16:38]

The CQ bit was checked by beaudoin@chromium.org

[commit-bot: I haz the power, 2014-11-21 21:17:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701973002/80001

[commit-bot: I haz the power, 2014-11-21 22:06:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-21 22:06:50]

Patchset 5 (id:??) landed as
https://crrev.com/709ff0205573855bf5087fbc82303dd96feff13d
Cr-Commit-Position: refs/heads/master@{#305295}