MD Settings: Update Content Settings Types URLs.

MD Settings: Update Content Settings Types URLs.

Previously, in Old Options, the URL for specific content settings
types was at chrome://settings/contentExceptions#plugins, etc.

This updates navigations from the C++ client to the new URLs:
chrome://settings/content/flash, etc.

The old URLs were generated from the content settings group names.
Unfortunately we can't simply update the group names without
breaking Old Options, so this CL has an override list where the
group names differ from the last URL segment.

BUG=623587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/654a66eaa9461214c9599738215372b55e6633c0
Cr-Commit-Position: refs/heads/master@{#428561}

[tommycli, 2016-10-27 18:42:25]

Description was changed from

==========
MD Settings: Update Content Settings Types URLs.

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

to

==========
MD Settings: Update Content Settings Types URLs.

BUG=623587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2016-10-27 18:42:29]

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

[commit-bot: I haz the power, 2016-10-27 18:43:25]

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 19:43:32]

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

[commit-bot: I haz the power, 2016-10-27 19:43:34]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-10-27 20:53:11]

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

[tommycli, 2016-10-27 20:53:12]

dbeam: PTAL, thanks!

[tommycli, 2016-10-27 21:01:17]

Description was changed from

==========
MD Settings: Update Content Settings Types URLs.

BUG=623587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Update Content Settings Types URLs.

Previously, in Old Options, the URL for specific content settings
types was at chrome://settings/contentExceptions#plugins, etc.

This updates navigations from the C++ client to the new URLs:
chrome://settings/content/flash, etc.

The old URLs were generated from the content settings group names.
Unfortunately we can't simply update the group names without
breaking Old Options, so this CL has an override list where the
group names differ from the last URL segment.

BUG=623587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-10-27 23:44:41]

lgtm

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

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:145: kSettingsContentTypePathOverrides[] = {
why are these "Override"s?

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/webui/bid...
File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right):

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/webui/bid...
chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:623: url +=
chrome::kDeprecatedOptionsContentSettingsExceptionsSubPage;
good job, Mario! but these tests are all DISABLED_

[tommycli, 2016-10-28 00:01:25]

dbeam: thanks!

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

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:145: kSettingsContentTypePathOverrides[] = {
On 2016/10/27 23:44:41, Dan Beam wrote:
> why are these "Override"s?

They override the group names that have been used as for the URLs in Options.

Since some of the group names still work, this list just contains the
"overrides".

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/webui/bid...
File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right):

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/webui/bid...
chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:623: url +=
chrome::kDeprecatedOptionsContentSettingsExceptionsSubPage;
On 2016/10/27 23:44:41, Dan Beam wrote:
> good job, Mario! but these tests are all DISABLED_

Yeah... I still needed to change them to keep it correct in case these get
reenabled

[tommycli, 2016-10-28 00:28:52]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-10-28 00:29:23]

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-28 00:44:06]

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

[commit-bot: I haz the power, 2016-10-28 00:44:08]

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-28 17:47:41]

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

[tommycli, 2016-10-28 17:47:57]

pkasting: PTAL

  chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc
  chrome/browser/ui/browser_navigator_browsertest.cc
  chrome/browser/ui/chrome_pages.cc


Thanks!

[Peter Kasting, 2016-10-28 21:13:34]

LGTM

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/autocomplete...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/autocomplete...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:193: 
Nit: No blank line here

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

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:144: const
site_settings::ContentSettingsTypeNameEntry
Nit: Scope this kind of a constant inside the function where it's used and
declare as static constexpr.

Note: Replace "static" with use of CR_DEFINE_STATIC_LOCAL if this becomes
non-POD; see below.

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:155: if
(base::FeatureList::IsEnabled(features::kMaterialDesignSettings)) {
Nit: If you reverse this condition, the body effectively becomes a brief early
return, and you can unindent the longer block below.

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:159: for (size_t i = 0; i <
arraysize(kSettingsContentTypePathOverrides); ++i) {
Nit: Personally I would find it more intuitive here to use a map, since you're
trying to "map" from input types to output strings; you could do something like

  CR_DEFINE_STATIC_LOCAL(
      std::map<ContentSettingsType, std::string>, kOverrides,
      ({CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS, "automaticDownloads"},
       {CONTENT_SETTINGS_TYPE_BACKGROUND_SYNC, "backgroundSync"},
       ...));  // Might need to use {{...}} instead of ({...}), not sure
  const auto it = kOverrides.find(type);
  const std::string content_type_path =
      (it == kOverrides.end())
          ? site_settings::ContentSettingsTypeToGroupName(type)
          : it->second;

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:165: } else {
Nit: No else after return.

[tommycli, 2016-10-28 21:57:13]

thanks!

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/autocomplete...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/autocomplete...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:193: 
On 2016/10/28 21:13:34, Peter Kasting wrote:
> Nit: No blank line here

Done.

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

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:144: const
site_settings::ContentSettingsTypeNameEntry
On 2016/10/28 21:13:34, Peter Kasting wrote:
> Nit: Scope this kind of a constant inside the function where it's used and
> declare as static constexpr.
> 
> Note: Replace "static" with use of CR_DEFINE_STATIC_LOCAL if this becomes
> non-POD; see below.

Done.

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:155: if
(base::FeatureList::IsEnabled(features::kMaterialDesignSettings)) {
On 2016/10/28 21:13:34, Peter Kasting wrote:
> Nit: If you reverse this condition, the body effectively becomes a brief early
> return, and you can unindent the longer block below.

Done.

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:159: for (size_t i = 0; i <
arraysize(kSettingsContentTypePathOverrides); ++i) {
On 2016/10/28 21:13:34, Peter Kasting wrote:
> Nit: Personally I would find it more intuitive here to use a map, since you're
> trying to "map" from input types to output strings; you could do something
like
> 
>   CR_DEFINE_STATIC_LOCAL(
>       std::map<ContentSettingsType, std::string>, kOverrides,
>       ({CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS, "automaticDownloads"},
>        {CONTENT_SETTINGS_TYPE_BACKGROUND_SYNC, "backgroundSync"},
>        ...));  // Might need to use {{...}} instead of ({...}), not sure
>   const auto it = kOverrides.find(type);
>   const std::string content_type_path =
>       (it == kOverrides.end())
>           ? site_settings::ContentSettingsTypeToGroupName(type)
>           : it->second;

Done.

https://codereview.chromium.org/2454093003/diff/1/chrome/browser/ui/chrome_pa...
chrome/browser/ui/chrome_pages.cc:165: } else {
On 2016/10/28 21:13:34, Peter Kasting wrote:
> Nit: No else after return.

Done.

[tommycli, 2016-10-28 21:57:17]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-10-28 21:57:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2454093003/#ps40001
(title: "address pkasting comments")

[commit-bot: I haz the power, 2016-10-28 21:57:44]

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-28 22:52:12]

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

[commit-bot: I haz the power, 2016-10-28 22:52:14]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tommycli, 2016-10-29 00:01:05]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-10-29 00:01:30]

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-29 00:37:30]

Description was changed from

==========
MD Settings: Update Content Settings Types URLs.

Previously, in Old Options, the URL for specific content settings
types was at chrome://settings/contentExceptions#plugins, etc.

This updates navigations from the C++ client to the new URLs:
chrome://settings/content/flash, etc.

The old URLs were generated from the content settings group names.
Unfortunately we can't simply update the group names without
breaking Old Options, so this CL has an override list where the
group names differ from the last URL segment.

BUG=623587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Update Content Settings Types URLs.

Previously, in Old Options, the URL for specific content settings
types was at chrome://settings/contentExceptions#plugins, etc.

This updates navigations from the C++ client to the new URLs:
chrome://settings/content/flash, etc.

The old URLs were generated from the content settings group names.
Unfortunately we can't simply update the group names without
breaking Old Options, so this CL has an override list where the
group names differ from the last URL segment.

BUG=623587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-29 00:37:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-29 00:44:45]

Description was changed from

==========
MD Settings: Update Content Settings Types URLs.

Previously, in Old Options, the URL for specific content settings
types was at chrome://settings/contentExceptions#plugins, etc.

This updates navigations from the C++ client to the new URLs:
chrome://settings/content/flash, etc.

The old URLs were generated from the content settings group names.
Unfortunately we can't simply update the group names without
breaking Old Options, so this CL has an override list where the
group names differ from the last URL segment.

BUG=623587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Update Content Settings Types URLs.

Previously, in Old Options, the URL for specific content settings
types was at chrome://settings/contentExceptions#plugins, etc.

This updates navigations from the C++ client to the new URLs:
chrome://settings/content/flash, etc.

The old URLs were generated from the content settings group names.
Unfortunately we can't simply update the group names without
breaking Old Options, so this CL has an override list where the
group names differ from the last URL segment.

BUG=623587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/654a66eaa9461214c9599738215372b55e6633c0
Cr-Commit-Position: refs/heads/master@{#428561}
==========

[commit-bot: I haz the power, 2016-10-29 00:44:46]

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