Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684633005:
Move VTTRegion Timers to TaskRunnerTimer (Closed)
|
Created:
3 years, 10 months ago by maksims (do not use this acc) Modified:
3 years, 10 months ago CC:
fs, blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, gasubic, nessy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove VTTRegion Timers to TaskRunnerTimer
BUG=624694
Review-Url: https://codereview.chromium.org/2684633005
Cr-Commit-Position: refs/heads/master@{#450942}
Committed: https://chromium.googlesource.com/chromium/src/+/c84a352c73d0597256e30ec9caee64c8fd23ed3d
Patch Set 1 #
Total comments: 6
Patch Set 2 : use UnspecedTimer #
Total comments: 5
Patch Set 3 : Move VTTRegion Timers to TaskRunnerTimer #
Messages
Total messages: 29 (16 generated)
Description was changed from ========== timer BUG= ========== to ========== Move VTTRegion Timers to TaskRunnerTimer BUG=624694 ==========
The CQ bit was checked by maksim.sisov@intel.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...
maksim.sisov@intel.com changed reviewers: + foolip@chromium.org, haraken@chromium.org
Please review. I hope I chose right TaskType for this class.
Description was changed from ========== Move VTTRegion Timers to TaskRunnerTimer BUG=624694 ========== to ========== Move VTTRegion Timers to TaskRunnerTimer Use TaskRunnerTimer with MediaElementEvent type, because WebVTT corresponds to https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements BUG=624694 ==========
https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:446: TaskRunnerHelper::get(TaskType::MediaElementEvent, &document), this, foolip@ should know better but I guess this is not a speced task. Maybe TaskType::UnspecedTimer? https://w3c.github.io/webvtt/ https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.h (right): https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.h:155: std::unique_ptr<TaskRunnerTimer<VTTRegion>> m_scrollTimer; Can we just use TaskRunnerTimer<VTTRegion> ?
fs@opera.com changed reviewers: + fs@opera.com
https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:443: void VTTRegion::prepareScrollTimer(Document& document) { Maybe just fold this into startTimer(), and possibly add a new method for checking if the timer is active (that considers the possibility of a null timer - if on-demand allocation is deemed necessary.) Could also fold stopTimer() into it's only caller (to avoid pondering the contract there.) https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:446: TaskRunnerHelper::get(TaskType::MediaElementEvent, &document), this, AFAIK, this timer is an implementation detail, so I guess "UnspecedTimer" would be more appropriate.
https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.h (right): https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.h:155: std::unique_ptr<TaskRunnerTimer<VTTRegion>> m_scrollTimer; On 2017/02/08 11:49:40, haraken wrote: > > Can we just use TaskRunnerTimer<VTTRegion> ? Otherwise I cannot provide TaskRunnerTimer with appropriate task runner, which is taken from document's frame later on, early in ctor
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/2684633005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:446: TaskRunnerHelper::get(TaskType::MediaElementEvent, &document), this, On 2017/02/08 11:57:54, fs wrote: > AFAIK, this timer is an implementation detail, so I guess "UnspecedTimer" would > be more appropriate. Yep, I think so too. If that means that the &document isn't needed, I guess much about this will be simplified? If this timer could lead to any events being fired, and if it could be triggered at a predictable time by scripts, then it'd be a different matter. That it's not speced could always be a spec bug, but if it's unobservable then it just doesn't matter.
https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:95: TaskRunnerHelper::get(TaskType::UnspecedTimer, (Document*)nullptr), What about this solution? I've tested it and it seemed to work.
https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:95: TaskRunnerHelper::get(TaskType::UnspecedTimer, (Document*)nullptr), On 2017/02/14 08:28:58, maksims wrote: > What about this solution? I've tested it and it seemed to work. The cast is a little suspicious. Does just nullptr not work, and is the value here not actually used for anything?
https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:95: TaskRunnerHelper::get(TaskType::UnspecedTimer, (Document*)nullptr), On 2017/02/14 13:37:09, foolip wrote: > On 2017/02/14 08:28:58, maksims wrote: > > What about this solution? I've tested it and it seemed to work. > > The cast is a little suspicious. Does just nullptr not work, and is the value > here not actually used for anything? Check TaskRunnerHelper::get(). It's an overloaded function with different parameters. Nullptr leads to ambiguous call. But if that is casted to a document or anything else, then task runner is taken from the Platform::current().
https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:95: TaskRunnerHelper::get(TaskType::UnspecedTimer, (Document*)nullptr), On 2017/02/16 06:44:05, maksims wrote: > On 2017/02/14 13:37:09, foolip wrote: > > On 2017/02/14 08:28:58, maksims wrote: > > > What about this solution? I've tested it and it seemed to work. > > > > The cast is a little suspicious. Does just nullptr not work, and is the value > > here not actually used for anything? > > Check TaskRunnerHelper::get(). It's an overloaded function with different > parameters. Nullptr leads to ambiguous call. But if that is casted to a document > or anything else, then task runner is taken from the Platform::current(). From cs.chromium.org at a glance, I can't find any other call sites where the second argument is always null, probably because that's a roundabout way of writing Platform::current()->currentThread()->getWebTaskRunner(). Can you use Platform::current()->currentThread()->getWebTaskRunner() instead? Somehow getting the right task queue from the document in which the region will be rendered, something like PS1, feels like the other best option, but that might require some refactoring to not have to do the std::unique_ptr thing.
The CQ bit was checked by maksim.sisov@intel.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...
https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp (right): https://codereview.chromium.org/2684633005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/track/vtt/VTTRegion.cpp:95: TaskRunnerHelper::get(TaskType::UnspecedTimer, (Document*)nullptr), On 2017/02/16 07:46:09, foolip wrote: > On 2017/02/16 06:44:05, maksims wrote: > > On 2017/02/14 13:37:09, foolip wrote: > > > On 2017/02/14 08:28:58, maksims wrote: > > > > What about this solution? I've tested it and it seemed to work. > > > > > > The cast is a little suspicious. Does just nullptr not work, and is the > value > > > here not actually used for anything? > > > > Check TaskRunnerHelper::get(). It's an overloaded function with different > > parameters. Nullptr leads to ambiguous call. But if that is casted to a > document > > or anything else, then task runner is taken from the Platform::current(). > > From http://cs.chromium.org at a glance, I can't find any other call sites where the > second argument is always null, probably because that's a roundabout way of > writing Platform::current()->currentThread()->getWebTaskRunner(). > > Can you use Platform::current()->currentThread()->getWebTaskRunner() instead? > > Somehow getting the right task queue from the document in which the region will > be rendered, something like PS1, feels like the other best option, but that > might require some refactoring to not have to do the std::unique_ptr thing. Done.
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 maksim.sisov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move VTTRegion Timers to TaskRunnerTimer Use TaskRunnerTimer with MediaElementEvent type, because WebVTT corresponds to https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements BUG=624694 ========== to ========== Move VTTRegion Timers to TaskRunnerTimer BUG=624694 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487250054747290,
"parent_rev": "2857e9d1e613b6d375ca72cf0241bd73a4f19b08", "commit_rev":
"c84a352c73d0597256e30ec9caee64c8fd23ed3d"}
Message was sent while issue was closed.
Description was changed from ========== Move VTTRegion Timers to TaskRunnerTimer BUG=624694 ========== to ========== Move VTTRegion Timers to TaskRunnerTimer BUG=624694 Review-Url: https://codereview.chromium.org/2684633005 Cr-Commit-Position: refs/heads/master@{#450942} Committed: https://chromium.googlesource.com/chromium/src/+/c84a352c73d0597256e30ec9caee... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c84a352c73d0597256e30ec9caee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
