769998 - Make nsIWidget::IsVisible return bool

Status: RESOLVED FIXED

(Core :: Widget, defect)

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 IsVisible() const;

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

Comment 1

[jdev005]

can i be assigned to work on this bug ?

Comment 2

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

Certainly.

Comment 3

[jdev005]

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]]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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]]

Attachment: 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]]

Attachment: Patch (v3)

Fixedup commit message, seeking review.

Comment 8

[Mark Capella [:capella]]

Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=c30c2aafba3f

Comment 9

[Mark Capella [:capella]]

On to inbound it went:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1986d30771e9

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1986d30771e9