629189 - Rect.contains should be inclusive

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Ian Gilman [:iangilman]]

I was reminded today that our Rect.contains looks like so:

  contains: function Rect_contains(a) {
    return (a.left > this.left &&
            a.right < this.right &&
            a.top > this.top &&
            a.bottom < this.bottom);
  },

... whereas it should look like so:

  contains: function Rect_contains(a) {
    return (a.left >= this.left &&
            a.right <= this.right &&
            a.top >= this.top &&
            a.bottom <= this.bottom);
  },

(I believe this is all my fault; a misunderstanding)

It should be an easy enough fix... just have to make sure it doesn't have any unwanted repercussions.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

It's possible that this will just barely cause one of the dragging/drop/snapping tests to fail, but tweaking the rect size of whatever group or tab we're creating should fix it.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Ian, I assume Rect_intersects can stay as it is?

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Created trivial patch, no changes to tests (all passed locally without tweaking), so sent to try.

Comment 4

[Ian Gilman [:iangilman]]

(In reply to comment #2)
> Ian, I assume Rect_intersects can stay as it is?

Yes please; it's good how it is. 

(In reply to comment #3)
> Created trivial patch, no changes to tests (all passed locally without
> tweaking), so sent to try.

Attach the patch?

Comment 5

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Trivial patch

Passed try.

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 508957 [details] [diff] [review]
Trivial patch

Thanks!

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 508957 [details] [diff] [review]
Trivial patch

Not going to approve this without a test.  The expected behavior was clearly not tested before, so let's get one added.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

The corresponding method in Toolkit's Geometry.jsm (which we hope to move to in the post-fx4-soon future) also lacks tests for this method: filed bug 631757.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch with test

Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d462db8c443a

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

All passed, modulo two platforms with known intermittent oranges, and one platform whose tests failed prematurely due to a known intermittent orange. Getting that one platform rerun (bug 631867).

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Passed try. Here's the log for osx debug whose mochitest was rerun:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d462db8c443a/tryserver-macosx-debug/tryserver_leopard-o-debug_test-mochitest-other-build642.txt.gz

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6f4c4e84943d

Comment 14

[jag (Peter Annema)]

Comment on attachment 510485 [details] [diff] [review]
Patch for checkin


>+  // Returns a boolean denoting if the given <Rect> is contained within
>+  // the this rectangle.

// this rectangle.


>+  let {Rect} = Components.utils.import("resource:///modules/tabview/utils.jsm", {});
>+  
>+  let referenceRect = new Rect(50,50,150,150);
>+  let rect = new Rect(100,100,100,100);
>+  
>+  ok(referenceRect.contains(referenceRect), "A rect contains itself");
>+  ok(referenceRect.contains(rect), "[50,50,150,150] contains [100,100,99,99]");

Shouldn't this say [100,100,100,100]?

I'm commenting here since neither change is a change in functionality and can hopefully be done as a typo-fix style follow-up checkin.

Comment 15

[Ian Gilman [:iangilman]]

Mitcho, do you want to address the items highlighted in comment 14?

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Followup patch with comment and test message changes

Thanks jst.

Comment 17

[(no longer active)]

Pushed the follow-up: http://hg.mozilla.org/mozilla-central/rev/4a6ba480150a

(In reply to comment #16)
> Thanks jst.

s/st/ag/

:-)

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #17)
> (In reply to comment #16)
> > Thanks jst.
> 
> s/st/ag/
> 
> :-)

Sorry... hope I don't have to file another typo patch for that too.

Comment 19

[jag (Peter Annema)]

LOL. Also, being mistaken for jst is quite the honor!