Remove OneClickSigninHelper since it is no longer used.

Remove OneClickSigninHelper since it is no longer used.

BUG=260022, 350202
Committed: https://crrev.com/be592667bdf919fb8292c88c1b59767add856ba4
Cr-Commit-Position: refs/heads/master@{#319662}

Committed: https://crrev.com/f8c08ce45dd8c610db8f6b54e96128191d42a89b
Cr-Commit-Position: refs/heads/master@{#320337}

[Roger Tawa OOO till Jul 10th, 2015-02-24 14:59:04]

Patchset #12 (id:220001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 19:23:52]

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

[Roger Tawa OOO till Jul 10th, 2015-02-24 19:23:53]

Hi Monica,

Please take a look.  Thanks.

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:18:17]

Patchset #1 (id:1) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:18:24]

Patchset #1 (id:20001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:18:30]

Patchset #1 (id:40001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:18:43]

Patchset #1 (id:60001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:18:49]

Patchset #1 (id:80001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:18:55]

Patchset #1 (id:100001) has been deleted

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

Patchset #1 (id:120001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:19:08]

Patchset #1 (id:140001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:19:14]

Patchset #1 (id:160001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:19:20]

Patchset #1 (id:180001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:19:26]

Patchset #1 (id:200001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-02-24 20:19:33]

Patchset #1 (id:240001) has been deleted

[noms (inactive), 2015-02-25 15:44:02]

I think it looks good! Here's a bunch of nits :D

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

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:68:
content::WebContents* contents,
nit: i thinkkkk the two parameters fit after the bracket, on each separate line
(i.e. contents goes on the same line as the method name)

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:89:
content::WebContents* contents,
nit: same comment as above about the parameters

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:92:
RedirectToNtpOrAppsPage(contents, source);
nit: no {}

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:149: callback),
contents);
nit: i think `callback` can go on the previous line, and the second contents can
align with the first contents

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:387:
!HandleCrossAccountError(result.refresh_token, source,
nit: I think !Handle... fits on the first line

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:413: if
(!last_email.empty() && !gaia::AreEmailsSame(last_email, email_)) {
nit: maybe flip the condition and return early instead?

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:444: // If the new
email address is different from the email address that
It looks like the comment doesn't really match the code

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:535:
SigninManagerFactory::GetForProfile(profile);
nit: i think this fits on the top line

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:542: if
(!email.empty()) {
nit: i would return early if the email is empty. there's a looot of indenting
going on here :)

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:544: return false;
nit: maybe this should go after line 534?

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right):

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:256:
"user@gmail.com", &error_message));
super optional nit: make this, and foo@gmail.com constants?

[Roger Tawa OOO till Jul 10th, 2015-02-25 19:05:06]

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

[Roger Tawa OOO till Jul 10th, 2015-02-25 19:05:07]

Thanks Monica.

Alexei, can you look at:
chrome/browser/ui/cocoa/one_click_signin_view_controller.mm

Scott, can you look at:
chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc
chrome/browser/chrome_browser_field_trials_desktop.cc
chrome/browser/renderer_host/

Thanks.

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

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:68:
content::WebContents* contents,
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: i thinkkkk the two parameters fit after the bracket, on each separate
line
> (i.e. contents goes on the same line as the method name)

Done.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:89:
content::WebContents* contents,
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: same comment as above about the parameters

Done.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:92:
RedirectToNtpOrAppsPage(contents, source);
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: no {}

Done.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:149: callback),
contents);
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: i think `callback` can go on the previous line, and the second contents
can
> align with the first contents

Callback can go on previous line, but second contents must align with new.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:387:
!HandleCrossAccountError(result.refresh_token, source,
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: I think !Handle... fits on the first line

Done.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:413: if
(!last_email.empty() && !gaia::AreEmailsSame(last_email, email_)) {
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: maybe flip the condition and return early instead?

Done.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:444: // If the new
email address is different from the email address that
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> It looks like the comment doesn't really match the code

Deleted comment.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:535:
SigninManagerFactory::GetForProfile(profile);
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: i think this fits on the top line

Done.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:542: if
(!email.empty()) {
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: i would return early if the email is empty. there's a looot of indenting
> going on here :)

I think it makes sense this way since you know which checks apply only when
email is not empty.  If I did return early, the code looks a little weird: two
early returns that return false, the next early return returns true, and the
next one returns false.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:544: return false;
On 2015/02/25 15:44:01, Monica Dinculescu wrote:
> nit: maybe this should go after line 534?

No.  If email is empty, we want to return true, not false.  So we check email
first.

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right):

https://codereview.chromium.org/914363003/diff/280001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:256:
"user@gmail.com", &error_message));
On 2015/02/25 15:44:02, Monica Dinculescu wrote:
> super optional nit: make this, and mailto:foo@gmail.com constants?

I think I'll leave it like this, I think the tests are easier to read with
explicit strings.

[Alexei Svitkine (slow), 2015-02-25 19:25:14]

cocoa lgtm

[noms (inactive), 2015-02-25 22:38:12]

"things that i own" lgtm

[sky, 2015-03-09 15:48:45]

LGTM

[Roger Tawa OOO till Jul 10th, 2015-03-09 17:11:38]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-03-09 17:11:38]

The patchset sent to the CQ was uploaded after l-g-t-m from noms@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/914363003/#ps340001
(title: "rebased")

[commit-bot: I haz the power, 2015-03-09 17:12:32]

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

[commit-bot: I haz the power, 2015-03-09 17:17:09]

Committed patchset #5 (id:340001)

[commit-bot: I haz the power, 2015-03-09 17:18:28]

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

[rmcilroy, 2015-03-10 11:59:50]

A revert of this CL (patchset #5 id:340001) has been created in
https://codereview.chromium.org/998513002/ by rmcilroy@chromium.org.

The reason for reverting is: Broke the tab_switching.tough_energy_cases
benchmark on the perf bots.

BUG=465692.

[Roger Tawa OOO till Jul 10th, 2015-03-10 14:04:34]

Hi Ross,

Can you provide a link to the breakage info?  I don't see anything in the
CLs or crbug, and all trybots are green.

Roger

-

On Tue, Mar 10, 2015 at 7:59 AM, <rmcilroy@chromium.org> wrote:

> A revert of this CL (patchset #5 id:340001) has been created in
> https://codereview.chromium.org/998513002/ by rmcilroy@chromium.org.
>
> The reason for reverting is: Broke the tab_switching.tough_energy_cases
> benchmark on the perf bots.
>
> BUG=465692.
>
> https://codereview.chromium.org/914363003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sullivan, 2015-03-12 01:39:46]

sullivan@chromium.org changed reviewers:
+ sullivan@chromium.org

[sullivan, 2015-03-12 01:39:47]

lgtm

Telemetry changes in new patchset lgtm

[tfarina, 2015-03-12 01:55:14]

On Tue, Mar 10, 2015 at 11:04 AM, Roger Tawa <rogerta@chromium.org> wrote:

> Hi Ross,
>
> Can you provide a link to the breakage info?  I don't see anything in the
> CLs or crbug, and all trybots are green.
>
> In my experience, being trybots green is not a guarantee that it won't
break anything on the waterfall when it lands. That is because there might
be differences between CQ and Waterfall configurations, i.e. they are not
exactly the same, although we have been doing everything to close the gap.


> Roger
>
> -
>
> On Tue, Mar 10, 2015 at 7:59 AM, <rmcilroy@chromium.org> wrote:
>
>> A revert of this CL (patchset #5 id:340001) has been created in
>> https://codereview.chromium.org/998513002/ by rmcilroy@chromium.org.
>>
>> The reason for reverting is: Broke the tab_switching.tough_energy_cases
>> benchmark on the perf bots.
>>
>> BUG=465692.
>>
>> https://codereview.chromium.org/914363003/
>>
>
>


-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Roger Tawa OOO till Jul 10th, 2015-03-12 12:12:13]

Patchset #7 (id:380001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-03-12 12:20:46]

Patchset #7 (id:400001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-03-12 17:15:16]

Patchset #8 (id:440001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-03-12 17:15:32]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-03-12 17:15:33]

The patchset sent to the CQ was uploaded after l-g-t-m from noms@chromium.org,
asvitkine@chromium.org, sky@chromium.org, sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/914363003/#ps420001
(title: "rebased")

[commit-bot: I haz the power, 2015-03-12 17:16:03]

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

[commit-bot: I haz the power, 2015-03-12 19:01:57]

Committed patchset #7 (id:420001)

[commit-bot: I haz the power, 2015-03-12 19:02:47]

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

[brucedawson, 2015-03-13 18:43:47]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-03-13 18:43:49]

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

https://codereview.chromium.org/914363003/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:570: ProfileManager*
manager = g_browser_process->profile_manager();
The use of a manager variable here shadows the manager variable in the outer
scope, which makes the code more confusing to read. In the first case it is a
SigninManager*, and now it is a ProfileManager*. This was found by /analyze. Not
strictly a bug, but perhaps worth fixing?

https://codereview.chromium.org/914363003/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:574: std::string
current_email =
Ditto for this variable. In this case the type is the same but the meaning is
different, which can be confusing.

[Roger Tawa OOO till Jul 10th, 2015-03-13 21:24:54]

Thank Bruce.

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

https://codereview.chromium.org/914363003/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:570: ProfileManager*
manager = g_browser_process->profile_manager();
On 2015/03/13 18:43:48, brucedawson wrote:
> The use of a manager variable here shadows the manager variable in the outer
> scope, which makes the code more confusing to read. In the first case it is a
> SigninManager*, and now it is a ProfileManager*. This was found by /analyze.
Not
> strictly a bug, but perhaps worth fixing?

Yes worth fixing.  I'll do so in a new CL.  Thanks for the tip.

https://codereview.chromium.org/914363003/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:574: std::string
current_email =
On 2015/03/13 18:43:48, brucedawson wrote:
> Ditto for this variable. In this case the type is the same but the meaning is
> different, which can be confusing.

Ditto.

[brucedawson, 2015-03-13 21:46:30]

Please put BUG=427616 on the fix CL -- that's the blanket /analyze bug.