775041 - Make nsIWidget::IsEnabled return bool

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Currently, it returns an nsresult, but all implementations return NS_OK. Instead, the boolean should be returned directly; the signature should be

virtual bool IsEnabled() const;

http://mxr.mozilla.org/mozilla-central/ident?i=IsEnabled&tree=mozilla-central&filter=&strict=1

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Initial patch ... passes try server builds / tests ... posting so I can look @ the side-by-side DIFF, then I'll ping for feedback ...

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 644495 [details] [diff] [review]
Patch (v1)

Review of attachment 644495 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: widget/nsIWidget.h
@@ +765,5 @@
>      NS_IMETHOD Enable(bool aState) = 0;
>  
>      /**
>       * Ask whether the widget is enabled
>       * @param aState returns true if the widget is enabled

This isn't a param anymore

::: widget/os2/nsWindow.cpp
@@ -569,5 @@
>  {
> -  NS_ENSURE_ARG_POINTER(aState);
> -  HWND hMain = GetMainWindow();
> -  *aState = !hMain || WinIsWindowEnabled(hMain);
> -  return NS_OK;

You should probably keep the null check on hMain here.

::: widget/windows/nsWindow.cpp
@@ +1687,5 @@
>  // Return the current enable state
> +bool nsWindow::IsEnabled() const
> +{
> +  return !mWnd ||
> +    (::IsWindowEnabled(mWnd) &&

Can you line up the ( with the ! on the line above?

Comment 3

[Mark Capella [:capella]]

Cool.... 

FYI for the code in widget/os2/nsWindow.cpp, I had figured it would be ok to slightly modify, based on the OS2/API's here:

http://www.warpspeed.com.au/cgi-bin/inf2html.cmd?..\html\book\Toolkt40\PM2.INF+1350
http://www.warpspeed.com.au/cgi-bin/inf2html.cmd?..\html\book\Toolkt40\PM2.INF+1330

and the code we just put in place in the same module for IsVisible() here:

http://mxr.mozilla.org/mozilla-central/source/widget/os2/nsWindow.cpp#601

But leaving it alone is safest ... and will change it as suggested.

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v2) Nits Fixed

Nits fixed and TRY run ... 
https://tbpl.mozilla.org/?tree=Try&rev=7213f82ba428

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 644695 [details] [diff] [review]
Patch (v2) Nits Fixed

Review of attachment 644695 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIWidget.h
@@ +769,2 @@
>       */
> +    virtual bool IsEnabled() const = 0;

Need to update the IID on nsIWidget.

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v3) - Nits and IID Fixed

Comment 7

[Mark Capella [:capella]]

Comment on attachment 644802 [details] [diff] [review]
Patch (v3) - Nits and IID Fixed

Doh!

Comment 8

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3bc4b4396d75

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3bc4b4396d75