Implement measurement using futures

Implement measurement using promises

BUG=
TEST=

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

[Jacob, 2011-10-27 02:12:38]

Almost all DOM measurement is now performed Async using futures.  All existing
samples are updated to use the new API.

[arv (Not doing code reviews), 2011-10-27 03:16:13]

I'm off to dinner. I'll continue when I come back.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/DocumentFrag...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:137: ClientRect get
client() => new SimpleClientRect(0, 0, 0, 0);
Does this still work?

const SimpleClientRect(0, 0, 0, 0)

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:472: List<ClientRect> get
clientRects() {
Maybe just an iterable instead?

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:483: static final
_START_TAG_REGEXP = const RegExp('<(\\w+)');
Can this be moved to a different patch?

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:509: final temp =
dom.document.createElement(parentTag);
This code is really great. A big improvement over the jQuery code. Good thing we
don't need to support old IE versions where innerHTML does not work on some of
these.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:516: // Work around for edge
case where both body and head
Which browsers? Can we link to bugs here?

I think the following would be clearer/better:

if (parentTag == 'html' && temp.childElementCount == 2)
  return LevelDom.wrapElement(temp.children.item(tag == 'head' ? 0 : 1));

http://codereview.chromium.org/8363040/diff/2012/client/html/src/EventWrappin...
File client/html/src/EventWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/html/src/EventWrappin...
client/html/src/EventWrappingImplementation.dart:9: bool _stoppedPropagation =
false;
What is this for?

[arv (Not doing code reviews), 2011-10-27 05:50:24]

This turned out better than I expected. I was very worried that the Futures
would make the code too hard to follow.

My main concern with this CL is that it contains unrelated awesomeness.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
File client/html/src/Measurement.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
client/html/src/Measurement.dart:22: if (_nextMeasurementFrameScheduled ==
false) {
if (!_nextMeasurementFrameScheduled) {

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
client/html/src/Measurement.dart:27: // Messages from other windows do not cause
a security risk as
as as

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
client/html/src/Measurement.dart:31: window.on.message.add(Isolate.bind(
If anyone else is listening to on message they will get this. I wonder if there
is an alternative that does not expose this message?

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
client/html/src/Measurement.dart:53: Future
_createMeasurementFuture(ComputeValue computeValue, Completer completer) {
long line

http://codereview.chromium.org/8363040/diff/2012/client/samples/swarm/App.dart
File client/samples/swarm/App.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/samples/swarm/App.dar...
client/samples/swarm/App.dart:28: window.setTimeout(() => onLoad(), 0);
this could use a comment

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Dom...
File client/samples/total/src/DomUtils.dart (left):

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Dom...
client/samples/total/src/DomUtils.dart:21: while (curr.offsetParent != null) {
It is good that we can rid of this buggy code.

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Inn...
File client/samples/total/src/InnerMenuView.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Inn...
client/samples/total/src/InnerMenuView.dart:52: window.requestLayoutFrame(() {
Is there nothing like Q's join?

https://github.com/kriskowal/q

var Q = require("q");
var aPromise = aFunction();
var bPromise = bFunction();
Q.join(aPromise, bPromise, function (aValue, bValue) {
    return cValue;
});

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Sel...
File client/samples/total/src/SelectionManager.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Sel...
client/samples/total/src/SelectionManager.dart:152: // We need bounding box
relative to the container which will be offset by
by what?

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Sel...
client/samples/total/src/SelectionManager.dart:162: (minP.left -
orgP.left).toInt(),
it is sad that we do toInt() here when we for once have an API that correctly
uses double.

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Spr...
File client/samples/total/src/SpreadsheetPresenter.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Spr...
client/samples/total/src/SpreadsheetPresenter.dart:685: int x = startX + e.x -
mouseStartX;
e.x? We should kill that property... in another CL

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Spr...
client/samples/total/src/SpreadsheetPresenter.dart:893:
divStyle.setProperty("left", HtmlUtils.toPx(box.left));
Is there a reason why we don't do divStyle.left = ...?

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Spr...
client/samples/total/src/SpreadsheetPresenter.dart:1691:
_tableScrollContainer.style.setProperty("width", HtmlUtils.toPx(rect.width +
10));
long lines

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/html/Doc...
File client/tests/client/html/DocumentFragmentTests.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/html/Doc...
client/tests/client/html/DocumentFragmentTests.dart:320: // TODO(nweiz):
re-enable when const is better supported in dartc and/or frog
and/or frog :-)

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/html/Ele...
File client/tests/client/html/ElementTests.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/html/Ele...
client/tests/client/html/ElementTests.dart:69: (element) => element is
AnchorElement);
can't you just pass AnchorElement instead of a function?

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/layout/G...
File client/tests/client/layout/GridLayoutDemo.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/layout/G...
client/tests/client/layout/GridLayoutDemo.dart:113: window.requestLayoutFrame(()
{
This is the first case where this is feeling ugly.

It might actually be cleaner if a Future<List<Rect>> was used here.

http://codereview.chromium.org/8363040/diff/2012/client/touch/Scroller.dart
File client/touch/Scroller.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/touch/Scroller.dart#n...
client/touch/Scroller.dart:547: void _resize(Callback callback) {
Maybe this should just return a future instead?

http://codereview.chromium.org/8363040/diff/2012/client/view/view.dart
File client/view/view.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/view/view.dart#newcod...
client/view/view.dart:327: Future<bool> _measureLayout() {
Yeah, this is cleaner than using a Callback like in the other file.

[Jacob, 2011-10-27 20:59:25]

Replied to all comments and uploaded a new version of the CL

http://codereview.chromium.org/8363040/diff/2012/client/html/src/DocumentFrag...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:137: ClientRect get
client() => new SimpleClientRect(0, 0, 0, 0);
On 2011/10/27 03:16:13, arv wrote:
> Does this still work?
> 
> const SimpleClientRect(0, 0, 0, 0)

Done.  Switched these all to fields and took advantage of const.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:472: List<ClientRect> get
clientRects() {
On 2011/10/27 03:16:13, arv wrote:
> Maybe just an iterable instead?

Any reason why this case should just be iterable while we've used list for other
cases such as NodeList, ElementList, etc?

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:483: static final
_START_TAG_REGEXP = const RegExp('<(\\w+)');
On 2011/10/27 03:16:13, arv wrote:
> Can this be moved to a different patch?

I agree this bug fix is unrelated but at this early stage, I'd prefer to play
slightly loose and not worry about splitting it into a separate CL.
The reason it is with this CL is that I want to land some relatively complete
tests for custom Element behavior as part of this CL and to achieve that I need
to fix the obvious bugs in the current behavior.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:509: final temp =
dom.document.createElement(parentTag);
On 2011/10/27 03:16:13, arv wrote:
> This code is really great. A big improvement over the jQuery code. Good thing
we
> don't need to support old IE versions where innerHTML does not work on some of
> these.

I think this can be improved further. Added a TODO noting what we should do to
further improve robustness and performance.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:516: // Work around for edge
case where both body and head
On 2011/10/27 03:16:13, arv wrote:
> Which browsers? Can we link to bugs here?
> 
> I think the following would be clearer/better:
> 
> if (parentTag == 'html' && temp.childElementCount == 2)
>   return LevelDom.wrapElement(temp.children.item(tag == 'head' ? 0 : 1));

The issue occurs in WebKit. I agree that your code is clearer and less likely to
work for the wrong reasons than my lazy solution of using a querySelector. Done.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/EventWrappin...
File client/html/src/EventWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/html/src/EventWrappin...
client/html/src/EventWrappingImplementation.dart:9: bool _stoppedPropagation =
false;
On 2011/10/27 03:16:13, arv wrote:
> What is this for?

Good catch. This is obsolete. I was trying to manually track when propagation
was aborted before I learned that postMessage would do what I wanted.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
File client/html/src/Measurement.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
client/html/src/Measurement.dart:22: if (_nextMeasurementFrameScheduled ==
false) {
On 2011/10/27 05:50:24, arv wrote:
> if (!_nextMeasurementFrameScheduled) {

Done.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
client/html/src/Measurement.dart:27: // Messages from other windows do not cause
a security risk as
On 2011/10/27 05:50:24, arv wrote:
> as as
as as ==> as all
done done

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
client/html/src/Measurement.dart:31: window.on.message.add(Isolate.bind(
On 2011/10/27 05:50:24, arv wrote:
> If anyone else is listening to on message they will get this. I wonder if
there
> is an alternative that does not expose this message? 

Do you have any ideas? Creating an iframe just for this seems like overkill and
I can't think of any other ideas.
I'm not too worried as pretty much everyone listening for post messages needs to
be examining the message data to determine whether they should handle the
message or ignore it.

http://codereview.chromium.org/8363040/diff/2012/client/html/src/Measurement....
client/html/src/Measurement.dart:53: Future
_createMeasurementFuture(ComputeValue computeValue, Completer completer) {
On 2011/10/27 05:50:24, arv wrote:
> long line

Done.

http://codereview.chromium.org/8363040/diff/2012/client/samples/swarm/App.dart
File client/samples/swarm/App.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/samples/swarm/App.dar...
client/samples/swarm/App.dart:28: window.setTimeout(() => onLoad(), 0);
On 2011/10/27 05:50:24, arv wrote:
> this could use a comment

Done.

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Dom...
File client/samples/total/src/DomUtils.dart (left):

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Dom...
client/samples/total/src/DomUtils.dart:21: while (curr.offsetParent != null) {
On 2011/10/27 05:50:24, arv wrote:
> It is good that we can rid of this buggy code.
Agreed. This code wasn't used and would be very painful to port over to use
async measurement even ignoring cross-browser bugs.

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Inn...
File client/samples/total/src/InnerMenuView.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Inn...
client/samples/total/src/InnerMenuView.dart:52: window.requestLayoutFrame(() {
On 2011/10/27 05:50:24, arv wrote:
> Is there nothing like Q's join?
> 
> https://github.com/kriskowal/q
> 
> var Q = require("q");
> var aPromise = aFunction();
> var bPromise = bFunction();
> Q.join(aPromise, bPromise, function (aValue, bValue) {
>     return cValue;
> });
there isn't anything yet but it would be easy enough to add. When I started out
I was considering that sort of design and used it for some of the list view and
scrolling code i ported.  then things got painful for the cases where N promises
needed to be joined and so I started using requestLayoutFrame instead which
leverages the fact that we know all the layout promises will be evaluated at
once so we don't need to explicitly join and wait for all of them separately.
That said, the name requestLayoutFrame is somewhat confusing as it is really
waitForLayoutToComplete.  If you have a better name or a better concept that
will meet these use cases, let me know.

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Sel...
File client/samples/total/src/SelectionManager.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Sel...
client/samples/total/src/SelectionManager.dart:152: // We need bounding box
relative to the container which will be offset by
On 2011/10/27 05:50:24, arv wrote:
> by what?
Oops. Somehow I accidentally removed the word css.

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Sel...
client/samples/total/src/SelectionManager.dart:162: (minP.left -
orgP.left).toInt(),
On 2011/10/27 05:50:24, arv wrote:
> it is sad that we do toInt() here when we for once have an API that correctly
> uses double.
Perhaps the toInt calls are unneeded but this is just a straight port of the
original code which also used toInt

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Spr...
File client/samples/total/src/SpreadsheetPresenter.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Spr...
client/samples/total/src/SpreadsheetPresenter.dart:685: int x = startX + e.x -
mouseStartX;
On 2011/10/27 05:50:24, arv wrote:
> e.x? We should kill that property... in another CL

Agreed. Can't wait till we cleanup the event classes.

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Spr...
client/samples/total/src/SpreadsheetPresenter.dart:893:
divStyle.setProperty("left", HtmlUtils.toPx(box.left));
On 2011/10/27 05:50:24, arv wrote:
> Is there a reason why we don't do divStyle.left = ...?
Only historical reasons.

I'll do a separate CL that regexp replaces all existing uses of setProperty 
now that Nathan's CL merging Css.dart directly into CSSStyleDeclaration has
landed.

http://codereview.chromium.org/8363040/diff/2012/client/samples/total/src/Spr...
client/samples/total/src/SpreadsheetPresenter.dart:1691:
_tableScrollContainer.style.setProperty("width", HtmlUtils.toPx(rect.width +
10));
On 2011/10/27 05:50:24, arv wrote:
> long lines

Done.

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/html/Ele...
File client/tests/client/html/ElementTests.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/html/Ele...
client/tests/client/html/ElementTests.dart:69: (element) => element is
AnchorElement);
On 2011/10/27 05:50:24, arv wrote:
> can't you just pass AnchorElement instead of a function?

I can't as Dart doesn't currently support first class classes.

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/layout/G...
File client/tests/client/layout/GridLayoutDemo.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/tests/client/layout/G...
client/tests/client/layout/GridLayoutDemo.dart:113: window.requestLayoutFrame(()
{
On 2011/10/27 05:50:24, arv wrote:
> This is the first case where this is feeling ugly.
> 
> It might actually be cleaner if a Future<List<Rect>> was used here.
Agreed. I felt dirty writing that test code.
Rewrote using a List<Future<Rect>> so it is a bit more verbose but less
dependent on futures being resolved in exactly the order they were requested.

http://codereview.chromium.org/8363040/diff/2012/client/touch/Scroller.dart
File client/touch/Scroller.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/touch/Scroller.dart#n...
client/touch/Scroller.dart:547: void _resize(Callback callback) {
On 2011/10/27 05:50:24, arv wrote:
> Maybe this should just return a future instead?

What would it return a future of?
Future<void> seems a bit odd and creating some arbitrary value to return a
future of seems ugly.

http://codereview.chromium.org/8363040/diff/2012/client/view/view.dart
File client/view/view.dart (right):

http://codereview.chromium.org/8363040/diff/2012/client/view/view.dart#newcod...
client/view/view.dart:327: Future<bool> _measureLayout() {
On 2011/10/27 05:50:24, arv wrote:
> Yeah, this is cleaner than using a Callback like in the other file.

I agree that when there is some valid value to return a future for it is
cleaner. I'm not as sure for cases where void was returned.

[Jacob, 2011-10-27 21:33:49]

Some high level comments:
The good: 
Switching to use futures in the existing client code was somewhat painful but
resulted in acceptably readable code with better performance characteristics in 
a few cases due to fewer layouts.

The bad:

1. requestLayoutFrame is a misleading name.  We need to come up with a better
name that indicates its true function. E.g.  requestLayoutFrame doesn't give you
a callback where layout can be performed, it gives you a callback that fires
after all layout has been performed.

2. the name rect and the class name ElementRect don't seem right.  Its members
are all rects but it isn't itself really a rectangle.

3. In reality, almost all the time we just wanted values from a single member of
ElementRect so computing all the measurements was a waste and made user code
more verbose. E.g.
element.rect.offset.then((ClientRect rect) {
  ...
}
is more pleasant than
element.rect.then((ElementRect elementRect) {
  final rect = elementRect.offset;
  ...
}

So I'd like to change Element from 

class Element {
  Future<ElementRect> get rect;
}

class ElementRect {
  ClientRect offset;
  ClientRect scroll;
  ...
}

to

class Element {
  ElementRect get rect();
}

class ElementRect {
  future<ClientRect> offset;
  future<ClientRect> scroll;
  ...
}

4. ClientRect is an ugly name. Can we just use Rect or Rectangle instead?

[Jacob, 2011-10-27 21:33:49]

Some high level comments:
The good: 
Switching to use futures in the existing client code was somewhat painful but
resulted in acceptably readable code with better performance characteristics in 
a few cases due to fewer layouts.

The bad:

1. requestLayoutFrame is a misleading name.  We need to come up with a better
name that indicates its true function. E.g.  requestLayoutFrame doesn't give you
a callback where layout can be performed, it gives you a callback that fires
after all layout has been performed.

2. the name rect and the class name ElementRect don't seem right.  Its members
are all rects but it isn't itself really a rectangle.

3. In reality, almost all the time we just wanted values from a single member of
ElementRect so computing all the measurements was a waste and made user code
more verbose. E.g.
element.rect.offset.then((ClientRect rect) {
  ...
}
is more pleasant than
element.rect.then((ElementRect elementRect) {
  final rect = elementRect.offset;
  ...
}

So I'd like to change Element from 

class Element {
  Future<ElementRect> get rect;
}

class ElementRect {
  ClientRect offset;
  ClientRect scroll;
  ...
}

to

class Element {
  ElementRect get rect();
}

class ElementRect {
  future<ClientRect> offset;
  future<ClientRect> scroll;
  ...
}

4. ClientRect is an ugly name. Can we just use Rect or Rectangle instead?

[nweiz, 2011-10-28 03:58:38]

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DOMWrapperBa...
File client/html/src/DOMWrapperBase.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DOMWrapperBa...
client/html/src/DOMWrapperBase.dart:16: unwrapDomObject(DOMWrapperBase wrapper)
{
Why not make this a method on DOMWrapperBase? You could call it _unwrap or
unwrapForTest to make it clear that it's for testing.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:130:
Future<CSSStyleDeclaration> _emptyStyleFuture() {
This pattern (initialize then complete immediately) seems like something that
should be done with a static method on Completer or Future, but I guess that's
something for a different CL.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:132:
completer.complete(new EmptyStyleDeclaration());
It seems potentially dangerous to complete this synchronously, if it's done
asynchronously for real Elements.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:141: final
List<ClientRect> clientRects = const <SimpleClientRect>[];
What's the reasoning behind specifying SimpleClientRect here rather than
ClientRect or just "const []"?

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:270:
Future<CSSStyleDeclaration> getComputedStyle(String pseudoElement) =>
Is there any reason not to have this call computedStyle and avoid defining
_emptyStyleFuture?

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:432: String toString() =>
"($left, $top, $width, $height)";
Might we want this to indicate that the object in question is a ClientRect? That
can be helpful when debugging.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:455: element.clientTop,
Isn't the correct indentation four spaces farther in than client? I'm mostly
curious so I can set it correctly for my Emacs mode.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:509: if (null != match) {
Style nit: match != null here and elsewhere.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
File client/html/src/Measurement.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:22: if (!_nextMeasurementFrameScheduled) {
Style nit: if (_nextMeasurementFrameScheduled) return;

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:30: // a noop if no requests are pending.
How sure are we that it isn't possible to at least annoy the user by spamming
tons of measurement messages and delaying entry to other event handlers and/or
repainting of the page?

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:31: window.on.message.add(Isolate.bind(
I believe Isolate.bind is no longer necessary (see review 8370031).

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:32: (e) => _onCompleteMeasurementFutures()
Style nit: 4 spaces

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:45: void
_addMeasurementFrameCallback(TimeoutHandler callback) {
It would be nice to have some documentation for these methods, in particular the
distinction between measurement futures and measurement frame callbacks.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:63: void _onCompleteMeasurementFutures() {
I feel like this name suggests that the function is called when the measurement
futures are completed, rather than that it completes the measurement futures
itself. Maybe "completeMeasurementFutures"?

http://codereview.chromium.org/8363040/diff/8001/client/html/src/WindowWrappi...
File client/html/src/WindowWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/WindowWrappi...
client/html/src/WindowWrappingImplementation.dart:780: int
setTimeout(TimeoutHandler handler, int timeout) {
Style nit: =>

http://codereview.chromium.org/8363040/diff/8001/client/layout/GridLayout.dart
File client/layout/GridLayout.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/layout/GridLayout.dar...
client/layout/GridLayout.dart:98: void measureLayout(Future<Size> size,
Completer<bool> changed) {
Passing in a Completer seems wrong... isn't the pattern for these sorts of
things to return a Future instead?

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dart
File client/layout/ViewLayout.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dar...
client/layout/ViewLayout.dart:112: return _cachedViewRect.value.offset.width;
What happens when _cachedViewRect hasn't fired yet? Seems like an NPE isn't
ideal... and if _cachedViewRect is always going to be concrete, why not store
ElementRect rather than a future?

Also, what if cacheExistingBrowserLayout hasn't been called yet?

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dar...
client/layout/ViewLayout.dart:119: int get borderLeftWidth() =>
_toPixels(_style.borderLeftWidth);
Can't _style be null sometimes?

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dar...
client/layout/ViewLayout.dart:125: 
Style nit: trailing whitespace

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/App.dart
File client/samples/swarm/App.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/App.dar...
client/samples/swarm/App.dart:30: window.setTimeout(() => onLoad(), 0);
I wonder if we should have a built-in way to run code when we next hit the event
loop, like we use when dispatching layout callbacks. Seems like using setTimeout
here could lead to undesirable flicker.

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/Views.dart
File client/samples/swarm/Views.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/Views.d...
client/samples/swarm/Views.dart:293: if (_vertical) {
I feel like ?: would read much better here.

http://codereview.chromium.org/8363040/diff/8001/client/samples/total/src/Spr...
File client/samples/total/src/SpreadsheetPresenter.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/samples/total/src/Spr...
client/samples/total/src/SpreadsheetPresenter.dart:1082: });
Style nit: aligned with _formulaInputMeasure

http://codereview.chromium.org/8363040/diff/8001/client/testing/unittest/unit...
File client/testing/unittest/unittestsuite.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/testing/unittest/unit...
client/testing/unittest/unittestsuite.dart:310: if (length > 0) {
Style nit: if (length == 0) return;

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/Ele...
File client/tests/client/html/ElementTests.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/Ele...
client/tests/client/html/ElementTests.dart:67: test('constructors', () {
Seems like maybe this wants to be a group, with testConstructorHelper defining
individual tests? That would make detecting multiple failures easier...

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/Mea...
File client/tests/client/html/MeasurementTests.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/Mea...
client/tests/client/html/MeasurementTests.dart:30: window.requestLayoutFrame(()
{
Shouldn't the call to requestLayoutFrame come before rect and computedStyle are
instantiated? The way it is now, requestLayoutFrame could run last just because
it was the last callback added.

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/htm...
File client/tests/client/html/html_tests.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/htm...
client/tests/client/html/html_tests.dart:5: #library('html_tests');
Haha, oops.

http://codereview.chromium.org/8363040/diff/8001/client/touch/Geometry.dart
File client/touch/Geometry.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/touch/Geometry.dart#n...
client/touch/Geometry.dart:58: String toString() => "($x, $y)";
Class name?

[Jacob, 2011-10-31 22:09:50]

PTAL

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DOMWrapperBa...
File client/html/src/DOMWrapperBase.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DOMWrapperBa...
client/html/src/DOMWrapperBase.dart:16: unwrapDomObject(DOMWrapperBase wrapper)
{
On 2011/10/28 03:58:38, nweiz wrote:
> Why not make this a method on DOMWrapperBase? You could call it _unwrap or
> unwrapForTest to make it clear that it's for testing.

I can't call it _unwrap as then DartVM will prevent me from using it in tests
running in other libraries.
Hopefully in the future there will be a way to run tests so that they are
allowed to access package private members across library boundaries.
I prefer to keep unwrapDomObject outside of the DOMWrapperBase class to keep the
testing only code separate from the production code although in practice it
makes very little difference.

You could imagine that what I am doing is similar to defining

class DomMirror {
  static unwrapDomObject(...);
}

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:130:
Future<CSSStyleDeclaration> _emptyStyleFuture() {
On 2011/10/28 03:58:38, nweiz wrote:
> This pattern (initialize then complete immediately) seems like something that
> should be done with a static method on Completer or Future, but I guess that's
> something for a different CL.
I agree.  Ideally the completer class should have a method that initializes,
completes immediately after a minimal async interval.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:132:
completer.complete(new EmptyStyleDeclaration());
On 2011/10/28 03:58:38, nweiz wrote:
> It seems potentially dangerous to complete this synchronously, if it's done
> asynchronously for real Elements.
Good point. Fixed these two cases.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:141: final
List<ClientRect> clientRects = const <SimpleClientRect>[];
On 2011/10/28 03:58:38, nweiz wrote:
> What's the reasoning behind specifying SimpleClientRect here rather than
> ClientRect or just "const []"?

that way

clientRects is List<ClientRect> will return true
and 
clientRects is List<FooBar> will return false

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:270:
Future<CSSStyleDeclaration> getComputedStyle(String pseudoElement) =>
On 2011/10/28 03:58:38, nweiz wrote:
> Is there any reason not to have this call computedStyle and avoid defining
> _emptyStyleFuture?
It matters very little.  My code is more verbose than needed but I think makes
the intent clearer.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:432: String toString() =>
"($left, $top, $width, $height)";
On 2011/10/28 03:58:38, nweiz wrote:
> Might we want this to indicate that the object in question is a ClientRect?
That
> can be helpful when debugging.

Chatted with the rest of the team over here are our current thinking is toString
should be optimized for terse debugging.
If you want to find out the class, etc then there will be other ways to achieve
that in the future using mirrors, convenient helpers built into the REPL, etc.

This way is better because 
print('client=$client offset=$offset  scroll=$scroll')
is easy to read instead of being overly verbose.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:455: element.clientTop,
On 2011/10/28 03:58:38, nweiz wrote:
> Isn't the correct indentation four spaces farther in than client? I'm mostly
> curious so I can set it correctly for my Emacs mode.
fixed the busted indentation.
I did a find and replace and forgot to fix the indentation.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:509: if (null != match) {
On 2011/10/28 03:58:38, nweiz wrote:
> Style nit: match != null here and elsewhere.

null != foo is faster
than foo != null

I agree that null != foo looked ugly in this code so i'm switching back to using
foo !== null
as it seems like the least ugly solution at the moment.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
File client/html/src/Measurement.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:22: if (!_nextMeasurementFrameScheduled) {
On 2011/10/28 03:58:38, nweiz wrote:
> Style nit: if (_nextMeasurementFrameScheduled) return;

Done.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:30: // a noop if no requests are pending.
On 2011/10/28 03:58:38, nweiz wrote:
> How sure are we that it isn't possible to at least annoy the user by spamming
> tons of measurement messages and delaying entry to other event handlers and/or
> repainting of the page?

100% sure due to the early terminate at the start of the function for
if (!_nextMeasurementFrameScheduled)

when a measurement frame has been scheduled, we will always have posted a
message from our window to itself so it isn't possible for an other window to
spam us in a meaningful way.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:31: window.on.message.add(Isolate.bind(
On 2011/10/28 03:58:38, nweiz wrote:
> I believe Isolate.bind is no longer necessary (see review 8370031).

Done.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:32: (e) => _onCompleteMeasurementFutures()
On 2011/10/28 03:58:38, nweiz wrote:
> Style nit: 4 spaces

Done.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:45: void
_addMeasurementFrameCallback(TimeoutHandler callback) {
On 2011/10/28 03:58:38, nweiz wrote:
> It would be nice to have some documentation for these methods, in particular
the
> distinction between measurement futures and measurement frame callbacks.
Agreed. Added some flower box comments

http://codereview.chromium.org/8363040/diff/8001/client/html/src/Measurement....
client/html/src/Measurement.dart:63: void _onCompleteMeasurementFutures() {
On 2011/10/28 03:58:38, nweiz wrote:
> I feel like this name suggests that the function is called when the
measurement
> futures are completed, rather than that it completes the measurement futures
> itself. Maybe "completeMeasurementFutures"?

Done.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/WindowWrappi...
File client/html/src/WindowWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/WindowWrappi...
client/html/src/WindowWrappingImplementation.dart:780: int
setTimeout(TimeoutHandler handler, int timeout) {
On 2011/10/28 03:58:38, nweiz wrote:
> Style nit: =>

Done.

http://codereview.chromium.org/8363040/diff/8001/client/layout/GridLayout.dart
File client/layout/GridLayout.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/layout/GridLayout.dar...
client/layout/GridLayout.dart:98: void measureLayout(Future<Size> size,
Completer<bool> changed) {
On 2011/10/28 03:58:38, nweiz wrote:
> Passing in a Completer seems wrong... isn't the pattern for these sorts of
> things to return a Future instead?
I agree this is a bit odd looking.
However, for this case, returning futures would be much much uglier as you end
up having non-leaf views listening on the futures returned by each of their
subviews and then aggregating that into a new future.
This way things are far simpler and more efficient as we only care that at least
one of the leaf views set complete to true.

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dart
File client/layout/ViewLayout.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dar...
client/layout/ViewLayout.dart:112: return _cachedViewRect.value.offset.width;
On 2011/10/28 03:58:38, nweiz wrote:
> What happens when _cachedViewRect hasn't fired yet? Seems like an NPE isn't
> ideal... and if _cachedViewRect is always going to be concrete, why not store
> ElementRect rather than a future?
> 
> Also, what if cacheExistingBrowserLayout hasn't been called yet?

by design it will always have fired due to appropriate calls to
requestLayoutFrame in the code that uses this.
This way we should get the right "future not available exception" instead of
just a NPE resulting in clearer error messages as well as making the code
simpler and more efficient as we avoid attaching completion listeners to each of
the individual futures.

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dar...
client/layout/ViewLayout.dart:119: int get borderLeftWidth() =>
_toPixels(_style.borderLeftWidth);
On 2011/10/28 03:58:38, nweiz wrote:
> Can't _style be null sometimes?

it can't be null, however a future not available yet exception could fire if the
user doesn't use this correctly.
This is ugly but I can't think of a cleaner way to modify this api to work
acceptably with async layout. If you or John can think of a better way to modify
this, let me know.

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dar...
client/layout/ViewLayout.dart:125: 
On 2011/10/28 03:58:38, nweiz wrote:
> Style nit: trailing whitespace

Done.

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/App.dart
File client/samples/swarm/App.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/App.dar...
client/samples/swarm/App.dart:30: window.setTimeout(() => onLoad(), 0);
On 2011/10/28 03:58:38, nweiz wrote:
> I wonder if we should have a built-in way to run code when we next hit the
event
> loop, like we use when dispatching layout callbacks. Seems like using
setTimeout
> here could lead to undesirable flicker.

I agree.  Got an idea for a good name for it?

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/Views.dart
File client/samples/swarm/Views.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/Views.d...
client/samples/swarm/Views.dart:293: if (_vertical) {
On 2011/10/28 03:58:38, nweiz wrote:
> I feel like ?: would read much better here.

Done.

http://codereview.chromium.org/8363040/diff/8001/client/samples/total/src/Spr...
File client/samples/total/src/SpreadsheetPresenter.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/samples/total/src/Spr...
client/samples/total/src/SpreadsheetPresenter.dart:1082: });
On 2011/10/28 03:58:38, nweiz wrote:
> Style nit: aligned with _formulaInputMeasure

Done.

http://codereview.chromium.org/8363040/diff/8001/client/testing/unittest/unit...
File client/testing/unittest/unittestsuite.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/testing/unittest/unit...
client/testing/unittest/unittestsuite.dart:310: if (length > 0) {
On 2011/10/28 03:58:38, nweiz wrote:
> Style nit: if (length == 0) return;
For this case i think the code is slightly easier to read as written

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/Ele...
File client/tests/client/html/ElementTests.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/Ele...
client/tests/client/html/ElementTests.dart:67: test('constructors', () {
On 2011/10/28 03:58:38, nweiz wrote:
> Seems like maybe this wants to be a group, with testConstructorHelper defining
> individual tests? That would make detecting multiple failures easier...
Interesting idea but it seems like 
I was on the fence but as I can make the change with just a regexp,  i went
ahead and did it.... now I have a lot more tests :-P

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/Mea...
File client/tests/client/html/MeasurementTests.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/Mea...
client/tests/client/html/MeasurementTests.dart:30: window.requestLayoutFrame(()
{
On 2011/10/28 03:58:38, nweiz wrote:
> Shouldn't the call to requestLayoutFrame come before rect and computedStyle
are
> instantiated? The way it is now, requestLayoutFrame could run last just
because
> it was the last callback added.

Fair enough. Moved it first to verify that things don't work by accident

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/htm...
File client/tests/client/html/html_tests.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/tests/client/html/htm...
client/tests/client/html/html_tests.dart:5: #library('html_tests');
On 2011/10/28 03:58:38, nweiz wrote:
> Haha, oops.

yep :)

http://codereview.chromium.org/8363040/diff/8001/client/touch/Geometry.dart
File client/touch/Geometry.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/touch/Geometry.dart#n...
client/touch/Geometry.dart:58: String toString() => "($x, $y)";
On 2011/10/28 03:58:38, nweiz wrote:
> Class name?
We intentionally avoid the class name.

If you wrote
print ("Drag from $start to $end")
would you rather get
"Drag from Coordinate(3,4), to Coordinate(8,9)"
or
"Drag from (3,4) to (8,9)"

?

[nweiz, 2011-11-01 00:49:22]

lgtm

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:141: final
List<ClientRect> clientRects = const <SimpleClientRect>[];
On 2011/10/31 22:09:50, Jacob wrote:
> On 2011/10/28 03:58:38, nweiz wrote:
> > What's the reasoning behind specifying SimpleClientRect here rather than
> > ClientRect or just "const []"?
> 
> that way
> 
> clientRects is List<ClientRect> will return true
> and 
> clientRects is List<FooBar> will return false

But why not "const <ClientRect>[]"?

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:432: String toString() =>
"($left, $top, $width, $height)";
On 2011/10/31 22:09:50, Jacob wrote:
> On 2011/10/28 03:58:38, nweiz wrote:
> > Might we want this to indicate that the object in question is a ClientRect?
> That
> > can be helpful when debugging.
> 
> Chatted with the rest of the team over here are our current thinking is
toString
> should be optimized for terse debugging.
> If you want to find out the class, etc then there will be other ways to
achieve
> that in the future using mirrors, convenient helpers built into the REPL, etc.
> 
> This way is better because 
> print('client=$client offset=$offset  scroll=$scroll')
> is easy to read instead of being overly verbose.
> 

In my experience, "what is this object I have?" is a pretty common question you
want answered when debugging with dynamic types. Some sort of typeof mechanism
will certainly help here, but I'm not sure "ClientRect(1, 2, 3, 4)" is enough
verbosity above "(1, 2, 3, 4)" to make it worth forcing people to change
"print(obj)" to "print(obj.type)".

This CL probably isn't the place to resolve this argument, though.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:509: if (null != match) {
On 2011/10/31 22:09:50, Jacob wrote:
> On 2011/10/28 03:58:38, nweiz wrote:
> > Style nit: match != null here and elsewhere.
> 
> null != foo is faster
> than foo != null
> 
> I agree that null != foo looked ugly in this code so i'm switching back to
using
> foo !== null
> as it seems like the least ugly solution at the moment.

That definitely seems like a problem for the compiler. It seems like something
is deeply wrong if we have to start worrying about the efficiency of the order
of equality comparisons.

In any case, it seems like we should be consistent within the codebase as to
which style of equality comparison we use. Currently the vast majority of code
uses "x == null" and "x != null". We should either use that here or push to
migrate other uses to ===.

http://codereview.chromium.org/8363040/diff/8001/client/layout/GridLayout.dart
File client/layout/GridLayout.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/layout/GridLayout.dar...
client/layout/GridLayout.dart:98: void measureLayout(Future<Size> size,
Completer<bool> changed) {
On 2011/10/31 22:09:50, Jacob wrote:
> On 2011/10/28 03:58:38, nweiz wrote:
> > Passing in a Completer seems wrong... isn't the pattern for these sorts of
> > things to return a Future instead?
> I agree this is a bit odd looking.
> However, for this case, returning futures would be much much uglier as you end
> up having non-leaf views listening on the futures returned by each of their
> subviews and then aggregating that into a new future.
> This way things are far simpler and more efficient as we only care that at
least
> one of the leaf views set complete to true.

I would argue that the correct pattern here is to return a future, then have a
library function that aggregates futures in the way you describe (it takes a
list of futures and returns a new future that will fire once any of the input
futures fire). That way this function has a type signature that matches its
semantics, and doesn't build in knowledge of the surrounding code structure.

Another thing: this function never returns false, so the bool value seems
out-of-place.

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dart
File client/layout/ViewLayout.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dar...
client/layout/ViewLayout.dart:112: return _cachedViewRect.value.offset.width;
On 2011/10/31 22:09:50, Jacob wrote:
> On 2011/10/28 03:58:38, nweiz wrote:
> > What happens when _cachedViewRect hasn't fired yet? Seems like an NPE isn't
> > ideal... and if _cachedViewRect is always going to be concrete, why not
store
> > ElementRect rather than a future?
> > 
> > Also, what if cacheExistingBrowserLayout hasn't been called yet?
> 
> by design it will always have fired due to appropriate calls to
> requestLayoutFrame in the code that uses this.
> This way we should get the right "future not available exception" instead of
> just a NPE resulting in clearer error messages as well as making the code
> simpler and more efficient as we avoid attaching completion listeners to each
of
> the individual futures.

Oh, futures throw an error when value doesn't exist? That makes sense.

http://codereview.chromium.org/8363040/diff/8001/client/layout/ViewLayout.dar...
client/layout/ViewLayout.dart:119: int get borderLeftWidth() =>
_toPixels(_style.borderLeftWidth);
On 2011/10/31 22:09:50, Jacob wrote:
> On 2011/10/28 03:58:38, nweiz wrote:
> > Can't _style be null sometimes?
> 
> it can't be null, however a future not available yet exception could fire if
the
> user doesn't use this correctly.
> This is ugly but I can't think of a cleaner way to modify this api to work
> acceptably with async layout. If you or John can think of a better way to
modify
> this, let me know.

The fact that it fires a "future not completed" error rather than an NPE makes
this better. I think the ideal scenario is to have all these properties return
Futures, but that's not really practical without more fluid language support for
async.

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/App.dart
File client/samples/swarm/App.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/App.dar...
client/samples/swarm/App.dart:30: window.setTimeout(() => onLoad(), 0);
On 2011/10/31 22:09:50, Jacob wrote:
> On 2011/10/28 03:58:38, nweiz wrote:
> > I wonder if we should have a built-in way to run code when we next hit the
> event
> > loop, like we use when dispatching layout callbacks. Seems like using
> setTimeout
> > here could lead to undesirable flicker.
> 
> I agree.  Got an idea for a good name for it?

beforeRedraw()? runAsync()?

http://codereview.chromium.org/8363040/diff/15001/client/html/src/ElementWrap...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/15001/client/html/src/ElementWrap...
client/html/src/ElementWrappingImplementation.dart:456: element.clientTop,
I think this indentation is still incorrect... according to my recollection of
what Bob was telling me, continued argument lists are always indented four
spaces more than the base line, and never aligned with previous arguments.

[Jacob, 2011-11-01 02:42:39]

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
File client/html/src/DocumentFragmentWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/DocumentFrag...
client/html/src/DocumentFragmentWrappingImplementation.dart:141: final
List<ClientRect> clientRects = const <SimpleClientRect>[];
On 2011/11/01 00:49:22, nweiz wrote:
> On 2011/10/31 22:09:50, Jacob wrote:
> > On 2011/10/28 03:58:38, nweiz wrote:
> > > What's the reasoning behind specifying SimpleClientRect here rather than
> > > ClientRect or just "const []"?
> > 
> > that way
> > 
> > clientRects is List<ClientRect> will return true
> > and 
> > clientRects is List<FooBar> will return false
> 
> But why not "const <ClientRect>[]"?

I agree.  const <ClientRect>[] is better. Fixed.

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/html/src/ElementWrapp...
client/html/src/ElementWrappingImplementation.dart:432: String toString() =>
"($left, $top, $width, $height)";
On 2011/11/01 00:49:22, nweiz wrote:
> On 2011/10/31 22:09:50, Jacob wrote:
> > On 2011/10/28 03:58:38, nweiz wrote:
> > > Might we want this to indicate that the object in question is a
ClientRect?
> > That
> > > can be helpful when debugging.
> > 
> > Chatted with the rest of the team over here are our current thinking is
> toString
> > should be optimized for terse debugging.
> > If you want to find out the class, etc then there will be other ways to
> achieve
> > that in the future using mirrors, convenient helpers built into the REPL,
etc.
> > 
> > This way is better because 
> > print('client=$client offset=$offset  scroll=$scroll')
> > is easy to read instead of being overly verbose.
> > 
> 
> In my experience, "what is this object I have?" is a pretty common question
you
> want answered when debugging with dynamic types. Some sort of typeof mechanism
> will certainly help here, but I'm not sure "ClientRect(1, 2, 3, 4)" is enough
> verbosity above "(1, 2, 3, 4)" to make it worth forcing people to change
> "print(obj)" to "print(obj.type)".
> 
> This CL probably isn't the place to resolve this argument, though.

Agreed. Lets take that discussion to the style guide. :)

http://codereview.chromium.org/8363040/diff/8001/client/layout/GridLayout.dart
File client/layout/GridLayout.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/layout/GridLayout.dar...
client/layout/GridLayout.dart:98: void measureLayout(Future<Size> size,
Completer<bool> changed) {
On 2011/11/01 00:49:22, nweiz wrote:
> On 2011/10/31 22:09:50, Jacob wrote:
> > On 2011/10/28 03:58:38, nweiz wrote:
> > > Passing in a Completer seems wrong... isn't the pattern for these sorts of
> > > things to return a Future instead?
> > I agree this is a bit odd looking.
> > However, for this case, returning futures would be much much uglier as you
end
> > up having non-leaf views listening on the futures returned by each of their
> > subviews and then aggregating that into a new future.
> > This way things are far simpler and more efficient as we only care that at
> least
> > one of the leaf views set complete to true.
> 
> I would argue that the correct pattern here is to return a future, then have a
> library function that aggregates futures in the way you describe (it takes a
> list of futures and returns a new future that will fire once any of the input
> futures fire). That way this function has a type signature that matches its
> semantics, and doesn't build in knowledge of the surrounding code structure.
> 
> Another thing: this function never returns false, so the bool value seems
> out-of-place.

Btw, the bool value is required as the completer is set to false if it was not
set to true before the next call to requestLayoutFrame. However, this suggests
the code isn't very readable as written. I've added a TODO to simplify the
logic.

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/App.dart
File client/samples/swarm/App.dart (right):

http://codereview.chromium.org/8363040/diff/8001/client/samples/swarm/App.dar...
client/samples/swarm/App.dart:30: window.setTimeout(() => onLoad(), 0);
On 2011/11/01 00:49:22, nweiz wrote:
> On 2011/10/31 22:09:50, Jacob wrote:
> > On 2011/10/28 03:58:38, nweiz wrote:
> > > I wonder if we should have a built-in way to run code when we next hit the
> > event
> > > loop, like we use when dispatching layout callbacks. Seems like using
> > setTimeout
> > > here could lead to undesirable flicker.
> > 
> > I agree.  Got an idea for a good name for it?
> 
> beforeRedraw()? runAsync()?

hmm. beforeRedraw is closer but not quite right.  If there isn't anything to
redraw, we'll still be called right away and the time interval doesn't really
have anything directly to do with redraw, that's just a propert it happens to
have.
It would seem beforeRedraw is really "requestAnimationFrame".
One name that has been tossed around for this concept in the past is "microTask"
so something like
"nextMicroTask", "requestMicroTask" or "runMicroTask" could make sense although
those don't sound quite right either.

http://codereview.chromium.org/8363040/diff/15001/client/html/src/ElementWrap...
File client/html/src/ElementWrappingImplementation.dart (right):

http://codereview.chromium.org/8363040/diff/15001/client/html/src/ElementWrap...
client/html/src/ElementWrappingImplementation.dart:456: element.clientTop,
On 2011/11/01 00:49:22, nweiz wrote:
> I think this indentation is still incorrect... according to my recollection of
> what Bob was telling me, continued argument lists are always indented four
> spaces more than the base line, and never aligned with previous arguments.
I guess I need to fight this style guide question out with Bob.
In most style guides, lines are either aligned 4 or aligned with the previous
arguments at the discretion of the author.  For this case I think this alignment
reads better.

[Ben Laurie (Google), 2011-11-02 12:29:17]

DBQ: I know we're planning to rename Promise to Future soon, but it seems you
have some other thing called a Future - what is it?