pdf: Create a separate component for using the pdf pepper plugin.

Committed: https://crrev.com/2f8807f697e2c043c857ec37c43d45573d239d8f
Cr-Commit-Position: refs/heads/master@{#292313}

[sadrul, 2014-08-21 20:06:20]

Hi! I wasn't sure from the doc
(https://docs.google.com/a/chromium.org/document/d/1ipfOd4Cj0mgFzPCmdFQYvwV4Q_...)
if I explained the plan clearly enough, or if you have general/specific
feedback. So I have cleaned up the patch. Please take a look. Thanks!

[raymes, 2014-08-22 00:29:20]

The idea makes sense to me but I'm not familiar with components.

One thing to note is that we are moving PDF out of process and it is implemented
as a chrome extension. Is that going to be an issue?

[jam, 2014-08-22 00:44:59]

it seems hacky that a component sends IPCs and the embedder dispatches them. IPC
shouldn't cross module/component boundaries since they're an internal
implementation detail. instead, there should be some sort of delegate interface
that chrome is notified by for the things it needs.

https://codereview.chromium.org/477263003/diff/70001/components/pdf/renderer/...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/477263003/diff/70001/components/pdf/renderer/...
components/pdf/renderer/ppb_pdf_impl.cc:13: #include
"content/app/strings/grit/content_strings.h"
why is this include there?

[sadrul, 2014-08-22 18:03:39]

On 2014/08/22 00:44:59, jam wrote:
> it seems hacky that a component sends IPCs and the embedder dispatches them.
IPC
> shouldn't cross module/component boundaries since they're an internal
> implementation detail. instead, there should be some sort of delegate
interface
> that chrome is notified by for the things it needs.

I was going to do it in the next patch; but I have gone ahead and included it
here. The change is to move PDFTabHelper into the component, with a
PDFTabHelperDelegate interface. Chrome implements it in
ChromePDFTabHelperDelegate. (updated the CL description accordingly)

The PepperPDFHost still lives in chrome, and uses the IPC messages from the pdf
component. I am going to pull that into the pdf component as well. But this
patch is getting rather large, so my plan is to do that in a separate patch. But
let me know if I should pull it all in in this single patch.

> 
>
https://codereview.chromium.org/477263003/diff/70001/components/pdf/renderer/...
> File components/pdf/renderer/ppb_pdf_impl.cc (right):
> 
>
https://codereview.chromium.org/477263003/diff/70001/components/pdf/renderer/...
> components/pdf/renderer/ppb_pdf_impl.cc:13: #include
> "content/app/strings/grit/content_strings.h"
> why is this include there?

This is necessary for the string resources used in GetLocalizedString().


On 2014/08/22 00:29:20, raymes wrote:
> The idea makes sense to me but I'm not familiar with components.
> 
> One thing to note is that we are moving PDF out of process and it is
implemented
> as a chrome extension. Is that going to be an issue?

It doesn't look like out-of-process will be an issue. Is there some doc
explaining how the extension is going to work?

[raymes, 2014-08-25 00:13:30]

> It doesn't look like out-of-process will be an issue. Is there some doc
> explaining how the extension is going to work?
>

I haven't updated the doc to talk about the extension yet. I think the
main question to answer is: do chrome extensions work in the
environments where you want to run PDF. If not then we will have to
think more about it!

> https://codereview.chromium.org/477263003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-08-25 01:52:07]

I'll defer to Lei and Raymes for the review other than the one thing about
strings.

On 2014/08/22 18:03:39, sadrul wrote:
> On 2014/08/22 00:44:59, jam wrote:
> > it seems hacky that a component sends IPCs and the embedder dispatches them.
> IPC
> > shouldn't cross module/component boundaries since they're an internal
> > implementation detail. instead, there should be some sort of delegate
> interface
> > that chrome is notified by for the things it needs.
> 
> I was going to do it in the next patch; but I have gone ahead and included it
> here. The change is to move PDFTabHelper into the component, with a
> PDFTabHelperDelegate interface. Chrome implements it in
> ChromePDFTabHelperDelegate. (updated the CL description accordingly)

sounds good, thanks

> 
> The PepperPDFHost still lives in chrome, and uses the IPC messages from the
pdf
> component. I am going to pull that into the pdf component as well. But this
> patch is getting rather large, so my plan is to do that in a separate patch.
But
> let me know if I should pull it all in in this single patch.

sg
> 
> > 
> >
>
https://codereview.chromium.org/477263003/diff/70001/components/pdf/renderer/...
> > File components/pdf/renderer/ppb_pdf_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/477263003/diff/70001/components/pdf/renderer/...
> > components/pdf/renderer/ppb_pdf_impl.cc:13: #include
> > "content/app/strings/grit/content_strings.h"
> > why is this include there?
> 
> This is necessary for the string resources used in GetLocalizedString().

it's a mistake that these strings are in content. they're not used there. they
should be in the component.

> 
> 
> On 2014/08/22 00:29:20, raymes wrote:
> > The idea makes sense to me but I'm not familiar with components.
> > 
> > One thing to note is that we are moving PDF out of process and it is
> implemented
> > as a chrome extension. Is that going to be an issue?
> 
> It doesn't look like out-of-process will be an issue. Is there some doc
> explaining how the extension is going to work?

[raymes, 2014-08-25 03:11:49]

I'm not sure about moving ppb_pdf_impl.cc and pepper_pdf_host.cc into the
component. I don't think we've ever done anything like that before. It does make
sense to me though. dmichael@ does this seem ok to you?

https://codereview.chromium.org/477263003/diff/150001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/477263003/diff/150001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:263: // XXX: Move into its own
file.
I guess you might as well move this into its own file now if you're happy to go
ahead with something like this?

[dmichael (off chromium), 2014-08-25 17:13:45]

On 2014/08/25 03:11:49, raymes wrote:
> I'm not sure about moving ppb_pdf_impl.cc and pepper_pdf_host.cc into the
> component. I don't think we've ever done anything like that before. It does
make
> sense to me though. dmichael@ does this seem ok to you?
> 
I think it's appropriate. We have a similar situation for
components/nacl/renderer/ppb_nacl_private_impl.

But please add the Pepper OWNERS as per-file owners for ppb_pdf_impl.*

[sadrul, 2014-08-26 17:09:54]

On 2014/08/25 01:52:07, jam wrote:
...
> > > components/pdf/renderer/ppb_pdf_impl.cc:13: #include
> > > "content/app/strings/grit/content_strings.h"
> > > why is this include there?
> > 
> > This is necessary for the string resources used in GetLocalizedString().
> 
> it's a mistake that these strings are in content. they're not used there. they
> should be in the component.

I will move them into the pdf component. But I would prefer to do this in a
separate patch too.


On 2014/08/25 00:13:30, raymes wrote:
...
> > It doesn't look like out-of-process will be an issue. Is there some doc
> > explaining how the extension is going to work?
> >
> 
> I haven't updated the doc to talk about the extension yet. I think the
> main question to answer is: do chrome extensions work in the
> environments where you want to run PDF. If not then we will have to
> think more about it!

Some of the chrome extensions do work in app-shell/athena (e.g. the ones that do
not have any //chrome dependency). We also have the virtual-keyboard working
through mojo-webui bindings (since the virtual-keyboard extension still has some
dependency on //chrome). So depending on the implementation details of the pdf
extension, it may 'just work' for app-shell/athena too, or it may need some
similar mechanism.


On 2014/08/25 17:13:45, dmichael wrote:
...
> But please add the Pepper OWNERS as per-file owners for ppb_pdf_impl.*

Done.

I have also added OWNERS for //components/pdf, and the security folks for the
IPC messages. PTAL.

https://codereview.chromium.org/477263003/diff/150001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/477263003/diff/150001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:263: // XXX: Move into its own
file.
On 2014/08/25 03:11:49, raymes wrote:
> I guess you might as well move this into its own file now if you're happy to
go
> ahead with something like this?

Done.

[sadrul, 2014-08-26 19:41:16]

On 2014/08/25 01:52:07, jam wrote:
...
> > 
> > The PepperPDFHost still lives in chrome, and uses the IPC messages from the
> pdf
> > component. I am going to pull that into the pdf component as well. But this
> > patch is getting rather large, so my plan is to do that in a separate patch.
> But
> > let me know if I should pull it all in in this single patch.
> 
> sg

Done in https://codereview.chromium.org/505243003

[sadrul, 2014-08-26 21:49:17]

On 2014/08/26 17:09:54, sadrul wrote:
> On 2014/08/25 01:52:07, jam wrote:
> ...
> > > > components/pdf/renderer/ppb_pdf_impl.cc:13: #include
> > > > "content/app/strings/grit/content_strings.h"
> > > > why is this include there?
> > > 
> > > This is necessary for the string resources used in GetLocalizedString().
> > 
> > it's a mistake that these strings are in content. they're not used there.
they
> > should be in the component.
> 
> I will move them into the pdf component. But I would prefer to do this in a
> separate patch too.

Done in https://codereview.chromium.org/506473003/

[Lei Zhang, 2014-08-26 22:41:31]

https://codereview.chromium.org/477263003/diff/170001/chrome/chrome_renderer....
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/477263003/diff/170001/chrome/chrome_renderer....
chrome/chrome_renderer.gypi:262: 'chrome_renderer_full_printing_sources': [
You need to update the corresponding BUILD.gn file for this.

https://codereview.chromium.org/477263003/diff/170001/chrome/chrome_renderer....
chrome/chrome_renderer.gypi:386: 'conditions': [
I'd just put enable_printing==1 at the same level as enable_printing!=0.

https://codereview.chromium.org/477263003/diff/170001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/477263003/diff/170001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:139: #if
defined(ENABLE_FULL_PRINTING)
nit: can you move this up to be with the other ENABLE_ ifdefs?

https://codereview.chromium.org/477263003/diff/170001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/477263003/diff/170001/components/components.g...
components/components.gyp:65: 'pdf.gypi',
How about we consistently make the pdf code conditional based on
"enable_plugins" ?

https://codereview.chromium.org/477263003/diff/170001/components/pdf.gypi
File components/pdf.gypi (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf.gypi#new...
components/pdf.gypi:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
You need to write a GN version.

https://codereview.chromium.org/477263003/diff/170001/components/pdf/OWNERS
File components/pdf/OWNERS (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/OWNERS#n...
components/pdf/OWNERS:1: raymes@chromium.org
Add jam and gene

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
File components/pdf/browser/pdf_tab_helper.cc (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper.cc:35:
IPC_MESSAGE_HANDLER(PDFHostMsg_PDFHasUnsupportedFeature,
nit: indent?

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
File components/pdf/browser/pdf_tab_helper_delegate.h (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper_delegate.h:8: #include <string>
you need base/strings/string16.h instead

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper_delegate.h:11: #include
"base/memory/scoped_ptr.h"
not needed, neither are the web_contents* #includes below

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
components/pdf/renderer/ppb_pdf_impl.cc:40: PPB_PDF_Impl::PrintDelegate*
print_delegate = NULL;
g_print_delegate

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
File components/pdf/renderer/ppb_pdf_impl.h (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
components/pdf/renderer/ppb_pdf_impl.h:20: virtual bool
IsPrintingEnabled(PP_Instance instance_id) = 0;
Comments please.

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
components/pdf/renderer/ppb_pdf_impl.h:33: static void
SetPrintDelegate(PrintDelegate* print_delegate);
Can you comment on the ownership of |print_delegate| ?

[sadrul, 2014-08-27 00:09:53]

https://codereview.chromium.org/477263003/diff/170001/chrome/chrome_renderer....
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/477263003/diff/170001/chrome/chrome_renderer....
chrome/chrome_renderer.gypi:262: 'chrome_renderer_full_printing_sources': [
On 2014/08/26 22:41:30, Lei Zhang wrote:
> You need to update the corresponding BUILD.gn file for this.

Done.

https://codereview.chromium.org/477263003/diff/170001/chrome/chrome_renderer....
chrome/chrome_renderer.gypi:386: 'conditions': [
On 2014/08/26 22:41:30, Lei Zhang wrote:
> I'd just put enable_printing==1 at the same level as enable_printing!=0.

Done.

https://codereview.chromium.org/477263003/diff/170001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/477263003/diff/170001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:139: #if
defined(ENABLE_FULL_PRINTING)
On 2014/08/26 22:41:30, Lei Zhang wrote:
> nit: can you move this up to be with the other ENABLE_ ifdefs?

Done.

https://codereview.chromium.org/477263003/diff/170001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/477263003/diff/170001/components/components.g...
components/components.gyp:65: 'pdf.gypi',
On 2014/08/26 22:41:30, Lei Zhang wrote:
> How about we consistently make the pdf code conditional based on
> "enable_plugins" ?

Done.

https://codereview.chromium.org/477263003/diff/170001/components/pdf.gypi
File components/pdf.gypi (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf.gypi#new...
components/pdf.gypi:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2014/08/26 22:41:30, Lei Zhang wrote:
> You need to write a GN version.

Done.

https://codereview.chromium.org/477263003/diff/170001/components/pdf/OWNERS
File components/pdf/OWNERS (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/OWNERS#n...
components/pdf/OWNERS:1: raymes@chromium.org
On 2014/08/26 22:41:30, Lei Zhang wrote:
> Add jam and gene

Done.

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
File components/pdf/browser/pdf_tab_helper.cc (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper.cc:35:
IPC_MESSAGE_HANDLER(PDFHostMsg_PDFHasUnsupportedFeature,
On 2014/08/26 22:41:30, Lei Zhang wrote:
> nit: indent?

Done. (I blame 'git cl format'!)

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
File components/pdf/browser/pdf_tab_helper_delegate.h (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper_delegate.h:8: #include <string>
On 2014/08/26 22:41:30, Lei Zhang wrote:
> you need base/strings/string16.h instead

Done.

https://codereview.chromium.org/477263003/diff/170001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper_delegate.h:11: #include
"base/memory/scoped_ptr.h"
On 2014/08/26 22:41:30, Lei Zhang wrote:
> not needed, neither are the web_contents* #includes below

Done.

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
components/pdf/renderer/ppb_pdf_impl.cc:40: PPB_PDF_Impl::PrintDelegate*
print_delegate = NULL;
On 2014/08/26 22:41:31, Lei Zhang wrote:
> g_print_delegate

Done.

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
File components/pdf/renderer/ppb_pdf_impl.h (right):

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
components/pdf/renderer/ppb_pdf_impl.h:20: virtual bool
IsPrintingEnabled(PP_Instance instance_id) = 0;
On 2014/08/26 22:41:31, Lei Zhang wrote:
> Comments please.

Done.

https://codereview.chromium.org/477263003/diff/170001/components/pdf/renderer...
components/pdf/renderer/ppb_pdf_impl.h:33: static void
SetPrintDelegate(PrintDelegate* print_delegate);
On 2014/08/26 22:41:31, Lei Zhang wrote:
> Can you comment on the ownership of |print_delegate| ?

Done.

[Lei Zhang, 2014-08-27 00:30:44]

lgtm

You'll need a security reviewer for the IPC message move and probably a
components/ reviewer as well.

[sadrul, 2014-08-27 00:43:00]

sadrul@chromium.org changed reviewers:
+ blundell@chromium.org, tsepez@chromium.org

[sadrul, 2014-08-27 00:43:00]

tsepez@chromium.org: Please review changes to move the IPC messages from
chrome/common/render_messages.h to components/pdf/common/pdf_messages.h

blundell@chromium.org: Please review changes in components/

[raymes, 2014-08-27 03:34:10]

lgtm

https://codereview.chromium.org/477263003/diff/250001/chrome/renderer/pepper/...
File chrome/renderer/pepper/chrome_pdf_print_delegate.cc (right):

https://codereview.chromium.org/477263003/diff/250001/chrome/renderer/pepper/...
chrome/renderer/pepper/chrome_pdf_print_delegate.cc:41:
printing::PrintWebViewHelper* helper = GetPrintWebViewHelper(element);
nit: looks like the above two lines are always called together, so you could
just combine them and have
printing::PrintWebViewHelper* helper = GetPrintWebViewHelper(instance_id)

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
File components/pdf/browser/pdf_tab_helper.cc (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper.cc:102: }
nit: it would be better for this to be in the same order as the header.

[blundell, 2014-08-27 09:12:33]

component structure looks good

https://codereview.chromium.org/477263003/diff/250001/components/pdf.gypi
File components/pdf.gypi (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf.gypi#new...
components/pdf.gypi:10: 'target_name': 'pdf_pepper_common',
nit: These targets should have the same names as the component, i.e. either the
component should be pdf_pepper or these names should be pdf_common etc.

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
File components/pdf/browser/open_pdf_in_reader_prompt_delegate.h (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/open_pdf_in_reader_prompt_delegate.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
nit: personally i wouldn't change this (same comment applies if there are other
copied files that you've done this for).

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/open_pdf_in_reader_prompt_delegate.h:16: class
OpenPDFInReaderPromptDelegate {
nit: for this class and the other Delegate class, the naming convention in
components is to use Client rather than Delegate for interfaces that are
implemented by the embedder.

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
File components/pdf/browser/pdf_tab_helper.h (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper.h:26: class PDFTabHelper : public
content::WebContentsObserver,
optional nit: PDFTabHelper might not be the appropriate name for this class
anymore.

[sadrul, 2014-08-27 11:49:57]

https://codereview.chromium.org/477263003/diff/250001/chrome/renderer/pepper/...
File chrome/renderer/pepper/chrome_pdf_print_delegate.cc (right):

https://codereview.chromium.org/477263003/diff/250001/chrome/renderer/pepper/...
chrome/renderer/pepper/chrome_pdf_print_delegate.cc:41:
printing::PrintWebViewHelper* helper = GetPrintWebViewHelper(element);
On 2014/08/27 03:34:09, raymes wrote:
> nit: looks like the above two lines are always called together, so you could
> just combine them and have
> printing::PrintWebViewHelper* helper = GetPrintWebViewHelper(instance_id)

|element| is actually used below for PrintNode too (line 49).

https://codereview.chromium.org/477263003/diff/250001/components/pdf.gypi
File components/pdf.gypi (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf.gypi#new...
components/pdf.gypi:10: 'target_name': 'pdf_pepper_common',
On 2014/08/27 09:12:32, blundell wrote:
> nit: These targets should have the same names as the component, i.e. either
the
> component should be pdf_pepper or these names should be pdf_common etc.

Done (renamed the components to pdf_browser, pdf_common, pdf_renderer).

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
File components/pdf/browser/open_pdf_in_reader_prompt_delegate.h (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/open_pdf_in_reader_prompt_delegate.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2014/08/27 09:12:32, blundell wrote:
> nit: personally i wouldn't change this (same comment applies if there are
other
> copied files that you've done this for).

Done.

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/open_pdf_in_reader_prompt_delegate.h:16: class
OpenPDFInReaderPromptDelegate {
On 2014/08/27 09:12:32, blundell wrote:
> nit: for this class and the other Delegate class, the naming convention in
> components is to use Client rather than Delegate for interfaces that are
> implemented by the embedder.

How strongly do you feel about this? Since this is relocation of existing code
(and there are a number of *Delegate interfaces in other components, e.g.
autofill, precache, web_modal, translate etc.), I would rather keep this
unchanged.

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
File components/pdf/browser/pdf_tab_helper.cc (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper.cc:102: }
On 2014/08/27 03:34:09, raymes wrote:
> nit: it would be better for this to be in the same order as the header.

Done.

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
File components/pdf/browser/pdf_tab_helper.h (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/pdf_tab_helper.h:26: class PDFTabHelper : public
content::WebContentsObserver,
On 2014/08/27 09:12:32, blundell wrote:
> optional nit: PDFTabHelper might not be the appropriate name for this class
> anymore.

Happy to rename. What would you suggest? PDFWebContentsHelper?

[blundell, 2014-08-27 12:24:48]

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
File components/pdf/browser/open_pdf_in_reader_prompt_delegate.h (right):

https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
components/pdf/browser/open_pdf_in_reader_prompt_delegate.h:16: class
OpenPDFInReaderPromptDelegate {
On 2014/08/27 11:49:57, sadrul wrote:
> On 2014/08/27 09:12:32, blundell wrote:
> > nit: for this class and the other Delegate class, the naming convention in
> > components is to use Client rather than Delegate for interfaces that are
> > implemented by the embedder.
> 
> How strongly do you feel about this? Since this is relocation of existing code
> (and there are a number of *Delegate interfaces in other components, e.g.
> autofill, precache, web_modal, translate etc.), I would rather keep this
> unchanged.

I feel pretty strongly about it. The ones that you point out are either (a)
vestiges from before this naming convention was crystallized (e.g.,
visitedlink_delegate.h) or (b) not interfaces that are implemented by the
embedder (e.g., translate_ui_delegate.h, autofill_external_delegate.h). Now that
the convention is in place, I'd rather not muddy the waters further.

[sadrul, 2014-08-27 13:42:44]

On 2014/08/27 12:24:48, blundell wrote:
>
https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
> File components/pdf/browser/open_pdf_in_reader_prompt_delegate.h (right):
> 
>
https://codereview.chromium.org/477263003/diff/250001/components/pdf/browser/...
> components/pdf/browser/open_pdf_in_reader_prompt_delegate.h:16: class
> OpenPDFInReaderPromptDelegate {
> On 2014/08/27 11:49:57, sadrul wrote:
> > On 2014/08/27 09:12:32, blundell wrote:
> > > nit: for this class and the other Delegate class, the naming convention in
> > > components is to use Client rather than Delegate for interfaces that are
> > > implemented by the embedder.
> > 
> > How strongly do you feel about this? Since this is relocation of existing
code
> > (and there are a number of *Delegate interfaces in other components, e.g.
> > autofill, precache, web_modal, translate etc.), I would rather keep this
> > unchanged.
> 
> I feel pretty strongly about it. The ones that you point out are either (a)
> vestiges from before this naming convention was crystallized (e.g.,
> visitedlink_delegate.h) or (b) not interfaces that are implemented by the
> embedder (e.g., translate_ui_delegate.h, autofill_external_delegate.h). Now
that
> the convention is in place, I'd rather not muddy the waters further.

Fair enough. Done.

[blundell, 2014-08-27 13:46:52]

//components LGTM

The clients should probably have class comments in the headers.

Thanks!

[blundell, 2014-08-27 13:49:46]

https://codereview.chromium.org/477263003/diff/310001/components/pdf/browser/...
File components/pdf/browser/open_pdf_in_reader_prompt_client.h (right):

https://codereview.chromium.org/477263003/diff/310001/components/pdf/browser/...
components/pdf/browser/open_pdf_in_reader_prompt_client.h:16: class
OpenPDFInReaderPromptClient {
Somehow I'm missing where this interface is implemented in //chrome...

[Tom Sepez, 2014-08-27 17:24:50]

Could you also put an include of your new message .h file into
//src/tools/ipc_fuzzer/message_lib/all_messages.h?

LGTM with nit otherwise.

https://codereview.chromium.org/477263003/diff/310001/components/pdf/browser/...
File components/pdf/browser/pdf_web_contents_helper.cc (right):

https://codereview.chromium.org/477263003/diff/310001/components/pdf/browser/...
components/pdf/browser/pdf_web_contents_helper.cc:46:
IPC_MESSAGE_HANDLER(PDFHostMsg_PDFHasUnsupportedFeature,
nit: generally, we indent the IPC_MESSAGE_HANDLER/UNHANDLED() macros 2 more
spaces than the surrounding IPC_BEGIN/END_MESSAGE_MAP() macros.

[dmichael (off chromium), 2014-08-27 20:03:13]

I suspect that's the clang formatter...
https://code.google.com/p/chromium/issues/detail?id=373340
(not passing judgement either way, personally...  those who care can decide
whether to go with the formatter or do this one manually)


On Wed, Aug 27, 2014 at 11:24 AM, <tsepez@chromium.org> wrote:

> Could you also put an include of your new message .h file into
> //src/tools/ipc_fuzzer/message_lib/all_messages.h?
>
> LGTM with nit otherwise.
>
>
> https://codereview.chromium.org/477263003/diff/310001/
> components/pdf/browser/pdf_web_contents_helper.cc
> File components/pdf/browser/pdf_web_contents_helper.cc (right):
>
> https://codereview.chromium.org/477263003/diff/310001/
> components/pdf/browser/pdf_web_contents_helper.cc#newcode46
> components/pdf/browser/pdf_web_contents_helper.cc:46:
> IPC_MESSAGE_HANDLER(PDFHostMsg_PDFHasUnsupportedFeature,
> nit: generally, we indent the IPC_MESSAGE_HANDLER/UNHANDLED() macros 2
> more spaces than the surrounding IPC_BEGIN/END_MESSAGE_MAP() macros.
>
>
> https://codereview.chromium.org/477263003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-08-27 23:39:20]

On 2014/08/27 17:24:50, Tom Sepez wrote:
> Could you also put an include of your new message .h file into
> //src/tools/ipc_fuzzer/message_lib/all_messages.h?

Done.

https://codereview.chromium.org/477263003/diff/310001/components/pdf/browser/...
File components/pdf/browser/open_pdf_in_reader_prompt_client.h (right):

https://codereview.chromium.org/477263003/diff/310001/components/pdf/browser/...
components/pdf/browser/open_pdf_in_reader_prompt_client.h:16: class
OpenPDFInReaderPromptClient {
On 2014/08/27 13:49:45, blundell wrote:
> Somehow I'm missing where this interface is implemented in //chrome...

There are a couple of implementations in pdf_unsupported_feature.cc

https://codereview.chromium.org/477263003/diff/310001/components/pdf/browser/...
File components/pdf/browser/pdf_web_contents_helper.cc (right):

https://codereview.chromium.org/477263003/diff/310001/components/pdf/browser/...
components/pdf/browser/pdf_web_contents_helper.cc:46:
IPC_MESSAGE_HANDLER(PDFHostMsg_PDFHasUnsupportedFeature,
On 2014/08/27 17:24:49, Tom Sepez wrote:
> nit: generally, we indent the IPC_MESSAGE_HANDLER/UNHANDLED() macros 2 more
> spaces than the surrounding IPC_BEGIN/END_MESSAGE_MAP() macros.

Done, sorry. I addressed this in an earlier patchset too, but git cl-format
keeps undoing it.

[sadrul, 2014-08-27 23:42:25]

sadrul@chromium.org changed reviewers:
+ darin@chromium.org

[sadrul, 2014-08-27 23:42:25]

+darin@ for added DEPS on third_party/WebKit/public, third_party/skia/include,
and ui/base/

[sadrul, 2014-08-28 03:49:43]

On 2014/08/27 23:42:25, sadrul wrote:
> +darin@ for added DEPS on third_party/WebKit/public, third_party/skia/include,
> and ui/base/

Going to TBR for this change since the other modules //component/pdf/ depends on
already have these dependencies as well (and also the existing code have the
same dependency as well).

[sadrul, 2014-08-28 03:51:01]

Committed patchset #18 (id:330001) to pending queue manually as 164c7ca
(presubmit successful).

[darin (slow to review), 2014-08-28 04:56:26]

https://codereview.chromium.org/477263003/diff/330001/components/pdf/renderer...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/477263003/diff/330001/components/pdf/renderer...
components/pdf/renderer/ppb_pdf_impl.cc:13: #include
"content/app/resources/grit/content_resources.h"
is it correct for things outside of content to refer to things in content/app/?
shouldn't code outside of content only refer to things inside of
content/public/?

[sadrul, 2014-08-28 04:58:28]

https://codereview.chromium.org/477263003/diff/330001/components/pdf/renderer...
File components/pdf/renderer/ppb_pdf_impl.cc (right):

https://codereview.chromium.org/477263003/diff/330001/components/pdf/renderer...
components/pdf/renderer/ppb_pdf_impl.cc:13: #include
"content/app/resources/grit/content_resources.h"
On 2014/08/28 04:56:26, darin wrote:
> is it correct for things outside of content to refer to things in
content/app/?
> shouldn't code outside of content only refer to things inside of
> content/public/?

Indeed. jam@ also raised this earlier. I am moving the relevant resources out of
content into this component as well so that we don't have this anymore:
https://codereview.chromium.org/506473003/ (I wanted to do it in a separate
patch to keep the CL size manageable).

[commit-bot: I haz the power, 2014-09-10 02:57:15]

Patchset 18 (id:??) landed as
https://crrev.com/2f8807f697e2c043c857ec37c43d45573d239d8f
Cr-Commit-Position: refs/heads/master@{#292313}