Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2717263002:
Convert m_deferredLoadTimer to Unthrottled frame-specific timer and include in didMoveToNewDocument. (Closed)
|
Created:
3 years, 9 months ago by joelhockey Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert m_deferredLoadTimer to Unthrottled frame-specific timer and include in didMoveToNewDocument.
I'm not actually sure what type of timer should be used.
I've used Unthrottled to match other timers in this class. It appears that this timer was missed in the previous conversion for this class https://codereview.chromium.org/2554113002.
BUG=624694
Review-Url: https://codereview.chromium.org/2717263002
Cr-Commit-Position: refs/heads/master@{#453520}
Committed: https://chromium.googlesource.com/chromium/src/+/d071171e53f710f3ac7870f2de47d7bd678720e5
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add m_deferredLoadTimer as Unthrottled frame-specific timer and include in didMoveToNewDocument. #Patch Set 3 : Add m_deferredLoadTimer as Unthrottled frame-specific timer and include in didMoveToNewDocument. #
Messages
Total messages: 24 (14 generated)
Description was changed from ========== Add m_deferredLoadTimer as Unthrottled frame-specific timer and include in didMoveToNewDocument. Convert timers to MediaElementEvent per-frame. BUG=624694 ========== to ========== Convert m_deferredLoadTimer to Unthrottled frame-specific timer and include in didMoveToNewDocument. I'm not actually sure what type of timer should be used. I've used Unthrottled to match other timers in this class. It appears that this timer was missed in the previous conversion for this class https://codereview.chromium.org/2554113002. BUG=624694 ==========
joelhockey@chromium.org changed reviewers: + altimin@chromium.org, haraken@chromium.org, mlamouri@chromium.org, slangley@chromium.org
The CQ bit was checked by joelhockey@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...
https://codereview.chromium.org/2717263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2717263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:522: TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())); Is this your intentional change? You're changing existing Unthrottled timers to MediaElementEvent timers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mlamouri@chromium.org changed reviewers: + foolip@chromium.org
+foolip@ for review as he has more background wrt why things are as they are
https://codereview.chromium.org/2717263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2717263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:522: TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())); On 2017/02/27 07:50:45, haraken wrote: > > Is this your intentional change? You're changing existing Unthrottled timers to > MediaElementEvent timers. Most of these changes are probably correct, but some probably refer to bits that don't correspond to a "queue a task" in the spec. Joel, can you go through the call sites to see which aren't obviously spec algorithms of the sort quoted below, and say something about those cases? https://codereview.chromium.org/2717263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:524: TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())); At the m_deferredLoadTimer.startOneShot call site we are inside HTMLMediaElement::deferLoad() which implements a spec algorithm https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... which should use the "media element event task source" because "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." So, this bit is correct.
> Is this your intentional change? You're changing existing Unthrottled timers to MediaElementEvent timers. Thanks for picking this up. No, my intention was for all timers to be Unthrottled in this cl. I will update the patch soon.
On 2017/02/27 at 20:54:47, joelhockey wrote: > > Is this your intentional change? You're changing existing Unthrottled timers to MediaElementEvent timers. > > Thanks for picking this up. No, my intention was for all timers to be Unthrottled in this cl. I will update the patch soon. everyone ptal. As I explained in my original description, I chose Unthrottled for this timer to match the others. My plan was to submit this CL to at least get this timer converted to be frame-specific, and then we can look back at https://codereview.chromium.org/2713613002 and figure out if we should change any of these Unthrottled timers to be MediaEventElement or something else?
The CQ bit was checked by joelhockey@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...
LGTM Given that other timers on HTMLMediaElement are on unthrottled timers, it would make sense to put the deferred load timer there as well.
lgtm
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 joelhockey@chromium.org
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": 40001, "attempt_start_ts": 1488262321594690,
"parent_rev": "49faab2e95fdb5f6cb63788ac6212676558402bf", "commit_rev":
"d071171e53f710f3ac7870f2de47d7bd678720e5"}
Message was sent while issue was closed.
Description was changed from ========== Convert m_deferredLoadTimer to Unthrottled frame-specific timer and include in didMoveToNewDocument. I'm not actually sure what type of timer should be used. I've used Unthrottled to match other timers in this class. It appears that this timer was missed in the previous conversion for this class https://codereview.chromium.org/2554113002. BUG=624694 ========== to ========== Convert m_deferredLoadTimer to Unthrottled frame-specific timer and include in didMoveToNewDocument. I'm not actually sure what type of timer should be used. I've used Unthrottled to match other timers in this class. It appears that this timer was missed in the previous conversion for this class https://codereview.chromium.org/2554113002. BUG=624694 Review-Url: https://codereview.chromium.org/2717263002 Cr-Commit-Position: refs/heads/master@{#453520} Committed: https://chromium.googlesource.com/chromium/src/+/d071171e53f710f3ac7870f2de47... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d071171e53f710f3ac7870f2de47... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
