FWL refcounts never incremented (part 2).

FWL refcounts never incremented (part 2).

Re-introduce create methods that actually create the
corresponding impl so we don't have ifaces without impls.
Remove per-subclass Initialize() methods.
Remove unused ctors.

BUG=pdfium:282
R=jun_fang@foxitsoftware.com, thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/8ee79f8f2824d5b7058d49ebb10ced6b001c178e

[Tom Sepez, 2015-11-16 19:49:03]

Description was changed from

==========
FWL refcounts never incremented.

This CL probabaly isn't for landing, but the fact that it builds
proves that the ref counts in these FWL_ classes are never incremnted,
and hence are pointless.  Please confirm my work.
==========

to

==========
FWL refcounts never incremented.

This CL probabaly isn't for landing, but the fact that it builds
proves that the ref counts in these FWL_ classes are never incremnted,
and hence are pointless.  Please confirm my work.

BUG=pdfium:282
==========

[Tom Sepez, 2015-11-16 19:49:03]

tsepez@chromium.org changed reviewers:
+ jun_fang@foxitsoftware.com, thestig@chromium.org

[Tom Sepez, 2015-11-16 19:50:26]

Please sanity check my reasoning.

[Tom Sepez, 2015-11-16 20:07:11]

https://codereview.chromium.org/1453473002/diff/1/xfa/include/fwl/core/fwl_no...
File xfa/include/fwl/core/fwl_note.h (right):

https://codereview.chromium.org/1453473002/diff/1/xfa/include/fwl/core/fwl_no...
xfa/include/fwl/core/fwl_note.h:133: virtual IFWL_Target* Retain() {
Doesn't override anything, provides its own m_dwRefcount.  I don't think these
objects belong in the same FWL hierarchy as the others, do they?

[Lei Zhang, 2015-11-16 20:34:49]

Analysis looks correct to me. None of the Retain() methods being removed are
ever called.

https://codereview.chromium.org/1453473002/diff/1/xfa/include/fwl/core/fwl_no...
File xfa/include/fwl/core/fwl_note.h (right):

https://codereview.chromium.org/1453473002/diff/1/xfa/include/fwl/core/fwl_no...
xfa/include/fwl/core/fwl_note.h:133: virtual IFWL_Target* Retain() {
On 2015/11/16 20:07:11, Tom Sepez wrote:
> Doesn't override anything, provides its own m_dwRefcount.  I don't think these
> objects belong in the same FWL hierarchy as the others, do they?

This is the root for a different class hierachy. I suspect this is a copy+paste
error from one of the other xfa/ Retain() methods.

[Tom Sepez, 2015-11-17 17:36:49]

On 2015/11/16 20:34:49, Lei Zhang wrote:
> Analysis looks correct to me. None of the Retain() methods being removed are
> ever called.
> 
>
https://codereview.chromium.org/1453473002/diff/1/xfa/include/fwl/core/fwl_no...
> File xfa/include/fwl/core/fwl_note.h (right):
> 
>
https://codereview.chromium.org/1453473002/diff/1/xfa/include/fwl/core/fwl_no...
> xfa/include/fwl/core/fwl_note.h:133: virtual IFWL_Target* Retain() {
> On 2015/11/16 20:07:11, Tom Sepez wrote:
> > Doesn't override anything, provides its own m_dwRefcount.  I don't think
these
> > objects belong in the same FWL hierarchy as the others, do they?
> 
> This is the root for a different class hierachy. I suspect this is a
copy+paste
> error from one of the other xfa/ Retain() methods.


Ok, then I think I will tidy this up into a form for landing, unless Jun comes
back with any objections from the FX side about what was supposed to be
happening here.

[jun_fang, 2015-11-19 16:30:02]

On 2015/11/17 17:36:49, Tom Sepez wrote:
> On 2015/11/16 20:34:49, Lei Zhang wrote:
> > Analysis looks correct to me. None of the Retain() methods being removed are
> > ever called.
> > 
> >
>
https://codereview.chromium.org/1453473002/diff/1/xfa/include/fwl/core/fwl_no...
> > File xfa/include/fwl/core/fwl_note.h (right):
> > 
> >
>
https://codereview.chromium.org/1453473002/diff/1/xfa/include/fwl/core/fwl_no...
> > xfa/include/fwl/core/fwl_note.h:133: virtual IFWL_Target* Retain() {
> > On 2015/11/16 20:07:11, Tom Sepez wrote:
> > > Doesn't override anything, provides its own m_dwRefcount.  I don't think
> these
> > > objects belong in the same FWL hierarchy as the others, do they?
> > 
> > This is the root for a different class hierachy. I suspect this is a
> copy+paste
> > error from one of the other xfa/ Retain() methods.
> 
> 
> Ok, then I think I will tidy this up into a form for landing, unless Jun comes
> back with any objections from the FX side about what was supposed to be
> happening here.

I don't have more comments for this CL. Let me double check with our developers.

[jun_fang, 2015-11-20 09:28:15]

jun_fang@foxitsoftware.com changed reviewers:
+ bin_wang@foxitsoftware.com

[jun_fang, 2015-11-20 09:28:15]

+Bin

[Tom Sepez, 2015-11-27 21:31:48]

On 2015/11/20 09:28:15, jun_fang wrote:
> +Bin

Any news?  Things become much simpler if these can be removed.  Thanks.

[jun_fang, 2015-11-30 04:27:21]

On 2015/11/27 21:31:48, Tom Sepez wrote:
> On 2015/11/20 09:28:15, jun_fang wrote:
> > +Bin
> 
> Any news?  Things become much simpler if these can be removed.  Thanks.

Discussed this issue with our developers, unused retain() can be removed. 
XFA and FWL are two standalone components. FWL is a cross-platform widget
library developed by Foxit. In XFA implementation, developers should remove
some dead code from FWL code. But it seems that some unused code is still there.

[Tom Sepez, 2015-11-30 21:32:16]

Description was changed from

==========
FWL refcounts never incremented.

This CL probabaly isn't for landing, but the fact that it builds
proves that the ref counts in these FWL_ classes are never incremnted,
and hence are pointless.  Please confirm my work.

BUG=pdfium:282
==========

to

==========
FWL refcounts never incremented.

Remove dead code.

BUG=pdfium:282
==========

[Tom Sepez, 2015-11-30 22:20:33]

Description was changed from

==========
FWL refcounts never incremented.

Remove dead code.

BUG=pdfium:282
==========

to

==========
FWL refcounts never incremented.

Remove dead code.
Convert to unique_ptrs.

BUG=pdfium:282
==========

[Tom Sepez, 2015-11-30 22:50:23]

Description was changed from

==========
FWL refcounts never incremented.

Remove dead code.
Convert to unique_ptrs.

BUG=pdfium:282
==========

to

==========
FWL refcounts never incremented.

Remove dead code.
Convert to unique_ptrs.
Remove static Create() methods, since special handling is no
longer required at create time.

BUG=pdfium:282
==========

[Tom Sepez, 2015-11-30 23:45:58]

PS4 for review.

[bin_wang, 2015-12-01 11:34:54]

https://codereview.chromium.org/1453473002/diff/120001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/include/fwl_comboboximp.h (right):

https://codereview.chromium.org/1453473002/diff/120001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/include/fwl_comboboximp.h:172: IFWL_Form* m_pForm;
Can m_pForm use unique_ptr?

https://codereview.chromium.org/1453473002/diff/120001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/include/fwl_editimp.h (right):

https://codereview.chromium.org/1453473002/diff/120001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/include/fwl_editimp.h:172: IFWL_AdapterTextField*
m_pTextField;
Why doesn't m_pTextField use unique_ptr?

[Tom Sepez, 2015-12-01 17:18:34]

https://codereview.chromium.org/1453473002/diff/120001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/include/fwl_comboboximp.h (right):

https://codereview.chromium.org/1453473002/diff/120001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/include/fwl_comboboximp.h:172: IFWL_Form* m_pForm;
On 2015/12/01 11:34:53, bin_wang wrote:
> Can m_pForm use unique_ptr?
As far as I can tell m_pForm never gets deleted, which is probably a bug in
itself, but I didn't want to introduce an implicit deletion of it in this
class's dtor where there hadn't been one before.

https://codereview.chromium.org/1453473002/diff/120001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/include/fwl_editimp.h (right):

https://codereview.chromium.org/1453473002/diff/120001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/include/fwl_editimp.h:172: IFWL_AdapterTextField*
m_pTextField;
On 2015/12/01 11:34:54, bin_wang wrote:
> Why doesn't m_pTextField use unique_ptr?
Again, because it isn't currently deleted as part of the existing dtor. 
Grepping around, it looks unused.  I'l pull it.

[Tom Sepez, 2015-12-02 21:54:09]

Description was changed from

==========
FWL refcounts never incremented.

Remove dead code.
Convert to unique_ptrs.
Remove static Create() methods, since special handling is no
longer required at create time.

BUG=pdfium:282
==========

to

==========
FWL refcounts never incremented.

Remove dead code, and convert to unique_ptrs. Make create methods actually
create the corresponding impl and remove
per-subclass Initialize() methods.

BUG=pdfium:282
==========

[Tom Sepez, 2015-12-02 23:31:48]

Description was changed from

==========
FWL refcounts never incremented.

Remove dead code, and convert to unique_ptrs. Make create methods actually
create the corresponding impl and remove
per-subclass Initialize() methods.

BUG=pdfium:282
==========

to

==========
FWL refcounts never incremented.

Remove dead code, and convert to unique_ptrs. Make create
methods actually create the corresponding impl and remove
per-subclass Initialize() methods. Remove unused ctors.

BUG=pdfium:282
==========

[Tom Sepez, 2015-12-03 00:35:11]

Please take a look at the latest patch set, this organizes things
in a more natural way.

[Tom Sepez, 2015-12-04 18:53:20]

Ping?  Its a lot, but it fits together nicely.  Thanks.

[Tom Sepez, 2015-12-04 19:01:27]

Hints:

https://codereview.chromium.org/1453473002/diff/260001/xfa/include/fwl/core/f...
File xfa/include/fwl/core/fwl_target.h (right):

https://codereview.chromium.org/1453473002/diff/260001/xfa/include/fwl/core/f...
xfa/include/fwl/core/fwl_target.h:39: FWL_ERR Initialize();
Hint: this is what allows me to remove many Initialize methods.

https://codereview.chromium.org/1453473002/diff/260001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/fwl_barcodeimp.cpp (left):

https://codereview.chromium.org/1453473002/diff/260001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/fwl_barcodeimp.cpp:20: return
pBarcodeImpl->Initialize();
Hint: this ->Initialize() is what the superclass Initialize() now does for us,
so all we need do is move the other logic into create method.

https://codereview.chromium.org/1453473002/diff/260001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/fwl_editimp.cpp (right):

https://codereview.chromium.org/1453473002/diff/260001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/fwl_editimp.cpp:33: CFWL_EditImp* pComboEditImpl =
new CFWL_ComboEditImp(properties, pOuter);
Hint:  Same IFWL_ may have alternate IMPLs under it.

[Tom Sepez, 2015-12-08 17:25:51]

Judging by the silence on this, maybe you want me to split it into pieces:  so
part one is now at 
https://codereview.chromium.org/1509203002/

[Tom Sepez, 2015-12-08 19:24:03]

Description was changed from

==========
FWL refcounts never incremented.

Remove dead code, and convert to unique_ptrs. Make create
methods actually create the corresponding impl and remove
per-subclass Initialize() methods. Remove unused ctors.

BUG=pdfium:282
==========

to

==========
FWL refcounts never incremented (part 2).

Make create methods actually create the corresponding impl 
and remove per-subclass Initialize() methods. Remove unused
ctors.

BUG=pdfium:282
==========

[Tom Sepez, 2015-12-08 19:25:27]

Description was changed from

==========
FWL refcounts never incremented (part 2).

Make create methods actually create the corresponding impl 
and remove per-subclass Initialize() methods. Remove unused
ctors.

BUG=pdfium:282
==========

to

==========
FWL refcounts never incremented (part 2).

Re-introduce create methods that actually create the
corresponding impl so we don't have ifaces without impls.
Remove per-subclass Initialize() methods.
Remove unused ctors.

BUG=pdfium:282
==========

[Tom Sepez, 2015-12-08 19:29:37]

Ok, so this is now slightly easier to review.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
File xfa/include/fwl/core/fwl_form.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
xfa/include/fwl/core/fwl_form.h:72: static IFWL_Form*
CreateFormProxy(CFWL_WidgetImpProperties& properties,
Note: some ifaces have multiple possible impls.  Make a create method for each
impl.

[Lei Zhang, 2015-12-09 03:43:42]

I only spot checked some of the widgets.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
File xfa/include/fwl/basewidget/fwl_barcode.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_barcode.h:54: IFWL_Widget* pOuter);
|pOuter| is always NULL?

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
File xfa/include/fwl/basewidget/fwl_checkbox.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_checkbox.h:75: IFWL_Widget* pOuter);
Always NULL

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_checkbox.h:85: static IFWL_RadioButton*
Create(const CFWL_WidgetImpProperties& properties,
Never called?

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
File xfa/include/fwl/basewidget/fwl_combobox.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_combobox.h:101: IFWL_Widget* pOuter);
Always NULL.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
File xfa/include/fwl/core/fwl_form.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
xfa/include/fwl/core/fwl_form.h:69: static IFWL_Form*
Create(CFWL_WidgetImpProperties& properties,
I don't see this getting called?

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
xfa/include/fwl/core/fwl_form.h:72: static IFWL_Form*
CreateFormProxy(CFWL_WidgetImpProperties& properties,
On 2015/12/08 19:29:37, Tom Sepez wrote:
> Note: some ifaces have multiple possible impls.  Make a create method for each
> impl.

Acknowledged.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
File xfa/include/fwl/core/fwl_grid.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
xfa/include/fwl/core/fwl_grid.h:41: static IFWL_Grid*
Create(CFWL_WidgetImpProperties& properties);
non-const ref

[jun_fang, 2015-12-09 10:38:27]

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/fwl_checkboximp.cpp (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/fwl_checkboximp.cpp:38:
pRadioButton->SetImpl(pCheckBoxImpl);
Nit: when I saw that pRadioButton used pCheckBoxImpl, I suspected that there
were something wrong. Should we add a comment here to clarify that radio button
interfaces share implementation with check boxes?

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/lightw...
File xfa/src/fwl/src/lightwidget/checkbox.cpp (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/lightw...
xfa/src/fwl/src/lightwidget/checkbox.cpp:18:
m_pProperties->MakeWidgetImpProperties(&m_checkboxData), nullptr));
On the left side, 
prop.m_pParent = m_pProperties->m_pParent->GetWidget();
prop.m_pOwner = m_pProperties->m_pOwner->GetWidget();

On the right side (after this CL), it's like:
prop.m_pParent = m_pProperties->m_pParent;
prop.m_pOwner = m_pProperties->m_pOwner;

It changes the behavior.

[Tom Sepez, 2015-12-09 20:32:06]

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
File xfa/include/fwl/basewidget/fwl_barcode.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_barcode.h:54: IFWL_Widget* pOuter);
On 2015/12/09 03:43:42, Lei Zhang wrote:
> |pOuter| is always NULL?

Sure looks that way, even going back before this round of refactoring started. 
Removed.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
File xfa/include/fwl/basewidget/fwl_checkbox.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_checkbox.h:85: static IFWL_RadioButton*
Create(const CFWL_WidgetImpProperties& properties,
On 2015/12/09 03:43:42, Lei Zhang wrote:
> Never called?
And hence the class is unused.  removed.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
File xfa/include/fwl/basewidget/fwl_combobox.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_combobox.h:101: IFWL_Widget* pOuter);
On 2015/12/09 03:43:42, Lei Zhang wrote:
> Always NULL.

Done.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
File xfa/include/fwl/core/fwl_form.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
xfa/include/fwl/core/fwl_form.h:69: static IFWL_Form*
Create(CFWL_WidgetImpProperties& properties,
On 2015/12/09 03:43:42, Lei Zhang wrote:
> I don't see this getting called?
Yeah, we only instantiate subclasses.  ....and its gone.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
File xfa/include/fwl/core/fwl_grid.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
xfa/include/fwl/core/fwl_grid.h:41: static IFWL_Grid*
Create(CFWL_WidgetImpProperties& properties);
On 2015/12/09 03:43:42, Lei Zhang wrote:
> non-const ref

Done.

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/fwl_checkboximp.cpp (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/fwl_checkboximp.cpp:38:
pRadioButton->SetImpl(pCheckBoxImpl);
On 2015/12/09 10:38:27, jun_fang wrote:
> Nit: when I saw that pRadioButton used pCheckBoxImpl, I suspected that there
> were something wrong. Should we add a comment here to clarify that radio
button
> interfaces share implementation with check boxes?
This code has been completely removed.

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/lightw...
File xfa/src/fwl/src/lightwidget/checkbox.cpp (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/lightw...
xfa/src/fwl/src/lightwidget/checkbox.cpp:18:
m_pProperties->MakeWidgetImpProperties(&m_checkboxData), nullptr));
On 2015/12/09 10:38:27, jun_fang wrote:
> On the left side, 
> prop.m_pParent = m_pProperties->m_pParent->GetWidget();
> prop.m_pOwner = m_pProperties->m_pOwner->GetWidget();
> 
> On the right side (after this CL), it's like:
> prop.m_pParent = m_pProperties->m_pParent;
> prop.m_pOwner = m_pProperties->m_pOwner;
> 
> It changes he behavior.
Good spotting.

[Tom Sepez, 2015-12-09 20:40:05]

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
File xfa/include/fwl/basewidget/fwl_checkbox.h (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_checkbox.h:14: #define FWL_CLASS_RadioButton
L"FWL_RADIOBUTTON"
Note: the places these are checked always do radio/checkbox the same.

https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
xfa/include/fwl/basewidget/fwl_checkbox.h:26: #define
FWL_STYLEEXT_CKB_RadioButton (1L << 7)
Note: A single class with this style attribute.

[Lei Zhang, 2015-12-09 23:15:57]

lgtm

I guess we can make another pass later and see if there's more params that are
always NULL.

https://codereview.chromium.org/1453473002/diff/320001/xfa/include/fwl/core/f...
File xfa/include/fwl/core/fwl_form.h (right):

https://codereview.chromium.org/1453473002/diff/320001/xfa/include/fwl/core/f...
xfa/include/fwl/core/fwl_form.h:69: static IFWL_Form*
CreateFormProxy(CFWL_WidgetImpProperties& properties,
Since the original Create() is gone, can this become Create()?

[jun_fang, 2015-12-10 00:06:04]

On 2015/12/09 20:32:06, Tom Sepez wrote:
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
> File xfa/include/fwl/basewidget/fwl_barcode.h (right):
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
> xfa/include/fwl/basewidget/fwl_barcode.h:54: IFWL_Widget* pOuter);
> On 2015/12/09 03:43:42, Lei Zhang wrote:
> > |pOuter| is always NULL?
> 
> Sure looks that way, even going back before this round of refactoring started.

> Removed.
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
> File xfa/include/fwl/basewidget/fwl_checkbox.h (right):
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
> xfa/include/fwl/basewidget/fwl_checkbox.h:85: static IFWL_RadioButton*
> Create(const CFWL_WidgetImpProperties& properties,
> On 2015/12/09 03:43:42, Lei Zhang wrote:
> > Never called?
> And hence the class is unused.  removed.
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
> File xfa/include/fwl/basewidget/fwl_combobox.h (right):
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/basewi...
> xfa/include/fwl/basewidget/fwl_combobox.h:101: IFWL_Widget* pOuter);
> On 2015/12/09 03:43:42, Lei Zhang wrote:
> > Always NULL.
> 
> Done.
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
> File xfa/include/fwl/core/fwl_form.h (right):
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
> xfa/include/fwl/core/fwl_form.h:69: static IFWL_Form*
> Create(CFWL_WidgetImpProperties& properties,
> On 2015/12/09 03:43:42, Lei Zhang wrote:
> > I don't see this getting called?
> Yeah, we only instantiate subclasses.  ....and its gone.
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
> File xfa/include/fwl/core/fwl_grid.h (right):
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/include/fwl/core/f...
> xfa/include/fwl/core/fwl_grid.h:41: static IFWL_Grid*
> Create(CFWL_WidgetImpProperties& properties);
> On 2015/12/09 03:43:42, Lei Zhang wrote:
> > non-const ref
> 
> Done.
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/basewi...
> File xfa/src/fwl/src/basewidget/fwl_checkboximp.cpp (right):
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/basewi...
> xfa/src/fwl/src/basewidget/fwl_checkboximp.cpp:38:
> pRadioButton->SetImpl(pCheckBoxImpl);
> On 2015/12/09 10:38:27, jun_fang wrote:
> > Nit: when I saw that pRadioButton used pCheckBoxImpl, I suspected that there
> > were something wrong. Should we add a comment here to clarify that radio
> button
> > interfaces share implementation with check boxes?
> This code has been completely removed.
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/lightw...
> File xfa/src/fwl/src/lightwidget/checkbox.cpp (right):
> 
>
https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/lightw...
> xfa/src/fwl/src/lightwidget/checkbox.cpp:18:
> m_pProperties->MakeWidgetImpProperties(&m_checkboxData), nullptr));
> On 2015/12/09 10:38:27, jun_fang wrote:
> > On the left side, 
> > prop.m_pParent = m_pProperties->m_pParent->GetWidget();
> > prop.m_pOwner = m_pProperties->m_pOwner->GetWidget();
> > 
> > On the right side (after this CL), it's like:
> > prop.m_pParent = m_pProperties->m_pParent;
> > prop.m_pOwner = m_pProperties->m_pOwner;
> > 
> > It changes he behavior.
> Good spotting.

lgtm

[jun_fang, 2015-12-10 00:06:18]

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/basewi...
File xfa/src/fwl/src/basewidget/fwl_checkboximp.cpp (right):

https://codereview.chromium.org/1453473002/diff/300001/xfa/src/fwl/src/basewi...
xfa/src/fwl/src/basewidget/fwl_checkboximp.cpp:38:
pRadioButton->SetImpl(pCheckBoxImpl);
On 2015/12/09 20:32:06, Tom Sepez wrote:
> On 2015/12/09 10:38:27, jun_fang wrote:
> > Nit: when I saw that pRadioButton used pCheckBoxImpl, I suspected that there
> > were something wrong. Should we add a comment here to clarify that radio
> button
> > interfaces share implementation with check boxes?
> This code has been completely removed.

Acknowledged.

[Tom Sepez, 2015-12-10 00:16:31]

Description was changed from

==========
FWL refcounts never incremented (part 2).

Re-introduce create methods that actually create the
corresponding impl so we don't have ifaces without impls.
Remove per-subclass Initialize() methods.
Remove unused ctors.

BUG=pdfium:282
==========

to

==========
FWL refcounts never incremented (part 2).

Re-introduce create methods that actually create the
corresponding impl so we don't have ifaces without impls.
Remove per-subclass Initialize() methods.
Remove unused ctors.

BUG=pdfium:282
R=jun_fang@foxitsoftware.com, thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/8ee79f8f2824d5b7058d49ebb10ced6b001c...
==========

[Tom Sepez, 2015-12-10 00:16:33]

Committed patchset #18 (id:340001) manually as
8ee79f8f2824d5b7058d49ebb10ced6b001c178e (presubmit successful).