Closed
Bug 1293596
Opened 8 years ago
Closed 8 years ago
DeCOMtaminate nsIWidget
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
(Whiteboard: tpi:+)
Attachments
(8 files, 3 obsolete files)
4.49 KB,
patch
|
karlt
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
73.24 KB,
patch
|
mstange
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
6.24 KB,
patch
|
mstange
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
27.30 KB,
patch
|
mstange
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
18.88 KB,
patch
|
mstange
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
20.88 KB,
patch
|
mstange
:
review+
baku
:
review+
|
Details | Diff | Splinter Review |
21.70 KB,
patch
|
n.nethercote
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
11.71 KB,
patch
|
jimm
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
nsIWidget has a lot of NS_IMETHOD methods that are infallible. It makes things clearer if we just make them |virtual void| instead. (That loses the |__stdcall| on Windows, but that doesn't matter.) This will help with bug 1292440.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8779266 -
Flags: review?(karlt)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
A couple of Destroy implementations did return a non-NS_OK value, but this was only used in a single assertion in PluginWidgetParent::KillWidget(). I've changed those implementations to assert in those cases where they used to return non-NS_OK.
Attachment #8779267 -
Flags: review?(karlt)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8779268 -
Flags: review?(karlt)
Updated•8 years ago
|
Attachment #8779266 -
Flags: review?(karlt) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8779267 [details] [diff] [review] (part 2) - Don't use NS_IMETHOD for nsIWidget::Destroy (In reply to Nicholas Nethercote [:njn] from comment #2) > A couple of Destroy implementations did return a non-NS_OK value, but this > was only used in a single assertion in PluginWidgetParent::KillWidget(). I've > changed those implementations to assert in those cases where they used to > return non-NS_OK. I didn't see any of those implementations, except for the NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT, which I expect is fine.
Attachment #8779267 -
Flags: review?(karlt) → review+
Comment 5•8 years ago
|
||
Comment on attachment 8779268 [details] [diff] [review] (part 3) - Don't use NS_IMETHOD for nsIWidget::Resize{,Client} I think this is good with one change below, thanks, but I'll get Markus to confirm that returning failure after ObjC failures is not important. Given other failure paths already return NS_OK, and nsXULWindow doesn't make much attempt to restore state after failures, I guess not. >- DispatchEvent(&event, status); >+ XUnused << DispatchEvent(&event, status); >- DispatchEvent(aEvent, status); >+ XUnused << DispatchEvent(aEvent, status); These should be in the patch for bug 1292440, I assume.
Attachment #8779268 -
Flags: review?(karlt)
Attachment #8779268 -
Flags: review+
Attachment #8779268 -
Flags: feedback?(mstange)
Updated•8 years ago
|
Whiteboard: tpi:+
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8779267 [details] [diff] [review] (part 2) - Don't use NS_IMETHOD for nsIWidget::Destroy I'm going to mark parts 2 and 3 as obsolete because I want to be more conservative about the Objective C exception stuff. So I'll only consider changing the methods that the Cocoa backend doesn't override.
Attachment #8779267 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8779268 -
Attachment is obsolete: true
Attachment #8779268 -
Flags: feedback?(mstange)
Comment 7•8 years ago
|
||
If the caller doesn't handle failure, then there's little use in returning failure when an exception occurs. I don't know whether we have any data on how frequently we catch Objective C exceptions in the wild.
Assignee | ||
Comment 8•8 years ago
|
||
mstange: I have questions about the bounds-getting functions. GetBounds() never fails and is never checked. Changing it to return |void| is straightforward. GetRestoredBounds() can fail on multiple platforms and it is checked at its single callsite. Leaving it unchanged is straightforward. nsCocoaWindow::GetClientBounds() is the only implementation of GetClientBounds() that can return non-NS_OK, and that can only happen if an Objective C exception occurs. Do you know if it's possible in practice for that function? None of the GetClientBounds() callsites checks for failure. nsCocoaWindow::GetScreenBounds() is similarly the only implementation of GetScreenBounds() that can fail, again only via Objective C exceptions. *However*, of the 14 or so callsites, 3 of them do check for failure. Any suggestions on what to do with GetClientBounds() and GetScreenBounds()?
Flags: needinfo?(mstange)
Assignee | ||
Comment 9•8 years ago
|
||
> Any suggestions on what to do with GetClientBounds() and GetScreenBounds()?
Looking more closely, GetScreenBounds() looks *very* unlikely to throw any kind of exception, certainly not in an opt build.
Comment 10•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #8) > nsCocoaWindow::GetClientBounds() is the only implementation of > GetClientBounds() that can return non-NS_OK, and that can only happen if an > Objective C exception occurs. Do you know if it's possible in practice for > that function? Possible, maybe, but extremely unlikely. Let's make all of the methods you mentioned infallible.
Flags: needinfo?(mstange)
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #10) > (In reply to Nicholas Nethercote [:njn] from comment #8) > > nsCocoaWindow::GetClientBounds() is the only implementation of > > GetClientBounds() that can return non-NS_OK, and that can only happen if an > > Objective C exception occurs. Do you know if it's possible in practice for > > that function? > > Possible, maybe, but extremely unlikely. Let's make all of the methods you > mentioned infallible. Even better: let's use NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(LayoutDeviceIntRect(0, 0, 0, 0)), similar to how GetClientOffset() already does it. That let's us make the function fallible while giving clear and reasonable behaviour in the unlikely exception case.
Assignee | ||
Comment 12•8 years ago
|
||
This patch makes GetBounds(), GetScreenBounds() and GetClientBounds() more obviously infallible, like existing functions such as GetNaturalBounds() and GetClientSize(). This results in clearer behaviour in nsCocoaWindow.mm if Objective C exceptions occur. Along the way, the patch removes some useless failure checks for these functions. The patch also removes the NS_IMETHOD from GetRestoredBounds and makes that function MOZ_MUST_USE.
Attachment #8781865 -
Flags: review?(mstange)
Assignee | ||
Comment 13•8 years ago
|
||
It's dead code -- the only use is a recursive call within nsWindow::GetNonClientMargins().
Attachment #8782213 -
Flags: review?(mstange)
Assignee | ||
Comment 14•8 years ago
|
||
They don't need to be NS_IMETHOD, but they should be MOZ_MUST_USE. Adding the latter catches a few missing checks, which the patch adds. The patch also gives PuppetWidget an InfallibleCreate() function, which makes the infallibility of PuppetWidget creation clear.
Attachment #8782285 -
Flags: review?(mstange)
Assignee | ||
Comment 15•8 years ago
|
||
They don't need to be NS_IMETHOD. The patch also gives nsBaseWidget an InfallibleMakeFullScreen() function, which avoids the need for some checks.
Attachment #8782286 -
Flags: review?(mstange)
Assignee | ||
Comment 16•8 years ago
|
||
It doesn't need to be NS_IMETHOD. The patch also gives nsBaseWidget an InfallibleSetSizeMode() function, which avoids the need for some checks. The only way SetSizeMode() can fail is if nsCocoaWindow::SetSizeMove() throws an Objective C exception.
Attachment #8782287 -
Flags: review?(mstange)
Comment 17•8 years ago
|
||
Comment on attachment 8781865 [details] [diff] [review] (part 2) - Rework nsIWidget bounds getters Review of attachment 8781865 [details] [diff] [review]: ----------------------------------------------------------------- This is so much better! ::: widget/nsIWidget.h @@ +1872,5 @@ > */ > virtual bool WidgetPaintsBackground() { return false; } > > virtual bool NeedsPaint() { > + return IsVisible() ? !GetBounds().IsEmpty() : false; return IsVisible() && !GetBounds().IsEmpty();
Attachment #8781865 -
Flags: review?(mstange) → review+
Updated•8 years ago
|
Attachment #8782213 -
Flags: review?(mstange) → review+
Updated•8 years ago
|
Attachment #8782285 -
Flags: review?(mstange) → review+
Comment 18•8 years ago
|
||
Comment on attachment 8782286 [details] [diff] [review] (part 5) - Tweak nsIWidget::MakeFullScreen{,WithNativeTransition}() Review of attachment 8782286 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsBaseWidget.h @@ +180,5 @@ > uint16_t aDuration, > nsISupports* aData, > nsIRunnable* aCallback) override; > + virtual nsresult MakeFullScreen(bool aFullScreen, > + nsIScreen* aScreen = nullptr) override; Do you want to override this method in nsBaseWidget at all? Looks like nsBaseWidget is already an abstract class, at least because ReparentNativeWidget is purely virtual, so MakeFullScreen might as well be pure virtual even in nsBaseWidget. (I'm having a hard time phrasing this properly, I hope you know what I mean.)
Attachment #8782286 -
Flags: review?(mstange) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8782287 [details] [diff] [review] (part 5) - Tweak nsIWidget::SetSizeMode() Review of attachment 8782287 [details] [diff] [review]: ----------------------------------------------------------------- What's the criterion that determines whether something can be virtual nsresult or needs to be NS_IMETHOD? > The only way SetSizeMode() can fail is if nsCocoaWindow::SetSizeMode() throws > an Objective C exception. That's extremely unlikely so I'd be fine with just swallowing those exceptions and making it infallible. nsCocoaWindow::SetSizeMode() also ignores failures of its call to MakeFullScreen().
Attachment #8782287 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 20•8 years ago
|
||
> What's the criterion that determines whether something can be virtual
> nsresult or needs to be NS_IMETHOD?
Good question :) The only difference between the two is that on Win32 NS_IMETHOD adds |__stdcall|, which is required by xptcall, which is what is used when C++ code is called from JS code. So any |scriptable| IDL methods must use NS_IMETHOD, which is why xpidl.py defaults to using it.
But nsIWidget doesn't involved IDL and is C++ only, which means that NS_IMETHOD is unnecessary. nsIWidget already has a mixture of |virtual nsresult| and |NS_IMETHOD|, and I prefer the former because it's standard C++ rather than an obscure Mozilla-ism!
Assignee | ||
Comment 21•8 years ago
|
||
> > + virtual nsresult MakeFullScreen(bool aFullScreen,
> > + nsIScreen* aScreen = nullptr) override;
>
> Do you want to override this method in nsBaseWidget at all? Looks like
> nsBaseWidget is already an abstract class, at least because
> ReparentNativeWidget is purely virtual, so MakeFullScreen might as well be
> pure virtual even in nsBaseWidget.
It's a good idea, but I tried it and it doesn't work, because PuppetWidget doesn't override MakeFullScreen(). So I think making nsBaseWidget::MakeFullScreen() crash is actually bogus. (I haven't run these patches through try yet.) I'll just make MakefullScreen() call into InfallibleMakeScreen().
Assignee | ||
Comment 22•8 years ago
|
||
mstange: I'm asking for re-review on the widget/ changes because making SetSizeMode() infallible has changed the patch significantly. baku: please review the dom/ changes. Thank you.
Attachment #8782706 -
Flags: review?(mstange)
Attachment #8782706 -
Flags: review?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Attachment #8782287 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Summary: Remove unnecessary NS_IMETHOD use in nsIWidget → DeCOMtaminate nsIWidget
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8782761 [details] [diff] [review] (part 7) - Make nsIWidget::Destroy infallible (Carrying over karlt's r+ from the earlier version of this patch, which was called part 2 originally.)
Attachment #8782761 -
Flags: review+
Updated•8 years ago
|
Attachment #8782706 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 26•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1360054f479200925e369d2f8a9441c6cf3b273f Bug 1293596 (part 1) - Don't use NS_IMETHOD for nsIWidget::AttachViewToTopLevel. r=karlt. https://hg.mozilla.org/integration/mozilla-inbound/rev/6a81bba012c8d4283978ae3c6920843916817bfc Bug 1293596 (part 2) - Rework nsIWidget bounds getters. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9ad3d5fd084750c5367c2f478ff188ae3c78b8 Bug 1293596 (part 3) - Remove GetNonClientMargins. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f8c906f4022dfd3a5f35aa1e08178660a8c95d Bug 1293596 (part 4) - Tweak nsIWidget::Create. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/581e898152d059ed2c2f9ba38c36f14291328e83 Bug 1293596 (part 5) - Tweak nsIWidget::MakeFullScreen{,WithNativeTransition}(). r=mstange.
Assignee | ||
Updated•8 years ago
|
Attachment #8779266 -
Flags: checkin+
Assignee | ||
Updated•8 years ago
|
Attachment #8781865 -
Flags: checkin+
Assignee | ||
Updated•8 years ago
|
Attachment #8782213 -
Flags: checkin+
Assignee | ||
Updated•8 years ago
|
Attachment #8782285 -
Flags: checkin+
Assignee | ||
Updated•8 years ago
|
Attachment #8782286 -
Flags: checkin+
Assignee | ||
Comment 27•8 years ago
|
||
Its return value is only checked in one low-value assertion. The patch also does the following. - Removes the Android and GTK overloadings of EnableDragDrop(), which are identical to the nsBaseWidget one. - Streamlines the Windows implementation: fixes the indentation and takes advantage of infallible |new|.
Attachment #8782791 -
Flags: review?(jmathies)
Comment 28•8 years ago
|
||
Comment on attachment 8782791 [details] [diff] [review] (part 8) - Make nsIWidget::EnableDragDrop() infallible Review of attachment 8782791 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsIWidget.h @@ +1372,5 @@ > */ > virtual bool AsyncPanZoomEnabled() const = 0; > > /** > * Enables the dropping of files to a widget (XXX this is temporary) might as well get rid of this '(XXX this is temporary)' ::: widget/windows/nsWindow.cpp @@ +3516,5 @@ > if (aEnable) { > + if (!mNativeDragTarget) { > + mNativeDragTarget = new nsNativeDragTarget(this); > + mNativeDragTarget->AddRef(); > + if (S_OK == ::CoLockObjectExternal((LPUNKNOWN)mNativeDragTarget,TRUE,FALSE)) { This can be a SUCCEEDED() check vs. a direct check for S_OK. Maybe fix the spacing of these params while you're in here.
Attachment #8782791 -
Flags: review?(jmathies) → review+
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1360054f4792 https://hg.mozilla.org/mozilla-central/rev/6a81bba012c8 https://hg.mozilla.org/mozilla-central/rev/9e9ad3d5fd08 https://hg.mozilla.org/mozilla-central/rev/a3f8c906f402 https://hg.mozilla.org/mozilla-central/rev/581e898152d0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment 30•8 years ago
|
||
Comment on attachment 8782706 [details] [diff] [review] (part 6) - Make nsIWidget::SetSizeMode() infallible Review of attachment 8782706 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +13485,5 @@ > nsGlobalChromeWindow::Maximize() > { > FORWARD_TO_INNER_CHROME(Maximize, (), NS_ERROR_UNEXPECTED); > > + Maximize(); This scares me. Should it be nsGlobalWindow::Maximize()? @@ +13506,5 @@ > nsGlobalChromeWindow::Minimize() > { > FORWARD_TO_INNER_CHROME(Minimize, (), NS_ERROR_UNEXPECTED); > > + Minimize(); same here
Attachment #8782706 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 31•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a07a5bac46b40627727b4d0dd58388f9502783f9 Bug 1293596 (part 6) - Make nsIWidget::SetSizeMode() infallible. r=mstange,baku. https://hg.mozilla.org/integration/mozilla-inbound/rev/08d20351a75460e22d4c1957d2c0e076b291700c Bug 1293596 (part 7) - Make nsIWidget::Destroy infallible. r=karlt. https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae5d84aabf94a85ee605b123040e7c2b7384a4d Bug 1293596 (part 8) - Make nsIWidget::EnableDragDrop() infallible. r=jimm.
Comment 32•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a07a5bac46b4 https://hg.mozilla.org/mozilla-central/rev/08d20351a754 https://hg.mozilla.org/mozilla-central/rev/4ae5d84aabf9
Assignee | ||
Updated•8 years ago
|
Attachment #8782761 -
Flags: checkin+
Assignee | ||
Updated•8 years ago
|
Attachment #8782791 -
Flags: checkin+
You need to log in
before you can comment on or make changes to this bug.
Description
•