Adding functionality to print PDF embedded in the html page.

Adding functionality to print PDF embedded in the html page.
Additional changes needed in PDF plugin to use this functionality.

BUG=89241
TEST=Test printing using PDF toolbar on the embedded and normal PDFs. Please do so AFTER PDF changes will be submitted (they have to be submitted separately).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117699

[jam, 2012-01-11 20:13:01]

the pdf plugin calls to chrome/renderer/chrome_ppb_pdf_impl.cc code, which then
calls content's PrintPlugin to sent a content IPC (ViewHostMsg_PrintPlugin).
This is received in TabContents which sends a chrome IPC (layering violation
which will cause a checkdeps error) to chrome's PrintWebViewHelper. why not just
have chrome_ppb_pdf_impl.cc talk to the PrintWebViewHelper directly?

[gene1, 2012-01-11 20:19:26]

On Wed, Jan 11, 2012 at 12:13 PM, <jam@chromium.org> wrote:

> the pdf plugin calls to chrome/renderer/chrome_ppb_**pdf_impl.cc code,
> which then
> calls content's PrintPlugin to sent a content IPC
> (ViewHostMsg_PrintPlugin).
> This is received in TabContents which sends a chrome IPC (layering
> violation
> which will cause a checkdeps error) to chrome's PrintWebViewHelper. why
> not just
> have chrome_ppb_pdf_impl.cc talk to the PrintWebViewHelper directly?
>

I was considering this approach as well, but after talking to thestig he
suggested this round trip. If this is a security issue, I am open to
revisit this.
Would you strongly advice talking to  PrintWebViewHelper directly  from
chrome_ppb_**pdf_impl.cc ???


>
>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>

[jam, 2012-01-11 20:26:09]

On Wed, Jan 11, 2012 at 12:19 PM, Gene Gutnik <gene@google.com> wrote:

> On Wed, Jan 11, 2012 at 12:13 PM, <jam@chromium.org> wrote:
>
>> the pdf plugin calls to chrome/renderer/chrome_ppb_**pdf_impl.cc code,
>> which then
>> calls content's PrintPlugin to sent a content IPC
>> (ViewHostMsg_PrintPlugin).
>> This is received in TabContents which sends a chrome IPC (layering
>> violation
>> which will cause a checkdeps error) to chrome's PrintWebViewHelper. why
>> not just
>> have chrome_ppb_pdf_impl.cc talk to the PrintWebViewHelper directly?
>>
>
> I was considering this approach as well, but after talking to thestig he
> suggested this round trip. If this is a security issue, I am open to
> revisit this.
>

Lei: I'm curious why you suggested this?

It's not a security issue. However there's a needless round trip to the
browser, and there's a layering violation. The latter definitely needs to
be fixed, you'll see the trybots fail because of this.

Would you strongly advice talking to  PrintWebViewHelper directly  from
> chrome_ppb_**pdf_impl.cc ???
>
>
>>
>>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>>
>
>

[Lei Zhang, 2012-01-12 00:04:48]

I wanted to avoid calls to PrintWebViewHelper::PrintPage() since that code
handles window.print(), which has additional complications that don't apply to
us.

I didn't know the pepper code goes into content/, which would present a layering
violation that's a no go. Given this, we should just have the code stay in the
renderer.

How do we get chrome_ppb_pdf_impl.cc to talk to PrintWebViewHelper though?
PrintWebViewHelper does everything via message handling. Can the renderer send
itself a message or do we add a public method to PrintWebViewHelper?

[jam, 2012-01-12 00:25:58]

On Wed, Jan 11, 2012 at 4:04 PM, <thestig@chromium.org> wrote:

> I wanted to avoid calls to PrintWebViewHelper::PrintPage(**) since that
> code
> handles window.print(), which has additional complications that don't
> apply to
> us.
>

I'm not sure why that's an issue? Why can't whatever method in
PrintWebViewHelper that's called from the current IPC from chrome should
just be called directly from chrome_ppb_pdf_impl.cc?


> I didn't know the pepper code goes into content/, which would present a
> layering
> violation that's a no go. Given this, we should just have the code stay in
> the
> renderer.
>
> How do we get chrome_ppb_pdf_impl.cc to talk to PrintWebViewHelper though?
> PrintWebViewHelper does everything via message handling. Can the renderer
> send
> itself a message or do we add a public method to PrintWebViewHelper?
>

why would you want to send IPCs from teh renderer to the renderer instead
of calling a public method on PWVH?


>
>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>

[Lei Zhang, 2012-01-12 00:37:17]

On 2012/01/12 00:25:58, John Abd-El-Malek wrote:
> On Wed, Jan 11, 2012 at 4:04 PM, <mailto:thestig@chromium.org> wrote:
> 
> > I wanted to avoid calls to PrintWebViewHelper::PrintPage(**) since that
> > code
> > handles window.print(), which has additional complications that don't
> > apply to
> > us.
> >
> 
> I'm not sure why that's an issue? Why can't whatever method in
> PrintWebViewHelper that's called from the current IPC from chrome should
> just be called directly from chrome_ppb_pdf_impl.cc?

None of the existing methods are appropriate, but Gene added one that is.

> > I didn't know the pepper code goes into content/, which would present a
> > layering
> > violation that's a no go. Given this, we should just have the code stay in
> > the
> > renderer.
> >
> > How do we get chrome_ppb_pdf_impl.cc to talk to PrintWebViewHelper though?
> > PrintWebViewHelper does everything via message handling. Can the renderer
> > send
> > itself a message or do we add a public method to PrintWebViewHelper?
> >
> 
> why would you want to send IPCs from teh renderer to the renderer instead
> of calling a public method on PWVH?

There's no way to access the PWVH currently. Just add one to
ChromeContentRendererClient?

[jam, 2012-01-12 00:42:30]

On Wed, Jan 11, 2012 at 4:37 PM, <thestig@chromium.org> wrote:

> On 2012/01/12 00:25:58, John Abd-El-Malek wrote:
>
>> On Wed, Jan 11, 2012 at 4:04 PM, <mailto:thestig@chromium.org> wrote:
>>
>
>  > I wanted to avoid calls to PrintWebViewHelper::PrintPage(****) since
>> that
>>
>> > code
>> > handles window.print(), which has additional complications that don't
>> > apply to
>> > us.
>> >
>>
>
>  I'm not sure why that's an issue? Why can't whatever method in
>> PrintWebViewHelper that's called from the current IPC from chrome should
>> just be called directly from chrome_ppb_pdf_impl.cc?
>>
>
> None of the existing methods are appropriate, but Gene added one that is.


ok, that seems to be a non-issue now then


>
>
>  > I didn't know the pepper code goes into content/, which would present a
>> > layering
>> > violation that's a no go. Given this, we should just have the code stay
>> in
>> > the
>> > renderer.
>> >
>> > How do we get chrome_ppb_pdf_impl.cc to talk to PrintWebViewHelper
>> though?
>> > PrintWebViewHelper does everything via message handling. Can the
>> renderer
>> > send
>> > itself a message or do we add a public method to PrintWebViewHelper?
>> >
>>
>
>  why would you want to send IPCs from teh renderer to the renderer instead
>> of calling a public method on PWVH?
>>
>
> There's no way to access the PWVH currently. Just add one to
> ChromeContentRendererClient?
>

ContentRendererClient is an interface to allow content to reach out to its
embedder (i.e. chrome). In this case, you want chrome to talk to chrome, so
we shouldn't need to modify the embedder interface.

PrintWebViewHelper derives from RenderViewObserverTracker, so we can get to
it from the RenderView*.

>
>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>

[gene1, 2012-01-12 01:03:47]

On Wed, Jan 11, 2012 at 4:42 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Wed, Jan 11, 2012 at 4:37 PM, <thestig@chromium.org> wrote:
>
>> On 2012/01/12 00:25:58, John Abd-El-Malek wrote:
>>
>>> On Wed, Jan 11, 2012 at 4:04 PM, <mailto:thestig@chromium.org> wrote:
>>>
>>
>>  > I wanted to avoid calls to PrintWebViewHelper::PrintPage(****) since
>>> that
>>>
>>> > code
>>> > handles window.print(), which has additional complications that don't
>>> > apply to
>>> > us.
>>> >
>>>
>>
>>  I'm not sure why that's an issue? Why can't whatever method in
>>> PrintWebViewHelper that's called from the current IPC from chrome should
>>> just be called directly from chrome_ppb_pdf_impl.cc?
>>>
>>
>> None of the existing methods are appropriate, but Gene added one that is.
>
>
> ok, that seems to be a non-issue now then
>
>
>>
>>
>>  > I didn't know the pepper code goes into content/, which would present a
>>> > layering
>>> > violation that's a no go. Given this, we should just have the code
>>> stay in
>>> > the
>>> > renderer.
>>> >
>>> > How do we get chrome_ppb_pdf_impl.cc to talk to PrintWebViewHelper
>>> though?
>>> > PrintWebViewHelper does everything via message handling. Can the
>>> renderer
>>> > send
>>> > itself a message or do we add a public method to PrintWebViewHelper?
>>> >
>>>
>>
>>  why would you want to send IPCs from teh renderer to the renderer instead
>>> of calling a public method on PWVH?
>>>
>>
>> There's no way to access the PWVH currently. Just add one to
>> ChromeContentRendererClient?
>>
>
> ContentRendererClient is an interface to allow content to reach out to its
> embedder (i.e. chrome). In this case, you want chrome to talk to chrome, so
> we shouldn't need to modify the embedder interface.
>
> PrintWebViewHelper derives from RenderViewObserverTracker, so we can get
> to it from the RenderView*.
>

Let me try this then. Any pointers for the similar existing code would be
appreciated :).


>>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>>
>
>

[jam, 2012-01-12 03:15:26]

On Wed, Jan 11, 2012 at 5:03 PM, Gene Gutnik <gene@google.com> wrote:

>
>
> On Wed, Jan 11, 2012 at 4:42 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>>
>>
>> On Wed, Jan 11, 2012 at 4:37 PM, <thestig@chromium.org> wrote:
>>
>>> On 2012/01/12 00:25:58, John Abd-El-Malek wrote:
>>>
>>>> On Wed, Jan 11, 2012 at 4:04 PM, <mailto:thestig@chromium.org> wrote:
>>>>
>>>
>>>  > I wanted to avoid calls to PrintWebViewHelper::PrintPage(****) since
>>>> that
>>>>
>>>> > code
>>>> > handles window.print(), which has additional complications that don't
>>>> > apply to
>>>> > us.
>>>> >
>>>>
>>>
>>>  I'm not sure why that's an issue? Why can't whatever method in
>>>> PrintWebViewHelper that's called from the current IPC from chrome should
>>>> just be called directly from chrome_ppb_pdf_impl.cc?
>>>>
>>>
>>> None of the existing methods are appropriate, but Gene added one that is.
>>
>>
>> ok, that seems to be a non-issue now then
>>
>>
>>>
>>>
>>>  > I didn't know the pepper code goes into content/, which would present
>>>> a
>>>> > layering
>>>> > violation that's a no go. Given this, we should just have the code
>>>> stay in
>>>> > the
>>>> > renderer.
>>>> >
>>>> > How do we get chrome_ppb_pdf_impl.cc to talk to PrintWebViewHelper
>>>> though?
>>>> > PrintWebViewHelper does everything via message handling. Can the
>>>> renderer
>>>> > send
>>>> > itself a message or do we add a public method to PrintWebViewHelper?
>>>> >
>>>>
>>>
>>>  why would you want to send IPCs from teh renderer to the renderer
>>>> instead
>>>> of calling a public method on PWVH?
>>>>
>>>
>>> There's no way to access the PWVH currently. Just add one to
>>> ChromeContentRendererClient?
>>>
>>
>> ContentRendererClient is an interface to allow content to reach out to
>> its embedder (i.e. chrome). In this case, you want chrome to talk to
>> chrome, so we shouldn't need to modify the embedder interface.
>>
>> PrintWebViewHelper derives from RenderViewObserverTracker, so we can get
>> to it from the RenderView*.
>>
>
> Let me try this then. Any pointers for the similar existing code would be
> appreciated :).
>

PrintWebViewHelper::Get(render_view)


>
>
>>>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>>>
>>
>>
>

[gene, 2012-01-12 21:56:46]

Done. Could you please take a look?


On 2012/01/12 03:15:26, John Abd-El-Malek wrote:
> On Wed, Jan 11, 2012 at 5:03 PM, Gene Gutnik <mailto:gene@google.com> wrote:
> 
> >
> >
> > On Wed, Jan 11, 2012 at 4:42 PM, John Abd-El-Malek <jam@chromium.org>wrote:
> >
> >>
> >>
> >> On Wed, Jan 11, 2012 at 4:37 PM, <mailto:thestig@chromium.org> wrote:
> >>
> >>> On 2012/01/12 00:25:58, John Abd-El-Malek wrote:
> >>>
> >>>> On Wed, Jan 11, 2012 at 4:04 PM, <mailto:thestig@chromium.org> wrote:
> >>>>
> >>>
> >>>  > I wanted to avoid calls to PrintWebViewHelper::PrintPage(****) since
> >>>> that
> >>>>
> >>>> > code
> >>>> > handles window.print(), which has additional complications that don't
> >>>> > apply to
> >>>> > us.
> >>>> >
> >>>>
> >>>
> >>>  I'm not sure why that's an issue? Why can't whatever method in
> >>>> PrintWebViewHelper that's called from the current IPC from chrome should
> >>>> just be called directly from chrome_ppb_pdf_impl.cc?
> >>>>
> >>>
> >>> None of the existing methods are appropriate, but Gene added one that is.
> >>
> >>
> >> ok, that seems to be a non-issue now then
> >>
> >>
> >>>
> >>>
> >>>  > I didn't know the pepper code goes into content/, which would present
> >>>> a
> >>>> > layering
> >>>> > violation that's a no go. Given this, we should just have the code
> >>>> stay in
> >>>> > the
> >>>> > renderer.
> >>>> >
> >>>> > How do we get chrome_ppb_pdf_impl.cc to talk to PrintWebViewHelper
> >>>> though?
> >>>> > PrintWebViewHelper does everything via message handling. Can the
> >>>> renderer
> >>>> > send
> >>>> > itself a message or do we add a public method to PrintWebViewHelper?
> >>>> >
> >>>>
> >>>
> >>>  why would you want to send IPCs from teh renderer to the renderer
> >>>> instead
> >>>> of calling a public method on PWVH?
> >>>>
> >>>
> >>> There's no way to access the PWVH currently. Just add one to
> >>> ChromeContentRendererClient?
> >>>
> >>
> >> ContentRendererClient is an interface to allow content to reach out to
> >> its embedder (i.e. chrome). In this case, you want chrome to talk to
> >> chrome, so we shouldn't need to modify the embedder interface.
> >>
> >> PrintWebViewHelper derives from RenderViewObserverTracker, so we can get
> >> to it from the RenderView*.
> >>
> >
> > Let me try this then. Any pointers for the similar existing code would be
> > appreciated :).
> >
> 
> PrintWebViewHelper::Get(render_view)
> 
> 
> >
> >
> >>>
>
http://codereview.chromium.**org/9139024/%3Chttp://codereview.chromium.org/91...>
> >>>
> >>
> >>
> >

[Lei Zhang, 2012-01-12 22:10:20]

LGTM with nits below:

http://codereview.chromium.org/9139024/diff/2014/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (left):

http://codereview.chromium.org/9139024/diff/2014/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:127: DCHECK_NE(NOT_PREVIEWING,
print_preview_state_);
Can you comment this out and add the following?
// TODO(thestig) Fix this. http://crbug.com/105640

http://codereview.chromium.org/9139024/diff/2014/chrome/renderer/chrome_ppb_p...
File chrome/renderer/chrome_ppb_pdf_impl.cc (right):

http://codereview.chromium.org/9139024/diff/2014/chrome/renderer/chrome_ppb_p...
chrome/renderer/chrome_ppb_pdf_impl.cc:27: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebNode.h"
nit: WebNode before WebPluginContainer.

http://codereview.chromium.org/9139024/diff/2014/chrome/renderer/chrome_ppb_p...
chrome/renderer/chrome_ppb_pdf_impl.cc:344: WebView* view =
instance->container()->element().document().frame()->view();
nit: you can make instance->container()->element() a variable and then you don't
have to write it out again on line 349.

[gene, 2012-01-12 22:21:18]

done.

On 2012/01/12 22:10:20, Lei Zhang wrote:
> LGTM with nits below:
> 
>
http://codereview.chromium.org/9139024/diff/2014/chrome/browser/printing/prin...
> File chrome/browser/printing/print_view_manager.cc (left):
> 
>
http://codereview.chromium.org/9139024/diff/2014/chrome/browser/printing/prin...
> chrome/browser/printing/print_view_manager.cc:127: DCHECK_NE(NOT_PREVIEWING,
> print_preview_state_);
> Can you comment this out and add the following?
> // TODO(thestig) Fix this. http://crbug.com/105640
> 
>
http://codereview.chromium.org/9139024/diff/2014/chrome/renderer/chrome_ppb_p...
> File chrome/renderer/chrome_ppb_pdf_impl.cc (right):
> 
>
http://codereview.chromium.org/9139024/diff/2014/chrome/renderer/chrome_ppb_p...
> chrome/renderer/chrome_ppb_pdf_impl.cc:27: #include
> "third_party/WebKit/Source/WebKit/chromium/public/WebNode.h"
> nit: WebNode before WebPluginContainer.
> 
>
http://codereview.chromium.org/9139024/diff/2014/chrome/renderer/chrome_ppb_p...
> chrome/renderer/chrome_ppb_pdf_impl.cc:344: WebView* view =
> instance->container()->element().document().frame()->view();
> nit: you can make instance->container()->element() a variable and then you
don't
> have to write it out again on line 349.

[jam, 2012-01-12 22:43:00]

lgtm

http://codereview.chromium.org/9139024/diff/4005/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/9139024/diff/4005/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:94: // directly from PPAPI for plugin
printing.
this comment is redundant since it's describing something that's obvious. the
comment for the method's definition should explain what the function does,
although in this particular case, the name is pretty self documenting.

see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...

[gene1, 2012-01-12 22:51:41]

ok, I'll remove the comment.


On Thu, Jan 12, 2012 at 2:43 PM, <jam@chromium.org> wrote:

> lgtm
>
>
> http://codereview.chromium.**org/9139024/diff/4005/chrome/**
>
renderer/print_web_view_**helper.h<http://codereview.chromium.org/9139024/diff/4005/chrome/renderer/print_web_view_helper.h>
> File chrome/renderer/print_web_**view_helper.h (right):
>
> http://codereview.chromium.**org/9139024/diff/4005/chrome/**
>
renderer/print_web_view_**helper.h#newcode94<http://codereview.chromium.org/9139024/diff/4005/chrome/renderer/print_web_view_helper.h#newcode94>
> chrome/renderer/print_web_**view_helper.h:94: // directly from PPAPI for
> plugin printing.
> this comment is redundant since it's describing something that's
> obvious. the comment for the method's definition should explain what the
> function does, although in this particular case, the name is pretty self
> documenting.
>
> see
> http://google-styleguide.**googlecode.com/svn/trunk/**
>
cppguide.xml?showone=**Implementation_Comments#**Implementation_Comments<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implementation_Comments#Implementation_Comments>
>
>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>

[commit-bot: I haz the power, 2012-01-12 22:53:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/9139024/51

[commit-bot: I haz the power, 2012-01-12 22:53:15]

Presubmit check for 9139024-51 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
Missing matching PPAPI definition:

***************
  ppapi/api/private/ppb_pdf.idl
***************

** Presubmit ERRORS **
Missing LGTM from an OWNER for: ppapi/c/private/ppb_pdf.h

Presubmit checks took 2.2s to calculate.

[piman, 2012-01-12 23:11:15]

http://codereview.chromium.org/9139024/diff/51/ppapi/c/private/ppb_pdf.h
File ppapi/c/private/ppb_pdf.h (right):

http://codereview.chromium.org/9139024/diff/51/ppapi/c/private/ppb_pdf.h#newc...
ppapi/c/private/ppb_pdf.h:13: #define PPB_PDF_INTERFACE "PPB_PDF;1"
The interface needs to rev if the interface changes

[gene, 2012-01-12 23:15:58]

John,
we've never updated this version in the past. Was there a reason for this?

[gene, 2012-01-12 23:29:51]

done. could you please take a look?

On 2012/01/12 23:11:15, piman wrote:
> http://codereview.chromium.org/9139024/diff/51/ppapi/c/private/ppb_pdf.h
> File ppapi/c/private/ppb_pdf.h (right):
> 
>
http://codereview.chromium.org/9139024/diff/51/ppapi/c/private/ppb_pdf.h#newc...
> ppapi/c/private/ppb_pdf.h:13: #define PPB_PDF_INTERFACE "PPB_PDF;1"
> The interface needs to rev if the interface changes

[jam, 2012-01-12 23:32:27]

On 2012/01/12 23:15:58, gene wrote:
> John,
> we've never updated this version in the past. Was there a reason for this?

this isn't a public API, and we only support the PDF plugin with the version of
chrome that it's built for. so revving the API version is unnecessary. that's
really done for plugins written by outside developers, or ones that don't get
built alongside chrome.

[piman, 2012-01-12 23:33:41]

On 2012/01/12 23:32:27, John Abd-El-Malek wrote:
> On 2012/01/12 23:15:58, gene wrote:
> > John,
> > we've never updated this version in the past. Was there a reason for this?
> 
> this isn't a public API, and we only support the PDF plugin with the version
of
> chrome that it's built for. so revving the API version is unnecessary. that's
> really done for plugins written by outside developers, or ones that don't get
> built alongside chrome.

Flash is out of tree and uses this interface.

[piman, 2012-01-12 23:33:56]

lgtm

[gene, 2012-01-12 23:36:43]

thanks everyone,
just to confirm, is everybody ok with pdf api version set to 2?

[jam, 2012-01-12 23:39:00]

On Thu, Jan 12, 2012 at 3:33 PM, <piman@chromium.org> wrote:

> On 2012/01/12 23:32:27, John Abd-El-Malek wrote:
>
>> On 2012/01/12 23:15:58, gene wrote:
>> > John,
>> > we've never updated this version in the past. Was there a reason for
>> this?
>>
>
>  this isn't a public API, and we only support the PDF plugin with the
>> version
>>
> of
>
>> chrome that it's built for. so revving the API version is unnecessary.
>> that's
>> really done for plugins written by outside developers, or ones that don't
>> get
>> built alongside chrome.
>>
>
> Flash is out of tree and uses this interface.
>

i didn't know that. for what? font stuff? if so, it's confusing that it's
still called pdf.

either way, since this is adding a function at the end, technically it
doesn't matter if we add a new function, no? if you rev this, won't we have
to keep a copy of both new and old interface? seems like unnecessary work.

>
>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>

[jam, 2012-01-12 23:40:17]

On Thu, Jan 12, 2012 at 3:36 PM, <gene@chromium.org> wrote:

> thanks everyone,
> just to confirm, is everybody ok with pdf api version set to 2?
>

I dont think revving the version would be enough. you'd have to also keep
the old struct around, and have the chrome code return different ones
depending on the requested version. i dont think all this is necessary
though.

>
>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>

[piman, 2012-01-12 23:49:11]

On Thu, Jan 12, 2012 at 3:38 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Thu, Jan 12, 2012 at 3:33 PM, <piman@chromium.org> wrote:
>
>> On 2012/01/12 23:32:27, John Abd-El-Malek wrote:
>>
>>> On 2012/01/12 23:15:58, gene wrote:
>>> > John,
>>> > we've never updated this version in the past. Was there a reason for
>>> this?
>>>
>>
>>  this isn't a public API, and we only support the PDF plugin with the
>>> version
>>>
>> of
>>
>>> chrome that it's built for. so revving the API version is unnecessary.
>>> that's
>>> really done for plugins written by outside developers, or ones that
>>> don't get
>>> built alongside chrome.
>>>
>>
>> Flash is out of tree and uses this interface.
>>
>
> i didn't know that. for what? font stuff? if so, it's confusing that it's
> still called pdf.
>

I'm not sure why, I think it's for font stuff indeed. Brett would know but
he's out.


> either way, since this is adding a function at the end, technically it
> doesn't matter if we add a new function, no?
>

It matters if you use a newer struct with an older browser. The result is
random crashes. Debugging ODR violations is really not fun. I've been
burned so many times with this that I'm asking to be strict.


> if you rev this, won't we have to keep a copy of both new and old
> interface? seems like unnecessary work.
>

Just revving will break Flash, but in an explicit way (it will fail to
load, not randomly crash).
The fix in Flash is trivial (just update ppapi and recompile).


>
>>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>>
>
>

[jam, 2012-01-13 00:09:16]

On Thu, Jan 12, 2012 at 3:48 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Thu, Jan 12, 2012 at 3:38 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>>
>>
>> On Thu, Jan 12, 2012 at 3:33 PM, <piman@chromium.org> wrote:
>>
>>> On 2012/01/12 23:32:27, John Abd-El-Malek wrote:
>>>
>>>> On 2012/01/12 23:15:58, gene wrote:
>>>> > John,
>>>> > we've never updated this version in the past. Was there a reason for
>>>> this?
>>>>
>>>
>>>  this isn't a public API, and we only support the PDF plugin with the
>>>> version
>>>>
>>> of
>>>
>>>> chrome that it's built for. so revving the API version is unnecessary.
>>>> that's
>>>> really done for plugins written by outside developers, or ones that
>>>> don't get
>>>> built alongside chrome.
>>>>
>>>
>>> Flash is out of tree and uses this interface.
>>>
>>
>> i didn't know that. for what? font stuff? if so, it's confusing that it's
>> still called pdf.
>>
>
> I'm not sure why, I think it's for font stuff indeed. Brett would know but
> he's out.
>
>
>> either way, since this is adding a function at the end, technically it
>> doesn't matter if we add a new function, no?
>>
>
> It matters if you use a newer struct with an older browser. The result is
> random crashes. Debugging ODR violations is really not fun. I've been
> burned so many times with this that I'm asking to be strict.
>
>
>> if you rev this, won't we have to keep a copy of both new and old
>> interface? seems like unnecessary work.
>>
>
> Just revving will break Flash, but in an explicit way (it will fail to
> load, not randomly crash).
> The fix in Flash is trivial (just update ppapi and recompile).
>

if you feel strongly about this, then i defer to you. i just feel in this
case, this is really not worth it. we added a new function. we know flash
won't call it anytime soon. there'll be no crashes. as opposed to now
revving it, which means also updating flash+pdf.


>
>
>>
>>>
http://codereview.chromium.**org/9139024/<http://codereview.chromium.org/9139...
>>>
>>
>>
>

[piman, 2012-01-13 00:48:06]

Ok, I checked with Trung, and he said he'll deal with the fallout of ODR
violations. So if you want to not rev the interface, go for it (I'm washing my
hands).
LGTM either way.

[commit-bot: I haz the power, 2012-01-13 20:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/9139024/10001

[commit-bot: I haz the power, 2012-01-13 20:06:09]

Presubmit check for 9139024-10001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
Missing matching PPAPI definition:

***************
  ppapi/api/private/ppb_pdf.idl
***************

Presubmit checks took 3.0s to calculate.