Example showing alternate async measurement solution

Example showing alternate async measurement solution


BUG=
TEST=

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

[Jacob, 2012-01-12 01:14:29]

Ready for initial review.  I'm still writing some tests that check that the
asserts fire when they should.

[Jennifer Messerly, 2012-01-13 02:19:44]

lgtm

http://codereview.chromium.org/9148015/diff/7001/client/html/src/CSSStyleDecl...
File client/html/src/CSSStyleDeclarationWrappingImplementation.dart (right):

http://codereview.chromium.org/9148015/diff/7001/client/html/src/CSSStyleDecl...
client/html/src/CSSStyleDeclarationWrappingImplementation.dart:50: String get
cssText() =>_ptr.cssText;
space after =>

http://codereview.chromium.org/9148015/diff/7001/client/html/src/NodeWrapping...
File client/html/src/NodeWrappingImplementation.dart (right):

http://codereview.chromium.org/9148015/diff/7001/client/html/src/NodeWrapping...
client/html/src/NodeWrappingImplementation.dart:12: return
LevelDom.wrapNode(node).dynamic._inDocument; // XXX remove dynamic
make the XXX into a TODO?

http://codereview.chromium.org/9148015/diff/7001/client/html/src/NodeWrapping...
client/html/src/NodeWrappingImplementation.dart:243: bool get _inDocument() {
bool get _inDocument() => document.contains(this);

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

http://codereview.chromium.org/9148015/diff/7001/client/layout/GridLayout.dar...
client/layout/GridLayout.dart:101: assert(window.inMeasurementFrame);
nice!

http://codereview.chromium.org/9148015/diff/7001/client/view/view.dart
File client/view/view.dart (right):

http://codereview.chromium.org/9148015/diff/7001/client/view/view.dart#newcod...
client/view/view.dart:335: // we're taking pains to not initialize _layout if
it's not needed. Is that
long line

http://codereview.chromium.org/9148015/diff/7001/client/view/view.dart#newcod...
client/view/view.dart:339: if (v.layout.measureLayout(new Size(rect.width,
rect.height))) {
I wonder if we still need the Size object now?

[nweiz, 2012-01-13 02:29:53]

lgtm

In the future, will these asserts turn into something more self-documenting? It
would be nice to have an error message that explains to the user to some extent
what's going on and how to fix it.

[Jacob, 2012-01-17 18:53:35]

On 2012/01/13 02:29:53, nweiz wrote:
> lgtm
> 
> In the future, will these asserts turn into something more self-documenting?
It
> would be nice to have an error message that explains to the user to some
extent
> what's going on and how to fix it.

Currently dart asserts cannot include a string message by design.  If they did,
that would be ideal for this case.  Alternately, we could throw exceptions but
then it becomes ugly to make them fire in developer mode but not production
mode.

[Jacob, 2012-01-17 19:30:44]

http://codereview.chromium.org/9148015/diff/7001/client/html/src/CSSStyleDecl...
File client/html/src/CSSStyleDeclarationWrappingImplementation.dart (right):

http://codereview.chromium.org/9148015/diff/7001/client/html/src/CSSStyleDecl...
client/html/src/CSSStyleDeclarationWrappingImplementation.dart:50: String get
cssText() =>_ptr.cssText;
On 2012/01/13 02:19:44, John Messerly wrote:
> space after =>

Done.

http://codereview.chromium.org/9148015/diff/7001/client/html/src/NodeWrapping...
File client/html/src/NodeWrappingImplementation.dart (right):

http://codereview.chromium.org/9148015/diff/7001/client/html/src/NodeWrapping...
client/html/src/NodeWrappingImplementation.dart:12: return
LevelDom.wrapNode(node).dynamic._inDocument; // XXX remove dynamic
On 2012/01/13 02:19:44, John Messerly wrote:
> make the XXX into a TODO?
Removed dynamic.
Done.

http://codereview.chromium.org/9148015/diff/7001/client/html/src/NodeWrapping...
client/html/src/NodeWrappingImplementation.dart:243: bool get _inDocument() {
On 2012/01/13 02:19:44, John Messerly wrote:
> bool get _inDocument() => document.contains(this);

Done.

http://codereview.chromium.org/9148015/diff/7001/client/view/view.dart
File client/view/view.dart (right):

http://codereview.chromium.org/9148015/diff/7001/client/view/view.dart#newcod...
client/view/view.dart:335: // we're taking pains to not initialize _layout if
it's not needed. Is that
On 2012/01/13 02:19:44, John Messerly wrote:
> long line

Done.

http://codereview.chromium.org/9148015/diff/7001/client/view/view.dart#newcod...
client/view/view.dart:339: if (v.layout.measureLayout(new Size(rect.width,
rect.height))) {
On 2012/01/13 02:19:44, John Messerly wrote:
> I wonder if we still need the Size object now?

agreed.  Switched back to just passing width and height as separate args.

[arv (Not doing code reviews), 2012-01-17 20:25:00]

lgtm

At least we tried to do the Future solution. I agree that this is cleaner and
simpler to work with.

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

http://codereview.chromium.org/9148015/diff/16001/client/html/src/ElementWrap...
client/html/src/ElementWrappingImplementation.dart:649: // TODO(jacobr): does
this dirty the layout?
Yes it does

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

http://codereview.chromium.org/9148015/diff/16001/client/layout/GridLayout.da...
client/layout/GridLayout.dart:98: // rather than taking a Completer as an
argument.
Update TODO

[Jacob, 2012-01-21 03:01:24]

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

http://codereview.chromium.org/9148015/diff/16001/client/html/src/ElementWrap...
client/html/src/ElementWrappingImplementation.dart:649: // TODO(jacobr): does
this dirty the layout?
On 2012/01/17 20:25:00, arv wrote:
> Yes it does

Done.

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

http://codereview.chromium.org/9148015/diff/16001/client/layout/GridLayout.da...
client/layout/GridLayout.dart:98: // rather than taking a Completer as an
argument.
On 2012/01/17 20:25:00, arv wrote:
> Update TODO

Done.

[Siggi Cherem (dart-lang), 2012-01-23 01:16:39]

https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
File client/samples/dartcombat/views.dart (right):

https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
client/samples/dartcombat/views.dart:97: return () {
overall feels good to chunk everthing in phases (measure, layout, measure,
layout), but syntactically, returning a callback for layout feels really awkward
to me.

I'm sure you guys have been debating for a while about this, so forgive me if
you already considered something of this sort. I was trying to think if we can
work around this awkwardness by using futures for each of these stages
(measure/layout), for example:

/**
 * Returns a nested future: the outer future indicates that
 * the frame is ready for measurements, the inner future
 * indicates that the frame is ready for for more layout,
 * but no longer for measurements.
 */
Future<Future> window.requestMeasurementsFrame();

// Usage example
final measureReady = window.requestMeasurementsFrame();
measureReady.then((layoutReady) {
  // ... measure
  // ... measure
  layoutReady.then((_) {
    // ... layout
    // ... layout
  });
};

Then, if we get await (or something similar) in the language this could be
written linearly as follows:

final measureReady = window.requestMeasurementsFrame();
final layoutReady = await measureReady;
// ... measure
// ... measure
await layoutReady;
// ... layout
// ... layout


So the example here would look as follows:
  window.requestMeasurementFrame().then((layoutReady) {
    final pos = ViewUtil.positionFromEvent(_rootNode, e);
    _boatStartX = pos[0];
    _boatStartY = pos[1];
    layoutReady.then((_) {
      // error case when the mouse was released out of the boat-placing area
      if (_moveListener != null) {
         _rootNode.on.mouseMove.remove(_moveListener, false);
         _possibleBoat.remove();
         _moveListener = null;
      }
      _possibleBoat = ViewUtil.createDiv("icons boat2");
      ViewUtil.placeNodeAt(_possibleBoat, _boatStartX, _boatStartY);
      _rootNode.nodes.add(_possibleBoat);
      _moveListener = handleMouseMove;
      _rootNode.on.mouseMove.add(_moveListener);
    });
  });

Thoughts?

https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
client/samples/dartcombat/views.dart:158: boat = new Boat(deltaX < 0 ?
(_boatStartX - boatSize + 1) : _boatStartX,
80 col

[nweiz, 2012-01-24 00:23:49]

https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
File client/samples/dartcombat/views.dart (right):

https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
client/samples/dartcombat/views.dart:97: return () {
On 2012/01/23 01:16:39, sigmund wrote:
> overall feels good to chunk everthing in phases (measure, layout, measure,
> layout), but syntactically, returning a callback for layout feels really
awkward
> to me.
> 
> I'm sure you guys have been debating for a while about this, so forgive me if
> you already considered something of this sort. I was trying to think if we can
> work around this awkwardness by using futures for each of these stages
> (measure/layout), for example:
> 
> /**
>  * Returns a nested future: the outer future indicates that
>  * the frame is ready for measurements, the inner future
>  * indicates that the frame is ready for for more layout,
>  * but no longer for measurements.
>  */
> Future<Future> window.requestMeasurementsFrame();
> 
> // Usage example
> final measureReady = window.requestMeasurementsFrame();
> measureReady.then((layoutReady) {
>   // ... measure
>   // ... measure
>   layoutReady.then((_) {
>     // ... layout
>     // ... layout
>   });
> };
> 
> Then, if we get await (or something similar) in the language this could be
> written linearly as follows:
> 
> final measureReady = window.requestMeasurementsFrame();
> final layoutReady = await measureReady;
> // ... measure
> // ... measure
> await layoutReady;
> // ... layout
> // ... layout
> 
> 
> So the example here would look as follows:
>   window.requestMeasurementFrame().then((layoutReady) {
>     final pos = ViewUtil.positionFromEvent(_rootNode, e);
>     _boatStartX = pos[0];
>     _boatStartY = pos[1];
>     layoutReady.then((_) {
>       // error case when the mouse was released out of the boat-placing area
>       if (_moveListener != null) {
>          _rootNode.on.mouseMove.remove(_moveListener, false);
>          _possibleBoat.remove();
>          _moveListener = null;
>       }
>       _possibleBoat = ViewUtil.createDiv("icons boat2");
>       ViewUtil.placeNodeAt(_possibleBoat, _boatStartX, _boatStartY);
>       _rootNode.nodes.add(_possibleBoat);
>       _moveListener = handleMouseMove;
>       _rootNode.on.mouseMove.add(_moveListener);
>     });
>   });
> 
> Thoughts?

I like the idea of using futures. The syntax without await is a little awkward,
but no more so than the return-a-closure syntax.

I'd still like to see Future chaining. Here's how the example would look using
Closure-style chaining:

  window.requestMeasurementFrame().then((layoutReady) {
    final pos = ViewUtil.positionFromEvent(_rootNode, e);
    _boatStartX = pos[0];
    _boatStartY = pos[1];
    return layoutReady;
  }).then((_) {
    // error case when the mouse was released out of the boat-placing area
    if (_moveListener != null) {
       _rootNode.on.mouseMove.remove(_moveListener, false);
       _possibleBoat.remove();
       _moveListener = null;
    }
    _possibleBoat = ViewUtil.createDiv("icons boat2");
    ViewUtil.placeNodeAt(_possibleBoat, _boatStartX, _boatStartY);
    _rootNode.nodes.add(_possibleBoat);
    _moveListener = handleMouseMove;
    _rootNode.on.mouseMove.add(_moveListener);
  });

[Jennifer Messerly, 2012-01-24 01:00:20]

On 2012/01/24 00:23:49, nweiz wrote:
>
https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
> File client/samples/dartcombat/views.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
> client/samples/dartcombat/views.dart:97: return () {
> On 2012/01/23 01:16:39, sigmund wrote:
> > overall feels good to chunk everthing in phases (measure, layout, measure,
> > layout), but syntactically, returning a callback for layout feels really
> awkward
> > to me.
> > 
> > I'm sure you guys have been debating for a while about this, so forgive me
if
> > you already considered something of this sort. I was trying to think if we
can
> > work around this awkwardness by using futures for each of these stages
> > (measure/layout), for example:
> > 
> > /**
> >  * Returns a nested future: the outer future indicates that
> >  * the frame is ready for measurements, the inner future
> >  * indicates that the frame is ready for for more layout,
> >  * but no longer for measurements.
> >  */
> > Future<Future> window.requestMeasurementsFrame();
> > 
> > // Usage example
> > final measureReady = window.requestMeasurementsFrame();
> > measureReady.then((layoutReady) {
> >   // ... measure
> >   // ... measure
> >   layoutReady.then((_) {
> >     // ... layout
> >     // ... layout
> >   });
> > };
> > 
> > Then, if we get await (or something similar) in the language this could be
> > written linearly as follows:
> > 
> > final measureReady = window.requestMeasurementsFrame();
> > final layoutReady = await measureReady;
> > // ... measure
> > // ... measure
> > await layoutReady;
> > // ... layout
> > // ... layout
> > 
> > 
> > So the example here would look as follows:
> >   window.requestMeasurementFrame().then((layoutReady) {
> >     final pos = ViewUtil.positionFromEvent(_rootNode, e);
> >     _boatStartX = pos[0];
> >     _boatStartY = pos[1];
> >     layoutReady.then((_) {
> >       // error case when the mouse was released out of the boat-placing area
> >       if (_moveListener != null) {
> >          _rootNode.on.mouseMove.remove(_moveListener, false);
> >          _possibleBoat.remove();
> >          _moveListener = null;
> >       }
> >       _possibleBoat = ViewUtil.createDiv("icons boat2");
> >       ViewUtil.placeNodeAt(_possibleBoat, _boatStartX, _boatStartY);
> >       _rootNode.nodes.add(_possibleBoat);
> >       _moveListener = handleMouseMove;
> >       _rootNode.on.mouseMove.add(_moveListener);
> >     });
> >   });
> > 
> > Thoughts?
> 
> I like the idea of using futures. The syntax without await is a little
awkward,
> but no more so than the return-a-closure syntax.
> 
> I'd still like to see Future chaining. Here's how the example would look using
> Closure-style chaining:
> 
>   window.requestMeasurementFrame().then((layoutReady) {
>     final pos = ViewUtil.positionFromEvent(_rootNode, e);
>     _boatStartX = pos[0];
>     _boatStartY = pos[1];
>     return layoutReady;
>   }).then((_) {
>     // error case when the mouse was released out of the boat-placing area
>     if (_moveListener != null) {
>        _rootNode.on.mouseMove.remove(_moveListener, false);
>        _possibleBoat.remove();
>        _moveListener = null;
>     }
>     _possibleBoat = ViewUtil.createDiv("icons boat2");
>     ViewUtil.placeNodeAt(_possibleBoat, _boatStartX, _boatStartY);
>     _rootNode.nodes.add(_possibleBoat);
>     _moveListener = handleMouseMove;
>     _rootNode.on.mouseMove.add(_moveListener);
>   });

The problem with the future chaining variant is layoutReady can't close over
measured data, which makes the code worse (e.g. capturing a bunch of temporary
data into class fields, or declaring them as variables outside of the
requestMeasurementFrame callback ...)

[Jacob, 2012-01-24 01:12:03]

If there is async support in the lamguage then using futures could be
interesting. Without it returning a Future<Future> seems too confusing for the
average developer.

On 2012/01/24 01:00:20, John Messerly wrote:
> On 2012/01/24 00:23:49, nweiz wrote:
> >
>
https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
> > File client/samples/dartcombat/views.dart (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9148015/diff/20001/client/samples/dart...
> > client/samples/dartcombat/views.dart:97: return () {
> > On 2012/01/23 01:16:39, sigmund wrote:
> > > overall feels good to chunk everthing in phases (measure, layout, measure,
> > > layout), but syntactically, returning a callback for layout feels really
> > awkward
> > > to me.
> > > 
> > > I'm sure you guys have been debating for a while about this, so forgive me
> if
> > > you already considered something of this sort. I was trying to think if we
> can
> > > work around this awkwardness by using futures for each of these stages
> > > (measure/layout), for example:
> > > 
> > > /**
> > >  * Returns a nested future: the outer future indicates that
> > >  * the frame is ready for measurements, the inner future
> > >  * indicates that the frame is ready for for more layout,
> > >  * but no longer for measurements.
> > >  */
> > > Future<Future> window.requestMeasurementsFrame();
> > > 
> > > // Usage example
> > > final measureReady = window.requestMeasurementsFrame();
> > > measureReady.then((layoutReady) {
> > >   // ... measure
> > >   // ... measure
> > >   layoutReady.then((_) {
> > >     // ... layout
> > >     // ... layout
> > >   });
> > > };
> > > 
> > > Then, if we get await (or something similar) in the language this could be
> > > written linearly as follows:
> > > 
> > > final measureReady = window.requestMeasurementsFrame();
> > > final layoutReady = await measureReady;
> > > // ... measure
> > > // ... measure
> > > await layoutReady;
> > > // ... layout
> > > // ... layout
> > > 
> > > 
> > > So the example here would look as follows:
> > >   window.requestMeasurementFrame().then((layoutReady) {
> > >     final pos = ViewUtil.positionFromEvent(_rootNode, e);
> > >     _boatStartX = pos[0];
> > >     _boatStartY = pos[1];
> > >     layoutReady.then((_) {
> > >       // error case when the mouse was released out of the boat-placing
area
> > >       if (_moveListener != null) {
> > >          _rootNode.on.mouseMove.remove(_moveListener, false);
> > >          _possibleBoat.remove();
> > >          _moveListener = null;
> > >       }
> > >       _possibleBoat = ViewUtil.createDiv("icons boat2");
> > >       ViewUtil.placeNodeAt(_possibleBoat, _boatStartX, _boatStartY);
> > >       _rootNode.nodes.add(_possibleBoat);
> > >       _moveListener = handleMouseMove;
> > >       _rootNode.on.mouseMove.add(_moveListener);
> > >     });
> > >   });
> > > 
> > > Thoughts?
> > 
> > I like the idea of using futures. The syntax without await is a little
> awkward,
> > but no more so than the return-a-closure syntax.
> > 
> > I'd still like to see Future chaining. Here's how the example would look
using
> > Closure-style chaining:
> > 
> >   window.requestMeasurementFrame().then((layoutReady) {
> >     final pos = ViewUtil.positionFromEvent(_rootNode, e);
> >     _boatStartX = pos[0];
> >     _boatStartY = pos[1];
> >     return layoutReady;
> >   }).then((_) {
> >     // error case when the mouse was released out of the boat-placing area
> >     if (_moveListener != null) {
> >        _rootNode.on.mouseMove.remove(_moveListener, false);
> >        _possibleBoat.remove();
> >        _moveListener = null;
> >     }
> >     _possibleBoat = ViewUtil.createDiv("icons boat2");
> >     ViewUtil.placeNodeAt(_possibleBoat, _boatStartX, _boatStartY);
> >     _rootNode.nodes.add(_possibleBoat);
> >     _moveListener = handleMouseMove;
> >     _rootNode.on.mouseMove.add(_moveListener);
> >   });
> 
> The problem with the future chaining variant is layoutReady can't close over
> measured data, which makes the code worse (e.g. capturing a bunch of temporary
> data into class fields, or declaring them as variables outside of the
> requestMeasurementFrame callback ...)