407616 - clean up nsResizerFrame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

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.

Comment 1

[Arpad Borsos [:Swatinem]]

Attachment: v1

This patch basically does what roc said it should do.

Comment 2

[Arpad Borsos [:Swatinem]]

Attachment: v2

This patch uses a static var instead of calling GetDirection on every mouse move.

Comment 3

[Arpad Borsos [:Swatinem]]

Attachment: v3

This patch should detect the default direction correctly.

Comment 4

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

> 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?

Comment 5

[Arpad Borsos [:Swatinem]]

Attachment: v4

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.

Comment 6

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

+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

Comment 7

[Arpad Borsos [:Swatinem]]

Attachment: v5

>+         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.

Comment 8

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

> 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 :-)

Comment 9

[Arpad Borsos [:Swatinem]]

Attachment: v6

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 :-)

Comment 10

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

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.

Comment 11

[Arpad Borsos [:Swatinem]]

Attachment: 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

Comment 12

[Mike Schroepfer]

(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?

Comment 13

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

That sounds hard. I think this kind of cleanup patch can wait till after 1.9 to land, personally.

Comment 14

[Mike Schroepfer]

Comment on attachment 298550 [details] [diff] [review]
final patch

Based on comment 13 let's land this on hg-central instead

Comment 15

[Reed Loden [:reed]]

We've been taking deCOM patches before without tests? How is this any different?

Comment 16

[Mike Schroepfer]

(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.   

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 18

[Mike Schroepfer]

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 :-).

Comment 19

[Arpad Borsos [:Swatinem]]

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.

Comment 20

[Arpad Borsos [:Swatinem]]

Is this patch ready to land on mozilla-central or are there still things left to do?

Comment 21

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

Attachment: updated patch

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.

Comment 22

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

Checked in, thanks!

Comment 23

[Arpad Borsos [:Swatinem]]

Thanks again for your help roc.

Comment 24

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

No, thank you!

If you want to take on something more substantial, send me an email and I can help you.