Closed Bug 407616 Opened 13 years ago Closed 12 years ago

clean up nsResizerFrame

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: Swatinem)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 7 obsolete files)

nsResizerFrame could be simplified:
1) there is no need to store mDirection. It's simpler to just test the direction attribute every time it's needed.
2) Instead of a custom enum eDirection, manipulate directions as a pair of integers, each one either -1, 0, or 1. Then the code can be simplified by writing expressions like "current + dirX*change" instead of doing a switch on an enum value.
Whiteboard: good first bug
Whiteboard: good first bug → [good first bug]
Attached patch v1 (obsolete) — Splinter Review
This patch basically does what roc said it should do.
Assignee: nobody → arpad.borsos
Attachment #297825 - Flags: superreview?(roc)
Attachment #297825 - Flags: review?(roc)
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
Attached patch v2 (obsolete) — Splinter Review
This patch uses a static var instead of calling GetDirection on every mouse move.
Attachment #297825 - Attachment is obsolete: true
Attachment #297879 - Flags: superreview?(roc)
Attachment #297879 - Flags: review?(roc)
Attachment #297825 - Flags: superreview?(roc)
Attachment #297825 - Flags: review?(roc)
Attached patch v3 (obsolete) — Splinter Review
This patch should detect the default direction correctly.
Attachment #297879 - Attachment is obsolete: true
Attachment #297955 - Flags: superreview?(roc)
Attachment #297955 - Flags: review?(roc)
Attachment #297879 - Flags: superreview?(roc)
Attachment #297879 - Flags: review?(roc)
> This patch uses a static var instead of calling GetDirection on every mouse
> move.

Don't do that. There is no performance issue with calling GetDirection on every mouse move, this is an unnecessary optimization. It's also incorrect if something changes the direction during a mouse move.

Make GetDirection return a struct with two PRInt8 elements.

+    if( value.Equals( NS_LITERAL_STRING("topleft") ) )

Instead of these 8 ifs, use FindAttrValueIn plus an array of 8 of those two-element structs.

+         x-= (directionHor == -1 ? -1 : 0)*nsMouseMove.x;
+         y-= (directionVer == -1 ? -1 : 0)*nsMouseMove.y;
+         cx+= directionHor*nsMouseMove.x;
+         cy+= directionVer*nsMouseMove.y;
+         mLastPoint.x+= (directionHor == 1 ? 1 : 0)*nsMouseMove.x;
+         mLastPoint.y+= (directionVer == 1 ? 1 : 0)*nsMouseMove.y;

This logic is hard to understand. Can you rewrite it in a way that makes it obvious what the code is trying to do?
Attached patch v4 (obsolete) — Splinter Review
This patch uses FindAttrValueIn and GetDirection returns a struct.
I added a few comments to the manual move-and-resize code to make it more clearer.
I also added a few missing Atoms to nsGkAtomList.h.
I hope I got that struct-inside-class thing right according to the mozilla coding style.
Attachment #297955 - Attachment is obsolete: true
Attachment #298256 - Flags: superreview?(roc)
Attachment #298256 - Flags: review?(roc)
Attachment #297955 - Flags: superreview?(roc)
Attachment #297955 - Flags: review?(roc)
+nsResizerFrame::sDirection::sDirection(PRInt8 aHorizontal, PRInt8 aVertical)
+: mHorizontal(aHorizontal), mVertical (aVertical) {}

You don't actually need this constructor. In GetDirection, just use a {} initializer for each struct in 'directions'.

sDirection should just be Direction. Types are supposed to be leading Uppercase. Some enumerations are just an exception sorry :-(

+         nsResizerFrame::sDirection direction = GetDirection();

You don't need nsResizerFrame:: here

+         nsResizerFrame::sDirection direction = GetDirection();

Didn't you already get this above?

+         x-= (direction.mHorizontal == -1 ? -1 : 0)*nsMouseMove.x;
+         y-= (direction.mVertical == -1 ? -1 : 0)*nsMouseMove.y;
+         cx+= direction.mHorizontal*nsMouseMove.x;
+         cy+= direction.mVertical*nsMouseMove.y;

I think here it would be clearer to have a little helper function
  static void AdjustDimensions(PRInt32* aPos, PRInt32* aSize, PRInt32 aMovement, PRInt8 aResizerDirection)
which just does a switch on aResizerDirection.

+  static nsIContent::AttrValuesArray strings[] =
+  static nsResizerFrame::sDirection directions[] =

Make these const
Attached patch v5 (obsolete) — Splinter Review
>+         nsResizerFrame::sDirection direction = GetDirection();
>
> Didn't you already get this above?

Yes, once on mousedown and on every mousemove. That's what you said in comment #4

> static void AdjustDimensions(PRInt32* aPos, PRInt32* aSize, PRInt32
> aMovement, PRInt8 aResizerDirection)

The compiler complains about the static, otherwise I have updated the patch according to your comments.
Attachment #298256 - Attachment is obsolete: true
Attachment #298347 - Flags: superreview?(roc)
Attachment #298347 - Flags: review?(roc)
Attachment #298256 - Flags: superreview?(roc)
Attachment #298256 - Flags: review?(roc)
> Yes, once on mousedown and on every mousemove. That's what you said in comment
> #4

Oh, well just get it at the start of HandleEvent.

+  switch(aResizerDirection)
   {
-    aDir = topleft;
+    case -1: // only move the window when the direction is top and/or left
+      *aPos+= aMovement;
+    default:
+      *aSize+= aResizerDirection*aMovement;

It's more obvious to have cases -1, 1, default (does nothing)

Nearly done. Writing the simplest possible code is hard :-)
Attached patch v6 (obsolete) — Splinter Review
Ok, this patch fixes the last comments, and I also added back the static on AdjustDimensions. Looks like the compiler only complains about it in the .cpp but not in the .h, well I guess I still have a lot to learn about C++.
Thanks a lot for helping me this much with my first bug :-)
Attachment #298347 - Attachment is obsolete: true
Attachment #298430 - Flags: superreview?(roc)
Attachment #298430 - Flags: review?(roc)
Attachment #298347 - Flags: superreview?(roc)
Attachment #298347 - Flags: review?(roc)
Comment on attachment 298430 [details] [diff] [review]
v6

+      *aPos+= aMovement;
+    case 1:
+      *aSize+= aResizerDirection*aMovement;

Add a comment here to note that you're "falling through" --- that's unusual and usually means a bug, although it doesn't here.
Attachment #298430 - Flags: superreview?(roc)
Attachment #298430 - Flags: superreview+
Attachment #298430 - Flags: review?(roc)
Attachment #298430 - Flags: review+
Attached patch final patch (obsolete) — Splinter Review
Same patch with comment added, carrying over r+
This patch simply cleans up nsResizerFrame

Reward: less, clearer code

Risk: none, functionality stays the same
Attachment #298430 - Attachment is obsolete: true
Attachment #298550 - Flags: review+
Attachment #298550 - Flags: approval1.9?
(In reply to comment #11)
> Created an attachment (id=298550) [details]
> final patch
> 
> Same patch with comment added, carrying over r+
> This patch simply cleans up nsResizerFrame
> 
> Reward: less, clearer code
> 
> Risk: none, functionality stays the same
> 

Do we have any (or can we create) automated test coverage for this?
That sounds hard. I think this kind of cleanup patch can wait till after 1.9 to land, personally.
Comment on attachment 298550 [details] [diff] [review]
final patch

Based on comment 13 let's land this on hg-central instead
Attachment #298550 - Flags: approval1.9? → approval1.9-
We've been taking deCOM patches before without tests? How is this any different?
(In reply to comment #15)
> We've been taking deCOM patches before without tests? How is this any
> different?
> 

This is a good patch and there is quite a bit more than just changing return values here, there are no tests and we are getting closer and closer to 1.9 being final.

So your question is backwards, why do we need this change now in the release cycle.  Do we have any perf, regression, or other reason why it is worth taking a change in this area?  There will be a next release (the sooner we finish this one, the sooner we'll do the next) but we have to stop taking changes unless there is a direct benefit for them.   

What kind of tests would be needed for this bug? If tests can be satisfactorily written, why not do a little more deCOM for Fx3? Every little bit helps.
To Arpad - don't let the debate over timing overshadow your contribution here. 
We are excited to get this patch in and getting r+ from roc is a great thing. 
I'm just gatekeeping the release so we can get it done :-).

I haven't found any tests for this in the tree. If someone could point me in the right direction, especially how to test the fallback code that is only hit when the widget implementation doesn't provide a native resize drag, I could come up with some tests.
Is this patch ready to land on mozilla-central or are there still things left to do?
Attached patch updated patchSplinter Review
We need some tests for <resizer>, so I wrote some.

Good thing I did, since I found a bug in the patch. After we've resized the window we need to calculate mLastPoint more carefully because it's relative to the window, which has moved.

Anyway, it's ready to land now.
Attachment #298550 - Attachment is obsolete: true
Attachment #325211 - Flags: superreview+
Attachment #325211 - Flags: review+
Checked in, thanks!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Thanks again for your help roc.
No, thank you!

If you want to take on something more substantial, send me an email and I can help you.
You need to log in before you can comment on or make changes to this bug.