Start removing deprecated Options UI code

Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings.

R=dpapad@chromium.org,thestig@chromium.org
BUG=728353
Review-Url: https://codereview.chromium.org/2913343002
Cr-Commit-Position: refs/heads/master@{#477519}
Committed: https://chromium.googlesource.com/chromium/src/+/e1a5fa50529b3c83abb5db46772caa79ba210f43

[Dan Beam, 2017-05-31 23:33:02]

Description was changed from

==========
Start removing deprecated Options UI code

R=dpapad@chromium.org
BUG=728353
==========

to

==========
Start removing deprecated Options UI code

This just removes features::kMaterialDesignSettings.

R=dpapad@chromium.org
BUG=728353
==========

[Dan Beam, 2017-05-31 23:33:17]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 23:33:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 23:47:02]

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

[commit-bot: I haz the power, 2017-05-31 23:47:03]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2017-06-01 00:12:55]

Overall this L-G-T-M, some nits and a question below. Also perhaps you can add a
more detailed description for this CL. Per my understanding this CL

 - is not deleting all Options related code.
 - it does effectively make old Options code unreachable.
 - it removes all tests that exercised the old code.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:93: // Redirect chrome://settings,
unless MD settings is enabled.
Comment obsolete?

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/policy/polic...
File chrome/browser/policy/policy_browsertest.cc (left):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/policy/polic...
chrome/browser/policy/policy_browsertest.cc:4426: "(function () {\n"
I am so glad we are removing this kind of tests.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_client.cc (right):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_client.cc:418: page =
chrome::kNetworkDetailSubPage + "?guid=" + net::EscapeUrlEncodedData(network_id,
true);
Nit: This seems more than 80 columns.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/chrome_pa...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:145: // In MD Settings, the exceptions no
longer have a separate subpage.
This comment contrasts old and new, but it seems a bit obsolete when the old is
not anywhere to be seen anymore.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:151: // TODO(tommycli): Update the group names
defined in site_settings_helper once
Maybe add a reference to crbug.com/728353 here, so that we decrease the chance
of forgetting.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (left):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:445:
::switches::SettingsWindowEnabled())) {
Don't fully understand what this code did, but can you explain how the new code
does not break Android?

[Dan Beam, 2017-06-02 23:22:53]

Description was changed from

==========
Start removing deprecated Options UI code

This just removes features::kMaterialDesignSettings.

R=dpapad@chromium.org
BUG=728353
==========

to

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings and
disables chrome://settings-frame, essentially disabling
the old options UI from ever being seen again.

R=dpapad@chromium.org
BUG=728353
==========

[Dan Beam, 2017-06-03 00:10:52]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[Dan Beam, 2017-06-03 00:11:03]

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:93: // Redirect chrome://settings,
unless MD settings is enabled.
On 2017/06/01 00:12:54, dpapad wrote:
> Comment obsolete?

Done.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/policy/polic...
File chrome/browser/policy/policy_browsertest.cc (left):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/policy/polic...
chrome/browser/policy/policy_browsertest.cc:4426: "(function () {\n"
On 2017/06/01 00:12:54, dpapad wrote:
> I am so glad we are removing this kind of tests.

i think the idea of the test is noble.  the implementation is complex (out of
necessity from this layer), but is better in a more isolated, mocha-based test,
imo.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_client.cc (right):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_client.cc:418: page =
chrome::kNetworkDetailSubPage + "?guid=" + net::EscapeUrlEncodedData(network_id,
true);
On 2017/06/01 00:12:54, dpapad wrote:
> Nit: This seems more than 80 columns.

Done.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/chrome_pa...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:145: // In MD Settings, the exceptions no
longer have a separate subpage.
On 2017/06/01 00:12:54, dpapad wrote:
> This comment contrasts old and new, but it seems a bit obsolete when the old
is
> not anywhere to be seen anymore.

Acknowledged.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:151: // TODO(tommycli): Update the group names
defined in site_settings_helper once
On 2017/06/01 00:12:54, dpapad wrote:
> Maybe add a reference to crbug.com/728353 here, so that we decrease the chance
> of forgetting.

Done.

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/2913343002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:395: #if
!defined(OS_ANDROID)
^ this is how it doesn't break android ;)

[commit-bot: I haz the power, 2017-06-03 00:11:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2017-06-03 00:40:12]

dbeam@chromium.org changed reviewers:
+ thestig@chromium.org

[Dan Beam, 2017-06-03 00:40:12]

+thestig@ for

    chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc
    chrome/browser/browser_about_handler.cc
    chrome/browser/browser_about_handler_unittest.cc
   
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
    chrome/browser/policy/policy_browsertest.cc
    chrome/browser/ui/ash/system_tray_client.cc
    chrome/browser/ui/chrome_pages.cc
    chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc

[Dan Beam, 2017-06-03 00:40:45]

Description was changed from

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings and
disables chrome://settings-frame, essentially disabling
the old options UI from ever being seen again.

R=dpapad@chromium.org
BUG=728353
==========

to

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings and disables
chrome://settings-frame, essentially disabling the old options UI from
ever being seen again.

R=dpapad@chromium.org
BUG=728353
==========

[Lei Zhang, 2017-06-03 01:03:50]

https://codereview.chromium.org/2913343002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/2913343002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:9: #include
"base/feature_list.h"
Delete? I'd just grep for FeatureList in all the modified files.

[commit-bot: I haz the power, 2017-06-03 01:05:21]

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

[commit-bot: I haz the power, 2017-06-03 01:05:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2017-06-03 01:36:58]

Description was changed from

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings and disables
chrome://settings-frame, essentially disabling the old options UI from
ever being seen again.

R=dpapad@chromium.org
BUG=728353
==========

to

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings.

R=dpapad@chromium.org
BUG=728353
==========

[Dan Beam, 2017-06-03 01:38:46]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-03 01:39:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-03 02:23:04]

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

[commit-bot: I haz the power, 2017-06-03 02:23:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2017-06-06 03:42:59]

Patchset #4 (id:60001) has been deleted

[Dan Beam, 2017-06-06 03:52:40]

Patchset #4 (id:80001) has been deleted

[Dan Beam, 2017-06-06 03:52:50]

https://codereview.chromium.org/2913343002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/2913343002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:9: #include
"base/feature_list.h"
On 2017/06/03 01:03:49, Lei Zhang wrote:
> Delete? I'd just grep for FeatureList in all the modified files.

Done.

[Dan Beam, 2017-06-06 03:52:56]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 03:53:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2017-06-06 04:35:57]

Description was changed from

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings.

R=dpapad@chromium.org
BUG=728353
==========

to

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings.

R=dpapad@chromium.org,thestig@chromium.org
BUG=728353
==========

[commit-bot: I haz the power, 2017-06-06 05:14:58]

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

[commit-bot: I haz the power, 2017-06-06 05:14:59]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-06-06 19:12:58]

LGTM

[Lei Zhang, 2017-06-06 21:10:54]

lgtm

https://codereview.chromium.org/2913343002/diff/100001/chrome/browser/browser...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/2913343002/diff/100001/chrome/browser/browser...
chrome/browser/browser_about_handler.cc:96: // Redirect chrome://help, unless MD
settings is enabled.
Obsolete comment?

[Dan Beam, 2017-06-06 21:32:56]

https://codereview.chromium.org/2913343002/diff/100001/chrome/browser/browser...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/2913343002/diff/100001/chrome/browser/browser...
chrome/browser/browser_about_handler.cc:96: // Redirect chrome://help, unless MD
settings is enabled.
On 2017/06/06 21:10:54, Lei Zhang wrote:
> Obsolete comment?

Done.

[Dan Beam, 2017-06-06 21:33:02]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 21:33:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 22:46:27]

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

[commit-bot: I haz the power, 2017-06-06 22:46:28]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Dan Beam, 2017-06-07 01:21:00]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2017-06-07 01:21:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2913343002/#ps120001
(title: "thestig@ review")

[commit-bot: I haz the power, 2017-06-07 01:21:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 02:27:40]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496798460334980,
"parent_rev": "57130e07aa7e05d90d2f851e9865537bd196f5a7", "commit_rev":
"e1a5fa50529b3c83abb5db46772caa79ba210f43"}

[commit-bot: I haz the power, 2017-06-07 02:27:55]

Description was changed from

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings.

R=dpapad@chromium.org,thestig@chromium.org
BUG=728353
==========

to

==========
Start removing deprecated Options UI code

This CL removes features::kMaterialDesignSettings.

R=dpapad@chromium.org,thestig@chromium.org
BUG=728353

Review-Url: https://codereview.chromium.org/2913343002
Cr-Commit-Position: refs/heads/master@{#477519}
Committed:
https://chromium.googlesource.com/chromium/src/+/e1a5fa50529b3c83abb5db46772c...
==========

[commit-bot: I haz the power, 2017-06-07 02:27:59]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e1a5fa50529b3c83abb5db46772c...