Closed
Bug 769998
Opened 12 years ago
Closed 12 years ago
Make nsIWidget::IsVisible return bool
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: Ms2ger, Assigned: capella)
Details
(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])
Attachments
(1 file, 2 obsolete files)
25.94 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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
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•12 years ago
|
||
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.
Reporter | ||
Comment 5•12 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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Push to TRY: https://tbpl.mozilla.org/?tree=Try&rev=c30c2aafba3f
Assignee | ||
Comment 9•12 years ago
|
||
On to inbound it went: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1986d30771e9
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1986d30771e9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•