Add a PDF component extension

Add a PDF component extension

This adds a PDF component extension which is used for rendering PDFs with an
out of process PDF plugin. It adds the extension as a MIME handler for pdf file
types. The extension is currently only added if the --out-of-process-pdf switch
is passed to Chrome, AND the PDF plugin is available.

BUG=303491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239451

[tapted, 2013-12-02 04:17:19]

The adding-a-component-extension bits lg, just some questions. And I haven't
done much js, so comments there are somewhat hunch-y.

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/browser_re...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/browser_re...
chrome/browser/browser_resources.grd:259: <include name="IDR_PDF_MANIFEST"
file="resources\pdf\manifest.json" type="BINDATA" />
nit: I think this file is vaguely sorted in <structures> and <includes>. Maybe
put around line 198.

Also, do these need to be guarded with `pp_ifdef('_google_chrome')` so it
doesn't show up in Chromium builds?

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:559: base::FilePath pdf_path;
does it need a #if defined(GOOGLE_CHROME_BUILD) guard?

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:562: base::PathExists(pdf_path)) {
nit: I always indent these at 4 spaces. E.g. line 511, 436

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
File chrome/browser/resources/pdf/manifest.json (right):

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/manifest.json:2: "manifest_version": 2,
nit: maybe put the extension id in a comment as well

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/manifest.json:12: "streamsPrivate"],
nit: closing `],` on new line

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/pdf.js:4: var plugin;
nit: maybe do that closury thing, putting

`(function() {` before this line and a `})();` after the last line

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/pdf.js:22: window.onscroll = function() {
webkitRequestAnimationFrame(onScroll); };
Is this the same as window.addEventListener('scroll', ...)? I think that's more
common

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/pdf.js:24: var url = location.search.substring(1);
Maybe a comment, "pop off the leading '?'". And is 'window.location' more
consistent/common with other Chrome js?

[jam, 2013-12-02 17:29:34]

which parts do you want me to look at?

you should pick extensions OWNERS for the extensions directories

[raymes, 2013-12-02 23:56:58]

On 2013/12/02 17:29:34, jam wrote:
> which parts do you want me to look at?
> 
> you should pick extensions OWNERS for the extensions directories

Sorry John, I just wanted you to review:
chrome/common/chrome_content_client.cc

And as an FYI:
chrome/browser/resources/pdf/pdf.html
chrome/browser/resources/pdf/pdf.js

[jam, 2013-12-03 22:21:14]

On 2013/12/02 23:56:58, raymes wrote:
> On 2013/12/02 17:29:34, jam wrote:
> > which parts do you want me to look at?
> > 
> > you should pick extensions OWNERS for the extensions directories
> 
> Sorry John, I just wanted you to review:
> chrome/common/chrome_content_client.cc

lgtm

> 
> And as an FYI:
> chrome/browser/resources/pdf/pdf.html
> chrome/browser/resources/pdf/pdf.js

[raymes, 2013-12-04 04:54:49]

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/browser_re...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/browser_re...
chrome/browser/browser_resources.grd:259: <include name="IDR_PDF_MANIFEST"
file="resources\pdf\manifest.json" type="BINDATA" />
On 2013/12/02 04:17:19, tapted wrote:
> nit: I think this file is vaguely sorted in <structures> and <includes>. Maybe
> put around line 198.

Done.

> 
> Also, do these need to be guarded with `pp_ifdef('_google_chrome')` so it
> doesn't show up in Chromium builds?

As discussed I think it's ok to include this in Chromium builds. It won't
actually get added as an extension unless the PDF plugin is present.

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:559: base::FilePath pdf_path;
On 2013/12/02 04:17:19, tapted wrote:
> does it need a #if defined(GOOGLE_CHROME_BUILD) guard?

This is the same check as in chrome_content_client.cc. It allows the plugin to
work in Chromium as long as the dll is present.

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:562: base::PathExists(pdf_path)) {
On 2013/12/02 04:17:19, tapted wrote:
> nit: I always indent these at 4 spaces. E.g. line 511, 436

Done.

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
File chrome/browser/resources/pdf/manifest.json (right):

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/manifest.json:2: "manifest_version": 2,
On 2013/12/02 04:17:19, tapted wrote:
> nit: maybe put the extension id in a comment as well

Done.

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/manifest.json:12: "streamsPrivate"],
On 2013/12/02 04:17:19, tapted wrote:
> nit: closing `],` on new line

Done.

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/pdf.js:4: var plugin;
On 2013/12/02 04:17:19, tapted wrote:
> nit: maybe do that closury thing, putting
> 
> `(function() {` before this line and a `})();` after the last line

Done.

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/pdf.js:22: window.onscroll = function() {
webkitRequestAnimationFrame(onScroll); };
On 2013/12/02 04:17:19, tapted wrote:
> Is this the same as window.addEventListener('scroll', ...)? I think that's
more
> common

Done.

https://codereview.chromium.org/96233002/diff/40001/chrome/browser/resources/...
chrome/browser/resources/pdf/pdf.js:24: var url = location.search.substring(1);
On 2013/12/02 04:17:19, tapted wrote:
> Maybe a comment, "pop off the leading '?'". And is 'window.location' more
> consistent/common with other Chrome js?

Done.

[raymes, 2013-12-04 04:56:17]

+cc zork as FYI

[tapted, 2013-12-05 03:34:36]

lgtm

https://codereview.chromium.org/96233002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/96233002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:568:
PathService::Get(chrome::FILE_PDF_PLUGIN, &pdf_path);
nit: maybe include this in the `if` condition, so the return value gets checked.

[raymes, 2013-12-05 03:47:14]

+thestig,kalman for OWNERS

thestig:
  chrome/browser/browser_resources.grd
  chrome/browser/extensions/component_loader.cc
  chrome/browser/resources/component_extension_resources.grd
  chrome/browser/resources/pdf/background.js
  chrome/browser/resources/pdf/manifest.json
  chrome/browser/resources/pdf/pdf.html
  chrome/browser/resources/pdf/pdf.js
  chrome/common/chrome_content_client.cc
  chrome/common/extensions/api/_manifest_features.json
  chrome/common/extensions/api/_permission_features.json
  chrome/common/extensions/extension_constants.cc
  chrome/common/extensions/extension_constants.h
  chrome/common/extensions/mime_types_handler.cc
kalman:
  chrome/browser/extensions/component_loader.cc
  chrome/common/extensions/api/_manifest_features.json
  chrome/common/extensions/api/_permission_features.json
  chrome/common/extensions/extension_constants.cc
  chrome/common/extensions/extension_constants.h
  chrome/common/extensions/mime_types_handler.cc

[Lei Zhang, 2013-12-05 04:08:22]

lgtm

https://codereview.chromium.org/96233002/diff/90001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/96233002/diff/90001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:247:
"mhjfbmdgcfjbbpaeojofohoefgiehjai"   // PDF
I think you can use the new style extension id here too? I don't know what
extensions team's preference is, considering you also have |kPdfExtensionId|.

[not at google - send to devlin, 2013-12-05 18:14:39]

lgtm

https://codereview.chromium.org/96233002/diff/110001/chrome/common/extensions...
File chrome/common/extensions/mime_types_handler.cc (right):

https://codereview.chromium.org/96233002/diff/110001/chrome/common/extensions...
chrome/common/extensions/mime_types_handler.cc:31:
extension_misc::kPdfExtensionId
nit: alphabetical

[raymes, 2013-12-09 02:33:34]

https://codereview.chromium.org/96233002/diff/90001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/96233002/diff/90001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:247:
"mhjfbmdgcfjbbpaeojofohoefgiehjai"   // PDF
On 2013/12/05 04:08:23, Lei Zhang wrote:
> I think you can use the new style extension id here too? I don't know what
> extensions team's preference is, considering you also have |kPdfExtensionId|.

I just went with consistency with the surrounding IDs since they didn't give a
preference.

https://codereview.chromium.org/96233002/diff/110001/chrome/common/extensions...
File chrome/common/extensions/mime_types_handler.cc (right):

https://codereview.chromium.org/96233002/diff/110001/chrome/common/extensions...
chrome/common/extensions/mime_types_handler.cc:31:
extension_misc::kPdfExtensionId
On 2013/12/05 18:14:40, kalman wrote:
> nit: alphabetical

Done.

[commit-bot: I haz the power, 2013-12-09 02:33:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/96233002/130001

[commit-bot: I haz the power, 2013-12-09 02:45:49]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-09 02:49:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/96233002/150001

[commit-bot: I haz the power, 2013-12-09 03:00:18]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-09 03:02:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/96233002/170001

[commit-bot: I haz the power, 2013-12-09 03:17:37]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-09 03:21:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/96233002/190001

[commit-bot: I haz the power, 2013-12-09 03:49:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/96233002/200001

[commit-bot: I haz the power, 2013-12-09 06:38:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/96233002/220001

[commit-bot: I haz the power, 2013-12-09 08:43:26]

Change committed as 239451