MD Settings: Fix Client Navigations to Search Query URLs

MD Settings: Fix Client Navigations to Search Query URLs

Previously, ChromeOS client would navigate directly to URLs such as:
chrome://settings/search#Bluetooth.

This was a hack because Old Options did not have section routes. This
CL replaces all those navigations with section URLs, such as:
chrome://settings/bluetooth

This is part of the legacy URL migration. The if-branches will be
removed once Old Options is removed from Chrome.

BUG=623587
Committed: https://crrev.com/d5c63244f585d1dfa18f9532c4882b1dea93488b
Cr-Commit-Position: refs/heads/master@{#428215}

[tommycli, 2016-10-27 16:25:40]

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

[commit-bot: I haz the power, 2016-10-27 16:25:59]

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, 2016-10-27 16:27:58]

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

[commit-bot: I haz the power, 2016-10-27 16:27:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-10-27 16:31:59]

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

[commit-bot: I haz the power, 2016-10-27 16:32:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-10-27 16:32:41]

Description was changed from

==========
MD Settings: Fix Client Navigations to Search Query URLs

Previously, ChromeOS client would navigate directly to URLs such as:
chrome://settings/search#Bluetooth.

This was a hack because Old Options did not have section routes. This
CL replaces all those navigations with section URLs, such as:
chrome://settings/bluetooth

This is part of the legacy URL migration. The if-branches will be
removed once Old Options is removed from Chrome.

BUG=623587
==========

to

==========
MD Settings: Fix Client Navigations to Search Query URLs

Previously, ChromeOS client would navigate directly to URLs such as:
chrome://settings/search#Bluetooth.

This was a hack because Old Options did not have section routes. This
CL replaces all those navigations with section URLs, such as:
chrome://settings/bluetooth

This is part of the legacy URL migration. The if-branches will be
removed once Old Options is removed from Chrome.

BUG=623587
==========

[tommycli, 2016-10-27 16:32:42]

tommycli@chromium.org changed reviewers:
+ stevenjb@chromium.org

[tommycli, 2016-10-27 16:34:15]

tommycli@chromium.org changed reviewers:
+ rsleevi@chromium.org

[tommycli, 2016-10-27 16:34:15]

stevenjb: PTAL
  chrome/browser/ui/ash/system_tray_client.cc
  chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
  chrome/common

rsleevi: PTAL
  chrome/browser/interstitials/chrome_controller_client.cc

Thanks all!

[Ryan Sleevi, 2016-10-27 17:05:20]

Wrong reviewer? I'm not an owner for that file / have no knowledge of it :)

[tommycli, 2016-10-27 17:06:38]

On 2016/10/27 17:05:20, Ryan Sleevi wrote:
> Wrong reviewer? I'm not an owner for that file / have no knowledge of it :)

Oh that

[tommycli, 2016-10-27 17:06:49]

On 2016/10/27 17:06:38, tommycli wrote:
> On 2016/10/27 17:05:20, Ryan Sleevi wrote:
> > Wrong reviewer? I'm not an owner for that file / have no knowledge of it :)
> 
> Oh that

Oh that's odd, you seem to be in chrome/browser/interstitials/OWNERS

[Ryan Sleevi, 2016-10-27 17:10:33]

On 2016/10/27 17:06:49, tommycli wrote:
> Oh that's odd, you seem to be in chrome/browser/interstitials/OWNERS

... What? Okay!

(Yeah, I have no idea why, I should remove myself, sorry)

[Ryan Sleevi, 2016-10-27 17:11:12]

https://codereview.chromium.org/2455113003/diff/20001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_controller_client.cc (right):

https://codereview.chromium.org/2455113003/diff/20001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_controller_client.cc:58: sub_page =
chrome::kBluetoothSubPage;
Date time

https://codereview.chromium.org/2455113003/diff/20001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_controller_client.cc:62:
l10n_util::GetStringUTF8(IDS_OPTIONS_SETTINGS_SECTION_TITLE_BLUETOOTH);
Shouldn't this be DATETIME?

[tommycli, 2016-10-27 17:11:48]

On 2016/10/27 17:10:33, Ryan Sleevi wrote:
> On 2016/10/27 17:06:49, tommycli wrote:
> > Oh that's odd, you seem to be in chrome/browser/interstitials/OWNERS
> 
> ... What? Okay!
> 
> (Yeah, I have no idea why, I should remove myself, sorry)

Gotcha. Not a problem, I can find another reviewer. The change is fairly trivial
/ not directly related to interstitials anyways.

Thanks!

[tommycli, 2016-10-27 17:14:03]

rsleevi: Thanks!

https://codereview.chromium.org/2455113003/diff/20001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_controller_client.cc (right):

https://codereview.chromium.org/2455113003/diff/20001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_controller_client.cc:58: sub_page =
chrome::kBluetoothSubPage;
On 2016/10/27 17:11:12, Ryan Sleevi wrote:
> Date time

Done.

https://codereview.chromium.org/2455113003/diff/20001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_controller_client.cc:62:
l10n_util::GetStringUTF8(IDS_OPTIONS_SETTINGS_SECTION_TITLE_BLUETOOTH);
On 2016/10/27 17:11:12, Ryan Sleevi wrote:
> Shouldn't this be DATETIME?

Done.

[Ryan Sleevi, 2016-10-27 17:16:17]

lgtm

[stevenjb, 2016-10-27 19:10:56]

lgtm as-is or with suggested change.

https://codereview.chromium.org/2455113003/diff/40001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_controller_client.cc (right):

https://codereview.chromium.org/2455113003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_controller_client.cc:63: }
Optional suggestion: Just use chrome::kDateTimeSubPage and move this logic to
chrome::ShowSettingsSubPageForProfile:

if (sub_page == chrome::kDateTimeSubPage &&
!base::FeatureList::IsEnabled(features::kMaterialDesignSettings))
  sub_page = std::string(chrome::kDeprecatedOptionsSearchSubPage) + "#"...

Then we can clean up all existing calls to chrome::ShowSettingsSubPageForProfile
and only have to remove the deprecated code in one place later.

[tommycli, 2016-10-27 20:58:28]

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

[commit-bot: I haz the power, 2016-10-27 20:59:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-10-27 21:03:50]

stevenjb: Thanks! New version is indeed cleaner.
rsleevi: Thank you as well!

https://codereview.chromium.org/2455113003/diff/40001/chrome/browser/intersti...
File chrome/browser/interstitials/chrome_controller_client.cc (right):

https://codereview.chromium.org/2455113003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/chrome_controller_client.cc:63: }
On 2016/10/27 19:10:56, stevenjb wrote:
> Optional suggestion: Just use chrome::kDateTimeSubPage and move this logic to
> chrome::ShowSettingsSubPageForProfile:
> 
> if (sub_page == chrome::kDateTimeSubPage &&
> !base::FeatureList::IsEnabled(features::kMaterialDesignSettings))
>   sub_page = std::string(chrome::kDeprecatedOptionsSearchSubPage) + "#"...
> 
> Then we can clean up all existing calls to
chrome::ShowSettingsSubPageForProfile
> and only have to remove the deprecated code in one place later.

Done.

[tommycli, 2016-10-27 21:05:51]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-10-27 21:05:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2455113003/#ps80001
(title: "Fix implementation")

[commit-bot: I haz the power, 2016-10-27 21:06:48]

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, 2016-10-27 21:21:13]

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

[commit-bot: I haz the power, 2016-10-27 21:21:15]

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

[tommycli, 2016-10-27 22:08:10]

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

[tommycli, 2016-10-27 22:08:11]

thestig: PTAL changes to chrome/browser/ui/chrome_pages.cc thanks!

[Lei Zhang, 2016-10-27 22:17:11]

lgtm

[tommycli, 2016-10-27 22:19:14]

On 2016/10/27 22:17:11, Lei Zhang wrote:
> lgtm

thanks!

[tommycli, 2016-10-27 22:19:20]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-10-27 22:19:59]

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, 2016-10-27 23:36:58]

Description was changed from

==========
MD Settings: Fix Client Navigations to Search Query URLs

Previously, ChromeOS client would navigate directly to URLs such as:
chrome://settings/search#Bluetooth.

This was a hack because Old Options did not have section routes. This
CL replaces all those navigations with section URLs, such as:
chrome://settings/bluetooth

This is part of the legacy URL migration. The if-branches will be
removed once Old Options is removed from Chrome.

BUG=623587
==========

to

==========
MD Settings: Fix Client Navigations to Search Query URLs

Previously, ChromeOS client would navigate directly to URLs such as:
chrome://settings/search#Bluetooth.

This was a hack because Old Options did not have section routes. This
CL replaces all those navigations with section URLs, such as:
chrome://settings/bluetooth

This is part of the legacy URL migration. The if-branches will be
removed once Old Options is removed from Chrome.

BUG=623587
==========

[commit-bot: I haz the power, 2016-10-27 23:36:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-27 23:39:40]

Description was changed from

==========
MD Settings: Fix Client Navigations to Search Query URLs

Previously, ChromeOS client would navigate directly to URLs such as:
chrome://settings/search#Bluetooth.

This was a hack because Old Options did not have section routes. This
CL replaces all those navigations with section URLs, such as:
chrome://settings/bluetooth

This is part of the legacy URL migration. The if-branches will be
removed once Old Options is removed from Chrome.

BUG=623587
==========

to

==========
MD Settings: Fix Client Navigations to Search Query URLs

Previously, ChromeOS client would navigate directly to URLs such as:
chrome://settings/search#Bluetooth.

This was a hack because Old Options did not have section routes. This
CL replaces all those navigations with section URLs, such as:
chrome://settings/bluetooth

This is part of the legacy URL migration. The if-branches will be
removed once Old Options is removed from Chrome.

BUG=623587

Committed: https://crrev.com/d5c63244f585d1dfa18f9532c4882b1dea93488b
Cr-Commit-Position: refs/heads/master@{#428215}
==========

[commit-bot: I haz the power, 2016-10-27 23:39:41]

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