Cleaning methods and visibility in cfwl_* files.

Cleaning methods and visibility in cfwl_* files.

First pass at cleaning up visibility, return values and const'ness of
cfwl_* class.

Committed: https://pdfium.googlesource.com/pdfium/+/05b9a317d66c29f740e4b0e89ea87143a11d61fc

[dsinclair, 2016-11-14 22:08:41]

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

[dsinclair, 2016-11-14 22:08:41]

PTAL.

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
File xfa/fwl/core/cfwl_widgetmgr.cpp (left):

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
xfa/fwl/core/cfwl_widgetmgr.cpp:417: IFWL_Widget*
CFWL_WidgetMgr::GetSiblingRadioButton(IFWL_Widget* pWidget,
Always nullptr, which caused a bunch of the cleanup below. Please give a good
look to make sure I didn't mess up any of the logic during removal.

[dsinclair, 2016-11-14 22:08:47]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 22:08:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 22:43:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 22:43:51]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-11-14 23:03:04]

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr.h
File xfa/fwl/core/cfwl_widgetmgr.h (right):

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
xfa/fwl/core/cfwl_widgetmgr.h:47: class IFWL_WidgetMgrDelegate {
I'd expect this to be in whatever file defines IFWL_WidgetManager

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
xfa/fwl/core/cfwl_widgetmgr.h:144: std::unique_ptr<IFWL_WidgetMgrDelegate>
m_pDelegate;
Do we still need this now that we are one?

[dsinclair, 2016-11-14 23:10:27]

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr.h
File xfa/fwl/core/cfwl_widgetmgr.h (right):

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
xfa/fwl/core/cfwl_widgetmgr.h:47: class IFWL_WidgetMgrDelegate {
On 2016/11/14 23:03:04, Tom Sepez wrote:
> I'd expect this to be in whatever file defines IFWL_WidgetManager

There is no IFWL_WidgetMgr. This file defined CFWL_WidgetMgrDelegate but I
changed it to be a pure interface and merged the delegate into CFWL_WidgetMgr.
This class will eventually get moved to its own file once I get to that stage of
the cleanup.

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
xfa/fwl/core/cfwl_widgetmgr.h:144: std::unique_ptr<IFWL_WidgetMgrDelegate>
m_pDelegate;
On 2016/11/14 23:03:04, Tom Sepez wrote:
> Do we still need this now that we are one?

Done.

[npm, 2016-11-15 18:20:19]

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr.h
File xfa/fwl/core/cfwl_widgetmgr.h (right):

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
xfa/fwl/core/cfwl_widgetmgr.h:47: class IFWL_WidgetMgrDelegate {
On 2016/11/14 23:10:26, dsinclair wrote:
> On 2016/11/14 23:03:04, Tom Sepez wrote:
> > I'd expect this to be in whatever file defines IFWL_WidgetManager
> 
> There is no IFWL_WidgetMgr. This file defined CFWL_WidgetMgrDelegate but I
> changed it to be a pure interface and merged the delegate into CFWL_WidgetMgr.
> This class will eventually get moved to its own file once I get to that stage
of
> the cleanup.

So why not name this one IFWL_WidgetMgr?

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
File xfa/fwl/core/cfwl_widgetmgr.h (right):

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
xfa/fwl/core/cfwl_widgetmgr.h:26: class IFWL_WidgetMgrDelegate;
Is this forward declare needed?

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
xfa/fwl/core/cfwl_widgetmgr.h:47: class IFWL_WidgetMgrDelegate {
If this class is only being implemented by CFWL_WidgetMgr, maybe we
could just get rid of it?

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
xfa/fwl/core/cfwl_widgetmgr.h:50: uint32_t dwCapability =
FWL_WGTMGR_DisableThread) = 0;
Do we need to have the parameter default?

[Tom Sepez, 2016-11-15 18:27:22]

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr.h
File xfa/fwl/core/cfwl_widgetmgr.h (right):

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
xfa/fwl/core/cfwl_widgetmgr.h:47: class IFWL_WidgetMgrDelegate {
Also, who calls the delegate methods?  Maybe this is really IFWL_WidgetMgr per
npm's suggestion.

[dsinclair, 2016-11-15 18:40:36]

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr.h
File xfa/fwl/core/cfwl_widgetmgr.h (right):

https://codereview.chromium.org/2501743002/diff/1/xfa/fwl/core/cfwl_widgetmgr...
xfa/fwl/core/cfwl_widgetmgr.h:47: class IFWL_WidgetMgrDelegate {
On 2016/11/15 18:27:21, Tom Sepez wrote:
> Also, who calls the delegate methods?  Maybe this is really IFWL_WidgetMgr per
> npm's suggestion.
> 


The delegate methods are called. We don't have need of an IFWL_WidgetMgr at the
moment, but do need the Delegate. The delegate is passed into the IFWL_Widget
along with being used in a few other places.

I think keeping this separate from an IFWL_WidgetMgr makes the code that uses
the delegate clearer in what it's doing.

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
File xfa/fwl/core/cfwl_widgetmgr.h (right):

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
xfa/fwl/core/cfwl_widgetmgr.h:47: class IFWL_WidgetMgrDelegate {
On 2016/11/15 18:20:19, npm wrote:
> If this class is only being implemented by CFWL_WidgetMgr, maybe we
> could just get rid of it?


The interface is passed into other classes. I could change the other classes to
accept a CFWL_WidgetMgr instead, but I'm preference is to only present the
limited methods to those classes.

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
xfa/fwl/core/cfwl_widgetmgr.h:50: uint32_t dwCapability =
FWL_WGTMGR_DisableThread) = 0;
On 2016/11/15 18:20:19, npm wrote:
> Do we need to have the parameter default?

No, but I'm not cleaning up default params yet, heh.

[dsinclair, 2016-11-15 19:05:16]

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
File xfa/fwl/core/cfwl_widgetmgr.h (right):

https://codereview.chromium.org/2501743002/diff/20001/xfa/fwl/core/cfwl_widge...
xfa/fwl/core/cfwl_widgetmgr.h:26: class IFWL_WidgetMgrDelegate;
On 2016/11/15 18:20:19, npm wrote:
> Is this forward declare needed?

Done.

[npm, 2016-11-15 19:40:33]

lgtm

[dsinclair, 2016-11-15 19:42:01]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-11-15 19:42:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 20:00:43]

Description was changed from

==========
Cleaning methods and visibility in cfwl_* files.

First pass at cleaning up visibility, return values and const'ness of
cfwl_* class.
==========

to

==========
Cleaning methods and visibility in cfwl_* files.

First pass at cleaning up visibility, return values and const'ness of
cfwl_* class.

Committed:
https://pdfium.googlesource.com/pdfium/+/05b9a317d66c29f740e4b0e89ea87143a11d...
==========

[commit-bot: I haz the power, 2016-11-15 20:00:44]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/05b9a317d66c29f740e4b0e89ea87143a11d...