OneClickSigninHelper::CanOffer checks IsEnableWebBasedSignin

OneClickSigninHelper::CanOffer checks IsEnableWebBasedSignin

Before, the CanOffer method could return true, while the web based signin was disabled through about:flags. This meant that neither the save password prompt, nor the sign-in interstitial were shown.

This CL ensures that CanOffer returns false if the web based signing is disabled in about:flags.

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

[vabr (Chromium), 2014-02-12 14:58:08]

Hi Roger,

Could you please take a look?

I tested that the save password infobar appears if and only if the
enable-web-based-signing is not set.

However, if it is set (both through a command line flag and in about:flags), I
could not see any one-click interstitial. This is true for the tip of the tree
even without my patch. Do I do something wrong?

Cheers,
Vaclav

[Roger Tawa OOO till Jul 10th, 2014-02-12 15:36:50]

The interstitial is turned off on the server side at the moment, so that's OK.

Did you try signing in to chrome without the command line?  I believe it may be
broken because of this line:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Without the command line flag, try the following:

1/ Create a new profile.
2/ Sign in to the profile with account A.
3/ Create a second profile.
4/ Sign in to the second profile with the same account A.

You should see a bubble with an error message telling you the account is already
used.

To fix, I think there are two options:

Option A
1/ Leave OneClickSigninHelper::CanOffer().
2/ Change the two places in the code that you care about to look for the command
line flags:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas...

Option B
1/ Write a new function like that may or may not check the command line flag:
   bool OneClickSigninHelper::CanOfferImpl(
       content::WebContents* web_contents,
       CanOfferFor can_offer_for,
       const std::string& email,
       std::string* error_message,
       bool check_command_line);
2/ Have OneClickSigninHelper::CanOffer() call the above with
ignore_command_line=true
3/ Change inline_login_handler_impl.cc:173 to call the above with
ignore_command_line=false.

What do you think?

https://codereview.chromium.org/160363002/diff/100001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/160363002/diff/100001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:418: }
Please use testing::TestWithParam and TEST_P macro to setup tests with and
without the command line flag.  See accelerator_commands_browsertest.cc for an
example.

https://codereview.chromium.org/160363002/diff/100001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:885:
WillRepeatedly(Return(true));
Indent 2 less.

[vabr (Chromium), 2014-02-12 18:42:15]

rogerta@chromium.org:
Thanks for your review, and for spotting the problem with not reporting the
double use of a profile. As expected that problem was real, so I went with your
option A. Now the duplicate use is properly reported, and the save password
infobar still shows up.
It still feels a bit hacky, but I guess this can be solved once the
once-click-signin gets back, so I put TODOs in place.

avi@chromium.org: Please review changes in chrome/browser/ui/tab_helpers.cc.

gcasto@chromium.org: Please review changes in
chrome/browser/password_manager/save_password_infobar_delegate.cc

Thanks all!
Vaclav

https://codereview.chromium.org/160363002/diff/100001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/160363002/diff/100001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:418: }
On 2014/02/12 15:36:50, Roger Tawa wrote:
> Please use testing::TestWithParam and TEST_P macro to setup tests with and
> without the command line flag.  See accelerator_commands_browsertest.cc for an
> example.

Modifications to the unit-test dropped.

https://codereview.chromium.org/160363002/diff/100001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:885:
WillRepeatedly(Return(true));
On 2014/02/12 15:36:50, Roger Tawa wrote:
> Indent 2 less.

Modifications to the unit-test dropped.

[Roger Tawa OOO till Jul 10th, 2014-02-12 20:24:57]

lgtm

Thanks Vaclav.  The one click sign in code will be refactored shortly, so this
can be cleaned up then.

[vabr (Chromium), 2014-02-13 08:44:15]

Thanks, Roger.

Avi, Garrett, please take a look. It would be great if we got this in before the
branch point.

avi@chromium.org: Please review changes in chrome/browser/ui/tab_helpers.cc.

gcasto@chromium.org: Please review changes in
chrome/browser/password_manager/save_password_infobar_delegate.cc

Thanks!
Vaclav

[Avi (use Gerrit), 2014-02-13 16:10:45]

tab helper change LGTM

https://codereview.chromium.org/160363002/diff/200001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/160363002/diff/200001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:209: if (switches::IsEnableWebBasedSignin() &&
This, and other functions in profile_management_switches.h, read awkwardly in
English. I would prefer

IsEnableWebBasedSignin -> IsWebBasedSigninEnabled
IsGoogleProfileInfo() -> IsGoogleProfileInfoUsed (maybe not "used" but this
needs a verb)
IsNewProfileManagement -> IsNewProfileManagementEnabled

This rename is almost certainly out of the scope of this change, though.

[vabr (Chromium), 2014-02-13 16:24:22]

Thanks, Avi.

Garrett, since rumours have it you are on vacation :), and Roger approved the
whole CL, I'll use my newly earned OWNERship of the password manager stuff and
land this.

Cheers,
Vaclav

https://codereview.chromium.org/160363002/diff/200001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/160363002/diff/200001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:209: if (switches::IsEnableWebBasedSignin() &&
On 2014/02/13 16:10:45, Avi wrote:
> This, and other functions in profile_management_switches.h, read awkwardly in
> English. I would prefer
> 
> IsEnableWebBasedSignin -> IsWebBasedSigninEnabled
> IsGoogleProfileInfo() -> IsGoogleProfileInfoUsed (maybe not "used" but this
> needs a verb)
> IsNewProfileManagement -> IsNewProfileManagementEnabled
> 
> This rename is almost certainly out of the scope of this change, though.

Thanks for the comment. I second that it is out of scope of this CL, but Roger
mentioned some refactoring planned, which could possible handle this.

[vabr (Chromium), 2014-02-13 16:24:38]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-02-13 16:25:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/160363002/200001

[commit-bot: I haz the power, 2014-02-13 19:47:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/160363002/200001

[commit-bot: I haz the power, 2014-02-13 21:23:17]

Change committed as 251125