Export a private webstore API to call into the new inline sign-in flow.

Export a private webstore API to call into the new inline sign-in flow.

BUG=347247
TEST=
  (1) Start out with a brand new Chrome profile, that you've never signed into.
  (2) Navigate to https://chrome.google.com/webstore/category/collection/for_your_desktop
  (3) Click on an app (any app).
  (4) Click on the "+ Free" link to install the app.
  At this point, you should be automatically navigated to a sign-in URL.
  That URL should be hosted on chrome://chrome-signin/, *not* on
  https://accounts.google.com/ServiceLogin
R=guohui@chromium.org

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

[Ilya Sherman, 2014-03-12 06:34:29]

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:692: bool
WebstorePrivateSignInFunction::RunImpl() {
Hui, this CL is not nearly ready for a full review, but could you please take a
look at this method and let me know if it looks like I'm on the right track?

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:704:
return false;
Am I right to be using a SigninManager here?

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:708:
return true;
Is it appropriate to bail if the user is already signed in?

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:711: //
if (signin_manager->AuthInProgress()) ...
What's the expected behavior if there is already an authentication in progress? 
Incidentally, does "AuthInProgress()" returning |true| mean that there is a
sign-in page open, or that the user has submitted data through a sign-in page,
but the server hasn't yet replied?

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:714: //
in a new tab.
Are you aware of existing code that I can re-use to do this, or do I likely need
to build some plumbing here to enable this functionality?

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:718:
login_ui_service->ShowLoginPopup();
Is a LoginUIService the best way to show an inline weblogin page hosted on the
chrome:// protocol?

[guohui, 2014-03-12 18:17:39]

Thanks Ilya!

I think it looks good in general, please see my comments below.

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:692: bool
WebstorePrivateSignInFunction::RunImpl() {
On 2014/03/12 06:34:29, Ilya Sherman wrote:
> Hui, this CL is not nearly ready for a full review, but could you please take
a
> look at this method and let me know if it looks like I'm on the right track?
it looks good in general, plz see my comments below.

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:704:
return false;
On 2014/03/12 06:34:29, Ilya Sherman wrote:
> Am I right to be using a SigninManager here?
yes, i think so, though i think you need to notify the webstore of the failure
through a callback, so that webstore could handle different cases properly.

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:708:
return true;
On 2014/03/12 06:34:29, Ilya Sherman wrote:
> Is it appropriate to bail if the user is already signed in?

yup, again we should notify the webstore of the result via a callback.

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:711: //
if (signin_manager->AuthInProgress()) ...
On 2014/03/12 06:34:29, Ilya Sherman wrote:
> What's the expected behavior if there is already an authentication in
progress? 
> Incidentally, does "AuthInProgress()" returning |true| mean that there is a
> sign-in page open, or that the user has submitted data through a sign-in page,
> but the server hasn't yet replied?
this means signin is already triggered through a sign-in page, but not finished
yet, since signin is a multi-step process involving multiple calls to Gaia and
sync server thus it could take a few seconds or more. The simplest solution is
to just return a status code via callback.

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:714: //
in a new tab.
On 2014/03/12 06:34:29, Ilya Sherman wrote:
> Are you aware of existing code that I can re-use to do this, or do I likely
need
> to build some plumbing here to enable this functionality?

hmm i think you could simply navigate the current tab to the signin URL, see
example at 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/196783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:718:
login_ui_service->ShowLoginPopup();
On 2014/03/12 06:34:29, Ilya Sherman wrote:
> Is a LoginUIService the best way to show an inline weblogin page hosted on the
> chrome:// protocol?

LoginUIService::ShowLoginPopup shows the login UI in a singleton tab, thus i
don't think we should use it here.

[guohui, 2014-03-12 18:18:59]

Let me know when it is ready for a full review, looking forward to it =)

https://codereview.chromium.org/196783002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/webstore_private.json (right):

https://codereview.chromium.org/196783002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/webstore_private.json:263: "description": "The
continue URL to navigate to once sign-in completes.",
you are probably still working on it, but i think the function needs at least
one input param for the continue url and one input param for the callback to
notify webstore of the result.

[Ilya Sherman, 2014-03-14 05:42:56]

The code is now functional, except that I'm not handling the case where the user
chooses to tweak Sync settings after sign-in (see my question inline).

I think the code is more or less ready for a full review, though it'll still
need at least one more nontrivial iteration before it's in really good shape.  I
still have lots of questions, which I've written as TODOs in the code, and tried
to call out via inline comments.  I also haven't written any tests yet, because
I'm not at all confident that the behavior is generally headed in the right
direction or not.  I'm mostly keen on gaining better understanding of these
questions right now, so if you prefer to just focus on those and any high-level
issues (rather than giving a detailed review), that's perfectly great too :)

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:708: //
to set it as the last |error_|?
^^^

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:739: //
always the option of exponential backoff or something...)
^^^

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:744: //
significantly delayed, then the extra message is probably needed.)
^^^

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:771: //
anything like that.
Do you think we should try to have a smaller set of error strings that might be
less descriptive than possible in some cases; or would it be better to have a
distinct error string for each possible code path?

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:781: //
desired?
^^^

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:791: //  
    complete?  If not, is there a way to listen for a failure?
^^^

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:800: //
merging the session fail?
^^^

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:66: // to pass it
on, the tab crashes... I'm not exactly sure why.
^^^

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:336: // reached, but
then the tab is hijacked by chrome://settings/syncSetup.
I'm not sure what to do about this.  Conceptually, what should happen here? 
Should the sync setup open in a new tab?  Should the sync setup open in this
tab; and then redirect to the continue URL after the user accepts or cancels the
Sync setup dialog?  (The latter would feel weird, IMO.)  Something else?  Are
there any existing flows that I can crib from?

[guohui, 2014-03-14 22:09:44]

looks much better now =) See my answers below.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:708: //
to set it as the last |error_|?
On 2014/03/14 05:42:56, Ilya Sherman wrote:
> ^^^

i think it is better to pass the error to callback, otherwise we need another
function to expose the last error to webstore.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:739: //
always the option of exponential backoff or something...)
On 2014/03/14 05:42:56, Ilya Sherman wrote:
> ^^^
definitely better to wait for it to complete, as you said it is tricky for
webstore to know when to retry. and webstore probably should have a timeout on
their side to avoid waiting forever.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:744: //
significantly delayed, then the extra message is probably needed.)
On 2014/03/14 05:42:56, Ilya Sherman wrote:
> ^^^
hmm i think webstore could have a timeout on their side, since this should be an
edge case, i think it is worth to send another reply.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:771: //
anything like that.
On 2014/03/14 05:42:56, Ilya Sherman wrote:
> Do you think we should try to have a smaller set of error strings that might
be
> less descriptive than possible in some cases; or would it be better to have a
> distinct error string for each possible code path?
i think it is better to have a smaller set of error strings, since webstore has
to duplicate them in their code, it would be hard to process and to maintain a
large set of errors in two different code bases.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:781: //
desired?
On 2014/03/14 05:42:56, Ilya Sherman wrote:
> ^^^

i don't have a strong preference. Since this is an edge case of an edge case,
you may just return error for simplicity.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:791: //  
    complete?  If not, is there a way to listen for a failure?
On 2014/03/14 05:42:56, Ilya Sherman wrote:
> ^^^
yes we should wait for mergeSession to complete otherwise the gaia cookie would
not be set, and yes signinSuccess is guaranteed to be followed by a merge
session complete.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:800: //
merging the session fail?
On 2014/03/14 05:42:56, Ilya Sherman wrote:
> ^^^

you may return a specific error 'merge_session_failed', which means Chrome
signin is successful but gaia cookie in the content area is not updated
properly, and webstore could then navigate to the gaia webpage or show an error
accordingly.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:66: // to pass it
on, the tab crashes... I'm not exactly sure why.
On 2014/03/14 05:42:56, Ilya Sherman wrote:
> ^^^

nope gaia does not need to know the continue url. Gaia is embedded in an iframe
on the inline signin webui, the gaia iframe always redirects to a blank page
with URL like www.google.com/blank upon successful gaia signin, and then the
inline signin UI would trigger Chrome signin process and may navigate to sync
settings page if user needs to configure sync settings, or if there is any error
that user needs to address, then at the end, it would navigate to the webstore
continue url.

https://codereview.chromium.org/196783002/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:336: // reached, but
then the tab is hijacked by chrome://settings/syncSetup.
yup sync setup should open in the current tab, and upon sync setup completes
Chrome navigates to the continue URL. The logic should be similar to
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Ilya Sherman, 2014-03-20 08:41:45]

Thanks for the recommendations and the code pointers!  I think the code in this
CL is more or less functional now.  I haven't written tests yet, but I'm
uploading another snapshot because I have further questions about handling the
"Choose what to sync" case:

https://chromiumcodereview.appspot.com/196783002/diff/70001/chrome/browser/ui...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://chromiumcodereview.appspot.com/196783002/diff/70001/chrome/browser/ui...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:39: // service?  Never
redirect?
^^^

https://chromiumcodereview.appspot.com/196783002/diff/70001/chrome/browser/ui...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:96: // houses the
"Configure sync" settings dialog.  WDYT?
^^^

https://chromiumcodereview.appspot.com/196783002/diff/70001/chrome/browser/ui...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:105: // "Cancel" after
opting to "Choose what to sync"?
^^^

https://chromiumcodereview.appspot.com/196783002/diff/70001/chrome/browser/ui...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://chromiumcodereview.appspot.com/196783002/diff/70001/chrome/browser/ui...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:530: // configuration
page?
^^^

[blundell, 2014-03-21 15:31:07]

Is it correct that SigninManager's *SigninProcess* APis can be deleted after
this CL lands? Thanks!

[guohui, 2014-03-21 19:04:15]

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:39: // service?  Never
redirect?
On 2014/03/20 08:41:45, Ilya Sherman wrote:
> ^^^

PSS is null in incognito mode or if sync is disabled. chrome://chrome-signin
does not load in incognito mode. Thus the only remaining case for null PSS is
when sync is disabled. Personally i think you should ignore the error and
redirect back to webstore immediately, since signin is indeed successful.

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:96: // houses the
"Configure sync" settings dialog.  WDYT?
On 2014/03/20 08:41:45, Ilya Sherman wrote:
> ^^^

these sounds like existing bugs in sync code. I suspect the culprit is in
sync_setup_handler, though i am not familiar with it. If you could repro the bug
on canary, then you may just file a bug for sync team and add a todo here. I
agree this is an edge case so we could fix it in later CLs.

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:105: // "Cancel" after
opting to "Choose what to sync"?
On 2014/03/20 08:41:45, Ilya Sherman wrote:
> ^^^

If user presses 'cancel', Chrome would sign out. In this case, i think Chrome
should just abort redirect, and stay on the settings page.

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:530: // configuration
page?
On 2014/03/20 08:41:45, Ilya Sherman wrote:
> ^^^

|sub_page| is not configuration page only if sync is disabled, in this case you
may redirect back to webstore since signin is successful.

[Ilya Sherman, 2014-03-21 19:30:18]

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:96: // houses the
"Configure sync" settings dialog.  WDYT?
On 2014/03/21 19:04:15, guohui wrote:
> On 2014/03/20 08:41:45, Ilya Sherman wrote:
> > ^^^
> 
> these sounds like existing bugs in sync code. I suspect the culprit is in
> sync_setup_handler, though i am not familiar with it. If you could repro the
bug
> on canary, then you may just file a bug for sync team and add a todo here. I
> agree this is an edge case so we could fix it in later CLs.

I don't actually know how to trigger a similar flow on Canary.  Could you give
me some STR for triggering an inline sign-in flow?

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:105: // "Cancel" after
opting to "Choose what to sync"?
On 2014/03/21 19:04:15, guohui wrote:
> On 2014/03/20 08:41:45, Ilya Sherman wrote:
> > ^^^
> 
> If user presses 'cancel', Chrome would sign out. In this case, i think Chrome
> should just abort redirect, and stay on the settings page.

Actually, the behavior that I observed was that Chrome doesn't sign out.  What
happens instead is that the user remains signed-in, but Sync is disabled.  Does
that change your answer?

[Ilya Sherman, 2014-03-21 19:32:45]

On 2014/03/21 15:31:07, blundell wrote:
> Is it correct that SigninManager's *SigninProcess* APis can be deleted after
> this CL lands? Thanks!

I'm pretty sure this CL is removing the last dependency on the https://-hosted,
web-based sign-in flow.  (Well, except that there's actually still a flag in
chrome:flags to revert all sign-in attempts back to that mode.)  I'm not
familiar enough with the code to know whether the SigninProcess APIs are no
longer needed for the chrome://-hosted inline sign-in flow.

[guohui, 2014-03-21 19:47:53]

On 2014/03/21 19:32:45, Ilya Sherman wrote:
> On 2014/03/21 15:31:07, blundell wrote:
> > Is it correct that SigninManager's *SigninProcess* APis can be deleted after
> > this CL lands? Thanks!
> 
> I'm pretty sure this CL is removing the last dependency on the
https://-hosted,
> web-based sign-in flow.  (Well, except that there's actually still a flag in
> chrome:flags to revert all sign-in attempts back to that mode.)  I'm not
> familiar enough with the code to know whether the SigninProcess APIs are no
> longer needed for the chrome://-hosted inline sign-in flow.

Discussed this with Roger. SigninProcess APIs attempts to load the chrome signin
page in an isolated process, since chrome://chrome-signin always loads in an
isolated process, thus after Ilya's CL is landed, and after the
enable-web-based-signin flag is removed, we could then remove SigninProcess
APIs.

[guohui, 2014-03-21 20:00:20]

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:96: // houses the
"Configure sync" settings dialog.  WDYT?
On 2014/03/21 19:30:18, Ilya Sherman wrote:
> On 2014/03/21 19:04:15, guohui wrote:
> > On 2014/03/20 08:41:45, Ilya Sherman wrote:
> > > ^^^
> > 
> > these sounds like existing bugs in sync code. I suspect the culprit is in
> > sync_setup_handler, though i am not familiar with it. If you could repro the
> bug
> > on canary, then you may just file a bug for sync team and add a todo here. I
> > agree this is an edge case so we could fix it in later CLs.
> 
> I don't actually know how to trigger a similar flow on Canary.  Could you give
> me some STR for triggering an inline sign-in flow?
you can trigger inline sign-in flow from the signin button on ntp or wrench
menu. I actually just tried it on canary and did not see the issue, so it may be
a real bug in this CL. Have you debugged it? Could you please the callstack when
OnStateChanged is triggered from closing the first settings page?

Also could you please elaborate on what you mean by "doesn't seem to handle the
case other than sync everthing", does the if check above fail?

[Ilya Sherman, 2014-03-21 23:53:31]

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:96: // houses the
"Configure sync" settings dialog.  WDYT?
On 2014/03/21 20:00:21, guohui wrote:
> On 2014/03/21 19:30:18, Ilya Sherman wrote:
> > On 2014/03/21 19:04:15, guohui wrote:
> > > On 2014/03/20 08:41:45, Ilya Sherman wrote:
> > > > ^^^
> > > 
> > > these sounds like existing bugs in sync code. I suspect the culprit is in
> > > sync_setup_handler, though i am not familiar with it. If you could repro
the
> > bug
> > > on canary, then you may just file a bug for sync team and add a todo here.
I
> > > agree this is an edge case so we could fix it in later CLs.
> > 
> > I don't actually know how to trigger a similar flow on Canary.  Could you
give
> > me some STR for triggering an inline sign-in flow?
> you can trigger inline sign-in flow from the signin button on ntp or wrench
> menu. I actually just tried it on canary and did not see the issue, so it may
be
> a real bug in this CL.

I tried triggering the sign-in flow from the wrench menu.  I observed that if I
"Choose what to sync", the flow never redirects back to the NTP; whereas if I
leave that checkbox unchecked, it does redirect.  I'm testing on a Mac, fwiw.

> Have you debugged it? Could you please the callstack when
> OnStateChanged is triggered from closing the first settings page?

Here's the trimmed callstack:

OneClickSigninSyncObserver::OnStateChanged() + 1900
ProfileSyncService::NotifyObservers() + 126
ProfileSyncService::OnConfigureStart() + 105
...
ProfileSyncService::ConfigureDataTypeManager() + 692
ProfileSyncService::ReconfigureDatatypeManager() + 301
ProfileSyncService::SetSetupInProgress(bool) + 170
SyncSetupHandler::CloseSyncSetup() + 734
SyncSetupHandler::~SyncSetupHandler() + 121
...
content::WebUIImpl::~WebUIImpl() + 146

It looks like a bug in the Sync WebUI handler code.  Though, I think it's
supposed to be guaranteed that the settings page is a singleton... so I'm not
sure if the maintainers of the handler would agree that this is a bug in that
code, vs. a bug in how we're using the settings page.

> Also could you please elaborate on what you mean by "doesn't seem to handle
the
> case other than sync everthing", does the if check above fail?

More specifically, this code is reached, and the LoadURL() call is issued.  It
seems like something else is also calling LoadURL() very shortly after it's
called here, and that call is overriding this one.

Here's a log with debugging statements in it:

[one_click_signin_sync_observer.cc(53)] Reached
OneClickSigninSyncObserver::OnStateChanged()
[one_click_signin_sync_observer.cc(70)] First setup is still in progress; return
early.
... The above lines are repeated three more times, then: ...
[one_click_signin_sync_observer.cc(53)] Reached
OneClickSigninSyncObserver::OnStateChanged()
[one_click_signin_sync_observer.cc(78)] Initialized successfully!  Time to
redirect to the continue URL.
[one_click_signin_sync_observer.cc(108)] Sync setup is done.  Removing self as
an observer and dying.

[Ilya Sherman, 2014-03-22 00:06:28]

Uploading a new snapshot that includes the logging lines referenced in my
previous comment.  Also addressing two of the review comments from the previous
snapshot :)

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:39: // service?  Never
redirect?
On 2014/03/21 19:04:15, guohui wrote:
> On 2014/03/20 08:41:45, Ilya Sherman wrote:
> > ^^^
> 
> PSS is null in incognito mode or if sync is disabled. chrome://chrome-signin
> does not load in incognito mode. Thus the only remaining case for null PSS is
> when sync is disabled. Personally i think you should ignore the error and
> redirect back to webstore immediately, since signin is indeed successful.

Done.

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:530: // configuration
page?
On 2014/03/21 19:04:15, guohui wrote:
> On 2014/03/20 08:41:45, Ilya Sherman wrote:
> > ^^^
> 
> |sub_page| is not configuration page only if sync is disabled, in this case
you
> may redirect back to webstore since signin is successful.

Done.

[guohui, 2014-03-24 20:55:58]

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:711: //
or to set it as the last |error_|?
how do you expose the last |error_| to webstore?

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:729:
SendResponse(false);
i think it should return true in this case, since signin is indeed successful.

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/signin/s...
File chrome/browser/signin/signin_promo.cc (right):

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/signin/s...
chrome/browser/signin/signin_promo.cc:48: const char
kSignInPromoQueryKeyContinueUrl[] = "continueUrl";
for better consistency with other param names such as "auto_close", i think we
should use continue_url here.

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_helper.cc:1560: new
OneClickSigninSyncObserver(contents, original_continue_url_);
OneClickSigninHelper is mostly used for web-based signin flow, the new inline
sign flow is handled by InlineSigninHandlerImpl, so i suspect you don't need to
touch this file at all.

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:78: DLOG(WARNING) <<
"Initialized successfully!  Time to redirect to the continue URL.";
nits: 80 chars

https://codereview.chromium.org/196783002/diff/110001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:108: DLOG(WARNING) <<
"Sync setup is done.  Removing self as an observer and dying.";
nits: 80 chars

https://codereview.chromium.org/196783002/diff/110001/chrome/common/extension...
File chrome/common/extensions/api/webstore_private.json (right):

https://codereview.chromium.org/196783002/diff/110001/chrome/common/extension...
chrome/common/extensions/api/webstore_private.json:268: "description": "Called
if the sign-in method was invoked incorrectly (e.g. with an invalid continue
URL), sign-in is unavailable (e.g. in Incognito), or if the user is already
signed in. Upon such a failure, chrome.runtime.lastError will be set to
'invalid_continue_url', 'signin_is_disallowed', 'signin_failed',
'merge_session_failed' or 'user_is_already_signed_in'.",
i think webstore has no access to chrome.runtime.lastError, have you verified
this?

[guohui, 2014-03-24 21:31:51]

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:96: // houses the
"Configure sync" settings dialog.  WDYT?
i don't think settings page is always a singleton. E.g. user can always open
multiple settings tab by typing the URL in omnibox or bookmark.

From the callstack, it seems the issue is CloseSyncSetup should not call
SetSetupInProgress when the first settings tab is closed since signin setup is
still in progress on a second tab. It does seem to me a bug in sync code. I
repro similar bug with the following steps:
1. open a settings tab
2. open a second tab and manually navigate to settings and click 'advanced sync
settings;
3. open a third tab and manually navigate to settings

at this point both tab 1 and 3 show "sync setup in progress".
Then close the tab 1 and switch to tab 3, observe that it no longer shows 'sync
setup in progress'. This means closing tab 1 does abort sync setup, even though
the setup is actually handled on the second tab.

I think you could file a bug for sync team and add a todo here.

https://codereview.chromium.org/196783002/diff/70001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:105: // "Cancel" after
opting to "Choose what to sync"?
this is actually a known bug, and roger is working on it now.

[Ilya Sherman, 2014-03-26 08:21:38]

Now with API tests!  (//chrome/browser/ui/sync/ tests still to come...) 
Apologies that I've left some debug code still in -- it's late, and I'm tired of
shaving yaks for tonight.  

This CL has gotten to be really quite large, especially with all of the tests
added.  Would you like me to start carving off smaller chunks into independent
CLs for review?  Or would you rather keep everything in a single CL, so that
there's maximal context?

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/e...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/e...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:711: //
or to set it as the last |error_|?
On 2014/03/24 20:55:58, guohui wrote:
> how do you expose the last |error_| to webstore?

Apparently, chrome.runtime.lastError is available within the scope of the
callback function.

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/e...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:729:
SendResponse(false);
On 2014/03/24 20:55:58, guohui wrote:
> i think it should return true in this case, since signin is indeed successful.

Hmm, I'm not sure that it's appropriate to return true *and* set an |error_|. 
I'll need to check with folks more familiar with the details of extensions to
get a better understanding of how this works.  I'll add a TODO for now, as a
reminder to ask about this before landing the CL.

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/s...
File chrome/browser/signin/signin_promo.cc (right):

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/s...
chrome/browser/signin/signin_promo.cc:48: const char
kSignInPromoQueryKeyContinueUrl[] = "continueUrl";
On 2014/03/24 20:55:58, guohui wrote:
> for better consistency with other param names such as "auto_close", i think we
> should use continue_url here.

Done.

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/u...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/u...
chrome/browser/ui/sync/one_click_signin_helper.cc:1560: new
OneClickSigninSyncObserver(contents, original_continue_url_);
On 2014/03/24 20:55:58, guohui wrote:
> OneClickSigninHelper is mostly used for web-based signin flow, the new inline
> sign flow is handled by InlineSigninHandlerImpl, so i suspect you don't need
to
> touch this file at all.

I'm factoring out some code from this file into the OneClickSigninSyncObserver
class.  All of the changes in this file ought to be related to that refactoring.
 There's not intended to be any changes to the functionality of this class.

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/u...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/u...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:78: DLOG(WARNING) <<
"Initialized successfully!  Time to redirect to the continue URL.";
On 2014/03/24 20:55:58, guohui wrote:
> nits: 80 chars

I'm not planning to commit these DLOG statements.  I left them in just to give
you full context about my comments on Patch Set 5.

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/common/ex...
File chrome/common/extensions/api/webstore_private.json (right):

https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/common/ex...
chrome/common/extensions/api/webstore_private.json:268: "description": "Called
if the sign-in method was invoked incorrectly (e.g. with an invalid continue
URL), sign-in is unavailable (e.g. in Incognito), or if the user is already
signed in. Upon such a failure, chrome.runtime.lastError will be set to
'invalid_continue_url', 'signin_is_disallowed', 'signin_failed',
'merge_session_failed' or 'user_is_already_signed_in'.",
On 2014/03/24 20:55:58, guohui wrote:
> i think webstore has no access to chrome.runtime.lastError, have you verified
> this?

I checked with Yoyo from the extensions team, and he informed me that it's
available in the context of the callback function.  So, for example, the call

f = function() { console.log(chrome.runtime.lastError); }
chrome.webstorePrivate.signIn("https://chrome.google.com/webstore", f);

will print

Object {message: "user_is_already_signed_in"}

if the user is already signed in.

https://chromiumcodereview.appspot.com/196783002/diff/220001/chrome/browser/e...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://chromiumcodereview.appspot.com/196783002/diff/220001/chrome/browser/e...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:153:
ASSERT_TRUE(token_service_);*/
I'm having no end of trouble from trying to substitute in these fakes, due to
lots of observers registered with the real versions of these classes not being
prepared for them being swapped out.  Is there a different/better approach that
you'd recommend for implementing the SignIn_AuthInProgress_MergeSessionFails and
SignIn_AuthInProgress_Success test cases?

[guohui, 2014-03-28 17:34:29]

On 2014/03/26 08:21:38, Ilya Sherman wrote:
> Now with API tests!  (//chrome/browser/ui/sync/ tests still to come...) 
> Apologies that I've left some debug code still in -- it's late, and I'm tired
of
> shaving yaks for tonight.  
> 
> This CL has gotten to be really quite large, especially with all of the tests
> added.  Would you like me to start carving off smaller chunks into independent
> CLs for review?  Or would you rather keep everything in a single CL, so that
> there's maximal context?
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/e...
> File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/e...
> chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:711: //
> or to set it as the last |error_|?
> On 2014/03/24 20:55:58, guohui wrote:
> > how do you expose the last |error_| to webstore?
> 
> Apparently, chrome.runtime.lastError is available within the scope of the
> callback function.
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/e...
> chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:729:
> SendResponse(false);
> On 2014/03/24 20:55:58, guohui wrote:
> > i think it should return true in this case, since signin is indeed
successful.
> 
> Hmm, I'm not sure that it's appropriate to return true *and* set an |error_|. 
> I'll need to check with folks more familiar with the details of extensions to
> get a better understanding of how this works.  I'll add a TODO for now, as a
> reminder to ask about this before landing the CL.
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/s...
> File chrome/browser/signin/signin_promo.cc (right):
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/s...
> chrome/browser/signin/signin_promo.cc:48: const char
> kSignInPromoQueryKeyContinueUrl[] = "continueUrl";
> On 2014/03/24 20:55:58, guohui wrote:
> > for better consistency with other param names such as "auto_close", i think
we
> > should use continue_url here.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/u...
> File chrome/browser/ui/sync/one_click_signin_helper.cc (right):
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/u...
> chrome/browser/ui/sync/one_click_signin_helper.cc:1560: new
> OneClickSigninSyncObserver(contents, original_continue_url_);
> On 2014/03/24 20:55:58, guohui wrote:
> > OneClickSigninHelper is mostly used for web-based signin flow, the new
inline
> > sign flow is handled by InlineSigninHandlerImpl, so i suspect you don't need
> to
> > touch this file at all.
> 
> I'm factoring out some code from this file into the OneClickSigninSyncObserver
> class.  All of the changes in this file ought to be related to that
refactoring.
>  There's not intended to be any changes to the functionality of this class.
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/u...
> File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/browser/u...
> chrome/browser/ui/sync/one_click_signin_sync_observer.cc:78: DLOG(WARNING) <<
> "Initialized successfully!  Time to redirect to the continue URL.";
> On 2014/03/24 20:55:58, guohui wrote:
> > nits: 80 chars
> 
> I'm not planning to commit these DLOG statements.  I left them in just to give
> you full context about my comments on Patch Set 5.
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/common/ex...
> File chrome/common/extensions/api/webstore_private.json (right):
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/110001/chrome/common/ex...
> chrome/common/extensions/api/webstore_private.json:268: "description": "Called
> if the sign-in method was invoked incorrectly (e.g. with an invalid continue
> URL), sign-in is unavailable (e.g. in Incognito), or if the user is already
> signed in. Upon such a failure, chrome.runtime.lastError will be set to
> 'invalid_continue_url', 'signin_is_disallowed', 'signin_failed',
> 'merge_session_failed' or 'user_is_already_signed_in'.",
> On 2014/03/24 20:55:58, guohui wrote:
> > i think webstore has no access to chrome.runtime.lastError, have you
verified
> > this?
> 
> I checked with Yoyo from the extensions team, and he informed me that it's
> available in the context of the callback function.  So, for example, the call
> 
> f = function() { console.log(chrome.runtime.lastError); }
> chrome.webstorePrivate.signIn("https://chrome.google.com/webstore", f);
> 
> will print
> 
> Object {message: "user_is_already_signed_in"}
> 
> if the user is already signed in.
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/220001/chrome/browser/e...
> File
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/196783002/diff/220001/chrome/browser/e...
>
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:153:
> ASSERT_TRUE(token_service_);*/
> I'm having no end of trouble from trying to substitute in these fakes, due to
> lots of observers registered with the real versions of these classes not being
> prepared for them being swapped out.  Is there a different/better approach
that
> you'd recommend for implementing the SignIn_AuthInProgress_MergeSessionFails
and
> SignIn_AuthInProgress_Success test cases?
+rogerta@

[Ilya Sherman, 2014-03-28 18:25:46]

Note: You are more than welcome to review this CL, but I am planning to start
breaking it up into smaller pieces today to make reviewing easier.

[gubalav, 2014-04-09 14:49:00]

lgtm

[commit-bot: I haz the power, 2014-04-09 14:49:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/196783002/220001

[commit-bot: I haz the power, 2014-04-09 14:49:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-09 14:49:14]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[gubalav, 2014-04-13 02:38:36]

lgtm

[commit-bot: I haz the power, 2014-04-13 02:38:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/196783002/220001

[commit-bot: I haz the power, 2014-04-13 02:38:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-13 02:38:47]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Ilya Sherman, 2014-04-19 07:32:39]

Ben and Charlie, please review the files with "extensions" in their name.  I
still intend to split off some of the other code into separate CLs, but the
extensions API part ought to be complete and independently reviewable at this
point.  Thanks!

[Charlie Reis, 2014-04-21 17:50:34]

On 2014/04/19 07:32:39, Ilya Sherman wrote:
> Ben and Charlie, please review the files with "extensions" in their name.  I
> still intend to split off some of the other code into separate CLs, but the
> extensions API part ought to be complete and independently reviewable at this
> point.  Thanks!

I'll defer to Ben here, since I'm not a good extensions reviewer.  (No
objections from what I saw.)

[not at google - send to devlin, 2014-04-21 18:04:41]

lgtm

the webstore internals are over my head, just one comment (repeated 3 times,
sorry) about the user action bit.

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:753:
return true;
true means to reply straight away. you should return false.

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:152:
WebstorePrivateSignInFunction::SetIgnoreUserGestureForTests(true);
you can do better than this: wrap the webstore API call in
chrome.test.runWithUserGesture from your js files, which will automatically set
that user action bit (and if you like you can even test without it).

https://codereview.chromium.org/196783002/diff/410001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/webstore_private/common.js (right):

https://codereview.chromium.org/196783002/diff/410001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webstore_private/common.js:114:
chrome.webstorePrivate.signIn(continue_url, callbackFail(error));
i.e. this would be

chrome.test.runWithUserGesture(function() {
  chrome.webstorePrivate.signIn(..);
});

.. which will give you a chance to test without a user gesture.

[Elliot Glaysher, 2014-04-21 18:19:44]

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:130:
NULL, &FakeSigninManagerBase::Build);
So I came here from the other review. You're setting this value here, but I
don't see you unsetting the global value.

(This was why I did everything based on Profile lifetime. There's no equivalent
to removing testing_factories_ in BrowserContextDestroyed here.)

[Ilya Sherman, 2014-04-22 05:02:31]

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:753:
return true;
On 2014/04/21 18:04:42, kalman wrote:
> true means to reply straight away. you should return false.

Hmm, I tried changing this to |return false|, and found that my tests broke,
whereas they worked as I expected with a |return true| here.  The semantics of
the return value from this method are pretty mysterious to me.  Could you please
clarify exactly what the return value from this method means?  Returning false
seems to set an error, based on the test output.

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:152:
WebstorePrivateSignInFunction::SetIgnoreUserGestureForTests(true);
On 2014/04/21 18:04:42, kalman wrote:
> you can do better than this: wrap the webstore API call in
> chrome.test.runWithUserGesture from your js files, which will automatically
set
> that user action bit (and if you like you can even test without it).

Done.

https://codereview.chromium.org/196783002/diff/410001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/webstore_private/common.js (right):

https://codereview.chromium.org/196783002/diff/410001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webstore_private/common.js:114:
chrome.webstorePrivate.signIn(continue_url, callbackFail(error));
On 2014/04/21 18:04:42, kalman wrote:
> i.e. this would be
> 
> chrome.test.runWithUserGesture(function() {
>   chrome.webstorePrivate.signIn(..);
> });
> 
> .. which will give you a chance to test without a user gesture.

Done.

[not at google - send to devlin, 2014-04-22 16:22:20]

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/196783002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:753:
return true;
On 2014/04/22 05:02:32, Ilya Sherman wrote:
> On 2014/04/21 18:04:42, kalman wrote:
> > true means to reply straight away. you should return false.
> 
> Hmm, I tried changing this to |return false|, and found that my tests broke,
> whereas they worked as I expected with a |return true| here.  The semantics of
> the return value from this method are pretty mysterious to me.  Could you
please
> clarify exactly what the return value from this method means?  Returning false
> seems to set an error, based on the test output.

I'm wrong, sorry about that. I filed bug 365732 about it.

[Roger Tawa OOO till Jul 10th, 2014-04-24 17:27:16]

lgtm chrome/browser/signin, chrome/browser/ui/sync, component/signin

[guohui, 2014-04-24 22:37:49]

lgtm

[Ilya Sherman, 2014-04-25 23:00:07]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:03:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/196783002/440001

[commit-bot: I haz the power, 2014-04-26 03:33:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 03:33:37]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Ilya Sherman, 2014-04-26 03:41:53]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-26 03:43:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/196783002/440001

[commit-bot: I haz the power, 2014-04-26 04:35:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 04:35:01]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[Ilya Sherman, 2014-04-28 19:25:21]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:26:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/196783002/440001

[commit-bot: I haz the power, 2014-04-28 19:26:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:26:27]

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #1 FAILED at 794.
  1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej

Patch:       extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
084c2b917e045b01dd5e0b6c1a062d0533daffa7..622ccb6893141828e4888fed398d9bfad8b71b76
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -794,6 +794,7 @@ enum HistogramValue {
   BLUETOOTHLOWENERGY_WRITEDESCRIPTORVALUE,
   BOOKMARKMANAGERPRIVATE_CREATEWITHMETAINFO,
   BOOKMARKMANAGERPRIVATE_UPDATEMETAINFO,
+  WEBSTOREPRIVATE_SIGNINFUNCTION,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY

[Ilya Sherman, 2014-04-28 21:26:29]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-28 21:27:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/196783002/460001

[commit-bot: I haz the power, 2014-04-28 22:12:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:12:28]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Ilya Sherman, 2014-04-28 23:49:09]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:49:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/196783002/460001

[commit-bot: I haz the power, 2014-04-29 00:35:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:35:54]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Ilya Sherman, 2014-04-29 00:39:39]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:40:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/196783002/460001

[commit-bot: I haz the power, 2014-04-29 09:47:06]

Change committed as 266812