Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2082553003:
More documentation for zones. (Closed)
|
Created:
4 years, 6 months ago by floitsch Modified:
4 years, 5 months ago Reviewers:
Lasse Reichstein Nielsen CC:
reviews_dartlang.org, Hixie Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionMore documentation for zones.
R=lrn@google.com
Committed: https://github.com/dart-lang/sdk/commit/9413d62bf620c09a406f1a3204538eee34487bd2
Patch Set 1 #
Total comments: 114
Patch Set 2 : Some comments addressed. #
Total comments: 2
Patch Set 3 : Address comments. #Patch Set 4 : Remove spurious "way". #Patch Set 5 : A few small improvements. #
Total comments: 32
Patch Set 6 : Address comments. #Patch Set 7 : Rebase #Patch Set 8 : Rebase #Patch Set 9 : Address comment. #
Total comments: 2
Messages
Total messages: 14 (2 generated)
floitsch@google.com changed reviewers: + lrn@google.com
This CL depends on the zone-task CLs.
Initial comments, need more time. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:263: * This class wraps zones for delegation. Doesn't make sense. You can wrap a zone, but not (plural) zones. It's not wrapping, that implies having the same interface. Rather it's adapting - except that's an implementation detail and this is an interface. So, maybe something like: * A representation of the "parent" zone of a custom zone. * Allows the implementation of a zone method to delegate the method call to the parent zone while retaining knowledge of the originating zone (as the parameter called `zone`). Or something like that? https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:266: * implementations of most members of zones. This is similar to overriding members of `Zone`? "members" feels weird, since they are all methods. Just use "methods"? https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:269: * A very common operation of intercepting function is to eventually delegate Drop "very". intercepting functions? But "intercepting" is a new word here and hasn't been explained. I guess you just mean a member implementation (like mentioned in the previous paragraph), so keep using the same words for that. So maybe: A custom method implementation typically records or wraps its parameters and then delegates the operation to its parent zone using its provided [ZoneDelegate]. The alternative is to intercept the call and implement the functionality without using the parent zone implementation at all. Because the parent [ZoneDelegate] is provided as a parameter, the same implementation function can be used in multiple zones. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:273: * Providing zone delegates to intercepting functions has two advantages: I don't think this belongs in user-directed documentation. We don't need to sell the implementation, it's already in, and it doesn't help the user use the zone. Just the facts :) https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:311: * A Zone represents an environment that remains stable across asynchronous `Zone` https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:312: * calls, and which is responsible for handling uncaught asynchronous errors, Full stop after "calls", then new paragraph. The zone is responsible for many things, not just handling uncaught errors, so maybe something like: The zone provides a range of functionality that both platform and user code can access. You can create a new zone that overrides some of the functionality of an existing zone. This is similar to creating a new class that extends the base `Zone` class and that overrides some methods, except without actually creating a new class. Instead the overriding methods are provided as functions that explicitly take the equivalent of their own class, the "super" class and the `this` object as parameters. If you override functionality that platform libraries are based on, you can modify or replace the behavior of some core, mostly asynchronous, operations. This includes creating timers, scheduling microtasks and handling uncaught errors. (This is technical and should probably go later in the paragraph, after saying how zones are propagated to async callbacks). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:319: * Code is always executed inside a zone. When a program is started, the "inside" feels a little too concrete for me. How about: Code is always executed in the context of a zone, available as [Zone.current]. The initial `main` function runs in the context of the default zone ([Zone.ROOT]). Code can be run in a different zone using either [runZoned], to create a new zone, or [Zone.run] to run code in the context of an existing zone likely created using [Zone.fork]. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:329: * Asynchronous callbacks always return in the zone in which they have been have been -> are callbacks are functions, when you say that they *return*, it looks like it refers to where they put their value, not where they actually run (which I think is what this is about). Maybe: Asynchronous callbacks always run in the context of the zone where they were scheduled. This is implemented using two steps: - The callback is first registered using one of [...]. This allows the zone to record that a callback exists and to potentially modify the callback. The code doing the registration (e.g., `Future.then`) also remembers the current zone so that it can later run the callback in that zone. - At a later point the registered callback is actually run in the remembered zone. This is all handled internally by the platform code, but if you implement a new asynchronous operation (<your examples below>), you need to follow the same protocol to be "zone compatible". ... (Then the text about where to store the zone (bindCallback etc) fits in) https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:351: * The root zone that is implicitly created. The root zone. This is the zone that the isolate entry function (`main` or spawn function) runs in. Asynchronous events inherit the existing zone, so the entire program will run in this zone if another zone isn't introduced.
Lots of nitpicks on specific sentences (which may or may not be improvements in practice), but very good job writing every all this documentation! https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:370: * listeners. Errors are propagated this way, until, either a listener handles remove both commas. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:373: * zone's [handleUncaughtError]. Mention that we also handle uncuaght errors from other async callbacks, like timers. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:378: * one last time). Actually not. An uncaught error that hits the uncaught error handler will not reschedule the error to the end of the microtask queue. It will reschedule to the *front* of the microtask queue (and multiple errors are queued in order, so the first to happen is the first to be reported). The error might have been delayed before getting to the error handler, but once it hits the uncaught error handler, we don't give other microtasks a chance to run, we only give the current microtask the chance to complete. I think we even interrupt _propagateToListeners at an error. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:388: * on an existing zone. The new zone keeps the forking zone as [parent] zone. Move parentheses to after "an existing zone". Maybe just make it a comma separated clause then. "keeps" is vague. Maybe: "This is the parent zone that this zone was forked from." (And this getter probably shouldn't have been public either, it's bad for abstraction). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:399: * Asynchronous errors never cross zone boundaries of zones with different ... boundaries between zones ... https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:400: * error-zones. error-zones -> error handlers. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:413: * // The following `catchError` is never reached, because the custom zone is never reached -> never sees the error (sounds like the *call* to catchError is never reached). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:414: * // that is created by the call to `runZoned` provides an error handler. -> custom zone created by `runZoned` (just because it's shorter) https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:419: * Note that errors are not entering zones with different error handlers are not entering -> cannot enter Maybe: *enter* (to emphasize). It's not clear what "entering" means. To me, it's basically "not leaving" where "leaving" is going from child zone to parent zone (which is the traditional way for exceptions to flow). It's crossing the boundary between a zone and another zone that isn't a parent of the first zone? Or only child zones? The example is fine, but this is introducing terminology without explaining it. Maybe: cannot enter -> cannot enter a child zones https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:439: Zone get errorZone; And this shouldn't be public either. :) https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:444: * Two zones are in the same error zone if they inherit their Rewrite: if they have the same [errorZone]. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:454: * are automatically delegated to the parent zone (`this`). Maybe: are automatically delegated to -> inherits the behavior of (This is a place where I want to emphasize the "like subclassing" methaphor) https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:457: * `operator []`) as this zone, but updated with the keys and values drop "but". Maybe: The new zone inherits the stored values (accessed through `operator []`) of this zone and updates them with values from `zoneValues`, which either add new values or override existing ones. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:461: * Note that the fork operation is interceptible. A zone can thus replace replace -> change (change covers both "modify" and "replace". I tend to read "replace" as "throw away the old one and create a new from scratch", not as "make a new one that slightly modifies the existing one", and I want to cover both with this description). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:462: * the zone specification (or zone value), giving the parent zone full control value -> values parent zone -> forking zone ("parent zone" here could be seen as referring to [parent], not [this], but it's the latter that is in control of the forking). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:469: * Executes the given function [f] in this zone. We should rename `f` to `action` or something non-minimal. Then: Executes [action] in this zone. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:471: * By default (as implemented in the [ROOT] zone, this updates the [current] By default, as implemented in the [ROOT] zone, this runs [action] with this zone as [current] zone. (I don't like "update", it makes it sound like "current" is modifiable). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:472: * zone to this zone, and executes [f]. Add: If [action] throws, the synchronous exception is not caught by the zone's error handler. Use [runGuarded] to achieve that. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:474: * Since the root zone is the only zone that can modify the [current] getter, the [current] getter -> the value of [current] (never refer to a property as a "getter", it might be a field in a later version). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:475: * custom zones have to delegate to their parent zone if they wish to run custom zone intercepting [run] should always delegate the call to their parent zone, but they can take actions before and after the call. (No reason to leave the door open - always delegate. If someone breaks that, they really need to know what they are doing. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:481: * Executes the given callback [f] with argument [arg] in this zone. arg -> argument consider f -> action. (the "no abbreviations" rule) https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:483: * See [run]. As [run] except that [action] is called with one [argument] instead of none. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:488: * Executes the given callback [f] with argument [arg1] and [arg2] in this Executes [action] with two arguments. As [run] except that [action] is called with the arguments [argument1] and [argument2] instead of no arguments. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:497: * Executes the given function [f] in this zone and catches synchronous f->action. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:503: * return run(f); consider using "this.run" for emphasiz (and "this.handleUncaughtError" below). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:533: * It is good practice to register asynchronous or delayed callbacks before "It is good practice" isn't enough. If you don't, you break zones that needs it. How about: When implementing an asynchronous primitive that uses callbacks, a callback must be registered using this function at the point where the user provides the callback. This allows zones to record other information that they need at the same time, perhaps even wrapping the callback, so that the callback is prepared when it is later run in the same zones (e.g., using [run]). For example ... https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:539: * Returns a potentially new callback that should be used in place of the Returns the, potentially new, callback that will later be run in the zone. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:542: * Custom zones may intercept this operation. Add: The default implementation of [Zone.ROOT] returns the original callback unchanged. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:599: * or [Future] constructors that take an error or a callback that may throw, that take a callback that may throw synchronously, or that take an error directly ... (to make it clear that it's not the "error that may throw"). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:613: * Custom zones may intercept this operation. Maybe add: Implementations of a new asynchronous primitive that converts synchronous errors to asynchronous errors may want to invoke the `errorCallback` on their zone. If the error is always passed into an existing future completer or stream controller, the error will be handled there anyway. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:618: * Runs [f] asynchronously in this zone. f -> action, task or callback. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:620: * The global `scheduleMicrotask` delegates to the current zone's (Ick. We need a scope override in DartDoc to be able to refer to the outer scheduleMicrotask here!) https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:622: * embedder to schedule the given callback as microtask. embedder -> underlying system ("embedder" is not a term understood by end-users). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:630: * Creates a task, given a [create] function and a [specification]. That's actually vacuous. It's a function named "createTask" with two parameters, "create" and "specification", so this line tells us nothing. How about: Creates a task in the current zone. then go on to describe what a task is before describing what the function does? That is, if we can describe what a task is, the text below just says which steps it goes through, but if we use the term, we should be able to define it, even if it ends up as "a task abstracts over <some vague concept>". Maybe: -- A task represents an asynchronous operation or process that reports back by calling a callback function a number of times, usually at least once unless the task operation is cancelled. This function is allows the zone to intercept the initialization of the task while the [runTask] function allows intercepting a call of the callback function. An asynchronous operation makes itself interceptable by calling the zone task methods at the appropriate time. --- https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:634: * future interactions with the zone. Generally, the object is a unique with the zone -> between the zone and the task https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:634: * future interactions with the zone. Generally, the object is a unique -> The object is a unique instance representing the task, and is generally also returned to whoever initiated the task. (It's not just generally unique). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:637: * task object, when users register an event listener. remove comma https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:640: * through the event loop. For example, a timer or an HTTP request both "return through the event loop" is a clever image, but I don't think it matches well enough. A periodic timer doesn't "return" because it reports back more than once. I prefer "reports back by asynchronously invoking a callback function from the event loop". https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:654: * (usually by interacting with the embedder, ar as a native extension), embedder -> underlying system (everywhere) ar -> or https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:658: * provided `create` closure (which may have been replaced at this point). parens into comma. (too mmany parenthesized clauses :) https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:660: * It invokes [Zone.runTask] on the zone the task should run on (which had on the zone in which the task should run (and which was originally passed to the `create` function by `createTask`). https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:664: * is invoked, running Dart code that has been registered to run when the too much "run". Maybe: Eventually the `run` function is invoked with the `argument`, unless a custom zone has aborted it, and that should run the user provided callback. Actually, this describes the behavior of the [runTask] function in detail. Would it be better to just refer to that function and describe the behavior there? https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:666: * not oneshot tasks (see [ZoneSpecification.isOneShot]. missing end paren. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:669: * modifying the task parameters. Add: An operation that wishes to be an interceptable task must publicly specify the types that intercepting code sees: - The specification type (extending [TaskSpecification]) which holds the information available when intercepting the `createTask` call. - The task object type, returned by `createTask` and [create]. - The argument type, if [runTask] takes a meaningful argument. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:672: TaskCreate/*<T, S>*/ create, TaskSpecification/*=S*/ specification); I would expand function types when used for parameters. it's much more readable when you can see that "create" is a function, and even see its type directly, without having to look up "TaskCreate". Ditto below. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:677: * This function is invoked, when an operation, started through [createTask], remove comma before "when" https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:692: TaskRun/*<T, A>*/ run, Object/*=T*/ task, Object/*=A*/ arg); arg->argument. https://codereview.chromium.org/2082553003/diff/1/sdk/lib/async/zone.dart#new... sdk/lib/async/zone.dart:710: * function which makes it possible to intercept the print function. intercept the print function -> intercept printing (or -> intercept calls to the global `print` function). (There have been two "print function" before in the sentence, so it feels repetitive and it's not clear which one you intercept).
https://codereview.chromium.org/2082553003/diff/20001/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/20001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:281: * 2. delegate calls are more efficient way, since the implementation knows how delete "way".
https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:263: * This class wraps zones for delegation. On 2016/06/20 21:19:16, Lasse Reichstein Nielsen wrote: > Doesn't make sense. > You can wrap a zone, but not (plural) zones. > > It's not wrapping, that implies having the same interface. > Rather it's adapting - except that's an implementation detail and this is an > interface. > > So, maybe something like: > > > * A representation of the "parent" zone of a custom zone. > > * Allows the implementation of a zone method to delegate the method call to the > parent zone while retaining knowledge of the originating zone (as the parameter > called `zone`). > > Or something like that? > > > > Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:266: * implementations of most members of zones. This is similar to overriding On 2016/06/20 21:19:15, Lasse Reichstein Nielsen wrote: > members of `Zone`? > > "members" feels weird, since they are all methods. Just use "methods"? Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:269: * A very common operation of intercepting function is to eventually delegate On 2016/06/20 21:19:16, Lasse Reichstein Nielsen wrote: > Drop "very". > intercepting functions? > > But "intercepting" is a new word here and hasn't been explained. I guess you > just mean a member implementation (like mentioned in the previous paragraph), so > keep using the same words for that. > > So maybe: > > A custom method implementation typically records or wraps its parameters and > then delegates the operation to its parent zone using its provided > [ZoneDelegate]. The alternative is to intercept the call and implement the > functionality without using the parent zone implementation at all. > > Because the parent [ZoneDelegate] is provided as a parameter, the same > implementation function can be used in multiple zones. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:273: * Providing zone delegates to intercepting functions has two advantages: On 2016/06/20 21:19:16, Lasse Reichstein Nielsen wrote: > I don't think this belongs in user-directed documentation. We don't need to sell > the implementation, it's already in, and it doesn't help the user use the zone. > Just the facts :) Reworded. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:311: * A Zone represents an environment that remains stable across asynchronous On 2016/06/20 21:19:16, Lasse Reichstein Nielsen wrote: > `Zone` > changed to 'zone'. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:312: * calls, and which is responsible for handling uncaught asynchronous errors, On 2016/06/20 21:19:16, Lasse Reichstein Nielsen wrote: > Full stop after "calls", then new paragraph. > > The zone is responsible for many things, not just handling uncaught errors, so > maybe something like: > > The zone provides a range of functionality that both platform and user code > can access. > You can create a new zone that overrides some of the functionality of an > existing zone. This is similar to creating a new class that extends the > base `Zone` class and that overrides some methods, except without actually > creating a new class. Instead the overriding methods are provided as > functions that explicitly take the equivalent of their own class, the > "super" class and the `this` object as parameters. > > If you override functionality that platform libraries are based on, you can > modify or replace the behavior of some core, mostly asynchronous, operations. > This includes creating timers, scheduling microtasks and handling uncaught > errors. > > (This is technical and should probably go later in the paragraph, after saying > how zones are propagated to async callbacks). Incorporated (partially) your text. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:319: * Code is always executed inside a zone. When a program is started, the On 2016/06/20 21:19:16, Lasse Reichstein Nielsen wrote: > "inside" feels a little too concrete for me. > How about: > > Code is always executed in the context of a zone, available as [Zone.current]. > The initial `main` function runs in the context of the default zone > ([Zone.ROOT]). Code can be run in a different zone using either [runZoned], to > create a new zone, or [Zone.run] to run code in the context of an existing zone > likely created using [Zone.fork]. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:329: * Asynchronous callbacks always return in the zone in which they have been On 2016/06/20 21:19:16, Lasse Reichstein Nielsen wrote: > have been -> are > > callbacks are functions, when you say that they *return*, it looks like it > refers to where they put their value, not where they actually run (which I think > is what this is about). > > Maybe: > Asynchronous callbacks always run in the context of the zone where they were > scheduled. This is implemented using two steps: > - The callback is first registered using one of [...]. This allows the zone to > record that a callback exists and to potentially modify the callback. The code > doing the registration (e.g., `Future.then`) also remembers the current zone so > that it can later run the callback in that zone. > - At a later point the registered callback is actually run in the remembered > zone. > > This is all handled internally by the platform code, but if you implement a new > asynchronous operation (<your examples below>), you need to follow the same > protocol to be "zone compatible". > > ... > (Then the text about where to store the zone (bindCallback etc) fits in) Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:351: * The root zone that is implicitly created. On 2016/06/20 21:19:16, Lasse Reichstein Nielsen wrote: > The root zone. > > This is the zone that the isolate entry function (`main` or spawn function) runs > in. Asynchronous events inherit the existing zone, so the entire program will > run in this zone if another zone isn't introduced. somehow incorporated. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:370: * listeners. Errors are propagated this way, until, either a listener handles On 2016/06/22 14:32:01, Lasse Reichstein Nielsen wrote: > remove both commas. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:373: * zone's [handleUncaughtError]. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > Mention that we also handle uncuaght errors from other async callbacks, like > timers. Reworded. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:378: * one last time). On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > Actually not. > An uncaught error that hits the uncaught error handler will not reschedule the > error to the end of the microtask queue. It will reschedule to the *front* of > the microtask queue (and multiple errors are queued in order, so the first to > happen is the first to be reported). > > The error might have been delayed before getting to the error handler, but once > it hits the uncaught error handler, we don't give other microtasks a chance to > run, we only give the current microtask the chance to complete. > I think we even interrupt _propagateToListeners at an error. Removed the mention of microtask. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:388: * on an existing zone. The new zone keeps the forking zone as [parent] zone. On 2016/06/22 14:32:01, Lasse Reichstein Nielsen wrote: > Move parentheses to after "an existing zone". Maybe just make it a comma > separated clause then. > > "keeps" is vague. Maybe: > "This is the parent zone that this zone was forked from." > > (And this getter probably shouldn't have been public either, it's bad for > abstraction). I don't know anymore why it was added. Probably because we needed it in the async library... done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:399: * Asynchronous errors never cross zone boundaries of zones with different On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > ... boundaries between zones ... Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:400: * error-zones. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > error-zones -> error handlers. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:413: * // The following `catchError` is never reached, because the custom zone On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > is never reached -> never sees the error > > (sounds like the *call* to catchError is never reached). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:414: * // that is created by the call to `runZoned` provides an error handler. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > -> custom zone created by `runZoned` > > (just because it's shorter) Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:419: * Note that errors are not entering zones with different error handlers On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > are not entering -> cannot enter > Maybe: *enter* > (to emphasize). > > It's not clear what "entering" means. To me, it's basically "not leaving" where > "leaving" is going from child zone to parent zone (which is the traditional way > for exceptions to flow). > It's crossing the boundary between a zone and another zone that isn't a parent > of the first zone? Or only child zones? > > The example is fine, but this is introducing terminology without explaining it. > > Maybe: cannot enter -> cannot enter a child zones Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:439: Zone get errorZone; On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > And this shouldn't be public either. :) That one is definitely used in the async library. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:444: * Two zones are in the same error zone if they inherit their On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > Rewrite: if they have the same [errorZone]. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:454: * are automatically delegated to the parent zone (`this`). On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > Maybe: are automatically delegated to -> inherits the behavior of > > (This is a place where I want to emphasize the "like subclassing" methaphor) > Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:457: * `operator []`) as this zone, but updated with the keys and values On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > drop "but". > Maybe: The new zone inherits the stored values (accessed through `operator []`) > of this zone and updates them with values from `zoneValues`, which either add > new values or override existing ones. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:461: * Note that the fork operation is interceptible. A zone can thus replace On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > replace -> change > (change covers both "modify" and "replace". I tend to read "replace" as "throw > away the old one and create a new from scratch", not as "make a new one that > slightly modifies the existing one", and I want to cover both with this > description). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:462: * the zone specification (or zone value), giving the parent zone full control On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > value -> values > parent zone -> forking zone > ("parent zone" here could be seen as referring to [parent], not [this], but it's > the latter that is in control of the forking). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:469: * Executes the given function [f] in this zone. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > We should rename `f` to `action` or something non-minimal. > Then: > Executes [action] in this zone. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:471: * By default (as implemented in the [ROOT] zone, this updates the [current] On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > By default, as implemented in the [ROOT] zone, this runs [action] with this zone > as [current] zone. > > (I don't like "update", it makes it sound like "current" is modifiable). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:472: * zone to this zone, and executes [f]. On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > Add: > If [action] throws, the synchronous exception is not caught by the zone's error > handler. Use [runGuarded] to achieve that. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:474: * Since the root zone is the only zone that can modify the [current] getter, On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > the [current] getter -> the value of [current] > > (never refer to a property as a "getter", it might be a field in a later > version). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:475: * custom zones have to delegate to their parent zone if they wish to run On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > custom zone intercepting [run] should always delegate the call to their parent > zone, but they can take actions before and after the call. > > (No reason to leave the door open - always delegate. If someone breaks that, > they really need to know what they are doing. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:481: * Executes the given callback [f] with argument [arg] in this zone. On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > arg -> argument > consider f -> action. > (the "no abbreviations" rule) Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:483: * See [run]. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > As [run] except that [action] is called with one [argument] instead of none. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:488: * Executes the given callback [f] with argument [arg1] and [arg2] in this On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > Executes [action] with two arguments. > > As [run] except that [action] is called with the arguments [argument1] and > [argument2] instead of no arguments. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:497: * Executes the given function [f] in this zone and catches synchronous On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > f->action. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:503: * return run(f); On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > consider using "this.run" for emphasiz (and "this.handleUncaughtError" below). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:533: * It is good practice to register asynchronous or delayed callbacks before On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > "It is good practice" isn't enough. If you don't, you break zones that needs it. > How about: > > When implementing an asynchronous primitive that uses callbacks, a callback must > be registered using this function at the point where the user provides the > callback. This allows zones to record other information that they need at the > same time, perhaps even wrapping the callback, so that the callback is prepared > when it is later run in the same zones (e.g., using [run]). > For example ... Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:539: * Returns a potentially new callback that should be used in place of the On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > Returns the, potentially new, callback that will later be run in the zone. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:542: * Custom zones may intercept this operation. On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > Add: The default implementation of [Zone.ROOT] returns the original callback > unchanged. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:599: * or [Future] constructors that take an error or a callback that may throw, On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > that take a callback that may throw synchronously, or that take an error > directly ... > (to make it clear that it's not the "error that may throw"). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:613: * Custom zones may intercept this operation. On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > Maybe add: > Implementations of a new asynchronous primitive that converts synchronous errors > to asynchronous errors may want to invoke the `errorCallback` on their zone. If > the error is always passed into an existing future completer or stream > controller, the error will be handled there anyway. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:618: * Runs [f] asynchronously in this zone. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > f -> action, task or callback. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:620: * The global `scheduleMicrotask` delegates to the current zone's On 2016/06/22 14:32:01, Lasse Reichstein Nielsen wrote: > (Ick. We need a scope override in DartDoc to be able to refer to the outer > scheduleMicrotask here!) Acknowledged. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:622: * embedder to schedule the given callback as microtask. On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > embedder -> underlying system > > ("embedder" is not a term understood by end-users). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:630: * Creates a task, given a [create] function and a [specification]. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > That's actually vacuous. It's a function named "createTask" with two parameters, > "create" and "specification", so this line tells us nothing. > > How about: > > Creates a task in the current zone. > > then go on to describe what a task is before describing what the function does? > > That is, if we can describe what a task is, the text below just says which steps > it goes through, but if we use the term, we should be able to define it, even if > it ends up as "a task abstracts over <some vague concept>". > > Maybe: > -- > A task represents an asynchronous operation or process that reports back by > calling a callback function a number of times, usually at least once unless the > task operation is cancelled. > > This function is allows the zone to intercept the initialization of the task > while the [runTask] function allows intercepting a call of the callback > function. An asynchronous operation makes itself interceptable by calling the > zone task methods at the appropriate time. > --- Partially incorporated. In particular I don't want to use "callback" for 'runTask', since that's not really the case. The 'runTask' gets a function that does an operation. It doesn't need to be a callback. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:634: * future interactions with the zone. Generally, the object is a unique On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > with the zone -> between the zone and the task Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:634: * future interactions with the zone. Generally, the object is a unique On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > -> The object is a unique instance representing the task, and is generally also > returned to whoever initiated the task. > > (It's not just generally unique). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:637: * task object, when users register an event listener. On 2016/06/22 14:32:01, Lasse Reichstein Nielsen wrote: > remove comma Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:640: * through the event loop. For example, a timer or an HTTP request both On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > "return through the event loop" is a clever image, but I don't think it matches > well enough. A periodic timer doesn't "return" because it reports back more than > once. > I prefer "reports back by asynchronously invoking a callback function from the > event loop". > Partially incorporated. Kept the 'return' in the next sentence. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:654: * (usually by interacting with the embedder, ar as a native extension), On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > embedder -> underlying system (everywhere) > ar -> or Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:658: * provided `create` closure (which may have been replaced at this point). On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > parens into comma. (too mmany parenthesized clauses :) Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:660: * It invokes [Zone.runTask] on the zone the task should run on (which had On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > on the zone in which the task should run (and which was originally passed to the > `create` function by `createTask`). Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:664: * is invoked, running Dart code that has been registered to run when the On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > too much "run". > Maybe: > > Eventually the `run` function is invoked with the `argument`, unless a custom > zone has aborted it, and that should run the user provided callback. > > > Actually, this describes the behavior of the [runTask] function in detail. Would > it be better to just refer to that function and describe the behavior there? Shortened it. I don't want to use 'callback' since that's not always the case. Sometimes it completes futures, triggers streams ... I prefer to keep the whole task-description in one place. It makes the doc on runTask slightly less useful, but should help understand the concepts. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:666: * not oneshot tasks (see [ZoneSpecification.isOneShot]. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > missing end paren. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:669: * modifying the task parameters. On 2016/06/22 14:32:02, Lasse Reichstein Nielsen wrote: > Add: > An operation that wishes to be an interceptable task must publicly specify the > types that intercepting code sees: > - The specification type (extending [TaskSpecification]) which holds the > information available when intercepting the `createTask` call. > - The task object type, returned by `createTask` and [create]. > - The argument type, if [runTask] takes a meaningful argument. > Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:672: TaskCreate/*<T, S>*/ create, TaskSpecification/*=S*/ specification); On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > I would expand function types when used for parameters. it's much more readable > when you can see that "create" is a function, and even see its type directly, > without having to look up "TaskCreate". > > Ditto below. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:677: * This function is invoked, when an operation, started through [createTask], On 2016/06/22 14:32:01, Lasse Reichstein Nielsen wrote: > remove comma before "when" Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:692: TaskRun/*<T, A>*/ run, Object/*=T*/ task, Object/*=A*/ arg); On 2016/06/22 14:32:01, Lasse Reichstein Nielsen wrote: > arg->argument. Done. https://chromiumcodereview.appspot.com/2082553003/diff/1/sdk/lib/async/zone.d... sdk/lib/async/zone.dart:710: * function which makes it possible to intercept the print function. On 2016/06/22 14:32:03, Lasse Reichstein Nielsen wrote: > intercept the print function -> intercept printing > (or -> intercept calls to the global `print` function). > > (There have been two "print function" before in the sentence, so it feels > repetitive and it's not clear which one you intercept). Done. https://chromiumcodereview.appspot.com/2082553003/diff/20001/sdk/lib/async/zo... File sdk/lib/async/zone.dart (right): https://chromiumcodereview.appspot.com/2082553003/diff/20001/sdk/lib/async/zo... sdk/lib/async/zone.dart:281: * 2. delegate calls are more efficient way, since the implementation knows how On 2016/06/30 14:37:47, Lasse Reichstein Nielsen wrote: > delete "way". Done.
More comments, not all. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:60: // Typedefs for methods, that will be deprecated once tasks are remove comma. This is not a dart-doc, so it doesn't document a declaration. It covers both the following typedefs, which isn't clear. Maybe: The following typedef declarations are used by functionality which will be removed and replaced by tasks if the task experiment is successful. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:338: * one where the `then` was invoked. Consider moving this paragraph somewhere after "Code is always executed in the context of a zone" since that paragraph defines what "the zone the have been queued in" means. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:359: * scheduled. This is implemented using two steps: Is this repeating paragraph 2 above? (if so, maybe just remove paragraph 2 instead of moving it). https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:387: * the root zone. Without any custom zone the whole program always runs in the Without any custom zone .. -> If no custom zone is created, the rest of the program will always run in the root zone. (drop the "Zone.current identical to [Zone.ROOT]" part, it sounds trivial when there are no other zones. If anything, add it as a comment to the first sentence: ... start running in the root zone (that is, [Zone.current] is [Zone.ROOT] when the entry function is called). https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:391: * Many methods, like [registerCallback] do the bare minimum, others, like ... do the bare minimum, and are only provided as a hook for a custom zone to do something. Othes, like ... I'm not entirely happy about an unqualified "bare minimum". I'd prefer if it was something like "bare minimum to do something" or a "bare minimum of something". Maybe "bare minimum required of the function". https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:392: * [scheduleMicrotask] interact with the underlying system to implement the comma before interact. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:408: * 1. uncaught errors that were thrown in asynchronous callbacks. For example, Start with capital letter. I'd prefer a comma instead of a full stop before "for example". https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:409: * a `throw` in [Timer.run]. ... a throw in the function passed to [Timer.run]. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:410: * 2. asynchronous errors that are pushed down [Future] and [Stream] chains Start with capital letter again. maybe: down -> through https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:411: * but for which no child registered an error handled. Comma before but (it's optional, but I would prefer it here). handled -> handler https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:419: * treated like synchronous uncaught exceptions. I'd say "uncaught synchronous exceptions". I just think it sounds better, and it's consistent with "uncaught async errors" above, but your call. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:424: * Returns the parent zone. Describe a getter like a property, not a method. In particular, no "Returns", it's just "The parent zone of the current zone". https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:426: * Returns `null` if `this` is the [ROOT] zone. -> Is `null` if ... https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:462: * Note that errors can not enter a child zone with a different error handler can not -> cannot (or can't). https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:652: * or for some [Future] constructors the current zone is allowed to intercept remove "for". Consider a comma before "the current" - I'm not sure it's required grammatically, but I need a break there when reading.
https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:60: // Typedefs for methods, that will be deprecated once tasks are On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > remove comma. > > This is not a dart-doc, so it doesn't document a declaration. It covers both the > following typedefs, which isn't clear. > > Maybe: > The following typedef declarations are used by functionality > which will be removed and replaced by tasks > if the task experiment is successful. > > Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:338: * one where the `then` was invoked. On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > Consider moving this paragraph somewhere after "Code is always executed in the > context of a zone" since that paragraph defines what "the zone the have been > queued in" means. Done. Removed. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:359: * scheduled. This is implemented using two steps: On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > Is this repeating paragraph 2 above? (if so, maybe just remove paragraph 2 > instead of moving it). Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:387: * the root zone. Without any custom zone the whole program always runs in the On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > Without any custom zone .. -> > If no custom zone is created, the rest of the program will always run in the > root zone. > > (drop the "Zone.current identical to [Zone.ROOT]" part, it sounds trivial when > there are no other zones. If anything, add it as a comment to the first > sentence: > > ... start running in the root zone (that is, [Zone.current] is [Zone.ROOT] when > the entry function is called). Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:391: * Many methods, like [registerCallback] do the bare minimum, others, like On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > ... do the bare minimum, and are only provided as a hook for a custom zone to do > something. Othes, like ... > > I'm not entirely happy about an unqualified "bare minimum". I'd prefer if it was > something like "bare minimum to do something" or a "bare minimum of something". > Maybe "bare minimum required of the function". Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:392: * [scheduleMicrotask] interact with the underlying system to implement the On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > comma before interact. Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:408: * 1. uncaught errors that were thrown in asynchronous callbacks. For example, On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > Start with capital letter. > I'd prefer a comma instead of a full stop before "for example". Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:409: * a `throw` in [Timer.run]. On 2016/07/01 11:25:02, Lasse Reichstein Nielsen wrote: > ... a throw in the function passed to [Timer.run]. Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:410: * 2. asynchronous errors that are pushed down [Future] and [Stream] chains On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > Start with capital letter again. > maybe: down -> through Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:411: * but for which no child registered an error handled. On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > Comma before but (it's optional, but I would prefer it here). > handled -> handler Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:419: * treated like synchronous uncaught exceptions. On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > I'd say "uncaught synchronous exceptions". > I just think it sounds better, and it's consistent with "uncaught async errors" > above, but your call. Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:424: * Returns the parent zone. On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > Describe a getter like a property, not a method. In particular, no "Returns", > it's just "The parent zone of the current zone". Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:426: * Returns `null` if `this` is the [ROOT] zone. On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > -> Is `null` if ... Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:462: * Note that errors can not enter a child zone with a different error handler On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > can not -> cannot (or can't). Done. https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:652: * or for some [Future] constructors the current zone is allowed to intercept On 2016/07/01 11:25:01, Lasse Reichstein Nielsen wrote: > remove "for". Consider a comma before "the current" - I'm not sure it's required > grammatically, but I need a break there when reading. Done.
https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:498: * The new zone inherits the stored values (accessed through `operator []`) [operator []] (if you can write that in DartDoc)
https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/80001/sdk/lib/async/zone.dart... sdk/lib/async/zone.dart:498: * The new zone inherits the stored values (accessed through `operator []`) On 2016/07/04 14:14:19, Lasse Reichstein Nielsen wrote: > [operator []] > > (if you can write that in DartDoc) Dartdoc doesn't link it, but at least doesn't "break" it (treats it like code). done.
lgtm https://codereview.chromium.org/2082553003/diff/160001/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/160001/sdk/lib/async/zone.dar... sdk/lib/async/zone.dart:176: this.createPeriodicTimer: null, Did createTask disappear?
https://codereview.chromium.org/2082553003/diff/160001/sdk/lib/async/zone.dart File sdk/lib/async/zone.dart (right): https://codereview.chromium.org/2082553003/diff/160001/sdk/lib/async/zone.dar... sdk/lib/async/zone.dart:176: this.createPeriodicTimer: null, On 2016/07/11 11:18:29, Lasse Reichstein Nielsen wrote: > Did createTask disappear? I moved the comments into the CLs that added them (they were not committed yet).
Description was changed from ========== More documentation for zones. ========== to ========== More documentation for zones. R=lrn@google.com Committed: https://github.com/dart-lang/sdk/commit/9413d62bf620c09a406f1a3204538eee34487bd2 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as 9413d62bf620c09a406f1a3204538eee34487bd2 (presubmit successful). |
