Move xfa/include/fxbarcode/BC_Barcode.h to xfa/fxbarcode.

Move xfa/include/fxbarcode/BC_Barcode.h to xfa/fxbarcode.

This CL splits apart the larger header into individual class headers in the
xfa/fxbarcode directory.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/a98600aeb8d815c297834aa5006f5c3ea20dde6d

[dsinclair, 2016-03-21 15:29:13]

dsinclair@chromium.org changed reviewers:
+ tsepez@chromium.org

[dsinclair, 2016-03-21 15:29:14]

PTAL.

[Tom Sepez, 2016-03-21 16:49:29]

Hard to say which of these you need to fix in this CL.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fwl/basewidget/fxma...
File xfa/fwl/basewidget/fxmath_barcodeimp.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fwl/basewidget/fxma...
xfa/fwl/basewidget/fxmath_barcodeimp.cpp:56: FX_BOOL
CFX_Barcode::Crreate(BC_TYPE type) {
nit: grrrrreat!

https://codereview.chromium.org/1816133002/diff/20001/xfa/fwl/basewidget/fxma...
xfa/fwl/basewidget/fxmath_barcodeimp.cpp:92: ?
static_cast<CBC_OneCode*>(m_pBCEngine)
follow-up: virtualize.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/BC_Librar...
File xfa/fxbarcode/BC_Library.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/BC_Librar...
xfa/fxbarcode/BC_Library.cpp:14: #include
"xfa/fxbarcode/pdf417/BC_PDF417CodewordDecoder.h"
duplicate.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codab...
File xfa/fxbarcode/cbc_codabar.h (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codab...
xfa/fxbarcode/cbc_codabar.h:29: BC_TYPE GetType() { return BC_CODABAR; }
Smells like it should be override?  Any others?

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_code1...
File xfa/fxbarcode/cbc_code128.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_code1...
xfa/fxbarcode/cbc_code128.cpp:36: delete (m_pBCReader);
nit: unique_ptrs.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_code1...
File xfa/fxbarcode/cbc_code128.h (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_code1...
xfa/fxbarcode/cbc_code128.h:29: BC_TYPE GetType() { return BC_CODE128; }
ditto for all of these.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codeb...
File xfa/fxbarcode/cbc_codebase.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codeb...
xfa/fxbarcode/cbc_codebase.cpp:31: if (m_pBCWriter)
nit: return m_pBCWriter && m_pBCWriter->SetCharEncoding(encoding);

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codeb...
File xfa/fxbarcode/cbc_codebase.h (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codeb...
xfa/fxbarcode/cbc_codebase.h:48: CBC_Writer* m_pBCWriter;
Would prefer these to be private, with at SetReader/SetWriter methods.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_datam...
File xfa/fxbarcode/cbc_datamatrix.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_datam...
xfa/fxbarcode/cbc_datamatrix.cpp:37: delete (m_pBCReader);
Again, the top-level class needs to do this via virtual dtors in m_pBCReader and
its subclasses.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/include/B...
File xfa/fxbarcode/include/BC_Library.h (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/include/B...
xfa/fxbarcode/include/BC_Library.h:10: enum BC_TEXT_LOC {
Feels like these enums belong in another file since they aren't used by any of
the functions in this header.

[dsinclair, 2016-03-21 17:52:39]

https://codereview.chromium.org/1816133002/diff/20001/xfa/fwl/basewidget/fxma...
File xfa/fwl/basewidget/fxmath_barcodeimp.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fwl/basewidget/fxma...
xfa/fwl/basewidget/fxmath_barcodeimp.cpp:56: FX_BOOL
CFX_Barcode::Crreate(BC_TYPE type) {
On 2016/03/21 16:49:29, Tom Sepez wrote:
> nit: grrrrreat!

Done.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fwl/basewidget/fxma...
xfa/fwl/basewidget/fxmath_barcodeimp.cpp:92: ?
static_cast<CBC_OneCode*>(m_pBCEngine)
On 2016/03/21 16:49:29, Tom Sepez wrote:
> follow-up: virtualize.

pdfium:440

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/BC_Librar...
File xfa/fxbarcode/BC_Library.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/BC_Librar...
xfa/fxbarcode/BC_Library.cpp:14: #include
"xfa/fxbarcode/pdf417/BC_PDF417CodewordDecoder.h"
On 2016/03/21 16:49:29, Tom Sepez wrote:
> duplicate.

Done.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codab...
File xfa/fxbarcode/cbc_codabar.h (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codab...
xfa/fxbarcode/cbc_codabar.h:29: BC_TYPE GetType() { return BC_CODABAR; }
On 2016/03/21 16:49:29, Tom Sepez wrote:
> Smells like it should be override?  Any others?

Done.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_code1...
File xfa/fxbarcode/cbc_code128.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_code1...
xfa/fxbarcode/cbc_code128.cpp:36: delete (m_pBCReader);
On 2016/03/21 16:49:29, Tom Sepez wrote:
> nit: unique_ptrs.

pdfium:441

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_code1...
File xfa/fxbarcode/cbc_code128.h (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_code1...
xfa/fxbarcode/cbc_code128.h:29: BC_TYPE GetType() { return BC_CODE128; }
On 2016/03/21 16:49:29, Tom Sepez wrote:
> ditto for all of these.

Done.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codeb...
File xfa/fxbarcode/cbc_codebase.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codeb...
xfa/fxbarcode/cbc_codebase.cpp:31: if (m_pBCWriter)
On 2016/03/21 16:49:29, Tom Sepez wrote:
> nit: return m_pBCWriter && m_pBCWriter->SetCharEncoding(encoding);

Done.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codeb...
File xfa/fxbarcode/cbc_codebase.h (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_codeb...
xfa/fxbarcode/cbc_codebase.h:48: CBC_Writer* m_pBCWriter;
On 2016/03/21 16:49:29, Tom Sepez wrote:
> Would prefer these to be private, with at SetReader/SetWriter methods.  

Acknowledged.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_datam...
File xfa/fxbarcode/cbc_datamatrix.cpp (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/cbc_datam...
xfa/fxbarcode/cbc_datamatrix.cpp:37: delete (m_pBCReader);
On 2016/03/21 16:49:29, Tom Sepez wrote:
> Again, the top-level class needs to do this via virtual dtors in m_pBCReader
and
> its subclasses.

Acknowledged.

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/include/B...
File xfa/fxbarcode/include/BC_Library.h (right):

https://codereview.chromium.org/1816133002/diff/20001/xfa/fxbarcode/include/B...
xfa/fxbarcode/include/BC_Library.h:10: enum BC_TEXT_LOC {
On 2016/03/21 16:49:29, Tom Sepez wrote:
> Feels like these enums belong in another file since they aren't used by any of
> the functions in this header.

Yea, these need some further thought. Things like BC_CHAR_ENCODING are
especially weird as they're never referenced inside fxbarcode, but are fxbarcode
that that is read by other parts of the system.

I'm not sure of a better place to put them at the moment.

[Tom Sepez, 2016-03-21 18:19:24]

lgtm

https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
File xfa/fxbarcode/cbc_codebase.cpp (right):

https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
xfa/fxbarcode/cbc_codebase.cpp:22: #include "xfa/fxbarcode/cbc_codebase.h"
nit: Style point - as we move to unix_hacker.cpp file naming style, I've
wondered about whether there should be an "_" at each camel-case transition, eg
cbc_code_base.h.  We should make a rule about this.

[Tom Sepez, 2016-03-21 18:21:14]

On 2016/03/21 18:19:24, Tom Sepez wrote:
> lgtm
> 
>
https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
> File xfa/fxbarcode/cbc_codebase.cpp (right):
> 
>
https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
> xfa/fxbarcode/cbc_codebase.cpp:22: #include "xfa/fxbarcode/cbc_codebase.h"
> nit: Style point - as we move to unix_hacker.cpp file naming style, I've
> wondered about whether there should be an "_" at each camel-case transition,
eg
> cbc_code_base.h.  We should make a rule about this.
That would be consistent with the chrome style, e.g. RenderViewHostImpl lives in
render_view_host_impl.cc

[dsinclair, 2016-03-21 19:12:48]

On 2016/03/21 18:21:14, Tom Sepez wrote:
> On 2016/03/21 18:19:24, Tom Sepez wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
> > File xfa/fxbarcode/cbc_codebase.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
> > xfa/fxbarcode/cbc_codebase.cpp:22: #include "xfa/fxbarcode/cbc_codebase.h"
> > nit: Style point - as we move to unix_hacker.cpp file naming style, I've
> > wondered about whether there should be an "_" at each camel-case transition,
> eg
> > cbc_code_base.h.  We should make a rule about this.
> That would be consistent with the chrome style, e.g. RenderViewHostImpl lives
in
> render_view_host_impl.cc


The google style guide says:  Follow the convention that your project uses. 
Which, in our case is the mixed _ style, currently.  Is it worth changing it?
Currently the filenames will match up with the class name as CDC_CodeBase and
cdc_codebase.h. Following chromium style it would be CdcCodeBase and
cdc_code_base.h.

[dsinclair, 2016-03-21 19:15:58]

Description was changed from

==========
Move xfa/include/fxbarcode/BC_Barcode.h to xfa/fxbarcode.

This CL splits apart the larger header into individual class headers in the
xfa/fxbarcode directory.
==========

to

==========
Move xfa/include/fxbarcode/BC_Barcode.h to xfa/fxbarcode.

This CL splits apart the larger header into individual class headers in the
xfa/fxbarcode directory.

R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/a98600aeb8d815c297834aa5006f5c3ea20d...
==========

[dsinclair, 2016-03-21 19:16:00]

Committed patchset #3 (id:40001) manually as
a98600aeb8d815c297834aa5006f5c3ea20dde6d (presubmit successful).

[Tom Sepez, 2016-03-21 19:24:36]

On 2016/03/21 19:12:48, dsinclair wrote:
> On 2016/03/21 18:21:14, Tom Sepez wrote:
> > On 2016/03/21 18:19:24, Tom Sepez wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
> > > File xfa/fxbarcode/cbc_codebase.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
> > > xfa/fxbarcode/cbc_codebase.cpp:22: #include "xfa/fxbarcode/cbc_codebase.h"
> > > nit: Style point - as we move to unix_hacker.cpp file naming style, I've
> > > wondered about whether there should be an "_" at each camel-case
transition,
> > eg
> > > cbc_code_base.h.  We should make a rule about this.
> > That would be consistent with the chrome style, e.g. RenderViewHostImpl
lives
> in
> > render_view_host_impl.cc
> 
> 
> The google style guide says:  Follow the convention that your project uses. 
> Which, in our case is the mixed _ style, currently.  Is it worth changing it?
> Currently the filenames will match up with the class name as CDC_CodeBase and
> cdc_codebase.h. Following chromium style it would be CdcCodeBase and
> cdc_code_base.h.
We've made a deliberate exception not to try to move to CdcCodeBase yet, so the
name
would remain CDC_CodeBase in either case.

Straining credibility, if there were a CDC_Codebase class as well, then having
one file
called cdc_code_base.h and the other called cdc_codebase.h would be helpful.

[dsinclair, 2016-03-21 19:26:49]

On 2016/03/21 19:24:36, Tom Sepez wrote:
> On 2016/03/21 19:12:48, dsinclair wrote:
> > On 2016/03/21 18:21:14, Tom Sepez wrote:
> > > On 2016/03/21 18:19:24, Tom Sepez wrote:
> > > > lgtm
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
> > > > File xfa/fxbarcode/cbc_codebase.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1816133002/diff/40001/xfa/fxbarcode/cbc_codeb...
> > > > xfa/fxbarcode/cbc_codebase.cpp:22: #include
"xfa/fxbarcode/cbc_codebase.h"
> > > > nit: Style point - as we move to unix_hacker.cpp file naming style, I've
> > > > wondered about whether there should be an "_" at each camel-case
> transition,
> > > eg
> > > > cbc_code_base.h.  We should make a rule about this.
> > > That would be consistent with the chrome style, e.g. RenderViewHostImpl
> lives
> > in
> > > render_view_host_impl.cc
> > 
> > 
> > The google style guide says:  Follow the convention that your project uses. 
> > Which, in our case is the mixed _ style, currently.  Is it worth changing
it?
> > Currently the filenames will match up with the class name as CDC_CodeBase
and
> > cdc_codebase.h. Following chromium style it would be CdcCodeBase and
> > cdc_code_base.h.
> We've made a deliberate exception not to try to move to CdcCodeBase yet, so
the
> name
> would remain CDC_CodeBase in either case.
> 
> Straining credibility, if there were a CDC_Codebase class as well, then having
> one file
> called cdc_code_base.h and the other called cdc_codebase.h would be helpful.


If I found a CDC_CodeBase and CDC_Codebase I'd rename one because I'd be very
sad. I'm fine going either cdc_codebase.h or cdc_code_base.h. The second is
certainly closer to chromium style. I can start going with that form from this
point on and fixup the others later.