Test CL.

Test CL.

[Lei Zhang, 2016-05-26 22:06:04]

Patchset #1 (id:1) has been deleted

[Tom Sepez, 2016-05-26 22:34:20]

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

[Tom Sepez, 2016-05-26 22:34:21]

https://codereview.chromium.org/2017773002/diff/20001/xfa/fwl/core/ifwl_widget.h
File xfa/fwl/core/ifwl_widget.h (right):

https://codereview.chromium.org/2017773002/diff/20001/xfa/fwl/core/ifwl_widge...
xfa/fwl/core/ifwl_widget.h:51: class CXFA_FFWidget;
This is the layering violation.  FWL shouldn't know that XFA exists on top of
it.  FWL is supposed to be an application agnostic layer.

We could try the same trick where IFWL_Widget has some dummy type, but having
done that yesterday, it still feels hokey.

[Lei Zhang, 2016-05-26 22:43:11]

On 2016/05/26 22:34:21, Tom Sepez wrote:
>
https://codereview.chromium.org/2017773002/diff/20001/xfa/fwl/core/ifwl_widget.h
> File xfa/fwl/core/ifwl_widget.h (right):
> 
>
https://codereview.chromium.org/2017773002/diff/20001/xfa/fwl/core/ifwl_widge...
> xfa/fwl/core/ifwl_widget.h:51: class CXFA_FFWidget;
> This is the layering violation.  FWL shouldn't know that XFA exists on top of
> it.  FWL is supposed to be an application agnostic layer.
> 
> We could try the same trick where IFWL_Widget has some dummy type, but having
> done that yesterday, it still feels hokey.

There are other places in xfa/fwl that #includes files from xfa/fxfa. Should we
file a bug and fix those then?

[Tom Sepez, 2016-05-26 23:38:23]

On 2016/05/26 22:43:11, Lei Zhang wrote:
> On 2016/05/26 22:34:21, Tom Sepez wrote:
> >
>
https://codereview.chromium.org/2017773002/diff/20001/xfa/fwl/core/ifwl_widget.h
> > File xfa/fwl/core/ifwl_widget.h (right):
> > 
> >
>
https://codereview.chromium.org/2017773002/diff/20001/xfa/fwl/core/ifwl_widge...
> > xfa/fwl/core/ifwl_widget.h:51: class CXFA_FFWidget;
> > This is the layering violation.  FWL shouldn't know that XFA exists on top
of
> > it.  FWL is supposed to be an application agnostic layer.
> > 
> > We could try the same trick where IFWL_Widget has some dummy type, but
having
> > done that yesterday, it still feels hokey.
> 
> There are other places in xfa/fwl that #includes files from xfa/fxfa. Should
we
> file a bug and fix those then?
SGTM.

[dsinclair, 2016-05-27 02:57:47]

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

[dsinclair, 2016-05-27 02:57:47]

https://codereview.chromium.org/2017773002/diff/20001/xfa/fwl/core/ifwl_widget.h
File xfa/fwl/core/ifwl_widget.h (right):

https://codereview.chromium.org/2017773002/diff/20001/xfa/fwl/core/ifwl_widge...
xfa/fwl/core/ifwl_widget.h:51: class CXFA_FFWidget;
On 2016/05/26 22:34:21, Tom Sepez wrote:
> This is the layering violation.  FWL shouldn't know that XFA exists on top of
> it.  FWL is supposed to be an application agnostic layer.
> 
> We could try the same trick where IFWL_Widget has some dummy type, but having
> done that yesterday, it still feels hokey.


If that's the case, should we move fwl out of the xfa directory to make the
layering clearer? I was assuming that things inside a top level directory (xfa/,
core/, fpdfsdk/) can see anything in their subtrees.

[Tom Sepez, 2016-05-27 17:17:49]

> I was assuming that things inside a top level directory (xfa/,
> core/, fpdfsdk/) can see anything in their subtrees.

That's a really good model to follow, agreed.

>If that's the case, should we move fwl out of the xfa directory to make the
> layering clearer?

That would be the conclusion, yes.  Dan would dig this :).

[Tom Sepez, 2016-05-27 18:09:15]

On 2016/05/27 17:17:49, Tom Sepez wrote:
> > I was assuming that things inside a top level directory (xfa/,
> > core/, fpdfsdk/) can see anything in their subtrees.
> 
> That's a really good model to follow, agreed.
> 
> >If that's the case, should we move fwl out of the xfa directory to make the
> > layering clearer?
> 
> That would be the conclusion, yes.  Dan would dig this :).

These are the includes that I found in fwl that would complicate that

"xfa/fxbarcode/cbc_codabar.h"
"xfa/fxbarcode/cbc_code128.h"
"xfa/fxbarcode/cbc_code39.h"
"xfa/fxbarcode/cbc_codebase.h"
"xfa/fxbarcode/cbc_datamatrix.h"
"xfa/fxbarcode/cbc_ean13.h"
"xfa/fxbarcode/cbc_ean8.h"
"xfa/fxbarcode/cbc_pdf417i.h"
"xfa/fxbarcode/cbc_qrcode.h"
"xfa/fxbarcode/cbc_upca.h"
"xfa/fxbarcode/include/BC_Library.h"
"xfa/fxbarcode/utils.h"
"xfa/fxfa/app/xfa_fwladapter.h"
"xfa/fxfa/include/xfa_ffapp.h"
"xfa/fxfa/include/xfa_ffdoc.h"
"xfa/fxfa/include/xfa_ffwidget.h"
"xfa/fxgraphics/cfx_color.h"
"xfa/fxgraphics/cfx_path.h"
"xfa/fxgraphics/cfx_shading.h"
"xfa/fxgraphics/include/cfx_graphics.h"

So maybe we should just treat XFA as a "top-level directory" and push the issue
down one dir without moving anything.


Only the fxfa includes abouve bother me.

[dsinclair, 2016-05-30 14:30:09]

On 2016/05/27 18:09:15, Tom Sepez wrote:
> On 2016/05/27 17:17:49, Tom Sepez wrote:
> > > I was assuming that things inside a top level directory (xfa/,
> > > core/, fpdfsdk/) can see anything in their subtrees.
> > 
> > That's a really good model to follow, agreed.
> > 
> > >If that's the case, should we move fwl out of the xfa directory to make the
> > > layering clearer?
> > 
> > That would be the conclusion, yes.  Dan would dig this :).
> 
> These are the includes that I found in fwl that would complicate that
> 
> "xfa/fxbarcode/cbc_codabar.h"
> "xfa/fxbarcode/cbc_code128.h"
> "xfa/fxbarcode/cbc_code39.h"
> "xfa/fxbarcode/cbc_codebase.h"
> "xfa/fxbarcode/cbc_datamatrix.h"
> "xfa/fxbarcode/cbc_ean13.h"
> "xfa/fxbarcode/cbc_ean8.h"
> "xfa/fxbarcode/cbc_pdf417i.h"
> "xfa/fxbarcode/cbc_qrcode.h"
> "xfa/fxbarcode/cbc_upca.h"
> "xfa/fxbarcode/include/BC_Library.h"
> "xfa/fxbarcode/utils.h"
> "xfa/fxfa/app/xfa_fwladapter.h"
> "xfa/fxfa/include/xfa_ffapp.h"
> "xfa/fxfa/include/xfa_ffdoc.h"
> "xfa/fxfa/include/xfa_ffwidget.h"
> "xfa/fxgraphics/cfx_color.h"
> "xfa/fxgraphics/cfx_path.h"
> "xfa/fxgraphics/cfx_shading.h"
> "xfa/fxgraphics/include/cfx_graphics.h"
> 
> So maybe we should just treat XFA as a "top-level directory" and push the
issue
> down one dir without moving anything.
> 
> 
> Only the fxfa includes abouve bother me.


What about if we move fwl/ up to top level, white list these violations in DEPS,
which would stop us from adding more. Then remove them.

We can then work towards moving pdfwindow and the other widget stuff to work
based on this top level fwl and hopefully get rid of the other widget code.