Correctly implement GetPartState() function

Correctly implement GetPartState() function

The original function doesn't seem right. Re-implement it.

Committed: https://pdfium.googlesource.com/pdfium/+/5aaa9bc9193d74bb188dd1f890140cf38fb06605

[Wei Li, 2016-05-23 17:05:00]

weili@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[Wei Li, 2016-05-23 17:05:01]

pls review, thanks

[Tom Sepez, 2016-05-23 17:06:45]

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

[Tom Sepez, 2016-05-23 17:06:46]

Deferring to Dan, I think he was mucking about with these #defines.

[dsinclair, 2016-05-24 00:28:53]

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

https://codereview.chromium.org/2001203002/diff/1/xfa/fwl/core/fwl_formimp.h#...
xfa/fwl/core/fwl_formimp.h:50: int32_t GetPartState() {
Can we update the return to be CFWL_PartState while we're at it?

https://codereview.chromium.org/2001203002/diff/1/xfa/fwl/core/fwl_formimp.h#...
xfa/fwl/core/fwl_formimp.h:56: return FWL_SYSBUTTONSTATE_Hover;
CFWL_PartState_Hovered?

[Wei Li, 2016-05-24 00:43:22]

thanks

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

https://codereview.chromium.org/2001203002/diff/1/xfa/fwl/core/fwl_formimp.h#...
xfa/fwl/core/fwl_formimp.h:50: int32_t GetPartState() {
On 2016/05/24 00:28:53, dsinclair wrote:
> Can we update the return to be CFWL_PartState while we're at it?

I am not sure about this. From the way how PartState is defined and used, looks
like it is not an enum, but a set of these enums.

https://codereview.chromium.org/2001203002/diff/1/xfa/fwl/core/fwl_formimp.h#...
xfa/fwl/core/fwl_formimp.h:56: return FWL_SYSBUTTONSTATE_Hover;
On 2016/05/24 00:28:53, dsinclair wrote:
> CFWL_PartState_Hovered?

oops, fixed.

[dsinclair, 2016-05-24 02:20:36]

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

https://codereview.chromium.org/2001203002/diff/1/xfa/fwl/core/fwl_formimp.h#...
xfa/fwl/core/fwl_formimp.h:50: int32_t GetPartState() {
On 2016/05/24 00:43:22, Wei Li wrote:
> On 2016/05/24 00:28:53, dsinclair wrote:
> > Can we update the return to be CFWL_PartState while we're at it?
> 
> I am not sure about this. From the way how PartState is defined and used,
looks
> like it is not an enum, but a set of these enums. 

PartState shouldn't be a set of enums anymore unless I missed some. This one
should have been consolidated into the enum from a set of #defines.  I guess the
question is, how is the return value here used? If it get's OR'd we should leave
it as an uint32_t.

[Wei Li, 2016-05-24 05:38:01]

On 2016/05/24 02:20:36, dsinclair wrote:
> https://codereview.chromium.org/2001203002/diff/1/xfa/fwl/core/fwl_formimp.h
> File xfa/fwl/core/fwl_formimp.h (right):
> 
>
https://codereview.chromium.org/2001203002/diff/1/xfa/fwl/core/fwl_formimp.h#...
> xfa/fwl/core/fwl_formimp.h:50: int32_t GetPartState() {
> On 2016/05/24 00:43:22, Wei Li wrote:
> > On 2016/05/24 00:28:53, dsinclair wrote:
> > > Can we update the return to be CFWL_PartState while we're at it?
> > 
> > I am not sure about this. From the way how PartState is defined and used,
> looks
> > like it is not an enum, but a set of these enums. 
> 
> PartState shouldn't be a set of enums anymore unless I missed some. This one
> should have been consolidated into the enum from a set of #defines.  I guess
the
> question is, how is the return value here used? If it get's OR'd we should
leave
> it as an uint32_t.

The class's own state m_dwState is not an enum type which may have multiple bits
on.
 
The return value of GetPartState() is assigned to CFWL_ThemeBackground's
m_dwStates, such
as
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pdfium...,
which is also a uint32_t type. 

Both m_dwState and m_dwStates have quite a few bit AND and OR operations.

[dsinclair, 2016-05-24 12:39:22]

lgtm

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

https://codereview.chromium.org/2001203002/diff/20001/xfa/fwl/core/fwl_formim...
xfa/fwl/core/fwl_formimp.h:50: int32_t GetPartState() {
nit: let's make this a uint32_t instead of int32_t.

[Wei Li, 2016-05-24 22:19:55]

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

https://codereview.chromium.org/2001203002/diff/20001/xfa/fwl/core/fwl_formim...
xfa/fwl/core/fwl_formimp.h:50: int32_t GetPartState() {
On 2016/05/24 12:39:22, dsinclair wrote:
> nit: let's make this a uint32_t instead of int32_t.

Done.

[Wei Li, 2016-05-24 22:20:25]

The CQ bit was checked by weili@chromium.org

[Wei Li, 2016-05-24 22:20:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/2001203002/#ps40001
(title: "change type")

[commit-bot: I haz the power, 2016-05-24 22:20:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001203002/40001

[commit-bot: I haz the power, 2016-05-24 22:35:06]

Description was changed from

==========
Correctly implement GetPartState() function

The original function doesn't seem right. Re-implement it.
==========

to

==========
Correctly implement GetPartState() function

The original function doesn't seem right. Re-implement it.

Committed:
https://pdfium.googlesource.com/pdfium/+/5aaa9bc9193d74bb188dd1f890140cf38fb0...
==========

[commit-bot: I haz the power, 2016-05-24 22:35:07]

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