Factor out the duplicated logic in Field::SetDisplay

Factor out the duplicated logic in Field::SetDisplay

CL introduces a helper function to share the common
logic.
No new tests, since there is no behavior change.

Committed: https://pdfium.googlesource.com/pdfium/+/7c05a7ac2738ef5e80692d5e3d2db68b86f27ea4

[tonikitoo, 2016-08-17 17:18:07]

Description was changed from

==========
Factor out the duplicated logic in Field::SetDisplay

CL introduces a helper function to share the common
logic.
No new tests, since there is no behavior change.
==========

to

==========
Factor out the duplicated logic in Field::SetDisplay

CL introduces a helper function to share the common
logic.
No new tests, since there is no behavior change.
==========

[tonikitoo, 2016-08-17 17:18:07]

tonikitoo@igalia.com changed reviewers:
+ dsinclair@chromium.org, tsepez@chromium.org

[tonikitoo, 2016-08-17 17:23:46]

PTAL

[Tom Sepez, 2016-08-17 17:33:31]

Very nice.  Just a few style things.  Thanks for your contribution.

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cpp
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:29: static bool
SetWidgetDisplayStatus(CPDFSDK_Widget* pWidget, int value) {
nit: prefer

namespace {

bool SetW...

}  // Namespace

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:36: dwFlag &= (~ANNOTFLAG_INVISIBLE);
nit: the () aren't strictly necessary here.

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:1279: if (nControlIndex < 0) {
Optional:  I didn't see nControlIndex being updated in this loop, maybe refactor
it outside and make two loops.  Or not.

[Tom Sepez, 2016-08-17 17:38:40]

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cpp
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:1286: bSet = SetWidgetDisplayStatus(pWidget,
number);
actually, we don't want to loose  previous TRUE value, so we have to do
if (...)
  bSet = TRUE;

[tonikitoo, 2016-08-17 17:40:48]

On 2016/08/17 17:38:40, Tom Sepez wrote:
> https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cpp
> File fpdfsdk/javascript/Field.cpp (right):
> 
>
https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
> fpdfsdk/javascript/Field.cpp:1286: bSet = SetWidgetDisplayStatus(pWidget,
> number);
> actually, we don't want to loose  previous TRUE value, so we have to do
> if (...)
>   bSet = TRUE;

Yep. I have seen this bug in the existing code. I will add a TODO(tonikitoo) and
fix it in a follow up, if you are fine this way.

[Tom Sepez, 2016-08-17 17:45:37]

Perpetual twiddling.  Feel free to ignore.

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cpp
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:1280: FX_BOOL bSet = FALSE;
nit: maybe call this bAnySet and make it a bool.

[tonikitoo, 2016-08-17 17:51:53]

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cpp
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:29: static bool
SetWidgetDisplayStatus(CPDFSDK_Widget* pWidget, int value) {
On 2016/08/17 17:33:31, Tom Sepez wrote:
> nit: prefer
> 
> namespace {
> 
> bool SetW...
> 
> }  // Namespace
> 

Done.

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:36: dwFlag &= (~ANNOTFLAG_INVISIBLE);
On 2016/08/17 17:33:31, Tom Sepez wrote:
> nit: the () aren't strictly necessary here.

Done.

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:1280: FX_BOOL bSet = FALSE;
On 2016/08/17 17:45:37, Tom Sepez wrote:
> nit: maybe call this bAnySet and make it a bool.

Done.

https://codereview.chromium.org/2255843002/diff/1/fpdfsdk/javascript/Field.cp...
fpdfsdk/javascript/Field.cpp:1286: bSet = SetWidgetDisplayStatus(pWidget,
number);
On 2016/08/17 17:38:40, Tom Sepez wrote:
> actually, we don't want to loose  previous TRUE value, so we have to do
> if (...)
>   bSet = TRUE;

Done.

[Tom Sepez, 2016-08-17 17:55:35]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-08-17 17:55:36]

lgtm

[commit-bot: I haz the power, 2016-08-17 17:55:39]

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-08-17 18:08:50]

Description was changed from

==========
Factor out the duplicated logic in Field::SetDisplay

CL introduces a helper function to share the common
logic.
No new tests, since there is no behavior change.
==========

to

==========
Factor out the duplicated logic in Field::SetDisplay

CL introduces a helper function to share the common
logic.
No new tests, since there is no behavior change.

Committed:
https://pdfium.googlesource.com/pdfium/+/7c05a7ac2738ef5e80692d5e3d2db68b86f2...
==========

[commit-bot: I haz the power, 2016-08-17 18:08:51]

Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/7c05a7ac2738ef5e80692d5e3d2db68b86f2...