Rect.contains should be inclusive

RESOLVED FIXED in Firefox 4.0b12

Status

defect
P4
normal
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: iangilman, Assigned: mitcho)

Tracking

unspecified
Firefox 4.0b12

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

8 years ago
Blocks: 627096
Priority: P3 → P4
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.
Ian, I assume Rect_intersects can stay as it is?
Created trivial patch, no changes to tests (all passed locally without tweaking), so sent to try.
(Reporter)

Comment 4

8 years ago
(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?
Posted patch Trivial patch (obsolete) — Splinter Review
Passed try.
Assignee: nobody → mitcho
Status: NEW → ASSIGNED
Attachment #508957 - Flags: review?(ian)
(Reporter)

Comment 6

8 years ago
Comment on attachment 508957 [details] [diff] [review]
Trivial patch

Thanks!
Attachment #508957 - Flags: review?(ian) → review+
Attachment #508957 - Flags: approval2.0?
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.
Attachment #508957 - Flags: approval2.0? → approval2.0-
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.
Posted patch Patch with test (obsolete) — Splinter Review
Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d462db8c443a
Attachment #508957 - Attachment is obsolete: true
Attachment #509984 - Flags: approval2.0?
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).
Attachment #509984 - Flags: approval2.0? → approval2.0+
Attachment #509984 - Attachment is obsolete: true
Keywords: checkin-needed

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/6f4c4e84943d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
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.
(Reporter)

Comment 15

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

Comment 17

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

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

s/st/ag/

:-)
(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.
LOL. Also, being mistaken for jst is quite the honor!
Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.