Last Comment Bug 775041 - Make nsIWidget::IsEnabled return bool
: Make nsIWidget::IsEnabled return bool
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-18 04:15 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-07-24 03:04 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (19.57 KB, patch)
2012-07-20 15:14 PDT, Mark Capella [:capella]
Ms2ger: feedback+
Details | Diff | Splinter Review
Patch (v2) Nits Fixed (19.65 KB, patch)
2012-07-21 15:46 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) - Nits and IID Fixed (20.27 KB, patch)
2012-07-22 17:07 PDT, Mark Capella [:capella]
roc: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-07-18 04:15:41 PDT
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] 2012-07-20 15:14:30 PDT
Created attachment 644495 [details] [diff] [review]
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 (⌚ UTC+1/+2) 2012-07-21 02:09:43 PDT
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] 2012-07-21 04:54:00 PDT
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] 2012-07-21 15:46:48 PDT
Created attachment 644695 [details] [diff] [review]
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) 2012-07-22 15:35:56 PDT
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] 2012-07-22 17:07:56 PDT
Created attachment 644802 [details] [diff] [review]
Patch (v3) - Nits and IID Fixed
Comment 7 Mark Capella [:capella] 2012-07-22 17:09:14 PDT
Comment on attachment 644802 [details] [diff] [review]
Patch (v3) - Nits and IID Fixed

Doh!
Comment 8 Mark Capella [:capella] 2012-07-22 23:34:14 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3bc4b4396d75
Comment 9 Ed Morley [:emorley] 2012-07-24 03:04:05 PDT
https://hg.mozilla.org/mozilla-central/rev/3bc4b4396d75

Note You need to log in before you can comment on or make changes to this bug.