Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2642923005:
Migrate Timer to TaskRunnerTimer in ScreenOrientationControllerImpl (Closed)
|
Created:
3 years, 11 months ago by Hwanseung Lee Modified:
3 years, 11 months ago CC:
blink-reviews, chromium-reviews, mlamouri+watch-blink_chromium.org, mlamouri+watch-screen-orientation_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrate Timer to TaskRunnerTimer in ScreenOrientationControllerImpl
Spec that defines :
[1]https://w3c.github.io/screen-orientation/#screenorientation-interface
BUG=624694
Review-Url: https://codereview.chromium.org/2642923005
Cr-Commit-Position: refs/heads/master@{#445856}
Committed: https://chromium.googlesource.com/chromium/src/+/a524dbf5bad0e4c8b479bb0b2915afed6d240d3d
Patch Set 1 #Patch Set 2 : Migrate Timer to TaskRunnerTimer in ScreenOrientationControllerImpl #
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hs1217.lee@samsung.com changed reviewers: + haraken@chromium.org
@haraken PTAL, thank you.
haraken@chromium.org changed reviewers: + mlamouri@chromium.org
Per the spec, it looks like that the task source for the onchange event is not defined. https://w3c.github.io/screen-orientation/#screenorientation-interface Maybe should we use TaskType::MiscPlatformAPI? MiscPlatformAPI is for a task that is defined in the spec but not associated with any task source. mlamouri@: Any idea?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hs1217.lee@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Migrate Timer to TaskRunnerTimer in ScreenOrientationControllerImpl BUG=624694 ========== to ========== Migrate Timer to TaskRunnerTimer in ScreenOrientationControllerImpl Spec that defines : [1]https://w3c.github.io/screen-orientation/#screenorientation-interface BUG=624694 ==========
On 2017/01/20 16:13:24, haraken wrote: > Per the spec, it looks like that the task source for the onchange event is not > defined. > > https://w3c.github.io/screen-orientation/#screenorientation-interface > > Maybe should we use TaskType::MiscPlatformAPI? MiscPlatformAPI is for a task > that is defined in the spec but not associated with any task source. > > mlamouri@: Any idea? first all, i change to Task Type to Misc PlatformAPI according to haraken's guide. i will wait for mlamouri's comment.
haraken@, my understanding of the specification is that we should fire the change event in a "animation frame task" but because it did not exist at the time, the implementation does something slightly different. foolip@, because the concept of "animation frame task" is borrowed from the fullscreen specification, would you have any advice? Unless foolip proposes something that is better, this lgtm FWIW, the spec used to have a task source but it was dropped in favour of the animation frame task.
hs1217.lee@samsung.com changed reviewers: + foolip@chromium.org
On 2017/01/23 00:10:04, mlamouri wrote: > haraken@, my understanding of the specification is that we should fire the > change event in a "animation frame task" but because it did not exist at the > time, the implementation does something slightly different. > > foolip@, because the concept of "animation frame task" is borrowed from the > fullscreen specification, would you have any advice? Unless foolip proposes > something that is better, this lgtm > > FWIW, the spec used to have a task source but it was dropped in favour of the > animation frame task. @foolip how do you think about mlamouri's comment.
On 2017/01/24 12:05:37, Hwanseung Lee wrote: > On 2017/01/23 00:10:04, mlamouri wrote: > > haraken@, my understanding of the specification is that we should fire the > > change event in a "animation frame task" but because it did not exist at the > > time, the implementation does something slightly different. > > > > foolip@, because the concept of "animation frame task" is borrowed from the > > fullscreen specification, would you have any advice? Unless foolip proposes > > something that is better, this lgtm > > > > FWIW, the spec used to have a task source but it was dropped in favour of the > > animation frame task. > > @foolip > how do you think about mlamouri's comment. This change in itself lgtm because I suppose it won't observable change the timing, at least not relative to anything for which the specs define an order. Per https://w3c.github.io/screen-orientation/#handling-screen-orientation-changes it looks like this should be implemented in terms of Document::enqueueAnimationFrameTask, which as mlamouri@ notes didn't exist at the time. However, I've filed https://github.com/w3c/screen-orientation/issues/97 because one could do this in multiple ways. Fullscreen is also a bit odd when exiting, precisely because a single animation frame task is used but multiple documents are affected.
LGTM
The CQ bit was checked by hs1217.lee@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485292623937600,
"parent_rev": "38992dee686ca58d5f1a61d44ef3e534b8c2967c", "commit_rev":
"a524dbf5bad0e4c8b479bb0b2915afed6d240d3d"}
Message was sent while issue was closed.
Description was changed from ========== Migrate Timer to TaskRunnerTimer in ScreenOrientationControllerImpl Spec that defines : [1]https://w3c.github.io/screen-orientation/#screenorientation-interface BUG=624694 ========== to ========== Migrate Timer to TaskRunnerTimer in ScreenOrientationControllerImpl Spec that defines : [1]https://w3c.github.io/screen-orientation/#screenorientation-interface BUG=624694 Review-Url: https://codereview.chromium.org/2642923005 Cr-Commit-Position: refs/heads/master@{#445856} Committed: https://chromium.googlesource.com/chromium/src/+/a524dbf5bad0e4c8b479bb0b2915... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a524dbf5bad0e4c8b479bb0b2915... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
