While trying to enable webview sign-in by default, I found a bunch of issues

While trying to enable webview sign-in by default, I found a bunch of issues
that needed to fixed first.  This CL contains only those fixes.  Once this
lands I can commit the CL (795873004) that actually enables webview sign-in.  Fixes
here include:

- making Authenticator.js more of a drop in replacement for GaiaAuthHost.  Important for tests
- fix to some race conditions in Authenticator.js wrt loadstop
- support in inline_login to work with webview
- fixes to tests that were hardcoded to only test the old style sign-in

BUG=441821
Committed: https://crrev.com/e77304c4963cfd00489f09c4645521752c49577c
Cr-Commit-Position: refs/heads/master@{#310224}

[Roger Tawa OOO till Jul 10th, 2014-12-18 22:30:52]

rogerta@chromium.org changed reviewers:
+ noms@chromium.org, wjmaclean@chromium.org, xiyuan@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-12-18 22:30:52]

Hi James Monica, Xiyuan,

Can you please review?  This fixes code to work with the webview based sign-in,
but since its not actually enabled yet this is mostly a noop.  This is to get
ready to turn on webview based sign-in by default.

James: please look at
chrome/browser/profiles/host_zoom_map_browsertest.cc
chrome/browser/ui/zoom/zoom_controller_browsertest.cc

Xiyuan: please look at:
chrome/browser/resources/gaia_auth_host/authenticator.js
chrome/browser/resources/inline_login/inline_login.js
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
chrome/browser/ui/webui/signin/inline_login_ui.cc
chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc

Monica: please do general review.

Thanks.

[Roger Tawa OOO till Jul 10th, 2014-12-18 22:31:33]

Patchset #1 (id:1) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-12-18 22:31:40]

Patchset #1 (id:20001) has been deleted

[xiyuan, 2014-12-19 17:22:50]

LGTM my part.

[noms (inactive), 2014-12-19 19:48:59]

https://codereview.chromium.org/807503004/diff/60001/chrome/browser/resources...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/807503004/diff/60001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:142: this.trusted_ =
false;
nit: no {}

https://codereview.chromium.org/807503004/diff/60001/chrome/browser/ui/bookma...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/807503004/diff/60001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:48: 
If none of the tests are for ChromeOS, can this file maybe be excluded
altogether in the .gyp file?
Also: does this bubble exist on android/ios?

[Roger Tawa OOO till Jul 10th, 2014-12-19 22:23:59]

Thanks Xiyuan. Monica.  A few more changes after some more manual testing.

Monica: you're right about just removing the file altogether from cros tests. 
I'll update the CL for that after the holidays :-)

https://codereview.chromium.org/807503004/diff/60001/chrome/browser/resources...
File chrome/browser/resources/gaia_auth_host/authenticator.js (right):

https://codereview.chromium.org/807503004/diff/60001/chrome/browser/resources...
chrome/browser/resources/gaia_auth_host/authenticator.js:142: this.trusted_ =
false;
On 2014/12/19 19:48:59, Monica Dinculescu wrote:
> nit: no {}

The convention in this file is to include { } in this case too.

[wjmaclean, 2014-12-29 13:58:16]

LGTM for zoom tests.

[Roger Tawa OOO till Jul 10th, 2015-01-05 20:19:19]

Thanks Monica.  Comments addressed.

Scott: can you review bookmark_bubble_sign_in_delegate_browsertest.cc and
bookmark_bubble_sign_in_delegate_unittest.cc?  What I did was move the unit
tests to browser tests because these tests need a browser window.

https://codereview.chromium.org/807503004/diff/60001/chrome/browser/ui/bookma...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/807503004/diff/60001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:48: 
On 2014/12/19 19:48:59, Monica (OOO until Jan 5) wrote:
> If none of the tests are for ChromeOS, can this file maybe be excluded
> altogether in the .gyp file?
> Also: does this bubble exist on android/ios?

Done.
Android already handled in gyp file in previous patchset.
ios does not use the gyp variable "chrome_browser_tests_sources" that includes
this source file.

[Roger Tawa OOO till Jul 10th, 2015-01-06 01:37:52]

rogerta@chromium.org changed reviewers:
+ sky@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-01-06 01:37:53]

Oops. forgot to add Scott as a reviewer.  Scott: please review
bookmark_bubble_sign_in_delegate_browsertest.cc and
bookmark_bubble_sign_in_delegate_unittest.cc?  What I did was move the unit
tests to browser tests because these tests need a browser window.  Thanks.

[sky, 2015-01-06 15:48:42]

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:39:
// The default browser created for tests start with one tab open on
Move description above declaration.

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:46:
chrome::ShowSingletonTabOverwritingNTP(browser, params);
Are you sure you don't need to wait for the navigation here?

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:53:
scoped_ptr<BookmarkBubbleDelegate> delegate;
Is there a reason you need to wrap in scoped_ptr instead of declaration
BookmarkBubbleSignInDelegate on the stack?

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:59:
content::RunAllPendingInMessageLoop();
Why do you need the RunAllPending in all of these?

[noms (inactive), 2015-01-06 21:58:27]

lgtm

[Roger Tawa OOO till Jul 10th, 2015-01-06 22:34:59]

Thanks Scott.  Comments addressed and changes uploaded.

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:39:
// The default browser created for tests start with one tab open on
On 2015/01/06 15:48:42, sky wrote:
> Move description above declaration.

Done.

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:46:
chrome::ShowSingletonTabOverwritingNTP(browser, params);
On 2015/01/06 15:48:42, sky wrote:
> Are you sure you don't need to wait for the navigation here?

Does not seem like it is needed.  Even though chrome:version will load
asynchronously, the current URL of the tab is updated to chrome:version, so the
sign in page does not consider it about:blank.

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:53:
scoped_ptr<BookmarkBubbleDelegate> delegate;
On 2015/01/06 15:48:42, sky wrote:
> Is there a reason you need to wrap in scoped_ptr instead of declaration
> BookmarkBubbleSignInDelegate on the stack?

The dtor of BookmarkBubbleSignInDelegate is private, so it cannot be
instantiated on the stack.

https://codereview.chromium.org/807503004/diff/120001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:59:
content::RunAllPendingInMessageLoop();
On 2015/01/06 15:48:42, sky wrote:
> Why do you need the RunAllPending in all of these?

Not needed.  All removed except line 118.

[sky, 2015-01-06 22:49:11]

LGTM

https://codereview.chromium.org/807503004/diff/160001/chrome/browser/ui/bookm...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/807503004/diff/160001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:114:
content::RunAllPendingInMessageLoop();
Please document why this one is needed.

[Roger Tawa OOO till Jul 10th, 2015-01-07 02:09:09]

Thanks Scott.

https://codereview.chromium.org/807503004/diff/160001/chrome/browser/ui/bookm...
File chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/807503004/diff/160001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:114:
content::RunAllPendingInMessageLoop();
On 2015/01/06 22:49:11, sky wrote:
> Please document why this one is needed.

Done.

[Roger Tawa OOO till Jul 10th, 2015-01-07 02:09:26]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2015-01-07 02:10:28]

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

[commit-bot: I haz the power, 2015-01-07 02:54:25]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-01-07 02:55:20]

Patchset 8 (id:??) landed as
https://crrev.com/e77304c4963cfd00489f09c4645521752c49577c
Cr-Commit-Position: refs/heads/master@{#310224}