Add the pdf plugin's source in src\pdf.

Add the pdf plugin's source in src\pdf.

I've updated gypi files to not use internal_pdf variable anymore, which was brought in from pdf repo's supplemental.gypi.

R=thestig@chromium.org
TBR=darin

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

[jam, 2014-05-19 23:16:36]

The only changes I made to src/pdf were:
-add OWNERS and DEPS files
-updated a bunch of header includes to get rid of out-of-order warnings
-fixed some over-80-chars warnings

[Lei Zhang, 2014-05-19 23:26:42]

https://codereview.chromium.org/294793003/diff/30001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/294793003/diff/30001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:1775: '../pdf/pdf.gyp:pdf',
No longer needed? Don't we have pdf browser tests?

https://codereview.chromium.org/294793003/diff/30001/pdf/Makefile
File pdf/Makefile (right):

https://codereview.chromium.org/294793003/diff/30001/pdf/Makefile#newcode1
pdf/Makefile:1: .PHONY: all run clean
Do we still need this file? The file paths are wrong.

https://codereview.chromium.org/294793003/diff/30001/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/294793003/diff/30001/pdf/document_loader.cc#n...
pdf/document_loader.cc:178: // FoxitSDKEngine::OnDocumentComplete() ->
FPDFAvail_IsPageAvail after
There's some outdated comments like this one.

https://codereview.chromium.org/294793003/diff/30001/pdf/logging.h
File pdf/logging.h (right):

https://codereview.chromium.org/294793003/diff/30001/pdf/logging.h#newcode1
pdf/logging.h:1: // Copyright (c) 2010 The Chromium Authors. All rights
reserved.
Can we get rid of this and just use base/logging.h ?

https://codereview.chromium.org/294793003/diff/30001/pdf/supplement.gypi
File pdf/supplement.gypi (right):

https://codereview.chromium.org/294793003/diff/30001/pdf/supplement.gypi#newc...
pdf/supplement.gypi:1: {
Delete this file?

[jam, 2014-05-19 23:57:40]

https://codereview.chromium.org/294793003/diff/30001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/294793003/diff/30001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:1775: '../pdf/pdf.gyp:pdf',
On 2014/05/19 23:26:43, Lei Zhang wrote:
> No longer needed? Don't we have pdf browser tests?

now chrome depends on it, and browser_tests depends on chrome so I thought it's
fine to remove?

https://codereview.chromium.org/294793003/diff/30001/pdf/Makefile
File pdf/Makefile (right):

https://codereview.chromium.org/294793003/diff/30001/pdf/Makefile#newcode1
pdf/Makefile:1: .PHONY: all run clean
On 2014/05/19 23:26:43, Lei Zhang wrote:
> Do we still need this file? The file paths are wrong.

Done.

https://codereview.chromium.org/294793003/diff/30001/pdf/document_loader.cc
File pdf/document_loader.cc (right):

https://codereview.chromium.org/294793003/diff/30001/pdf/document_loader.cc#n...
pdf/document_loader.cc:178: // FoxitSDKEngine::OnDocumentComplete() ->
FPDFAvail_IsPageAvail after
On 2014/05/19 23:26:43, Lei Zhang wrote:
> There's some outdated comments like this one.

Done.

https://codereview.chromium.org/294793003/diff/30001/pdf/logging.h
File pdf/logging.h (right):

https://codereview.chromium.org/294793003/diff/30001/pdf/logging.h#newcode1
pdf/logging.h:1: // Copyright (c) 2010 The Chromium Authors. All rights
reserved.
On 2014/05/19 23:26:43, Lei Zhang wrote:
> Can we get rid of this and just use base/logging.h ?

Done.

https://codereview.chromium.org/294793003/diff/30001/pdf/supplement.gypi
File pdf/supplement.gypi (right):

https://codereview.chromium.org/294793003/diff/30001/pdf/supplement.gypi#newc...
pdf/supplement.gypi:1: {
On 2014/05/19 23:26:43, Lei Zhang wrote:
> Delete this file?

Done.

[jam, 2014-05-20 00:04:58]

+darin for owners stamp of "new" top level directory

[Lei Zhang, 2014-05-20 00:13:09]

lgtm

https://codereview.chromium.org/294793003/diff/30001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/294793003/diff/30001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:1775: '../pdf/pdf.gyp:pdf',
On 2014/05/19 23:57:40, jam wrote:
> On 2014/05/19 23:26:43, Lei Zhang wrote:
> > No longer needed? Don't we have pdf browser tests?
> 
> now chrome depends on it, and browser_tests depends on chrome so I thought
it's
> fine to remove?

Ok, sg.

https://codereview.chromium.org/294793003/diff/50001/pdf/chunk_stream.h
File pdf/chunk_stream.h (right):

https://codereview.chromium.org/294793003/diff/50001/pdf/chunk_stream.h#newcode9
pdf/chunk_stream.h:9: #include <stddef.h>
nit: C #includes before C++

[jam, 2014-05-20 00:20:32]

https://codereview.chromium.org/294793003/diff/50001/pdf/chunk_stream.h
File pdf/chunk_stream.h (right):

https://codereview.chromium.org/294793003/diff/50001/pdf/chunk_stream.h#newcode9
pdf/chunk_stream.h:9: #include <stddef.h>
On 2014/05/20 00:13:10, Lei Zhang wrote:
> nit: C #includes before C++

Done.

[tfarina, 2014-05-20 20:33:07]

https://codereview.chromium.org/294793003/diff/70001/pdf/pdf.h
File pdf/pdf.h (right):

https://codereview.chromium.org/294793003/diff/70001/pdf/pdf.h#newcode18
pdf/pdf.h:18: virtual bool Init();
Are we going to work on making this code run under chromium-style clang plugin?

We exclude this directory in tools/clang/plugins/ChromeClassTester.cpp:161.

It would have warned to add OVERRIDE here.

[Lei Zhang, 2014-05-20 20:40:11]

On 2014/05/20 20:33:07, tfarina wrote:
> https://codereview.chromium.org/294793003/diff/70001/pdf/pdf.h
> File pdf/pdf.h (right):
> 
> https://codereview.chromium.org/294793003/diff/70001/pdf/pdf.h#newcode18
> pdf/pdf.h:18: virtual bool Init();
> Are we going to work on making this code run under chromium-style clang
plugin?
> 
> We exclude this directory in tools/clang/plugins/ChromeClassTester.cpp:161.
> 
> It would have warned to add OVERRIDE here.

https://code.google.com/p/pdfium/ currently says: "code has to conform to the
existing style and not Chromium/Google style"

[tfarina, 2014-05-20 20:43:58]

On Tue, May 20, 2014 at 5:40 PM, <thestig@chromium.org> wrote:

> On 2014/05/20 20:33:07, tfarina wrote:
>
>> https://codereview.chromium.org/294793003/diff/70001/pdf/pdf.h
>> File pdf/pdf.h (right):
>>
>
>  https://codereview.chromium.org/294793003/diff/70001/pdf/pdf.h#newcode18
>> pdf/pdf.h:18: virtual bool Init();
>> Are we going to work on making this code run under chromium-style clang
>>
> plugin?
>
>  We exclude this directory in tools/clang/plugins/
>> ChromeClassTester.cpp:161.
>>
>
>  It would have warned to add OVERRIDE here.
>>
>
> https://code.google.com/p/pdfium/ currently says: "code has to conform to
> the
> existing style and not Chromium/Google style"
>
> I think pdfium and src/pdf are "different" things. One is a third_party
library, other is checked in the Chromium tree (and thus I think
Chromium/Google style should be applied).

I'm commenting on a file that is Chromium:
https://chromium.googlesource.com/chromium/chromium/+/trunk/pdf/pdf.h

-- 
Thiago Farina

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

[Lei Zhang, 2014-05-20 20:48:47]

On 2014/05/20 20:43:58, tfarina wrote:
> I'm commenting on a file that is Chromium:
> https://chromium.googlesource.com/chromium/chromium/+/trunk/pdf/pdf.h

Oh, sorry, wasn't paying full attention. I defer to jam@ then.