[MD settings] settings for pdf documents

[MD settings] settings for pdf documents

This CL adds a PDF documents section to the content settings.

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

Committed: https://crrev.com/1bc2f0135011d68b7876c3ff5bf645d14ead0853
Cr-Commit-Position: refs/heads/master@{#432273}

[dschuyler, 2016-11-11 01:51:50]

Description was changed from

==========
[MD settings] settings for pdf documents

This CL adds a PDF documents section to the content settings.

BUG=663588
==========

to

==========
[MD settings] settings for pdf documents

This CL adds a PDF documents section to the content settings.

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

[dschuyler, 2016-11-11 01:51:52]

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

[commit-bot: I haz the power, 2016-11-11 01:52:20]

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

[dschuyler, 2016-11-11 01:55:49]

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

[commit-bot: I haz the power, 2016-11-11 01:56: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, 2016-11-11 01:59:42]

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

[commit-bot: I haz the power, 2016-11-11 01:59:43]

Dry run: 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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dschuyler, 2016-11-11 02:25:14]

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

[commit-bot: I haz the power, 2016-11-11 02:25:48]

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

[dschuyler, 2016-11-11 02:27:34]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-11-11 02:27:42]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-11 03:13:38]

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

[commit-bot: I haz the power, 2016-11-11 03:13:39]

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

[dschuyler, 2016-11-11 21:22:15]

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

[commit-bot: I haz the power, 2016-11-11 21:22:39]

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

[dschuyler, 2016-11-11 21:23:45]

Patchset #1 (id:40001) has been deleted

[dschuyler, 2016-11-11 21:24:30]

dschuyler@chromium.org changed reviewers:
+ dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-11 22:25:51]

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

[commit-bot: I haz the power, 2016-11-11 22:25:52]

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

[dpapad, 2016-11-11 22:28:04]

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

https://codereview.chromium.org/2484423004/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1670: <message
name="IDS_SETTINGS_SITE_SETTINGS_PDF_DEFAULT_VIEWER" desc="Label for openning
PDF documents using the default PDF viewer application.">
Does this setting apply to both Chromium and Chrome?

If yes, should there be a different string for each case (in
settings_chromium_strings.grdp and settings_google_chrome_strings.grdp)?

If no, should this string be in an "<if expr=" block (and similarly within an
ifdef at md_settings_localized_strings_provider.cc)?

https://codereview.chromium.org/2484423004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/pdf_documents.html (right):

https://codereview.chromium.org/2484423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/pdf_documents.html:5:
<dom-module id="pdf-documents">
Nit: Should this element be named settings-pdf-documents. I know that
site_settings/ has not followed that pattern as closely as the rest of the
codebase, so consider this optional (and probably better done as a cleanup
throughout site_settings/ later).

[dschuyler, 2016-11-12 00:09:59]

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

[commit-bot: I haz the power, 2016-11-12 00:10:40]

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

[dschuyler, 2016-11-12 00:11:53]

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

https://codereview.chromium.org/2484423004/diff/60001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1670: <message
name="IDS_SETTINGS_SITE_SETTINGS_PDF_DEFAULT_VIEWER" desc="Label for openning
PDF documents using the default PDF viewer application.">
On 2016/11/11 22:28:03, dpapad wrote:
> Does this setting apply to both Chromium and Chrome?
> 
> If yes, should there be a different string for each case (in
> settings_chromium_strings.grdp and settings_google_chrome_strings.grdp)?
> 
> If no, should this string be in an "<if expr=" block (and similarly within an
> ifdef at md_settings_localized_strings_provider.cc)? 

There's another problem here. The text can give the impression that
Chrome will become the default PDF viewer (like when opening a PDF
file from the desktop, outside of the browser). That's not accurate.
I've changed the text and sent the new text to Alan for approval.
I believe the new text is good enough to go in now - I believe it's 
likely to be approved or only need a small change (which will be 
an easy CL).

https://codereview.chromium.org/2484423004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/pdf_documents.html (right):

https://codereview.chromium.org/2484423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/pdf_documents.html:5:
<dom-module id="pdf-documents">
On 2016/11/11 22:28:03, dpapad wrote:
> Nit: Should this element be named settings-pdf-documents. I know that
> site_settings/ has not followed that pattern as closely as the rest of the
> codebase, so consider this optional (and probably better done as a cleanup
> throughout site_settings/ later).

Done.

[dpapad, 2016-11-12 00:30:43]

LGTM

[commit-bot: I haz the power, 2016-11-12 01:00:02]

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

[commit-bot: I haz the power, 2016-11-12 01:00:03]

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

[dschuyler, 2016-11-15 00:21:13]

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

[commit-bot: I haz the power, 2016-11-15 00:22:08]

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

[dschuyler, 2016-11-15 00:24:18]

In fixing a unit test, I changed how the routing 
or navigateTo() is called. It now uses more direct
values rather than looking up values in a large
switch(). I plan to move further in that direction
in a later CL.

[dpapad, 2016-11-15 00:32:46]

Still LGTM, with nit.

https://codereview.chromium.org/2484423004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/2484423004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:86:
onTapNavigate: function(event) {
Can this be private?

[dschuyler, 2016-11-15 01:29:11]

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

[dschuyler, 2016-11-15 01:29:43]

Thanks

https://codereview.chromium.org/2484423004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/2484423004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:86:
onTapNavigate: function(event) {
On 2016/11/15 00:32:46, dpapad wrote:
> Can this be private?

Done.

[commit-bot: I haz the power, 2016-11-15 01:29:44]

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-11-15 04:14:03]

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

[commit-bot: I haz the power, 2016-11-15 04:14:04]

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

[dschuyler, 2016-11-15 21:43:19]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-11-15 21:43:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2484423004/#ps120001
(title: "made method private")

[commit-bot: I haz the power, 2016-11-15 21:44:01]

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-11-15 22:00:50]

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

[commit-bot: I haz the power, 2016-11-15 22:00:51]

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

[dschuyler, 2016-11-15 22:07:47]

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

[dschuyler, 2016-11-15 22:07:48]

stevenjb@, please OWNER review:
- prefs_util.cc

[Dan Beam, 2016-11-15 22:15:54]

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

[Dan Beam, 2016-11-15 22:15:54]

lgtm

[dschuyler, 2016-11-15 22:16:52]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-11-15 22:17: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-11-15 22:34:07]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-11-15 22:40:02]

Description was changed from

==========
[MD settings] settings for pdf documents

This CL adds a PDF documents section to the content settings.

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

to

==========
[MD settings] settings for pdf documents

This CL adds a PDF documents section to the content settings.

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

Committed: https://crrev.com/1bc2f0135011d68b7876c3ff5bf645d14ead0853
Cr-Commit-Position: refs/heads/master@{#432273}
==========

[commit-bot: I haz the power, 2016-11-15 22:40:03]

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