Make nsIWidget::IsVisible return bool

RESOLVED FIXED in mozilla17

Status

()

Core
Widget
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: capella)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
can i be assigned to work on this bug ?
(Reporter)

Comment 2

5 years ago
Certainly.
Assignee: nobody → jdev005

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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.
Assignee: jdev005 → markcapella
Status: NEW → ASSIGNED
Attachment #642297 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 5

5 years ago
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.
Attachment #642297 - Flags: feedback?(Ms2ger) → feedback+
(Assignee)

Comment 6

5 years ago
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.
Attachment #642297 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 643303 [details] [diff] [review]
Patch (v3)

Fixedup commit message, seeking review.
Attachment #642390 - Attachment is obsolete: true
Attachment #643303 - Flags: review?(roc)
Attachment #643303 - Flags: review?(roc) → review+
(Assignee)

Comment 8

5 years ago
Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=c30c2aafba3f
(Assignee)

Comment 9

5 years ago
On to inbound it went:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1986d30771e9
https://hg.mozilla.org/mozilla-central/rev/1986d30771e9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.