582865 - Provide a shared "shape" module for Point, Rect, etc.

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Ed Lee :Mardak]

From bug 582023 comment 3:

Fennec has Rect/Point classes that are very similar to the ones in this file
already:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/Util.js#310

Their code is in a separate repository, so sharing it directly isn't possible
at the moment, but perhaps it would be worth copying it over rather than having
a slightly different (new) implementation here? I think switching wouldn't
require many changes, and it would make it possible to move the code into a
shared location (toolkit) in the future.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Sounds OK to the Fennec team

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

I'm sorry for my ignorance, but how would we go about requesting to move this code into the toolkit?

Mark, it looks like Point and Rect are the only components of this Util.js that we would want to use. I assume moving just Point and Rect out into a shared location but leaving the rest alone would also be alright for you?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #2)

> Mark, it looks like Point and Rect are the only components of this Util.js that
> we would want to use. I assume moving just Point and Rect out into a shared
> location but leaving the rest alone would also be alright for you?

Sounds fine to me.

Comment 4

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

(In reply to comment #2)
> how would we go about requesting to move this code into the toolkit?

Attach a patch that adds a Geometry.jsm to toolkit/content that exports Point/Rect symbols using the Fennec code, and ask review from Dave Townsend? Once it's landed we can file a followup on making Fennec use it instead of its own copy - same for tabcandy.

Comment 5

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Move Geometry utility classes to the toolkit

Thanks Gavin. Here's the patch.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Move Geometry utility classes (+tests) to the toolkit

Also added the browser_rect.js test from Mobile as browser_Geometry.js

http://mxr.mozilla.org/mobile-browser/source/chrome/tests/browser_rect.js

Comment 7

[Dave Townsend [:mossop]]

What am I reviewing here, all the code in the file or just code that is moving from somewhere else?

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #7)
> What am I reviewing here, all the code in the file or just code that is moving
> from somewhere else?

This is just code moving from Mobile to toolkit. Changes made are the license code and the JSM-ing.

Comment 9

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

A diff against the source file might be useful for review.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: diff mobile-browser/source/chrome/content/Util.js tabcandy-central/toolkit/content/Geometry.jsm

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: diff mobile-browser/source/chrome/tests/browser_rects.js tabcandy-central/toolkit/content/tests/browser_Geometry.js

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #9)
> A diff against the source file might be useful for review.

Thanks Gavin. Diffs for both files attached.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 464399 [details] [diff] [review]
diff mobile-browser/source/chrome/content/Util.js tabcandy-central/toolkit/content/Geometry.jsm

Unified diffs are generally preferred but ok. So this is a copy of code from, tabcandy-central. Who wrote that code and who did the code review on it?

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #13)
> So this is a copy of code from,
> tabcandy-central. Who wrote that code and who did the code review on it?

The idea of this ticket is to move code which could be reused from mobile to the toolkit, as was suggested by gavin. I've simply gone ahead and committed it against our branch of mozilla-central, tabcandy-central, but I'm requesting the patch be applied to mozilla-central.

The code itself—the geometry classes in Util.js (now Geometry.jsm) and tests/browser_rect.js (now tests/browser_Geometry.js—were mostly written by Benjamin Stover and reviwed by Gavin, as per the hg log:

http://hg.mozilla.org/mobile-browser/log/1693b30538a3/chrome/content/Util.js
http://hg.mozilla.org/mobile-browser/log/1693b30538a3/chrome/tests/browser_rect.js

Comment 15

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

Comment on attachment 464075 [details] [diff] [review]
Move Geometry utility classes (+tests) to the toolkit

>diff --git a/toolkit/content/Geometry.jsm b/toolkit/content/Geometry.jsm

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.

We should probably take this opportunity to change this to "the Mozilla Foundation".

>+ * Contributor(s):
>+ *   Roy Frostig <[e-mail]>
>+ *   Ben Combee <[e-mail]>
>+ *   Matt Brubeck <[e-mail]>
>+ *   Michael Yoshitaka Erlewine <[e-mail]>

and it wouldn't hurt to add "Benjamin Stover <[e-mail]>" here.

Comment 16

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

Also don't forget to get the Fennec bug about using this filed :)

Comment 17

[Ed Lee :Mardak]

Attachment: v2

I'll land it on m-c when the tree is green.

Comment 18

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/cdfff833edf9

Comment 19

[Michael Yoshitaka Erlewine [:mitcho]]

Dev doc needed:

https://developer.mozilla.org/en/JavaScript_code_modules

Comment 20

[Eric Shepherd [:sheppy]]

Question:

  expandToIntegers: function round() {
    this.left = Math.floor(this.left);
    this.top = Math.floor(this.top);
    this.right = Math.ceil(this.right);
    this.bottom = Math.ceil(this.bottom);
    return this;
  },


Shouldn't the "round()" here be "expandToIntegers()"?

Comment 21

[Eric Shepherd [:sheppy]]

Using this JSM doesn't work well in desktop right now due to conflicts with like-named classes. I'm calling this documentation complete for now; we'll revisit when it can be more easily fiddled with on Desktop.