[Clean-up] Remove some dead code for handling Webstore sign-in.

[Clean-up] Remove some dead code for handling Webstore sign-in.

BUG=388083
TEST=code still compiles
R=guohui@chromium.org
NOPRESUBMIT=true
NOTRY=true

Committed: https://crrev.com/3be26390084e495c4ef808f1a882f689fe2103ed
Cr-Commit-Position: refs/heads/master@{#296884}

[Ilya Sherman, 2014-09-17 22:42:02]

There might be more dead code that can be pruned, but this was what I could find
in a quick search.

https://codereview.chromium.org/579033002/diff/1/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/579033002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:307: // source is
SOURCE_WEBSTORE_INSTALL, |contents| might be showing an app
Note: I'm not sure how this comment should be updated now that
SOURCE_WEBSTORE_INSTALL has been removed.

[guohui, 2014-09-18 21:25:55]

thanks a lot!

https://codereview.chromium.org/579033002/diff/1/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/579033002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:307: // source is
SOURCE_WEBSTORE_INSTALL, |contents| might be showing an app
On 2014/09/17 22:42:02, Ilya Sherman wrote:
> Note: I'm not sure how this comment should be updated now that
> SOURCE_WEBSTORE_INSTALL has been removed.

i think the comment could be removed, together with the if (isContinueUrl..)
check below.

https://codereview.chromium.org/579033002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1557: new
OneClickSigninSyncObserver(contents, original_continue_url_);
i think original_continue_url was added for webstore signin flow, and now it is
useless. Also i remember you created the class OneClickSigninSyncObserver for
webstore signin flow, much logic in that class should now be pruned, e.g.
anything about continue url.

[Ilya Sherman, 2014-09-18 21:55:57]

https://codereview.chromium.org/579033002/diff/1/chrome/browser/ui/sync/one_c...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/579033002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:307: // source is
SOURCE_WEBSTORE_INSTALL, |contents| might be showing an app
On 2014/09/18 21:25:55, guohui wrote:
> On 2014/09/17 22:42:02, Ilya Sherman wrote:
> > Note: I'm not sure how this comment should be updated now that
> > SOURCE_WEBSTORE_INSTALL has been removed.
> 
> i think the comment could be removed, together with the if (isContinueUrl..)
> check below.

Done.

https://codereview.chromium.org/579033002/diff/1/chrome/browser/ui/sync/one_c...
chrome/browser/ui/sync/one_click_signin_helper.cc:1557: new
OneClickSigninSyncObserver(contents, original_continue_url_);
On 2014/09/18 21:25:55, guohui wrote:
> i think original_continue_url was added for webstore signin flow, and now it
is
> useless. Also i remember you created the class OneClickSigninSyncObserver for
> webstore signin flow, much logic in that class should now be pruned, e.g.
> anything about continue url.

I think you might be right, but I'm not 100% certain, and it wasn't obvious to
me from reading the code whether the original_continue_url_ was no longer
useful.  I seem to recall that it might have existed before my changes, though
my memory of this code is imperfect.  Do you know whether the "original_source
== signin::SOURCE_SETTINGS" case is no longer relevant?

I think this CL is approximately the limit of what I feel comfortable with
changing given my current knowledge of the signin code.  If you have time and
interest, it would be great if you could do a pass after this lands to see if
there's more that can be pruned.  I'm happy to help in any way I can; I'm just
not certain what paths are no longer reachable.

[guohui, 2014-09-23 19:38:47]

original_continue_url_ did exist before your change, i believe i added it for
webstore flow, so it should be useless now.

I agree that i am probably in a better position to clean up the remaining dead
code, so i am ok with the current change (except for the one thing below).

https://codereview.chromium.org/579033002/diff/20001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/579033002/diff/20001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:318:
contents->GetRenderProcessHost()->GetID(),
eh i think you should remove the check, and always post task, because other than
in webstore flows, the visible url should always be the blank page.

[Ilya Sherman, 2014-09-23 22:17:03]

https://codereview.chromium.org/579033002/diff/20001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/579033002/diff/20001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:318:
contents->GetRenderProcessHost()->GetID(),
On 2014/09/23 19:38:46, guohui wrote:
> eh i think you should remove the check, and always post task, because other
than
> in webstore flows, the visible url should always be the blank page.

Ah, I see -- that makes more sense :)

Done.

[guohui, 2014-09-25 18:29:34]

lgtm

[Ilya Sherman, 2014-09-25 18:30:17]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-25 18:31:27]

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

[commit-bot: I haz the power, 2014-09-25 18:42:18]

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

[commit-bot: I haz the power, 2014-09-25 18:42:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ilya Sherman, 2014-09-25 22:06:42]

isherman@chromium.org changed reviewers:
+ rogerta@chromium.org, yoz@chromium.org

[Ilya Sherman, 2014-09-25 22:06:43]

+rogerta for OWNERS approval
+yoz for chrome/browser/extensions OWNERS approval

[Yoyo Zhou, 2014-09-25 22:53:36]

c/b/extensions LGTM

[Roger Tawa OOO till Jul 10th, 2014-09-26 03:56:49]

lgtm

Thanks for the cleanup Ilya.

[Ilya Sherman, 2014-09-26 04:01:08]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-26 04:02:04]

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

[commit-bot: I haz the power, 2014-09-26 04:11:02]

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

[commit-bot: I haz the power, 2014-09-26 04:11:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ilya Sherman, 2014-09-26 04:30:45]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-26 04:31:47]

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

[commit-bot: I haz the power, 2014-09-26 04:32:26]

Committed patchset #4 (id:60001) as a2eac89bd8c42a75c82f4be72eab54406abbf996

[commit-bot: I haz the power, 2014-09-26 04:33:02]

Patchset 4 (id:??) landed as
https://crrev.com/3be26390084e495c4ef808f1a882f689fe2103ed
Cr-Commit-Position: refs/heads/master@{#296884}