Move Point and Rect over to dart:math, to be re-exported by dart:html.

Move Point and Rect over to dart:math, to be re-exported by dart:html.

BUG=
R=floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=28184

[Emily Fortuna, 2013-10-01 23:12:21]

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart
File sdk/lib/math/point.dart (right):

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:63: Point<T> ceil() => new Point<T>(x.ceil(), y.ceil());
One could argue that these should always return Point<int>.  But if you have a
Point<double> and you want to call ceil() on it, I'm guessing you still want it
to be able to be added to other Point<double>s, otherwise, you can call
truncate() to get a proper int. I can be persuaded otherwise, though.

[floitsch, 2013-10-02 08:54:09]

LGTM with comments and a few nits.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/jenkins_smi_h...
File sdk/lib/math/jenkins_smi_hash.dart (right):

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/jenkins_smi_h...
sdk/lib/math/jenkins_smi_hash.dart:23: // TODO: Bug 11617- This class should be
optimized and standardized elsewhere.
Please rewrite to: TODO(11617)

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart
File sdk/lib/math/point.dart (right):

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:13: const Point([T x = 0, T y = 0]): x = x, y = y;
*minor* nit: this.x = x, this.y = y;

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:32: Point<T> operator *(T factor) {
that one is hard.
There is no reason not to write Point<double> * 2

being pedantic: this is not really a point operation, but a vector operation.
And point - point should then rather return a vector.

But it is really annoying not to have a Point class, but also annoying to have a
Vector and Point class...

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:43: * Returns the distance between two points.
`this` and [other].

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:52: * Returns the squared distance between two points.
ditto.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:63: Point<T> ceil() => new Point<T>(x.ceil(), y.ceil());
On 2013/10/01 23:12:21, Emily Fortuna wrote:
> One could argue that these should always return Point<int>.  But if you have a
> Point<double> and you want to call ceil() on it, I'm guessing you still want
it
> to be able to be added to other Point<double>s, otherwise, you can call
> truncate() to get a proper int. I can be persuaded otherwise, though.

Are these functions really needed?

I think you need to make it Point<int>, since ceil always returns an int. Afaics
there is no way to convert a number to "T".

The current code doesn't even work since ceil returns an int. (Add a test: new
Point<double>(1.0, 2.0).ceil() )

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dart
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:10: // Not used, but keeps the VM from complaining
about Rectangle having a const
Remove comment. As a base class the const constructor is used from sub-classes.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:15: * The number of units `left` of the origin where
this rectangle's top left
The x-coordinate of the upper-left corner.
or
The x-coordinate of the left edge.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:20: * The number of units from the `top` origin
point where this rectangle's top
The y-coordinate of the upper left corner.
or
The y-coordinate of the top edge.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:41: return '($left, $top, $width, $height)';
We are using "( ... )" to write iterables now.
I'm ok with the notation, but would prefer another one.
Also: when reading (1, 2, 3, 4) I would never guess, that 3 and 4 are the width
and height.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:54: * Computes the intersection of this rectangle
and the rectangle parameter.
`this` and [other].

I tend to avoid repeating the type of `this`, so that the documentation works
also for sub-classes.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:55: * Returns null if there is no intersection.
New line before.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:74: * Returns whether a rectangle intersects this
rectangle.
Returns `true` if `this` intersects (with?) [other].

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:82: * Returns a new rectangle which completely
contains this rectangle and the
ditto (`this` [other])

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:106: * Tests whether this rectangle entirely
contains a point.
-entirely-

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:115: Rectangle<T> ceil() => new
Rectangle<T>(left.ceil(), top.ceil(), width.ceil(),
same as for Point: This must return a Rectangle<int>.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:134: var tempLeft;
Remove "temp" prefixes in the function.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:160: * This class is distinctive from RectangleBase
in that it enforces that its
This sentence isn't true: The RectangleBase only exposes getters. So this class
has the same interface as the Base.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:181: class MutableRectangle<T>  extends
RectangleBase<T> implements Rectangle<T> {
Unless used and needed, I would prefer not to have this class.

https://codereview.chromium.org/25623002/diff/9001/tests/lib/math/rectangle_t...
File tests/lib/math/rectangle_test.dart (right):

https://codereview.chromium.org/25623002/diff/9001/tests/lib/math/rectangle_t...
tests/lib/math/rectangle_test.dart:12: return a != null ? new Rectangle(a[0],
a[1], a[2] - a[0], a[3] - a[1]) : null;
long line.

https://codereview.chromium.org/25623002/diff/9001/tools/dom/templates/html/d...
File tools/dom/templates/html/dart2js/html_dart2js.darttemplate (right):

https://codereview.chromium.org/25623002/diff/9001/tools/dom/templates/html/d...
tools/dom/templates/html/dart2js/html_dart2js.darttemplate:59: export
'dart:math' show Rectangle, RectangleBase, Point;
Seems strange that we need to expose RectangleBase in dart:html.

[kevmoo-old, 2013-10-02 14:31:47]

DBR - but a very serious one, IMHO

https://codereview.chromium.org/25623002/diff/9001/tools/dom/templates/html/d...
File tools/dom/templates/html/dart2js/html_dart2js.darttemplate (right):

https://codereview.chromium.org/25623002/diff/9001/tools/dom/templates/html/d...
tools/dom/templates/html/dart2js/html_dart2js.darttemplate:59: export
'dart:math' show Rectangle, RectangleBase, Point;
On 2013/10/02 08:54:09, floitsch wrote:
> Seems strange that we need to expose RectangleBase in dart:html.

Exporting a subset of dart:math concerns me, given
https://code.google.com/p/dart/issues/detail?id=9516

It's common for me to import dart:math at the top level. Won't this cause
conflicts?
Also, if I have libraries that use Rectangle from dart:math, will those be
compatible with the exported version from dart:html? My understanding: the
runtime will treat them as incompatible types from different libraries.

[Emily Fortuna, 2013-10-02 18:32:39]

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/jenkins_smi_h...
File sdk/lib/math/jenkins_smi_hash.dart (right):

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/jenkins_smi_h...
sdk/lib/math/jenkins_smi_hash.dart:23: // TODO: Bug 11617- This class should be
optimized and standardized elsewhere.
On 2013/10/02 08:54:09, floitsch wrote:
> Please rewrite to: TODO(11617)

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart
File sdk/lib/math/point.dart (right):

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:13: const Point([T x = 0, T y = 0]): x = x, y = y;
On 2013/10/02 08:54:09, floitsch wrote:
> *minor* nit: this.x = x, this.y = y;

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:32: Point<T> operator *(T factor) {
On 2013/10/02 08:54:09, floitsch wrote:
> that one is hard.
> There is no reason not to write Point<double> * 2
> 
> being pedantic: this is not really a point operation, but a vector operation.
> And point - point should then rather return a vector.
> 
> But it is really annoying not to have a Point class, but also annoying to have
a
> Vector and Point class...

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:43: * Returns the distance between two points.
On 2013/10/02 08:54:09, floitsch wrote:
> `this` and [other].

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:52: * Returns the squared distance between two points.
On 2013/10/02 08:54:09, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:63: Point<T> ceil() => new Point<T>(x.ceil(), y.ceil());
On 2013/10/02 08:54:09, floitsch wrote:
> On 2013/10/01 23:12:21, Emily Fortuna wrote:
> > One could argue that these should always return Point<int>.  But if you have
a
> > Point<double> and you want to call ceil() on it, I'm guessing you still want
> it
> > to be able to be added to other Point<double>s, otherwise, you can call
> > truncate() to get a proper int. I can be persuaded otherwise, though.
> 
> Are these functions really needed?
> 
> I think you need to make it Point<int>, since ceil always returns an int.
Afaics
> there is no way to convert a number to "T".
> 
> The current code doesn't even work since ceil returns an int. (Add a test: new
> Point<double>(1.0, 2.0).ceil() )

Done (converted them to int).

These functions were just in the html version of the class, but I have no qualms
deleting them. Remove?

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dart
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:10: // Not used, but keeps the VM from complaining
about Rectangle having a const
On 2013/10/02 08:54:09, floitsch wrote:
> Remove comment. As a base class the const constructor is used from
sub-classes.

Oops, that was left over from when I was using this class as a mixin.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:15: * The number of units `left` of the origin where
this rectangle's top left
On 2013/10/02 08:54:09, floitsch wrote:
> The x-coordinate of the upper-left corner.
> or
> The x-coordinate of the left edge.

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:20: * The number of units from the `top` origin
point where this rectangle's top
On 2013/10/02 08:54:09, floitsch wrote:
> The y-coordinate of the upper left corner.
> or
> The y-coordinate of the top edge.

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:41: return '($left, $top, $width, $height)';
On 2013/10/02 08:54:09, floitsch wrote:
> We are using "( ... )" to write iterables now.
> I'm ok with the notation, but would prefer another one.
> Also: when reading (1, 2, 3, 4) I would never guess, that 3 and 4 are the
width
> and height.

Adjusted. Let me know if you'd like further change.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:54: * Computes the intersection of this rectangle
and the rectangle parameter.
On 2013/10/02 08:54:09, floitsch wrote:
> `this` and [other].
> 
> I tend to avoid repeating the type of `this`, so that the documentation works
> also for sub-classes.

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:55: * Returns null if there is no intersection.
On 2013/10/02 08:54:09, floitsch wrote:
> New line before.

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:74: * Returns whether a rectangle intersects this
rectangle.
On 2013/10/02 08:54:09, floitsch wrote:
> Returns `true` if `this` intersects (with?) [other].

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:82: * Returns a new rectangle which completely
contains this rectangle and the
On 2013/10/02 08:54:09, floitsch wrote:
> ditto (`this` [other])

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:115: Rectangle<T> ceil() => new
Rectangle<T>(left.ceil(), top.ceil(), width.ceil(),
On 2013/10/02 08:54:09, floitsch wrote:
> same as for Point: This must return a Rectangle<int>.

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:134: var tempLeft;
On 2013/10/02 08:54:09, floitsch wrote:
> Remove "temp" prefixes in the function.

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:160: * This class is distinctive from RectangleBase
in that it enforces that its
On 2013/10/02 08:54:09, floitsch wrote:
> This sentence isn't true: The RectangleBase only exposes getters. So this
class
> has the same interface as the Base.

Done.

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:181: class MutableRectangle<T>  extends
RectangleBase<T> implements Rectangle<T> {
On 2013/10/02 08:54:09, floitsch wrote:
> Unless used and needed, I would prefer not to have this class.

Removed.

https://codereview.chromium.org/25623002/diff/9001/tests/lib/math/rectangle_t...
File tests/lib/math/rectangle_test.dart (right):

https://codereview.chromium.org/25623002/diff/9001/tests/lib/math/rectangle_t...
tests/lib/math/rectangle_test.dart:12: return a != null ? new Rectangle(a[0],
a[1], a[2] - a[0], a[3] - a[1]) : null;
On 2013/10/02 08:54:09, floitsch wrote:
> long line.

Done.

https://codereview.chromium.org/25623002/diff/9001/tools/dom/templates/html/d...
File tools/dom/templates/html/dart2js/html_dart2js.darttemplate (right):

https://codereview.chromium.org/25623002/diff/9001/tools/dom/templates/html/d...
tools/dom/templates/html/dart2js/html_dart2js.darttemplate:59: export
'dart:math' show Rectangle, RectangleBase, Point;
On 2013/10/02 14:31:47, kevmoo wrote:
> On 2013/10/02 08:54:09, floitsch wrote:
> > Seems strange that we need to expose RectangleBase in dart:html.
> 
> Exporting a subset of dart:math concerns me, given
> https://code.google.com/p/dart/issues/detail?id=9516
> 
> It's common for me to import dart:math at the top level. Won't this cause
> conflicts?
> Also, if I have libraries that use Rectangle from dart:math, will those be
> compatible with the exported version from dart:html? My understanding: the
> runtime will treat them as incompatible types from different libraries.

Sorry Florian -- that was also left over from my mixin version of this CL.
Removed!

[Emily Fortuna, 2013-10-02 21:54:19]

Committed patchset #2 manually as r28184 (presubmit successful).

[Ivan Posva, 2013-10-08 17:28:43]

Delayed DBCs.

-Ivan

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dart
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:9: abstract class RectangleBase<T extends num> {
Why is this RectangleBase class exported? Do you expect other subclasses?

https://codereview.chromium.org/25623002/diff/54001/tools/dom/templates/html/...
File tools/dom/templates/html/dartium/html_dartium.darttemplate (right):

https://codereview.chromium.org/25623002/diff/54001/tools/dom/templates/html/...
tools/dom/templates/html/dartium/html_dartium.darttemplate:53: export
'dart:math' show Rectangle, RectangleBase, Point;
Why did RectangleBase survive here, but not in the dart2js template?

[Emily Fortuna, 2013-10-08 17:53:32]

On 2013/10/08 17:28:43, Ivan Posva wrote:
> Delayed DBCs.
> 
> -Ivan
> 
> https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dart
> File sdk/lib/math/rectangle.dart (right):
> 
>
https://codereview.chromium.org/25623002/diff/9001/sdk/lib/math/rectangle.dar...
> sdk/lib/math/rectangle.dart:9: abstract class RectangleBase<T extends num> {
> Why is this RectangleBase class exported? Do you expect other subclasses?
> 
>
https://codereview.chromium.org/25623002/diff/54001/tools/dom/templates/html/...
> File tools/dom/templates/html/dartium/html_dartium.darttemplate (right):
> 
>
https://codereview.chromium.org/25623002/diff/54001/tools/dom/templates/html/...
> tools/dom/templates/html/dartium/html_dartium.darttemplate:53: export
> 'dart:math' show Rectangle, RectangleBase, Point;
> Why did RectangleBase survive here, but not in the dart2js template?

Hi Ivan, This version of the CL ended up getting rolled back. Here's the latest
version that actually ended up getting committed and I believe addresses your
questions: https://codereview.chromium.org//25808002 I'm removing
"RectangleBase" from the dartium darttemplate in a separate CL.