Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039963003:
Add zone task support for request-anim. (Closed)
|
Created:
4 years, 6 months ago by floitsch Modified:
4 years, 5 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd zone task support for request-anim.
R=lrn@google.com
Committed: https://github.com/dart-lang/sdk/commit/726b9f8dc73b9a279ed6eb8bfff51ed091a4395b
Patch Set 1 #
Total comments: 30
Patch Set 2 : Upload #
Total comments: 12
Patch Set 3 : Address comments. Add experimental warning. #
Total comments: 8
Patch Set 4 : Address comments. #
Messages
Total messages: 11 (2 generated)
floitsch@google.com changed reviewers: + jacobr@google.com, lrn@google.com
Depends on https://codereview.chromium.org/2022263002
https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... File tools/dom/templates/html/impl/impl_Window.darttemplate (right): https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:14: * doesn't work. The [Window] class thus keeps a mapping from the id to the second "thus" in two lines. Consider rewording (maybe just remove "thus" here). https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:14: * doesn't work. The [Window] class thus keeps a mapping from the id to the id -> integer ID https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:17: * id. id -> ID https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:19: * Since this mapping takes space, it must be removed as soon as the animation- takes -> takes up https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:20: * frame task triggered. The default-implementations do this automatically, but triggered -> has triggered (or: is triggered, or: triggers) https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:20: * frame task triggered. The default-implementations do this automatically, but default implementations (no '-') Should probably not be plural - the user won't care that you have more than one implementation internally, it's just the default. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:26: /** The id that is returned to users. */ id -> ID https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:32: /** The zone in which the task should run (and cancel). */ (and cancel) -> (or be cancelled) https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:52: * function. This function must be called by the created task before it include->includes (or specification -> specifications). https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:53: * runs the frame-request callback. Sentence is hard to parse (what does "it" refer to, the function or the task?) Also not clear who must call the function (the task?) and whether it happens automatically. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:56: final Window window; Document. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:69: final RemoveFrameRequestMapping removeMapping; Does it make sense to override the removeMapping function? If not, could it just be a fixed function that you must remember to call? https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:169: * Maps animation-frame request ids to their task objects. ids -> IDs https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:234: var task = _animationFrameTasks[id]; Just do _animationFrameTasks.remove(id). That also returns the current value. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:236: _cancelAnimationFrame(id); and return? (otherwise you should reach `task.cancel(this)` below with a null `task`. Add test to catch this?)
https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... File tools/dom/templates/html/impl/impl_Window.darttemplate (right): https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:14: * doesn't work. The [Window] class thus keeps a mapping from the id to the On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > second "thus" in two lines. Consider rewording (maybe just remove "thus" here). reworded. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:14: * doesn't work. The [Window] class thus keeps a mapping from the id to the On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > id -> integer ID Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:17: * id. On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > id -> ID Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:19: * Since this mapping takes space, it must be removed as soon as the animation- On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > takes -> takes up Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:20: * frame task triggered. The default-implementations do this automatically, but On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > default implementations (no '-') > > Should probably not be plural - the user won't care that you have more than one > implementation internally, it's just the default. Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:20: * frame task triggered. The default-implementations do this automatically, but On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > triggered -> has triggered (or: is triggered, or: triggers) Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:26: /** The id that is returned to users. */ On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > id -> ID > Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:32: /** The zone in which the task should run (and cancel). */ On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > (and cancel) -> (or be cancelled) No cancel anymore. -> removed. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:52: * function. This function must be called by the created task before it On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > include->includes (or specification -> specifications). Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:53: * runs the frame-request callback. On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > Sentence is hard to parse (what does "it" refer to, the function or the task?) > Also not clear who must call the function (the task?) and whether it happens > automatically. Removed it. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:56: final Window window; On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > Document. Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:69: final RemoveFrameRequestMapping removeMapping; On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > Does it make sense to override the removeMapping function? > > If not, could it just be a fixed function that you must remember to call? > Moved it as a static function on AnimationFrameTask. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:169: * Maps animation-frame request ids to their task objects. On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > ids -> IDs Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:234: var task = _animationFrameTasks[id]; On 2016/06/07 12:57:35, Lasse Reichstein Nielsen wrote: > Just do _animationFrameTasks.remove(id). That also returns the current value. Done. https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... tools/dom/templates/html/impl/impl_Window.darttemplate:236: _cancelAnimationFrame(id); On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > and return? > (otherwise you should reach `task.cancel(this)` below with a null `task`. Add > test to catch this?) Done.
On 2016/06/10 15:45:29, floitsch wrote: > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > File tools/dom/templates/html/impl/impl_Window.darttemplate (right): > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:14: * doesn't work. The > [Window] class thus keeps a mapping from the id to the > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > second "thus" in two lines. Consider rewording (maybe just remove "thus" > here). > > reworded. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:14: * doesn't work. The > [Window] class thus keeps a mapping from the id to the > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > id -> integer ID > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:17: * id. > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > id -> ID > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:19: * Since this mapping > takes space, it must be removed as soon as the animation- > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > takes -> takes up > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:20: * frame task > triggered. The default-implementations do this automatically, but > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > default implementations (no '-') > > > > Should probably not be plural - the user won't care that you have more than > one > > implementation internally, it's just the default. > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:20: * frame task > triggered. The default-implementations do this automatically, but > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > triggered -> has triggered (or: is triggered, or: triggers) > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:26: /** The id that is > returned to users. */ > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > id -> ID > > > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:32: /** The zone in which > the task should run (and cancel). */ > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > (and cancel) -> (or be cancelled) > > No cancel anymore. -> removed. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:52: * function. This > function must be called by the created task before it > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > include->includes (or specification -> specifications). > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:53: * runs the > frame-request callback. > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > Sentence is hard to parse (what does "it" refer to, the function or the task?) > > Also not clear who must call the function (the task?) and whether it happens > > automatically. > > Removed it. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:56: final Window window; > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > Document. > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:69: final > RemoveFrameRequestMapping removeMapping; > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > Does it make sense to override the removeMapping function? > > > > If not, could it just be a fixed function that you must remember to call? > > > > Moved it as a static function on AnimationFrameTask. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:169: * Maps > animation-frame request ids to their task objects. > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > ids -> IDs > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:234: var task = > _animationFrameTasks[id]; > On 2016/06/07 12:57:35, Lasse Reichstein Nielsen wrote: > > Just do _animationFrameTasks.remove(id). That also returns the current value. > > Done. > > https://codereview.chromium.org/2039963003/diff/1/tools/dom/templates/html/im... > tools/dom/templates/html/impl/impl_Window.darttemplate:236: > _cancelAnimationFrame(id); > On 2016/06/07 12:57:34, Lasse Reichstein Nielsen wrote: > > and return? > > (otherwise you should reach `task.cancel(this)` below with a null `task`. Add > > test to catch this?) > > Done. ping.
lgtm https://codereview.chromium.org/2039963003/diff/20001/tools/dom/templates/htm... File tools/dom/templates/html/impl/impl_Window.darttemplate (right): https://codereview.chromium.org/2039963003/diff/20001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:29: /** The function that is invoked when the user cancels the request. */ -> A function which and consider: when -> if Maybe say why it is exposed? A custom or mock implementation may intercept this call. (I guess that's why it's here). https://codereview.chromium.org/2039963003/diff/20001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:32: /** The zone in which the task should run. */ should -> will https://codereview.chromium.org/2039963003/diff/20001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:41: * The function that should be invoked by user-implemented animation-frame -> A function which must ... https://codereview.chromium.org/2039963003/diff/20001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:64: * The task specification for an animation-frame request. Is there anything more to say about this class? Perhaps when it's used or who will use it. Otherwise it's just repeating the name. https://codereview.chromium.org/2039963003/diff/20001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:73: * The callback that should be executed when the animation-frame is ready. should -> will https://codereview.chromium.org/2039963003/diff/20001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:75: * Note that the callback hasn't been registered in any zone yet. "yet" isn't a good word for a final property which can be retained forever. Maybe "hasn't been registered in a zone when this object is passed to the `Zone.createTask` implementation".
https://chromiumcodereview.appspot.com/2039963003/diff/20001/tools/dom/templa... File tools/dom/templates/html/impl/impl_Window.darttemplate (right): https://chromiumcodereview.appspot.com/2039963003/diff/20001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:29: /** The function that is invoked when the user cancels the request. */ On 2016/06/29 09:22:12, Lasse Reichstein Nielsen wrote: > -> A function which > and consider: when -> if > > Maybe say why it is exposed? A custom or mock implementation may intercept this > call. (I guess that's why it's here). Done. https://chromiumcodereview.appspot.com/2039963003/diff/20001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:32: /** The zone in which the task should run. */ On 2016/06/29 09:22:12, Lasse Reichstein Nielsen wrote: > should -> will Done. https://chromiumcodereview.appspot.com/2039963003/diff/20001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:41: * The function that should be invoked by user-implemented animation-frame On 2016/06/29 09:22:12, Lasse Reichstein Nielsen wrote: > -> A function which must ... Reworded. https://chromiumcodereview.appspot.com/2039963003/diff/20001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:64: * The task specification for an animation-frame request. On 2016/06/29 09:22:12, Lasse Reichstein Nielsen wrote: > Is there anything more to say about this class? Perhaps when it's used or who > will use it. > Otherwise it's just repeating the name. I added an "experimental" note. https://chromiumcodereview.appspot.com/2039963003/diff/20001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:73: * The callback that should be executed when the animation-frame is ready. On 2016/06/29 09:22:12, Lasse Reichstein Nielsen wrote: > should -> will -> is. done. https://chromiumcodereview.appspot.com/2039963003/diff/20001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:75: * Note that the callback hasn't been registered in any zone yet. On 2016/06/29 09:22:12, Lasse Reichstein Nielsen wrote: > "yet" isn't a good word for a final property which can be retained forever. > > Maybe "hasn't been registered in a zone when this object is passed to the > `Zone.createTask` implementation". Reworded. The important thing is, that the create function receives an unregistered function.
lgtm https://chromiumcodereview.appspot.com/2039963003/diff/40001/tools/dom/templa... File tools/dom/templates/html/impl/impl_Window.darttemplate (right): https://chromiumcodereview.appspot.com/2039963003/diff/40001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:19: * Since this mapping takes up space, it must be removed as soon as the as soon as -> when It's not that urgent. https://chromiumcodereview.appspot.com/2039963003/diff/40001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:21: * automatically, but intercepted implementations of `requestAnimationFrame` intercepted -> intercepting ? https://chromiumcodereview.appspot.com/2039963003/diff/40001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:22: * must ensure to call the [AnimationFrameTask.removeMapping] ensure to call -> ensure that they call or: make sure to call. https://chromiumcodereview.appspot.com/2039963003/diff/40001/tools/dom/templa... tools/dom/templates/html/impl/impl_Window.darttemplate:37: * A call to [Window.cancelAnimationFrame] with an id equal to [id] forwards an id -> an `id` argument
https://codereview.chromium.org/2039963003/diff/40001/tools/dom/templates/htm... File tools/dom/templates/html/impl/impl_Window.darttemplate (right): https://codereview.chromium.org/2039963003/diff/40001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:19: * Since this mapping takes up space, it must be removed as soon as the On 2016/07/01 12:46:59, Lasse Reichstein Nielsen wrote: > as soon as -> when > > It's not that urgent. Done. https://codereview.chromium.org/2039963003/diff/40001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:21: * automatically, but intercepted implementations of `requestAnimationFrame` On 2016/07/01 12:46:59, Lasse Reichstein Nielsen wrote: > intercepted -> intercepting > ? Done. https://codereview.chromium.org/2039963003/diff/40001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:22: * must ensure to call the [AnimationFrameTask.removeMapping] On 2016/07/01 12:46:59, Lasse Reichstein Nielsen wrote: > ensure to call -> ensure that they call > or: make sure to call. Done. https://codereview.chromium.org/2039963003/diff/40001/tools/dom/templates/htm... tools/dom/templates/html/impl/impl_Window.darttemplate:37: * A call to [Window.cancelAnimationFrame] with an id equal to [id] forwards On 2016/07/01 12:46:59, Lasse Reichstein Nielsen wrote: > an id -> an `id` argument Done.
Description was changed from ========== Add zone task support for request-anim. ========== to ========== Add zone task support for request-anim. R=lrn@google.com Committed: https://github.com/dart-lang/sdk/commit/726b9f8dc73b9a279ed6eb8bfff51ed091a4395b ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 726b9f8dc73b9a279ed6eb8bfff51ed091a4395b (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
