Continue formatting fwl/core

Continue formatting fwl/core

Committed: https://pdfium.googlesource.com/pdfium/+/058d2d6b7851b77a371bb4d37d5da1148794397a

[dsinclair, 2016-11-16 18:47:48]

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

[dsinclair, 2016-11-16 18:47:49]

PTAL.  (Last one in fwl/core ....)

[npm, 2016-11-16 20:57:54]

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit.cpp
File xfa/fwl/core/ifwl_edit.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:1486: if ((pMsg->m_dwFlags & FWL_KEYFLAG_Shift) &&
m_nSelStart != nIndex) {
You might still need the "!!" for compiler to be happy... I don't know

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:1618: case 0x0A:  // '\n'
Can we make this a constant like the other ones?

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form.cpp
File xfa/fwl/core/ifwl_form.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form....
xfa/fwl/core/ifwl_form.cpp:351: #if (_FX_OS_ != _FX_MACOSX_)
nit: no ( )

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form....
xfa/fwl/core/ifwl_form.cpp:691: return
return;

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_listb...
File xfa/fwl/core/ifwl_listbox.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_listb...
xfa/fwl/core/ifwl_listbox.cpp:162: CFWL_ListItem*
IFWL_ListBox::GetSelItem(int32_t nIndexSel) {
Can this method just call the other GetSelItem?

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_month...
File xfa/fwl/core/ifwl_monthcalendar.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_month...
xfa/fwl/core/ifwl_monthcalendar.cpp:228: void
IFWL_MonthCalendar::DrawBkground(CFX_Graphics* pGraphics,
Can we name this method properly?

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_scrol...
File xfa/fwl/core/ifwl_scrollbar.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_scrol...
xfa/fwl/core/ifwl_scrollbar.cpp:260: std::min(std::max(m_fTrackPos,
m_fRangeMin), m_fRangeMax);
Shoudln't this be:
std::max(std::min(m_fRangeMax, m_fTrackPos), m_fRangeMin)?

[Tom Sepez, 2016-11-16 21:09:33]

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit.cpp
File xfa/fwl/core/ifwl_edit.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:87: if (!bAutoSize) {
nit: curious why this got negated?  You could keep the current ordering and
still insert a return?

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:779: } else {
nit: or just else if () ...

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:925: m_fVAlignOffset = std::max(fOffsetY, 0.f);
nit: 0.0f

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form.cpp
File xfa/fwl/core/ifwl_form.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form....
xfa/fwl/core/ifwl_form.cpp:690: #ifdef FWL_UseMacSystemBorder
I'd just have a single #ifndef block and lose the early return.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form....
xfa/fwl/core/ifwl_form.cpp:791: #if (_FX_OS_ == _FX_MACOSX_)
no: () as above.

[dsinclair, 2016-11-16 21:31:27]

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit.cpp
File xfa/fwl/core/ifwl_edit.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:87: if (!bAutoSize) {
On 2016/11/16 21:09:33, Tom Sepez wrote:
> nit: curious why this got negated?  You could keep the current ordering and
> still insert a return?


Consistency. Most other classes have a single line then return under !bAutoSize.
I flipped this one so we'd consistently do the !bAutoSize case first.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:779: } else {
On 2016/11/16 21:09:33, Tom Sepez wrote:
> nit: or just else if () ...

Done.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:925: m_fVAlignOffset = std::max(fOffsetY, 0.f);
On 2016/11/16 21:09:33, Tom Sepez wrote:
> nit: 0.0f

Done.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:1486: if ((pMsg->m_dwFlags & FWL_KEYFLAG_Shift) &&
m_nSelStart != nIndex) {
On 2016/11/16 20:57:53, npm wrote:
> You might still need the "!!" for compiler to be happy... I don't know

Seems to be happy ....

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_edit....
xfa/fwl/core/ifwl_edit.cpp:1618: case 0x0A:  // '\n'
On 2016/11/16 20:57:53, npm wrote:
> Can we make this a constant like the other ones?

Done.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form.cpp
File xfa/fwl/core/ifwl_form.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form....
xfa/fwl/core/ifwl_form.cpp:351: #if (_FX_OS_ != _FX_MACOSX_)
On 2016/11/16 20:57:54, npm wrote:
> nit: no ( )

Done.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form....
xfa/fwl/core/ifwl_form.cpp:690: #ifdef FWL_UseMacSystemBorder
On 2016/11/16 21:09:33, Tom Sepez wrote:
> I'd just have a single #ifndef block and lose the early return.

Done.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form....
xfa/fwl/core/ifwl_form.cpp:691: return
On 2016/11/16 20:57:54, npm wrote:
> return;

Hah, doh. Removed due to comment above.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_form....
xfa/fwl/core/ifwl_form.cpp:791: #if (_FX_OS_ == _FX_MACOSX_)
On 2016/11/16 21:09:33, Tom Sepez wrote:
> no: () as above.

Done.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_listb...
File xfa/fwl/core/ifwl_listbox.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_listb...
xfa/fwl/core/ifwl_listbox.cpp:162: CFWL_ListItem*
IFWL_ListBox::GetSelItem(int32_t nIndexSel) {
On 2016/11/16 20:57:54, npm wrote:
> Can this method just call the other GetSelItem?

Done. Nice catch.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_month...
File xfa/fwl/core/ifwl_monthcalendar.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_month...
xfa/fwl/core/ifwl_monthcalendar.cpp:228: void
IFWL_MonthCalendar::DrawBkground(CFX_Graphics* pGraphics,
On 2016/11/16 20:57:54, npm wrote:
> Can we name this method properly?

Done.

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_scrol...
File xfa/fwl/core/ifwl_scrollbar.cpp (right):

https://codereview.chromium.org/2510793003/diff/20001/xfa/fwl/core/ifwl_scrol...
xfa/fwl/core/ifwl_scrollbar.cpp:260: std::min(std::max(m_fTrackPos,
m_fRangeMin), m_fRangeMax);
On 2016/11/16 20:57:54, npm wrote:
> Shoudln't this be:
> std::max(std::min(m_fRangeMax, m_fTrackPos), m_fRangeMin)?

Done.

[dsinclair, 2016-11-16 21:31:32]

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

[commit-bot: I haz the power, 2016-11-16 21:31:36]

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

[npm, 2016-11-16 21:36:24]

lgtm

[dsinclair, 2016-11-16 21:36:51]

The CQ bit was unchecked by dsinclair@chromium.org

[dsinclair, 2016-11-16 21:36:51]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-11-16 21:36:55]

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-16 21:54:04]

Description was changed from

==========
Continue formatting fwl/core
==========

to

==========
Continue formatting fwl/core

Committed:
https://pdfium.googlesource.com/pdfium/+/058d2d6b7851b77a371bb4d37d5da1148794...
==========

[commit-bot: I haz the power, 2016-11-16 21:54:05]

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

[Lei Zhang, 2016-11-24 00:05:30]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2016-11-24 00:05:30]

+thakis FYI, not sure who should get the credit for catching this.

https://codereview.chromium.org/2510793003/diff/40001/xfa/fwl/core/ifwl_listb...
File xfa/fwl/core/ifwl_listbox.cpp (right):

https://codereview.chromium.org/2510793003/diff/40001/xfa/fwl/core/ifwl_listb...
xfa/fwl/core/ifwl_listbox.cpp:313: if (!m_pProperties->m_dwStyleExes &
FWL_STYLEEXT_LTB_MultiSelection)
This is probably wrong because (whatever newer version of) Clang in Chromium is
complaining:

error: logical not is only applied to the left hand side of this bitwise
operator [-Werror,-Wlogical-not-parentheses]

[Nico, 2016-11-24 00:12:39]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-11-24 00:12:40]

https://codereview.chromium.org/2510793003/diff/40001/xfa/fwl/core/ifwl_listb...
File xfa/fwl/core/ifwl_listbox.cpp (right):

https://codereview.chromium.org/2510793003/diff/40001/xfa/fwl/core/ifwl_listb...
xfa/fwl/core/ifwl_listbox.cpp:313: if (!m_pProperties->m_dwStyleExes &
FWL_STYLEEXT_LTB_MultiSelection)
On 2016/11/24 00:05:30, Lei Zhang (Mostly OOO) wrote:
> This is probably wrong because (whatever newer version of) Clang in Chromium
is
> complaining:
> 
> error: logical not is only applied to the left hand side of this bitwise
> operator [-Werror,-Wlogical-not-parentheses]

Nice! primiano requested this to be a warning in https://crbug.com/655213 and I
implemented it in
http://llvm.org/viewvc/llvm-project?view=revision&revision=285310 which landed a
little under a month ago. This rolled into chromium 7 hours (!) ago in
https://codereview.chromium.org/2523073002 (it was also briefly in the tree for
6h or so 5 days ago with https://codereview.chromium.org/2508323003/  but that
got reverted here https://codereview.chromium.org/2520453002/).

a) I'm pleasantly surprised that pdfium already has that 7-hour old compiler
b) It's really cool that this warning prevented a bug after being around for
only a few hours :-)

[Lei Zhang, 2016-11-24 00:24:46]

On 2016/11/24 00:12:40, Nico wrote:
> Nice! primiano requested this to be a warning in https://crbug.com/655213 and
I
> implemented it in
> http://llvm.org/viewvc/llvm-project?view=revision&revision=285310 which landed
a
> little under a month ago. This rolled into chromium 7 hours (!) ago in
> https://codereview.chromium.org/2523073002 (it was also briefly in the tree
for
> 6h or so 5 days ago with https://codereview.chromium.org/2508323003/  but that
> got reverted here https://codereview.chromium.org/2520453002/).
> 
> a) I'm pleasantly surprised that pdfium already has that 7-hour old compiler

It doesn't. I just got lucky because I sometimes build Chromium with PDFium's
XFA flag flipped on.

> b) It's really cool that this warning prevented a bug after being around for
> only a few hours :-)

We can... https://codereview.chromium.org/2528613003/ but I guess I gotta fix
the bug. (It's in another CL, will merge the two.)

[Lei Zhang, 2016-11-24 00:30:31]

... and we caught the bug earlier today and fixed it in
https://codereview.chromium.org/2524173002