SUCCEEDED macro is misused

SUCCEEDED macro is misused

SUCCEEDED macro is designed for HRESULT instead of BOOL.
This change exposes my lack of knowledge of native Windows APIs. :(

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=647067

Committed: https://crrev.com/84fbf9ee380ba107d8ba851dcd721b764d396d99
Cr-Commit-Position: refs/heads/master@{#14716}

[Hzj_jie, 2016-10-20 00:21:35]

Description was changed from

==========
SUCCEEDED macro is misused

SUCCEEDED macro is designed for HRESULT instead of BOOL.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=647067
==========

to

==========
SUCCEEDED macro is misused

SUCCEEDED macro is designed for HRESULT instead of BOOL.
This change exposes my lack of knowledge of native Windows APIs. :(

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=647067
==========

[Hzj_jie, 2016-10-20 00:59:39]

zijiehe@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-10-20 17:31:15]

lgtm when my comment is addressed

https://codereview.chromium.org/2440563003/diff/1/webrtc/modules/desktop_capt...
File webrtc/modules/desktop_capture/screen_drawer_win.cc (right):

https://codereview.chromium.org/2440563003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/screen_drawer_win.cc:178: == TRUE) {
git cl format please.
It formats this code differently:
  if (SetWindowPos(window_, HWND_TOPMOST, 0, 0, 0, 0,
                   SWP_NOMOVE | SWP_NOSIZE) == TRUE) {
    return;
  }

And this looks more readable.

[Hzj_jie, 2016-10-20 18:15:18]

https://codereview.chromium.org/2440563003/diff/1/webrtc/modules/desktop_capt...
File webrtc/modules/desktop_capture/screen_drawer_win.cc (right):

https://codereview.chromium.org/2440563003/diff/1/webrtc/modules/desktop_capt...
webrtc/modules/desktop_capture/screen_drawer_win.cc:178: == TRUE) {
On 2016/10/20 17:31:15, Sergey Ulanov wrote:
> git cl format please.
> It formats this code differently:
>   if (SetWindowPos(window_, HWND_TOPMOST, 0, 0, 0, 0,
>                    SWP_NOMOVE | SWP_NOSIZE) == TRUE) {
>     return;
>   }
> 
> And this looks more readable.

Done.

[tommi (sloooow) - chröme, 2016-10-20 19:51:19]

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

[tommi (sloooow) - chröme, 2016-10-20 19:51:20]

https://codereview.chromium.org/2440563003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_drawer_win.cc (right):

https://codereview.chromium.org/2440563003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_drawer_win.cc:179: return;
Nit: Just check for non 0. False has one value, true is all the other ones. See
e.g a couple of lines below.

[Hzj_jie, 2016-10-20 21:13:39]

https://codereview.chromium.org/2440563003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_drawer_win.cc (right):

https://codereview.chromium.org/2440563003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_drawer_win.cc:179: return;
On 2016/10/20 19:51:20, tommi (chrömium) wrote:
> Nit: Just check for non 0. False has one value, true is all the other ones.
See
> e.g a couple of lines below. 

Will value != FALSE better?

[Sergey Ulanov, 2016-10-20 21:43:57]

On 2016/10/20 21:13:39, Hzj_jie wrote:
>
https://codereview.chromium.org/2440563003/diff/20001/webrtc/modules/desktop_...
> File webrtc/modules/desktop_capture/screen_drawer_win.cc (right):
> 
>
https://codereview.chromium.org/2440563003/diff/20001/webrtc/modules/desktop_...
> webrtc/modules/desktop_capture/screen_drawer_win.cc:179: return;
> On 2016/10/20 19:51:20, tommi (chrömium) wrote:
> > Nit: Just check for non 0. False has one value, true is all the other ones.
> See
> > e.g a couple of lines below. 
> 
> Will value != FALSE better?

I would prefer implicit cast to bool, i.e.
  if (SetWindowPos(...)) { 
    ....
  }

[Hzj_jie, 2016-10-20 21:47:41]

On 2016/10/20 21:43:57, Sergey Ulanov wrote:
> On 2016/10/20 21:13:39, Hzj_jie wrote:
> >
>
https://codereview.chromium.org/2440563003/diff/20001/webrtc/modules/desktop_...
> > File webrtc/modules/desktop_capture/screen_drawer_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/2440563003/diff/20001/webrtc/modules/desktop_...
> > webrtc/modules/desktop_capture/screen_drawer_win.cc:179: return;
> > On 2016/10/20 19:51:20, tommi (chrömium) wrote:
> > > Nit: Just check for non 0. False has one value, true is all the other
ones.
> > See
> > > e.g a couple of lines below. 
> > 
> > Will value != FALSE better?
> 
> I would prefer implicit cast to bool, i.e.
>   if (SetWindowPos(...)) { 
>     ....
>   }

MSVC will trigger a compiler warning
(https://msdn.microsoft.com/en-us/library/b6801kcy.aspx?f=255&MSPPError=-21472...),
if we implicitly cast BOOL (int) to bool.

[tommi (sloooow) - chröme, 2016-10-20 22:55:18]

lgtm

[Hzj_jie, 2016-10-20 23:41:37]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-10-20 23:41:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2440563003/#ps40001
(title: "!= FALSE instead of == TRUE")

[commit-bot: I haz the power, 2016-10-20 23:41:45]

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

[commit-bot: I haz the power, 2016-10-21 00:00:38]

Description was changed from

==========
SUCCEEDED macro is misused

SUCCEEDED macro is designed for HRESULT instead of BOOL.
This change exposes my lack of knowledge of native Windows APIs. :(

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=647067
==========

to

==========
SUCCEEDED macro is misused

SUCCEEDED macro is designed for HRESULT instead of BOOL.
This change exposes my lack of knowledge of native Windows APIs. :(

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=647067
==========

[commit-bot: I haz the power, 2016-10-21 00:00:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 00:00:44]

Description was changed from

==========
SUCCEEDED macro is misused

SUCCEEDED macro is designed for HRESULT instead of BOOL.
This change exposes my lack of knowledge of native Windows APIs. :(

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=647067
==========

to

==========
SUCCEEDED macro is misused

SUCCEEDED macro is designed for HRESULT instead of BOOL.
This change exposes my lack of knowledge of native Windows APIs. :(

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=647067

Committed: https://crrev.com/84fbf9ee380ba107d8ba851dcd721b764d396d99
Cr-Commit-Position: refs/heads/master@{#14716}
==========

[commit-bot: I haz the power, 2016-10-21 00:00:45]

Patchset 3 (id:??) landed as
https://crrev.com/84fbf9ee380ba107d8ba851dcd721b764d396d99
Cr-Commit-Position: refs/heads/master@{#14716}