Remove enable-iframe-based-signin flag.

Remove enable-iframe-based-signin flag.  This affects only desktop chrome.  Users can no longer use the deprecated iframe based sign in flow.  This flow has been deprecated since m41.

This CL removes the code to get partitions ids and support for page zooming for the old iframe sign in flow.

BUG=493846
Committed: https://crrev.com/5f5bb820bb248b8ba47a53d60aefb85ca19ffb80
Cr-Commit-Position: refs/heads/master@{#365712}

[Roger Tawa OOO till Jul 10th, 2015-10-15 21:09:40]

Patchset #1 (id:1) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-10-22 12:33:12]

Patchset #2 (id:40001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-10-22 12:33:19]

Patchset #2 (id:60001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-10-29 20:57:24]

Patchset #2 (id:80001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-10-29 20:57:31]

Patchset #2 (id:100001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-10-29 20:57:41]

Patchset #2 (id:120001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-10-29 20:57:48]

Patchset #2 (id:140001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-10-30 14:58:15]

rogerta@chromium.org changed reviewers:
+ achuith@chromium.org, anthonyvd@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-10-30 14:58:16]

Hi Anthony, Achuith,

Please take a look.  I made a build of cros_linux and ran it on my machine. 
Adding an account and unlock work fine,  How do I test zooming the cros sign in
page, assuming that is possible?

Note that there are 4 questions/comments for you guys as reviewers.  Please
comment.  Thanks.

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (left):

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:609: 
Is this code needed for chromeos?  That is, should I put this block inside an
#ifdef OS_CHROMEOS instead of just deleting it?  I would think not because
:kChromeUIChromeSigninURL is used only on desktop.

Same comment for all changes in this file.

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (left):

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:65: bool
is_webview_signin_enabled = switches::IsEnableWebviewBasedSignin();
Why is this line not the same as lines 140-142 below?  How will the changes I've
done here affect chromeos?

https://codereview.chromium.org/1412453002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/1412453002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:778: }
kChromeUIChromeSigninURL is only used on desktop, so this should not affect
cros.

https://codereview.chromium.org/1412453002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:845: 
kChromeUIChromeSigninURL is only used on desktop, so this should not affect
cros.

[anthonyvd, 2015-11-04 22:29:26]

lgtm. I'm not sure I can adequately answer any of those ChromeOS questions so
I'll wait for achuithb@ for that.

Nice cleanup :)

[Alexei Svitkine (slow), 2015-11-05 16:26:33]

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

[Alexei Svitkine (slow), 2015-11-05 16:26:34]

https://codereview.chromium.org/1412453002/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1412453002/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:65301: -  <int value="-1357655121"
label="enable-iframe-based-signin"/>
Keep this here since it's needed for decoding old data.

[achuithb, 2015-11-05 22:22:13]

On 2015/10/30 14:58:16, Roger Tawa wrote:
> Hi Anthony, Achuith,
> 
> Please take a look.  I made a build of cros_linux and ran it on my machine. 
> Adding an account and unlock work fine,  How do I test zooming the cros sign
in
> page, assuming that is possible?
> 
> Note that there are 4 questions/comments for you guys as reviewers.  Please
> comment.  Thanks.
> 

Changing the zoom on the cros signin page doesn't work: http://crbug.com/413324

[achuithb, 2015-11-05 22:36:50]

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (left):

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:609: 
On 2015/10/30 14:58:15, Roger Tawa wrote:
> Is this code needed for chromeos?  That is, should I put this block inside an
> #ifdef OS_CHROMEOS instead of just deleting it?  I would think not because
> :kChromeUIChromeSigninURL is used only on desktop.
> 
> Same comment for all changes in this file.

Yup, I believe deleting is correct

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:1261:
content::HostZoomMap* host_zoom_map;
nit: combine this with the next line.

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:1265: double
default_level = host_zoom_map->GetDefaultZoomLevel();
nit: make this const

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (left):

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:65: bool
is_webview_signin_enabled = switches::IsEnableWebviewBasedSignin();
On 2015/10/30 14:58:15, Roger Tawa wrote:
> Why is this line not the same as lines 140-142 below?  How will the changes
I've
> done here affect chromeos?

My understanding is that this code is defunct on cros

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:138: #if defined(OS_CHROMEOS)
I think get rid of is_webview and this #if

https://codereview.chromium.org/1412453002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:150: } else {
Get rid of this else

https://codereview.chromium.org/1412453002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/1412453002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:778: }
On 2015/10/30 14:58:15, Roger Tawa wrote:
> kChromeUIChromeSigninURL is only used on desktop, so this should not affect
> cros.

Acknowledged.

https://codereview.chromium.org/1412453002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:845: 
On 2015/10/30 14:58:15, Roger Tawa wrote:
> kChromeUIChromeSigninURL is only used on desktop, so this should not affect
> cros.

Acknowledged.

https://codereview.chromium.org/1412453002/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right):

https://codereview.chromium.org/1412453002/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:299: //
crbug.com/422868
Is this still a valid concern after re-writing this test?

https://codereview.chromium.org/1412453002/diff/260001/chrome/browser/ui/zoom...
File chrome/browser/ui/zoom/zoom_controller_browsertest.cc (right):

https://codereview.chromium.org/1412453002/diff/260001/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_controller_browsertest.cc:275: #if
!defined(OS_CHROMEOS)
Do you know why this is?

[achuithb, 2015-11-05 22:38:47]

Could you expand the description a little to include some comments about what's
affected by this CL? It makes sheriffing and the search for regressions a little
easier.

[Roger Tawa OOO till Jul 10th, 2015-12-14 22:21:19]

Description was changed from

==========
Remove enable-iframe-based-signin flag.

BUG=493846
==========

to

==========
Remove enable-iframe-based-signin flag.  This affects only desktop chrome. 
Users can no longer use the deprecated iframe based sign in flow.  This flow has
been deprecated since m34.

BUG=493846
==========

[Roger Tawa OOO till Jul 10th, 2015-12-14 22:28:45]

Description was changed from

==========
Remove enable-iframe-based-signin flag.  This affects only desktop chrome. 
Users can no longer use the deprecated iframe based sign in flow.  This flow has
been deprecated since m34.

BUG=493846
==========

to

==========
Remove enable-iframe-based-signin flag.  This affects only desktop chrome. 
Users can no longer use the deprecated iframe based sign in flow.  This flow has
been deprecated since m41.

This CL removes the code to get partitions ids and support for page zooming for
the old iframe sign in flow.

BUG=493846
==========

[Roger Tawa OOO till Jul 10th, 2015-12-14 22:29:49]

Hi Achuith,

You comments addressed.  Please take another look.  Thanks.

[achuithb, 2015-12-14 23:00:47]

https://codereview.chromium.org/1412453002/diff/460001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/1412453002/diff/460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:139: is_webview =
chromeos::StartupUtils::IsWebviewSigninEnabled();
This is always true now:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

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

https://codereview.chromium.org/1412453002/diff/460001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/1412453002/diff/460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:139: is_webview =
chromeos::StartupUtils::IsWebviewSigninEnabled();
On 2015/12/14 23:00:47, achuithb wrote:
> This is always true now:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

Thanks for the tip Achuith.  Fixed.

[Alexei Svitkine (slow), 2015-12-15 15:37:14]

lgtm % comment

https://codereview.chromium.org/1412453002/diff/480001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1412453002/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:68165: -  <int value="-1357655121"
label="enable-iframe-based-signin"/>
Please keep this here, since it's used to decode data from older versions that
might still be reporting this.

[achuithb, 2015-12-15 18:43:18]

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:125:
guest_view::GuestViewManager* manager =
I think auto may be more readable here?

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:131:
base::Bind(&AddToSetIfSigninWebview, &frame_set, parent_frame_name));
Should this be renamed to AddToFrameSet or something similar? The
IfSigninWebview part doesn't make sense when there's no IFrame version, right?

[achuithb, 2015-12-15 18:44:23]

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:85: auto* web_view =
extensions::WebViewGuest::FromWebContents(web_contents);
Please note this use of auto which is fairly similar to what I'm proposing for
GuestViewManager::FromBrowserContext

[Roger Tawa OOO till Jul 10th, 2015-12-15 19:23:19]

Patchset #19 (id:500001) has been deleted

[Roger Tawa OOO till Jul 10th, 2015-12-15 19:23:42]

Thanks guys.  Comments addressed.  PTAL.

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:85: auto* web_view =
extensions::WebViewGuest::FromWebContents(web_contents);
On 2015/12/15 18:44:23, achuithb wrote:
> Please note this use of auto which is fairly similar to what I'm proposing for
> GuestViewManager::FromBrowserContext

Done.

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:125:
guest_view::GuestViewManager* manager =
On 2015/12/15 18:43:17, achuithb wrote:
> I think auto may be more readable here?

Done.

https://codereview.chromium.org/1412453002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:131:
base::Bind(&AddToSetIfSigninWebview, &frame_set, parent_frame_name));
On 2015/12/15 18:43:17, achuithb wrote:
> Should this be renamed to AddToFrameSet or something similar? The
> IfSigninWebview part doesn't make sense when there's no IFrame version, right?

Yes makes sense.  Done.

https://codereview.chromium.org/1412453002/diff/480001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1412453002/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:68165: -  <int value="-1357655121"
label="enable-iframe-based-signin"/>
On 2015/12/15 15:37:14, Alexei Svitkine (slow) wrote:
> Please keep this here, since it's used to decode data from older versions that
> might still be reporting this.

Done.

[achuithb, 2015-12-16 00:39:07]

lgtm

https://codereview.chromium.org/1412453002/diff/520001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/1412453002/diff/520001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:136: return NULL;
nit: return nullptr;

[Roger Tawa OOO till Jul 10th, 2015-12-16 20:43:44]

https://codereview.chromium.org/1412453002/diff/520001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_ui.cc (right):

https://codereview.chromium.org/1412453002/diff/520001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_ui.cc:136: return NULL;
On 2015/12/16 00:39:07, achuith (OOO till Jan 1) wrote:
> nit: return nullptr;

Done.

[Roger Tawa OOO till Jul 10th, 2015-12-16 20:44:01]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-12-16 20:44:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org, asvitkine@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/1412453002/#ps540001
(title: "Use nullptr")

[commit-bot: I haz the power, 2015-12-16 20:45:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412453002/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412453002/540001

[Roger Tawa OOO till Jul 10th, 2015-12-16 20:55:20]

rogerta@chromium.org changed reviewers:
+ estade@chromium.org, wjmaclean@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-12-16 20:55:21]

Hi Evan, James,

Evan: can you do owner review for:
chrome/browser/ui/webui/options/content_settings_handler.cc

James: can you do owner review for:
chrome/browser/ui/zoom/zoom_controller_browsertest.cc

Thanks.

[commit-bot: I haz the power, 2015-12-16 20:58:32]

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

[commit-bot: I haz the power, 2015-12-16 20:58:35]

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

[wjmaclean, 2015-12-16 21:12:53]

zoom_controller_browsertest LGTM

[Evan Stade, 2015-12-16 22:36:39]

content_settings_handler.cc lgtm!

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

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2015-12-17 02:35:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412453002/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412453002/540001

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

Description was changed from

==========
Remove enable-iframe-based-signin flag.  This affects only desktop chrome. 
Users can no longer use the deprecated iframe based sign in flow.  This flow has
been deprecated since m41.

This CL removes the code to get partitions ids and support for page zooming for
the old iframe sign in flow.

BUG=493846
==========

to

==========
Remove enable-iframe-based-signin flag.  This affects only desktop chrome. 
Users can no longer use the deprecated iframe based sign in flow.  This flow has
been deprecated since m41.

This CL removes the code to get partitions ids and support for page zooming for
the old iframe sign in flow.

BUG=493846
==========

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

Committed patchset #20 (id:540001)

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

Description was changed from

==========
Remove enable-iframe-based-signin flag.  This affects only desktop chrome. 
Users can no longer use the deprecated iframe based sign in flow.  This flow has
been deprecated since m41.

This CL removes the code to get partitions ids and support for page zooming for
the old iframe sign in flow.

BUG=493846
==========

to

==========
Remove enable-iframe-based-signin flag.  This affects only desktop chrome. 
Users can no longer use the deprecated iframe based sign in flow.  This flow has
been deprecated since m41.

This CL removes the code to get partitions ids and support for page zooming for
the old iframe sign in flow.

BUG=493846

Committed: https://crrev.com/5f5bb820bb248b8ba47a53d60aefb85ca19ffb80
Cr-Commit-Position: refs/heads/master@{#365712}
==========

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

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