520910 - Refactor Rect and add Point class

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: Patch

This patch does some refactoring around the Rect class and adds a Point class.  There is no longer a wsRect and Util.Rect, rects can now be empty, and intersections and union code has been cleaned up.

Comment 1

[Benjamin Stover (:stechz)]

> I keep asking... do we need clone?

Yes.  Most of the functions modify the rect/point itself, and clone is nice for situations where we explicitly want a new object.

> I guess I don't like all these specialized methods, when the longer version is
not much longer and is more explicit. E.g. :

Should be addressed in this patch.  Let me know what you think.

> Why do we need add and subtract? Wouldn't translate be good enough?

Oops, I overlooked this one.  I slightly prefer point.subtract(other) to point.translate(-other.x, -other.y), but maybe it's not worth a convenience function.

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Bit rot

Comment 3

[Benjamin Stover (:stechz)]

Attachment: Missed a centerRounded()

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 406254 [details] [diff] [review]
Missed a centerRounded()

>diff -r 331a2cea3888 -r 12d58da8d3b5 chrome/content/BrowserView.js

>-      this.browserToViewportRect(r);
>-      r.round();
>+      this.browserToViewportRect(r).expandToIntegers();

It's not obvious that browserToViewportRect modifies and returns "r" - I'd write this as:

r = this.browserToViewportRect(r);
r.expandToIntegers();

(and I'd also rename browserToViewportRect, but don't need to worry about that now)

>diff -r 331a2cea3888 -r 12d58da8d3b5 chrome/content/Util.js

General style nit: can you put returns like these:

>+    if (this.isEmpty()) return new Point(NaN, NaN);

on their own lines? Helps with readability.

>+  boundingClientRect: function boundingClientRect(element) {
>+    return Rect.fromRect(element.getBoundingClientRect()).round();
>+  },

This seems to be unused? Also is broken because round() doesn't exist.

>+Point.prototype = {

>+  map: function map(f) {
>+    this.left = f.call(this.left, this.left);
>+    this.top = f.call(this.top, this.top);
>+    this.right = f.call(this.right, this.right);
>+    this.bottom = f.call(this.bottom, this.bottom);
>+    return this;
>+  },

strange choice of thisObj - wouldn't just "this" be better?

also, this seems to be copied from the rect class without changing the code to apply to .x/.y rather than .top/left/right/bottom

>+  scale: function scale(s) {
>+    this.x *= scale;
>+    this.y *= scale;
>+    return this;
>+  },

typo: s/scale/s/

>+(function() {
>+  function takePointOrArgs(f) {
>+    return function(arg1, arg2) {
>+      if (arg2 === undefined)

Make this check (arg1 instanceof Point) as well?

>+Rect.fromRect = function fromRect(r) {
>+  return new Rect(r.left, r.top, r.right - r.left, r.bottom - r.top);
>+}

Use r.width and r.height here? This comment applies in a few other places - are you intentionally avoiding the getters?

>   center: function center() {
>+    if (this.isEmpty()) return new Point(NaN, NaN);

This is a bit odd... can we just throw here instead?

>   expandBy: function(b) {
>+    if (this.isEmpty()) return this;
>+
>     this.left += b.left;
>     this.right += b.right;
>     this.top += b.top;

Not really related to this patch, but this function doesn't seem to be used, and doesn't seem to actually do what I would expect it to. Can we just remove it?

>   contains: function(other) {

>+    if (other.isEmpty()) return true;

This seems counter-intuitive... but I don't know whether it matters in practice.

>+  intersects: function(r2) {
>+  restrictTo: function restrictTo(r2) {

Seems like we should be consistent with "other" or "r2" as the parameter name.

Would be good to get tests for all of the functionality in these classes, but could you write browser chrome tests for intersects() and restrictTo(), at the very least? I haven't taken the time to verify that they're equivalent to the old versions (or correct).

>+  map: function map(f) {
>+    this.left = f(this.left);
>+    this.top = f(this.top);
>+    this.right = f(this.right);
>+    this.bottom = f(this.bottom);

Seems like you should make this use apply() with the right thisObj, as with Point()'s version.

>diff -r 331a2cea3888 -r 12d58da8d3b5 chrome/content/browser.js

>-    // XXX computeSideBarVisibility will normally return 0.0015... for ritevis
>-    if (leftvis > 0.002 || ritevis > 0.002) {
>+    if (leftvis > 0 || ritevis > 0) {

Why did this stop happening?

>   dragMove: function dragMove(dx, dy, scroller) {

>-    this._panScroller(Browser.controlsScrollboxScroller, [doffset[0] + offsetX, 0]);
>+    doffset.x += panOffset;
>+    Browser.tryFloatToolbar(doffset.x, 0);
>+    this._panScroller(Browser.controlsScrollboxScroller, doffset);

Can you explain these changes? i.e. why you're now doing the tryFloatToolbar separately, and why you're using doffset.y rather than 0 for the call to _panScroller?

Comment 5

[Benjamin Stover (:stechz)]

> This seems to be unused? Also is broken because round() doesn't exist.

OK, removed.

> Make this check (arg1 instanceof Point) as well?

Is this for readability reasons?  The extra check seems equivalent to me (as in, if arg2 isn't given then arg1 should be a Point).

> Use r.width and r.height here? This comment applies in a few other places - are
> you intentionally avoiding the getters?

Yes, due to tracer issues.  I'm not sure if these are used in any critical trace paths, but I didn't see a downside.

> >   contains: function(other) {
> >+    if (other.isEmpty()) return true;
> This seems counter-intuitive... but I don't know whether it matters in
> practice.

I was borrowing from set theory.  ∀A ∅ ⊆ A because for every x in ∅, x is also in A by vacuous truth (since x in ∅ is always false, the implication is true).  It probably doesn't matter in practice.

>-    if (leftvis > 0.002 || ritevis > 0.002) {
>+    if (leftvis > 0 || ritevis > 0) {
> Why did this stop happening?

It had been fixed before this patch, by calling Math.round on bounding boxes here:
> let ritebar = new Rect(Math.round(ritebarCBR.left) - dx, 0, Math.round(ritebarCBR.width), 1);

Now when the visibility overlaps are calculated, subpixel overlaps no longer occur.

> >-    this._panScroller(Browser.controlsScrollboxScroller, [doffset[0] + offsetX, 0]);
> >+    doffset.x += panOffset;
> >+    Browser.tryFloatToolbar(doffset.x, 0);
> >+    this._panScroller(Browser.controlsScrollboxScroller, doffset);
> 
> Can you explain these changes? i.e. why you're now doing the tryFloatToolbar
> separately, and why you're using doffset.y rather than 0 for the call to
> _panScroller?

I was hoping to already have the toolbar floated before the pan, so that there isn't a "jump" at the top of the page.

Passing doffset.y is the same as passing 0 since the controls scrollbox cannot pan that way.  I thought it didn't hurt readability and it saves an object creation (this can be changed if it feels less clear).

Everything else I agree with (including some rect tests).  I'll submit a new patch.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> > Make this check (arg1 instanceof Point) as well?
> 
> Is this for readability reasons?  The extra check seems equivalent to me (as
> in, if arg2 isn't given then arg1 should be a Point).

Well, it was so that things don't go wonky if someone just passes in a single non-Point argument, but I suppose that would just make it go differently-wonky. I'm not sure whether it's worth doing argument validation for these...

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Code review, added tests

I didn't add too many tests, but it's a start!

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 406362 [details] [diff] [review]
Code review, added tests

>diff --git a/chrome/content/Util.js b/chrome/content/Util.js

>+  map: function map(f) {
>+    this.x = f(this.x);

would prefer f.apply(this, this.x); (here and in the other map())

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Pushed with that change:
https://hg.mozilla.org/mobile-browser/rev/7b010ad96dd7