Closed Bug 1293596 Opened 8 years ago Closed 8 years ago

DeCOMtaminate nsIWidget

Categories

(Core :: Widget, defect)

defect
Not set
normal

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: nobody → n.nethercote
Status: NEW → ASSIGNED
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)
Attachment #8779266 - Flags: review?(karlt) → review+
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 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)
Whiteboard: tpi:+
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
Attachment #8779268 - Attachment is obsolete: true
Attachment #8779268 - Flags: feedback?(mstange)
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.
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)
> 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.
(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)
(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.
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)
It's dead code -- the only use is a recursive call within
nsWindow::GetNonClientMargins().
Attachment #8782213 - Flags: review?(mstange)
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)
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)
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 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+
Attachment #8782213 - Flags: review?(mstange) → review+
Attachment #8782285 - Flags: review?(mstange) → review+
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 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+
> 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!
> > +  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().
Blocks: 431634
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)
Attachment #8782287 - Attachment is obsolete: true
Summary: Remove unnecessary NS_IMETHOD use in nsIWidget → DeCOMtaminate nsIWidget
No longer blocks: 431634
Blocks: deCOM
No longer blocks: 1292440
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+
Attachment #8782706 - Flags: review?(amarchesini) → review+
Attachment #8779266 - Flags: checkin+
Attachment #8781865 - Flags: checkin+
Attachment #8782213 - Flags: checkin+
Attachment #8782285 - Flags: checkin+
Attachment #8782286 - Flags: checkin+
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 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 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+
Blocks: 1296624
Depends on: 1296967
Attachment #8782761 - Flags: checkin+
Attachment #8782791 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.