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


Attachments
Patch (v1) (25.75 KB, patch)
2012-07-14 17:46 PDT, Mark Capella [:capella]
Ms2ger: feedback+
Details | Diff | Splinter Review
Patch (v2) (25.94 KB, patch)
2012-07-15 09:25 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (25.94 KB, patch)
2012-07-18 02:50 PDT, Mark Capella [:capella]
roc: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-07-01 03:01:15 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 IsVisible() const;

http://mxr.mozilla.org/mozilla-central/ident?i=IsVisible&tree=mozilla-central&filter=&strict=1
Comment 1 jdev005 2012-07-01 10:50:20 PDT
can i be assigned to work on this bug ?
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-07-01 11:42:57 PDT
Certainly.
Comment 3 jdev005 2012-07-02 21:46:34 PDT
i have never developed a patch before and i really don no how to get about it  ... can u plz help me getting started
i could meet u on irc at any time of  convenience to you
Comment 4 Mark Capella [:capella] 2012-07-14 17:46:03 PDT
Created attachment 642297 [details] [diff] [review]
Patch (v1)

I talked to jdev005 through email. He's busy with other bugs so he let me take this. 

Pushed it to TRY for yucks: https://tbpl.mozilla.org/?tree=Try&rev=04b314f638e7

This is out of my normal area, so let me know of any glaring errors.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-07-15 03:37:17 PDT
Comment on attachment 642297 [details] [diff] [review]
Patch (v1)

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

Thanks! f=me with the comments below.

::: view/src/nsViewManager.cpp
@@ +446,1 @@
>        return;

Nit: the return is indented two space too much, and please keep the {}s.

::: widget/cocoa/nsChildView.mm
@@ +530,2 @@
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;

Hmm, you'll need to update those.

@@ +530,5 @@
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>  
> +  if (!mVisible)
> +    return mVisible;

Keep the {}

@@ +543,3 @@
>    }
> +
> +  return outState;

I think this can be written as return ([mView window] != nil) && !NSIsEmptyRect([mView visibleRect]);.

::: widget/cocoa/nsCocoaWindow.mm
@@ +553,2 @@
>  
>    NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;

Same here

::: widget/os2/nsWindow.cpp
@@ +604,2 @@
>  {
> +  return WinIsWindowVisible(GetMainWindow()) ? true : false;

Just return WinIsWindowVisible(GetMainWindow());. Can you check if GetMainWindow() is const?

::: xpfe/appshell/src/nsXULWindow.cpp
@@ +459,3 @@
>      nsCOMPtr<nsIWidget> parentWidget;
>      parent->GetMainWidget(getter_AddRefs(parentWidget));
> +    if (parentWidget && parentWidget->IsVisible()) {

I think this should be !parentWidget || parentWidget->IsVisible() to keep the current behaviour.
Comment 6 Mark Capella [:capella] 2012-07-15 09:25:22 PDT
Created attachment 642390 [details] [diff] [review]
Patch (v2)

New patch attached...

>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>
>Hmm, you'll need to update those.

Dang macros hide code from me ... I defaulted both occurrences to return false ... I think this is correct.


> +  return WinIsWindowVisible(GetMainWindow()) ? true : false;
>
>Just return WinIsWindowVisible(GetMainWindow());. Can you check if GetMainWindow() is const?

GetMainWindow() is not const, with signature "inline HWND nsWindow::GetMainWindow()" ... I'm curious why that might be significant?


> +    if (parentWidget && parentWidget->IsVisible()) {
>
>I think this should be !parentWidget || parentWidget->IsVisible() to keep the current behaviour.

Nice. I had wondered why the original local var was defaulted to true ... I should have looked at that longer.
Comment 7 Mark Capella [:capella] 2012-07-18 02:50:31 PDT
Created attachment 643303 [details] [diff] [review]
Patch (v3)

Fixedup commit message, seeking review.
Comment 8 Mark Capella [:capella] 2012-07-18 10:20:36 PDT
Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=c30c2aafba3f
Comment 9 Mark Capella [:capella] 2012-07-19 03:28:03 PDT
On to inbound it went:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1986d30771e9
Comment 10 Ed Morley [:emorley] 2012-07-20 06:47:54 PDT
https://hg.mozilla.org/mozilla-central/rev/1986d30771e9

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