Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390073003:
Use per-frame WebTaskRunner on HTMLMediaElement (Closed)
|
Created:
4 years, 2 months ago by tzik Modified:
4 years, 2 months ago Reviewers:
haraken CC:
chromium-reviews, nessy, mlamouri+watch-blink_chromium.org, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews, Srirama, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse per-frame WebTaskRunner on HTMLMediaElement
This CL replaces global task runner usage in HTMLMediaElement with
per-frame task runner, so that we can control task priorities per subframes.
The posted task uses MediaElementEvent task type as specified by the spec:
https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements
BUG=624696
Committed: https://crrev.com/7828bcebff00de65eeb90abc52e2d1c71e9c35d1
Cr-Commit-Position: refs/heads/master@{#425259}
Patch Set 1 #Patch Set 2 : Microtask -> MediaElementEvent #
Total comments: 4
Patch Set 3 : s/getExecutionContext()/&document()/ #Messages
Total messages: 24 (16 generated)
The CQ bit was checked by tzik@chromium.org 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...
Description was changed from ========== Use per-frame WebTaskRunner on HTMLMediaElement BUG= ========== to ========== Use per-frame WebTaskRunner on HTMLMediaElement BUG= ==========
tzik@chromium.org changed reviewers: + haraken@chromium.org
Description was changed from ========== Use per-frame WebTaskRunner on HTMLMediaElement BUG= ========== to ========== Use per-frame WebTaskRunner on HTMLMediaElement BUG=624696 ==========
PTAL
Hmm, per the spec, shouldn't we use the MediaElement task source? "Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question." https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements Also please add more explanation to the CL description.
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 tzik@chromium.org 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...
Description was changed from ========== Use per-frame WebTaskRunner on HTMLMediaElement BUG=624696 ========== to ========== Use per-frame WebTaskRunner on HTMLMediaElement This CL replaces global task runner usage in HTMLMediaElement with per-frame task runner, so that we can control task priorities per subframes. The posted task uses MediaElementEvent task type as specified by the spec: https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements BUG=624696 ==========
On 2016/10/04 08:47:46, haraken wrote: > Hmm, per the spec, shouldn't we use the MediaElement task source? > > "Except where otherwise explicitly specified, the task source for all the tasks > queued in this section and its subsections is the media element event task > source of the media element in question." > > https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements > > Also please add more explanation to the CL description. Ah, right. I assumed tasks for Promise resolution should be a Microtask and should use the microtask queue. Updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2390073003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2390073003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3884: TaskRunnerHelper::get(TaskType::MediaElementEvent, getExecutionContext()) Nit: document() is better than getExecutionContext() because getExecutionContext() can return nullptr (although it won't happen in this specific case). https://codereview.chromium.org/2390073003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3909: TaskRunnerHelper::get(TaskType::MediaElementEvent, getExecutionContext()) Ditto.
https://codereview.chromium.org/2390073003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2390073003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3884: TaskRunnerHelper::get(TaskType::MediaElementEvent, getExecutionContext()) On 2016/10/13 13:19:38, haraken wrote: > > Nit: document() is better than getExecutionContext() because > getExecutionContext() can return nullptr (although it won't happen in this > specific case). Done. https://codereview.chromium.org/2390073003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3909: TaskRunnerHelper::get(TaskType::MediaElementEvent, getExecutionContext()) On 2016/10/13 13:19:38, haraken wrote: > > Ditto. Done.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2390073003/#ps40001 (title: "s/getExecutionContext()/&document()/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use per-frame WebTaskRunner on HTMLMediaElement This CL replaces global task runner usage in HTMLMediaElement with per-frame task runner, so that we can control task priorities per subframes. The posted task uses MediaElementEvent task type as specified by the spec: https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements BUG=624696 ========== to ========== Use per-frame WebTaskRunner on HTMLMediaElement This CL replaces global task runner usage in HTMLMediaElement with per-frame task runner, so that we can control task priorities per subframes. The posted task uses MediaElementEvent task type as specified by the spec: https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements BUG=624696 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use per-frame WebTaskRunner on HTMLMediaElement This CL replaces global task runner usage in HTMLMediaElement with per-frame task runner, so that we can control task priorities per subframes. The posted task uses MediaElementEvent task type as specified by the spec: https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements BUG=624696 ========== to ========== Use per-frame WebTaskRunner on HTMLMediaElement This CL replaces global task runner usage in HTMLMediaElement with per-frame task runner, so that we can control task priorities per subframes. The posted task uses MediaElementEvent task type as specified by the spec: https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements BUG=624696 Committed: https://crrev.com/7828bcebff00de65eeb90abc52e2d1c71e9c35d1 Cr-Commit-Position: refs/heads/master@{#425259} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7828bcebff00de65eeb90abc52e2d1c71e9c35d1 Cr-Commit-Position: refs/heads/master@{#425259} |
