Closed Bug 431634 Opened 13 years ago Closed 4 years ago

deCOMtaminate nsIWidget

Categories

(Core :: Widget, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1293596

People

(Reporter: brad.g.hall, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 16 obsolete files)

31.70 KB, patch
roc
: review+
Details | Diff | Splinter Review
584.12 KB, patch
Details | Diff | Splinter Review
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
Version: unspecified → Trunk
Attachment #318850 - Flags: review?
Remove GetBorderSize, SetColorMap, ShowMenuBar as they appear to be unused/unimplemted.
Attachment #318850 - Attachment is obsolete: true
Attachment #318858 - Flags: review?
Attachment #318850 - Flags: review?
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?
BTW, never request review? from no-one, that's a no-op :-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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..
Attachment #318858 - Attachment is obsolete: true
Attachment #318912 - Flags: review?(roc)
Attachment #318858 - Flags: review?
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.
Moved GetZIndex to nsBaseWidget
Attachment #318912 - Attachment is obsolete: true
Attachment #318915 - Flags: review?(roc)
Attachment #318912 - Flags: review?(roc)
small cleanup
Attachment #318915 - Attachment is obsolete: true
Attachment #318916 - Flags: review?(roc)
Attachment #318915 - Flags: review?(roc)
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&.
You're right, getzindex is used there so I moved it back for now.  Updated patch with the requested changes.
Attachment #318916 - Attachment is obsolete: true
Attachment #318919 - Flags: review?(roc)
Attachment #318916 - Flags: review?(roc)
+    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.
Updated with changes
Attachment #318919 - Attachment is obsolete: true
Attachment #318927 - Flags: review?(roc)
Attachment #318919 - Flags: review?(roc)
> +    virtual void Scroll(const nsIntPoint& aPoint, nsRect *aClipRect) = 0;
>
> const nsIntRect*

You didn't change this.
OK, got that one; converted all the others I missed as well.
Attachment #318927 - Attachment is obsolete: true
Attachment #318933 - Flags: review?(roc)
Attachment #318927 - Flags: review?(roc)
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 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?
Attachment #318933 - Flags: review?(roc) → review+
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.
I'll pick this one up
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?
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.
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)?
No, we should get rid of it by merging it into nsWindow.
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.
Depends on: 476726
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.
Attachment #363011 - Flags: review?(roc)
Remove low hanging fruit.
Attached patch Part 3 - DispatchEvent (obsolete) — Splinter Review
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.
Attached patch Part 4 - Get/SetClientData (obsolete) — Splinter Review
Should this be uplifted to nsIWidget and made non-virtual?
Attached patch Part 5 - Get/SetSizeMode (obsolete) — Splinter Review
Changed the signatures to take an enum, so the return could be void (enum moved to nsEvent.h in part 1).
Attached patch Part 6 - GetAttention (obsolete) — Splinter Review
Nothing to see here...
Attached patch Part 7 - Get/SetBounds, etc. (obsolete) — Splinter Review
Also not much to see, besides more kruft being removed...
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
This is cool!

hey, is your ogg.diff going upstream?
Why get rid of the nsNativeWidget typedef?

Get well soon!
(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.
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?
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.
What files need nsNativeWidget but don't use the nsIWidget class?
Oops, nsIView as you said.

OK, widgetCore.h should just be removed. We could put the typedef in a new header, nsNativeWidget.h
Attached patch WIP backup patchSplinter Review
Attach a WIP patch as a backup...  This is actually 18 patches in MQ.
Attachment #363012 - Attachment is obsolete: true
Attachment #363014 - Attachment is obsolete: true
Attachment #363015 - Attachment is obsolete: true
Attachment #363017 - Attachment is obsolete: true
Attachment #363019 - Attachment is obsolete: true
Attachment #363021 - Attachment is obsolete: true
Attached patch Part 1 - v2 [pushed: comment 42] (obsolete) — Splinter Review
This is the header file changes, keeping nsNativeWidget as a void* for the moment and also moving the init data into a separate header.
Attachment #363011 - Attachment is obsolete: true
Attachment #365358 - Flags: superreview?(roc)
Attachment #365358 - Flags: review?(roc)
Attachment #363011 - Flags: review?(roc)
Attachment #365358 - Attachment is patch: true
Attachment #365358 - Attachment mime type: application/octet-stream → text/plain
Attachment #365358 - Flags: superreview?(roc)
Attachment #365358 - Flags: superreview+
Attachment #365358 - Flags: review?(roc)
Attachment #365358 - Flags: review+
Comment on attachment 365358 [details] [diff] [review]
Part 1 - v2 [pushed: comment 42]

+class nIWidget;

Typo ... maybe we don't need it? :-)
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.
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.
Attachment #370596 - Flags: superreview?(roc)
Attachment #370596 - Flags: review?(roc)
Comment on attachment 370596 [details] [diff] [review]
Part 2 - v2 [pusheed: comment 45]

great!
Attachment #370596 - Flags: superreview?(roc)
Attachment #370596 - Flags: superreview+
Attachment #370596 - Flags: review?(roc)
Attachment #370596 - Flags: review+
Attachment #365358 - Attachment description: Part 1 - v2 → Part 1 - v2 [pushed: comment 42]
Attachment #365358 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs landing]
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #370596 - Attachment description: Part 2 - v2 → Part 2 - v2 [pusheed: comment 45]
Attachment #370596 - Attachment is obsolete: true
Blocks: deCOM
Should this bug still be open?
Depends on: 1293596
This bug hasn't been touched in a long time. All the action is now happening in bug 1293596.
Status: NEW → RESOLVED
Closed: 4 years ago
No longer depends on: 1293596
Resolution: --- → DUPLICATE
Duplicate of bug: 1293596
You need to log in before you can comment on or make changes to this bug.