Status

()

defect
RESOLVED DUPLICATE of bug 1293596
11 years ago
3 years ago

People

(Reporter: brad.g.hall, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 16 obsolete attachments)

31.70 KB, patch
roc
: review+
Details | Diff | Splinter Review
584.12 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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
(Reporter)

Updated

11 years ago
Version: unspecified → Trunk
(Reporter)

Comment 1

11 years ago
Attachment #318850 - Flags: review?
(Reporter)

Comment 2

11 years ago
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
(Reporter)

Comment 5

11 years ago
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.
(Reporter)

Comment 7

11 years ago
Moved GetZIndex to nsBaseWidget
Attachment #318912 - Attachment is obsolete: true
Attachment #318915 - Flags: review?(roc)
Attachment #318912 - Flags: review?(roc)
(Reporter)

Comment 8

11 years ago
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&.
(Reporter)

Comment 10

11 years ago
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.
(Reporter)

Comment 12

11 years ago
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.
(Reporter)

Comment 14

11 years ago
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+
(Reporter)

Comment 17

11 years ago
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

11 years ago
I'll pick this one up

Comment 19

11 years ago
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.

Comment 21

11 years ago
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.

Comment 23

11 years ago
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.

Updated

10 years ago
Depends on: 476726

Comment 24

10 years ago
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)

Comment 25

10 years ago
Remove low hanging fruit.

Comment 26

10 years ago
Posted 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.

Comment 27

10 years ago
Posted patch Part 4 - Get/SetClientData (obsolete) — Splinter Review
Should this be uplifted to nsIWidget and made non-virtual?

Comment 28

10 years ago
Posted 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).

Comment 29

10 years ago
Posted patch Part 6 - GetAttention (obsolete) — Splinter Review
Nothing to see here...

Comment 30

10 years ago
Posted patch Part 7 - Get/SetBounds, etc. (obsolete) — Splinter Review
Also not much to see, besides more kruft being removed...

Comment 31

10 years ago
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!

Comment 34

10 years ago
(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?

Comment 36

10 years ago
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

Comment 39

10 years ago
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

Comment 40

10 years ago
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)

Updated

10 years ago
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? :-)

Comment 42

10 years ago
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

10 years ago
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+

Updated

10 years ago
Attachment #365358 - Attachment description: Part 1 - v2 → Part 1 - v2 [pushed: comment 42]
Attachment #365358 - Attachment is obsolete: true

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]

Updated

10 years ago
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
Last Resolved: 3 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.