431634 - deCOMtaminate nsIWidget

Status: RESOLVED DUPLICATE of bug 1293596

(Core :: Widget, defect)

Description

[Brad Hall]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.10) Gecko/20071213 Fedora/2.0.0.10-3.fc8 Firefox/2.0.0.10
Build Identifier: 

This is a frequently used interface and a lot of methods in it use unnecessary out parameters, etc.

Reproducible: Always

Comment 1

[Brad Hall]

Attachment: Initial cut at decomtamination patch (header file only)

Comment 2

[Brad Hall]

Attachment: Initial cut at decomtamination patch (header file only)

Remove GetBorderSize, SetColorMap, ShowMenuBar as they appear to be unused/unimplemted.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

All those methods should still be virtual.

Use spaces, not tabs.

Let's make Move and Resize take const nsIntSize&, const nsIntPoint& and const nsIntRect& parameters instead of lots of PRInt32s.

All rects and points and sizes in this class should be nsIntRect/Point/Size, actually. nsRect/Point/Size should be for things in "appunits" (1/60 of a CSS pixel), but here we're talking about pixels always.

+    void SetForegroundColor(const nscolor &aColor) = 0;

This could just take "nscolor aColor", since nscolor is just a 32-bit int. Ditto for SetBackgroundColor.

The Get*Bounds methods can just return an nsIntRect directly.

+		   PRUint32 aHotspotX, PRUint32 aHotspotY) = 0;

Use nsIntPoint

+    void GetWindowType(nsWindowType& aWindowType) = 0;

Out parameter, so just return the value

+    void Scroll(PRInt32 aDx, PRInt32 aDy, nsRect *aClipRect) = 0;
+    void ScrollWidgets(PRInt32 aDx, PRInt32 aDy) = 0;

nsIntPoint

+    void ScrollRect(nsRect &aSrcRect, PRInt32 aDx, PRInt32 aDy) = 0;

const nsRect& and nsIntPoint.

+    void SetBorderStyle(nsBorderStyle aBorderStyle) = 0;

I think this is actually unused.

+    void WidgetToScreen(const nsRect& aOldRect, nsRect& aNewRect) = 0;
+    void ScreenToWidget(const nsRect& aOldRect, nsRect& aNewRect) = 0;

Turns out these are bogus. They always just add some offset to aOldRect to get aNewRect. It's simpler to have each method just return an nsIntPoint that the caller can add to aOldRect. So make it "nsIntPoint WidgetToScreenOffset();".

+    void GetPreferredSize(PRInt32& aWidth, PRInt32& aHeight) = 0;

Return an nsIntSize

+    void DispatchEvent(nsGUIEvent* event, nsEventStatus & aStatus) = 0;

aStatus is an out parameter, make it a result.

+    void ModalEventFilter(PRBool aRealEvent, void *aEvent, PRBool *aForWindow) = 0;

Similar for aForWindow

So you checked that for all the methods you switched from returning nsresult to void, either the method always returns NS_OK or callers never check the result?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW, never request review? from no-one, that's a no-op :-)

Comment 5

[Brad Hall]

Attachment: decomtamination of header file with requested changes

Fixed up per the changes above.  

As for SetBorderStyle not being used, it's called in src/windows/nsWindow.cpp unless I'm missing something..

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Like you mentioned to me, methods that are only used within the widget class can be private to the widget and shouldn't be in nsIWidget.

Comment 7

[Brad Hall]

Attachment: decomtamination of header file with requested changes

Moved GetZIndex to nsBaseWidget

Comment 8

[Brad Hall]

Attachment: decomtamination of header file with requested changes

small cleanup

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

GetZIndex is actually used by /xpfe/bootstrap/appleevents/nsWindowUtils.cpp, isn't it?

In some places you're passing nsIntPoint (or const nsIntPoint) where it should be const nsIntPoint&. This doesn't matter too much but const nsIntPoint& may be slightly more efficient.

In some places like Resize you're passing nsIntPoint where it should be nsIntSize.

WidgetToScreen/ScreenToWidget should have Offset at the end of the name.

BeginResizeDrag needs to take a const nsPoint& aOffset, and SetPreferredSize should take a const nsIntSize&.

Comment 10

[Brad Hall]

Attachment: decomtamination of header file with requested changes

You're right, getzindex is used there so I moved it back for now.  Updated patch with the requested changes.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    virtual void Resize(const nsIntPoint& aPoint, const nsIntSize& aWidth,
+                        const nsIntSize& aHeight, PRBool aRepaint) = 0;

What's going on here?

+    virtual void Scroll(const nsIntPoint& aPoint, nsRect *aClipRect) = 0;

const nsIntRect*

+    virtual void ScrollRect(const nsRect &aSrcRect,
+                            const nsIntPoint& aPoint) = 0;
+    virtual void Scroll(const nsIntPoint& aPoint, nsRect *aClipRect) = 0;

These points should be named aOffset since they're deltas, not absolute coordinates.

Comment 12

[Brad Hall]

Attachment: decomtamination of header file with requested changes

Updated with changes

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> +    virtual void Scroll(const nsIntPoint& aPoint, nsRect *aClipRect) = 0;
>
> const nsIntRect*

You didn't change this.

Comment 14

[Brad Hall]

Attachment: decomtamination of header file with requested changes

OK, got that one; converted all the others I missed as well.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You'll probably need to add "class nsIntRect;" at the top.

You might as well go ahead now and start converting implementations and users.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 318933 [details] [diff] [review]
decomtamination of header file with requested changes

This looked good, but it's incomplete.

Brad, are you still working on this?

Comment 17

[Brad Hall]

Yes, but slowly (lots of other things going on).  If someone wants to pick it up and continue that's OK with me, otherwise I'll continue on it as time permits.

Comment 18

[John Daniels]

I'll pick this one up

Comment 19

[John Daniels]

Here's a question, it looks like the gtk2 version of nsWindow doesn't redefine getParent from it's default behavior (in nsBaseWidget.cpp:284) of returning nsnull. Is that appropriate?

Also, SetParent in the windows version of nsWindow.cpp doesn't follow at all the same behavior as the GTK and QT versions. Both of those fail on being passed a null pointer, assuming that it is incorrect. The windows version assumes that a null pointer means to clear itself from the currentParent. Currently the two reasons for SetParent to return nsresult are the validation of the pointer as being non-null, and returning NS_NOT_IMPLEMENTED for objects of type nsBaseWidget, so this is partly an interface issue. What is the appropriate behavior here (nsIWidget.h is ambiguous on this, which I will fix as soon as I know what the behavior should be), and should this be filed as a separate bug?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess GetParent is not called anywhere in cross-platform code? If so, it should be removed from nsIWidget.

We shouldn't ever be passing a null pointer to SetParent, so feel free to make that a failure condition on all platforms.

Comment 21

[John Daniels]

Ok, so it turns out that the Gtk version inherits from nsCommonWidget, which is why firefox doesn't crash every time I run it in linux. The Gtk nsWindow is the only class that inherits from nsCommonWidget, and from what I understand, nsWindow is supposed to be the base for everything, so it seems like the nsCommonWidget class should be eliminated. Is there any reason not to get rid of it (although the patch would be under a different bug of course)?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, we should get rid of it by merging it into nsWindow.

Comment 23

[John Daniels]

I submitted that patch a seperate bug(#459075), since this one is a rather large project and removing nsCommonWidget isn't directly decomtamination. If it would be better to just make that part of this bug, that's fine, otherwise, it would probably be best if that patch is reviewed and merged into the tree (barring some major reason not to), before finishing everything up for this one. I'd probably set it as a dependency but I don't have permission to edit bugs yet.

Comment 24

[Jeremy Lea]

Attachment: Part 1 - clean up header pollution

Part 1 of 20 (so far - I've been ill and not feeling like working on my PhD ;-).

This mostly cleans up the includes, and fixes a few style issues.

Only looking for a preliminary review now, since I think at least some of the patches should be folded up into one big commit.

Comment 25

[Jeremy Lea]

Attachment: Part 2 - obviously unused methods

Remove low hanging fruit.

Comment 26

[Jeremy Lea]

Attachment: Part 3 - DispatchEvent

This fixes up DispatchEvent and related methods.  Needs some more work - probably many of the boolean returns should just be nsEventstatus, which is more obvious.

Fixes at least one bug on Windows, where the return status was being assigned to a block local which masked the function local.

Comment 27

[Jeremy Lea]

Attachment: Part 4 - Get/SetClientData

Should this be uplifted to nsIWidget and made non-virtual?

Comment 28

[Jeremy Lea]

Attachment: Part 5 - Get/SetSizeMode

Changed the signatures to take an enum, so the return could be void (enum moved to nsEvent.h in part 1).

Comment 29

[Jeremy Lea]

Attachment: Part 6 - GetAttention

Nothing to see here...

Comment 30

[Jeremy Lea]

Attachment: Part 7 - Get/SetBounds, etc.

Also not much to see, besides more kruft being removed...

Comment 31

[Jeremy Lea]

OK, that's enough for now...  I need to make a Patch 1.5 which removes all of the default methods from nsBaseWidget, since they are just wasted and actually hurt implementation, since you don't know it you didn't implement a method.  Also, all of the new methods on nsBaseWidget and nsWindow* should be non-virtual.

Also, I think windows and nsBaseWidget have some duplicated members, which cannot be healthy...

More patches at http://www.openpave.org/cgi-bin/hgweb.cgi

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is cool!

hey, is your ogg.diff going upstream?

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why get rid of the nsNativeWidget typedef?

Get well soon!

Comment 34

[Jeremy Lea]

(In reply to comment #33)
> Why get rid of the nsNativeWidget typedef?

Because it's obscurantist?  But seriously - typredef'ing a pointer to a type means you need to include the header to get the typedef, or sprinkle typedefs all over the place.  Since this was in nsIWidget.h, it meant that at least nsIDeviceContext.h was including nsIWidget.h just to get the typedef.  Another option would be to move it to a more general header like nscore.h.

And no C/C++ programmer should be confused by a void* in an interface.

> Get well soon!

Feeling better already...  Guess that means I have to get back to TeXing.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's actually useful to have the typedef here. It tells the programmer that this is not just a pointer to anything, it actually corresponds to some kind of platform-specific native widget. I don't think nscore.h is the right place for that.

Would you be happier if we used #ifdefs to typedef it to GtkWidget*, HWND, etc?

Comment 36

[Jeremy Lea]

Well, #ifdefing would be better, but it needs to be in a separate header.  Maybe widgetCore.h could be used, since it is actually unused (as far as I can tell).  nsWidgetInitData could also go in there.  It stopped me pulling nsIWidget.h out of nsIView.h.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What files need nsNativeWidget but don't use the nsIWidget class?

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, nsIView as you said.

OK, widgetCore.h should just be removed. We could put the typedef in a new header, nsNativeWidget.h

Comment 39

[Jeremy Lea]

Attachment: WIP backup patch

Attach a WIP patch as a backup...  This is actually 18 patches in MQ.

Comment 40

[Jeremy Lea]

Attachment: Part 1 - v2 [pushed: comment 42]

This is the header file changes, keeping nsNativeWidget as a void* for the moment and also moving the init data into a separate header.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 365358 [details] [diff] [review]
Part 1 - v2 [pushed: comment 42]

+class nIWidget;

Typo ... maybe we don't need it? :-)

Comment 42

[Jeremy Lea]

Part 1 pushed (with some minor un-rotting) as:
http://hg.mozilla.org/mozilla-central/rev/6052ce2d3507

I fixed the spelling mistake and kept the forward declare since nsIWidget is explicitly used in the header, so it should be there to be stand-alone.

Comment 43

[Jeremy Lea]

Attachment: Part 2 - v2 [pusheed: comment 45]

Revised patch to remove all (mostly) unused methods.  ShowMenuBar() is used but not implemented.  Get/SetMenuBar are used in cocoa widget code only, so I made them non-virtual.  grepping for "MenuBar" or "^+[^+]" might be effective ways to review this... 

Also, remove the definition of nsNativeWidget that I forgot to remove in the last patch (oops), bump the IID and remove the XXX comments on Scroll() saying it was going away.  It's been almost 10 years, and still the replacement methods have not been implemented or used.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 370596 [details] [diff] [review]
Part 2 - v2 [pusheed: comment 45]

great!

Comment 45

[Jeremy Lea]

Part 2 pushed as:
http://hg.mozilla.org/mozilla-central/rev/02817e69ad40

Comment 46

[Zack Weinberg (:zwol)]

Should this bug still be open?

Comment 47

[Nicholas Nethercote [inactive]]

This bug hasn't been touched in a long time. All the action is now happening in bug 1293596.