Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by tonikitoo Modified:
4 years, 4 months ago Reviewers:
Tom Sepez, dsinclair CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Project:
pdfium Visibility:
Public. |
DescriptionFactor 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
Patch Set 1 #
Total comments: 9
Patch Set 2 : Factor out the duplicated logic in Field::SetDisplay #
Created: 4 years, 4 months ago
Messages
Total messages: 13 (4 generated)
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@igalia.com changed reviewers: + dsinclair@chromium.org, tsepez@chromium.org
PTAL
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.
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;
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.
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.
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.
The CQ bit was checked by tsepez@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://pdfium.googlesource.com/pdfium/+/7c05a7ac2738ef5e80692d5e3d2db68b86f2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
