Add an initial implementation of the Privacy Page. No subpages yet.

Add an initial implementation of the Privacy Page. No subpages yet.

BUG=

Committed: https://crrev.com/4e3cc90185e8fb445ef4631df0fc283bfb15e407
Cr-Commit-Position: refs/heads/master@{#333686}

[Jeremy Klein, 2015-06-08 18:28:18]

jlklein@chromium.org changed reviewers:
+ khorimoto@chromium.org

[Kyle Horimoto, 2015-06-08 18:41:06]

https://codereview.chromium.org/1168793002/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1168793002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:126: <message
name="IDS_SETTINGS_PRIVACY_PAGE_TITLE" desc="Name of the settings page which
privacy and security settings.">
s/which/which allows users to modify/

https://codereview.chromium.org/1168793002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:130: Google Chrome may use <ph
name="BEGIN_LINK">&lt;a target="_blank" href="$1"&gt;</ph>web services<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph> to improve your browsing
experience. You may optionally disable these services at any time.
You need to use settings_chromium_strings.grdp and
settings_google_chrome_strings.grdp for all strings containing "Google Chrome"
or "Chromium".

https://codereview.chromium.org/1168793002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:168: 
nit: Remove newline.

[Jeremy Klein, 2015-06-08 19:42:44]

https://codereview.chromium.org/1168793002/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1168793002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:126: <message
name="IDS_SETTINGS_PRIVACY_PAGE_TITLE" desc="Name of the settings page which
privacy and security settings.">
On 2015/06/08 18:41:06, Kyle Horimoto wrote:
> s/which/which allows users to modify/

Done.

https://codereview.chromium.org/1168793002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:130: Google Chrome may use <ph
name="BEGIN_LINK">&lt;a target="_blank" href="$1"&gt;</ph>web services<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph> to improve your browsing
experience. You may optionally disable these services at any time.
On 2015/06/08 18:41:05, Kyle Horimoto wrote:
> You need to use settings_chromium_strings.grdp and
> settings_google_chrome_strings.grdp for all strings containing "Google Chrome"
> or "Chromium".

Done.

https://codereview.chromium.org/1168793002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:168: 
On 2015/06/08 18:41:06, Kyle Horimoto wrote:
> nit: Remove newline.

Done.

[Kyle Horimoto, 2015-06-08 20:18:57]

lgtm

[Jeremy Klein, 2015-06-08 20:22:30]

The CQ bit was checked by jlklein@chromium.org

[commit-bot: I haz the power, 2015-06-08 20:23:36]

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

[commit-bot: I haz the power, 2015-06-08 20:31:31]

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

[commit-bot: I haz the power, 2015-06-08 20:31:32]

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...)

[Jeremy Klein, 2015-06-08 21:16:18]

jlklein@chromium.org changed reviewers:
+ jhawkins@chromium.org

[Jeremy Klein, 2015-06-08 21:16:19]

+James for OWNER approval an a few files.

[James Hawkins, 2015-06-08 21:20:18]

https://codereview.chromium.org/1168793002/diff/40001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:159: <message
name="IDS_SETTINGS_PRIVACY_SITE_SETTINGS_BUTTON" desc="Text for site settings
button in Privacy options">
Do you need _PRIVACY_ here and the following?

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/prefs_util.cc:48: // Privacy
Hmm. We don't currently have these broken up by section. If we're going to do
that, I think it's preferable to break up into methods.  Like
GetPrivacyKeys(&whitelist) or something.  What do you think?

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:14:
i18n-values=".innerHTML:privacyImproveBrowsingExperience"></p>
</p> needs to be on a new line (because this line is wrapped).

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:63: <cr-button
raised i18n-content="privacyClearBrowsingDataLabel"></cr-button>
Aren't we sticking to 80 cols?

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:7: *
'cr-settings-privacy-page' is the settings page containing downloads
Downloads?

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/routes.html (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/routes.html:12: <more-route name="privacy"
path="/privacy"></more-route>
Alphabetize.

[Jeremy Klein, 2015-06-09 03:42:15]

https://codereview.chromium.org/1168793002/diff/40001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:159: <message
name="IDS_SETTINGS_PRIVACY_SITE_SETTINGS_BUTTON" desc="Text for site settings
button in Privacy options">
On 2015/06/08 21:20:17, James Hawkins wrote:
> Do you need _PRIVACY_ here and the following?

Done.

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/prefs_util.cc:48: // Privacy
On 2015/06/08 21:20:17, James Hawkins wrote:
> Hmm. We don't currently have these broken up by section. If we're going to do
> that, I think it's preferable to break up into methods.  Like
> GetPrivacyKeys(&whitelist) or something.  What do you think?

I'm worried about settings which may be in multiple pages... removed comment and
alphabetized.

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:14:
i18n-values=".innerHTML:privacyImproveBrowsingExperience"></p>
On 2015/06/08 21:20:18, James Hawkins wrote:
> </p> needs to be on a new line (because this line is wrapped).

Done.

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:63: <cr-button
raised i18n-content="privacyClearBrowsingDataLabel"></cr-button>
On 2015/06/08 21:20:17, James Hawkins wrote:
> Aren't we sticking to 80 cols?

Done.

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.js:7: *
'cr-settings-privacy-page' is the settings page containing downloads
On 2015/06/08 21:20:18, James Hawkins wrote:
> Downloads?

Done.

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/routes.html (right):

https://codereview.chromium.org/1168793002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/routes.html:12: <more-route name="privacy"
path="/privacy"></more-route>
On 2015/06/08 21:20:18, James Hawkins wrote:
> Alphabetize.

Done.

[James Hawkins, 2015-06-09 16:30:32]

https://codereview.chromium.org/1168793002/diff/60001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1168793002/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:159: <message
name="IDS_SETTINGS_SITE_SETTINGS_BUTTON" desc="Text for site settings button in
Privacy options">
Sorry, missed this before: these can be shorter if you don't include the detail
that this is for a button (which in this context is irrelevant; it's just a
string here).

E.g., IDS_SETTINGS_SITE_SETTINGS, IDS_SETTINGS_CLEAR_BROWSING_DATA

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:53:
pref="{{prefs.cros.device.attestation_for_content_protection_enabled}}"
:-/ This is unfortunate

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:61: 
Optional nit: Use whitespace consistently.  You don't use blank lines elsewhere.

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:123:
html_source->AddString("privacyImproveBrowsingExperience",
nit: You don't need the 'privacy' at the beginning of this string (or elsewhere,
except privacyPageTitle.

[Jeremy Klein, 2015-06-09 18:13:52]

https://codereview.chromium.org/1168793002/diff/60001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1168793002/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:159: <message
name="IDS_SETTINGS_SITE_SETTINGS_BUTTON" desc="Text for site settings button in
Privacy options">
On 2015/06/09 16:30:32, James Hawkins wrote:
> Sorry, missed this before: these can be shorter if you don't include the
detail
> that this is for a button (which in this context is irrelevant; it's just a
> string here).
> 
> E.g., IDS_SETTINGS_SITE_SETTINGS, IDS_SETTINGS_CLEAR_BROWSING_DATA

Done.

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:53:
pref="{{prefs.cros.device.attestation_for_content_protection_enabled}}"
On 2015/06/09 16:30:32, James Hawkins wrote:
> :-/ This is unfortunate

Yeah... stupid giant names. Not sure there's much we can do here.

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:61: 
On 2015/06/09 16:30:32, James Hawkins wrote:
> Optional nit: Use whitespace consistently.  You don't use blank lines
elsewhere.

Done.

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1168793002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:123:
html_source->AddString("privacyImproveBrowsingExperience",
On 2015/06/09 16:30:32, James Hawkins wrote:
> nit: You don't need the 'privacy' at the beginning of this string (or
elsewhere,
> except privacyPageTitle.

Done.

[James Hawkins, 2015-06-09 18:47:20]

lgtm

[Jeremy Klein, 2015-06-09 18:48:05]

The CQ bit was checked by jlklein@chromium.org

[Jeremy Klein, 2015-06-09 18:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/1168793002/#ps80001
(title: "More James comments")

[commit-bot: I haz the power, 2015-06-09 18:48:38]

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

[commit-bot: I haz the power, 2015-06-09 20:01:57]

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

[commit-bot: I haz the power, 2015-06-09 20:01:58]

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

[Jeremy Klein, 2015-06-10 04:55:25]

The CQ bit was checked by jlklein@chromium.org

[Jeremy Klein, 2015-06-10 04:55:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org, jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/1168793002/#ps100001
(title: "fix a broken test")

[commit-bot: I haz the power, 2015-06-10 04:55:44]

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

[Jeremy Klein, 2015-06-10 04:57:35]

I was able to fix the broken test, but stevenjb@, do you have any idea why the
two lines I added to md_settings_localized_strings_provider.cc fix
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
It would be nice if we could find a way to get a more helpful error message from
the test failure.

[commit-bot: I haz the power, 2015-06-10 05:41:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-10 05:42:26]

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