Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808843003:
[scheduler] Change TaskQueue observer call mechanism. (Closed)
Description[scheduler] Change TaskQueue observer call mechanism.
Instead of manually calling TaskQueue observer we can rely on TimeDomain
calling SetScheduledTimeDomainWakeUp and we can call the observer from
there.
This patch also fixes the problem where pages were stuck due to a missing
notification. Test coverage is improved and HasPendingImmediateWork renamed
to HasTaskToRunImmediately to better reflect its meaning.
R=alexclarke@chromium.org,skyostil@chromium.org
BUG=699541, 710095
Review-Url: https://codereview.chromium.org/2808843003
Cr-Commit-Position: refs/heads/master@{#470957}
Committed: https://chromium.googlesource.com/chromium/src/+/0f67a0c1b7f0e461114771b4297f5fc0a0202cee
Patch Set 1 #
Total comments: 4
Patch Set 2 : addressed comments #
Total comments: 6
Patch Set 3 : Tests & comments #
Total comments: 24
Patch Set 4 : Addressed comments from alexclarke@ #Patch Set 5 : Addressed comments #Patch Set 6 : Fix compilation #Messages
Total messages: 46 (29 generated)
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Alex, please also take a look -- I assume you guys talked about this offline? https://codereview.chromium.org/2808843003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2808843003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:185: // Used check if we need to generate notifications about delayed work. "Used to check"? https://codereview.chromium.org/2808843003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:186: bool HasPendingImmediateWorkWithoutDelayedIncomingQueue(); This name is pretty hard to parse. HasPendingNonDelayedWork or something? Edit: looks like that's not really accurate either. Can you suggest a better name?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. It's time to land this one too. https://codereview.chromium.org/2808843003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2808843003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:185: // Used check if we need to generate notifications about delayed work. On 2017/04/10 17:45:07, Sami wrote: > "Used to check"? Done. https://codereview.chromium.org/2808843003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:186: bool HasPendingImmediateWorkWithoutDelayedIncomingQueue(); On 2017/04/10 17:45:07, Sami wrote: > This name is pretty hard to parse. HasPendingNonDelayedWork or something? > > Edit: looks like that's not really accurate either. Can you suggest a better > name? HasPendingImmediateWork -> HasTasksToRunImmediately HasPendingImmediateWorkWithoutDelayedIncomingQueue -> HasPendingImmediateWork Done.
Thanks! https://codereview.chromium.org/2808843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2808843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:824: if (HasPendingImmediateWork() && main_thread_only().observer) { This could also use a comment saying why we need to ignore ready incoming delayed work. https://codereview.chromium.org/2808843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:825: main_thread_only().observer->OnQueueNextWakeUpChanged(this, Could you add a TODO where OnQueueNextWakeUpChanged is defined to change the parameter to an optional TimeTicks so we can tell the observer about cancellations too? https://codereview.chromium.org/2808843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:915: if (HasPendingImmediateWork()) I'm not sure why we need to ignore ready incoming delayed tasks here -- could you add a comment?
The CQ bit was checked by altimin@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 ========== [scheduler] Change TaskQueue observer call mechanism. Instead of manually calling NotifyWakeUpChangedOnMainThread issue a call to TaskQueue::Observer in TaskQueue::SetScheduledTimeDomainWakeUp. R=alexclarke@chromium.org,skyostil@chromium.org BUG=699541 ========== to ========== [scheduler] Change TaskQueue observer call mechanism. Instead of manually calling NotifyWakeUpChangedOnMainThread issue a call to TaskQueue::Observer in TaskQueue::SetScheduledTimeDomainWakeUp. R=alexclarke@chromium.org,skyostil@chromium.org BUG=699541,710095 ==========
PTAL https://codereview.chromium.org/2808843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2808843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:824: if (HasPendingImmediateWork() && main_thread_only().observer) { On 2017/05/08 17:33:37, Sami wrote: > This could also use a comment saying why we need to ignore ready incoming > delayed work. Done. https://codereview.chromium.org/2808843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:825: main_thread_only().observer->OnQueueNextWakeUpChanged(this, On 2017/05/08 17:33:37, Sami wrote: > Could you add a TODO where OnQueueNextWakeUpChanged is defined to change the > parameter to an optional TimeTicks so we can tell the observer about > cancellations too? Done. https://codereview.chromium.org/2808843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:915: if (HasPendingImmediateWork()) On 2017/05/08 17:33:37, Sami wrote: > I'm not sure why we need to ignore ready incoming delayed tasks here -- could > you add a comment? Done.
I don't understand the motivation behind this patch. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:293: ScheduleDelayedWorkInTimeDomain(now); Should we notify here? https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:183: // Check for pending immediate work, but do not look in s/but do not look in/but does not look in Also please reformat. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:213: base::Optional<base::TimeTicks> scheduled_time_domain_wake_up() const { Why do we need to use optional here? What's wrong with using zero as no wakeup?
Description was changed from ========== [scheduler] Change TaskQueue observer call mechanism. Instead of manually calling NotifyWakeUpChangedOnMainThread issue a call to TaskQueue::Observer in TaskQueue::SetScheduledTimeDomainWakeUp. R=alexclarke@chromium.org,skyostil@chromium.org BUG=699541,710095 ========== to ========== [scheduler] Change TaskQueue observer call mechanism. Instead of manually calling TaskQueue observer we can rely on TimeDomain calling SetScheduledTimeDomainWakeUp and we can call the observer from there. This patch also fixes the problem where pages were stuck due to a missing notification. Test coverage is improved and HasPendingImmediateWork renamed to HasTaskToRunImmediately to better reflect its meaning. R=alexclarke@chromium.org,skyostil@chromium.org BUG=699541,710095 ==========
OK so I understand now the motivation! :) A few comments. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue.h (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue.h:41: // TODO(altimin): Make it base::Optional<base::TimeTicks> to tell Probably a discussion for the follow on patch but I'd appreciate it if we can have a quick whiteboard session before changing this API. I think that'll make things much soother. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (left): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:903: NotifyWakeUpChangedOnMainThread( For my education, was this a bug since it looks like it would have overwritten the immediate time from TaskQueueImpl::EnableOrDisableWithSelector? https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:912: if (!scheduled_time_domain_wake_up) nit: Stylistically we tend to collapse all these onto one if https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:180: void SetScheduledTimeDomainWakeUp( Can you please add a brief comment mentioning that there is exactly one wake up, calling this cancels any previous wake up and if |scheduled_time_domain_wake_up| is nullopt then no wakeup will occur. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:181: base::Optional<base::TimeTicks> scheduled_time_domain_wake_up); Also please document which threads these two calls can be called from. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1842: std::unique_ptr<TimeDomain> mock_time_domain = Seems like we're testing several things here, should we split this into two tests?
Description was changed from ========== [scheduler] Change TaskQueue observer call mechanism. Instead of manually calling TaskQueue observer we can rely on TimeDomain calling SetScheduledTimeDomainWakeUp and we can call the observer from there. This patch also fixes the problem where pages were stuck due to a missing notification. Test coverage is improved and HasPendingImmediateWork renamed to HasTaskToRunImmediately to better reflect its meaning. R=alexclarke@chromium.org,skyostil@chromium.org BUG=699541,710095 ========== to ========== [scheduler] Change TaskQueue observer call mechanism. Instead of manually calling TaskQueue observer we can rely on TimeDomain calling SetScheduledTimeDomainWakeUp and we can call the observer from there. This patch also fixes the problem where pages were stuck due to a missing notification. Test coverage is improved and HasPendingImmediateWork renamed to HasTaskToRunImmediately to better reflect its meaning. R=alexclarke@chromium.org,skyostil@chromium.org BUG=699541,710095 ==========
PTAL https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue.h (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue.h:41: // TODO(altimin): Make it base::Optional<base::TimeTicks> to tell On 2017/05/09 12:50:50, alex clarke wrote: > Probably a discussion for the follow on patch but I'd appreciate it if we can > have a quick whiteboard session before changing this API. I think that'll make > things much soother. Acknowledged. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (left): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:903: NotifyWakeUpChangedOnMainThread( On 2017/05/09 12:50:50, alex clarke wrote: > For my education, was this a bug since it looks like it would have overwritten > the immediate time from TaskQueueImpl::EnableOrDisableWithSelector? Yes, but strictly speaking this wasn't a real bug because task queue throttler is the only user and it is resistant to extra notifications. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:293: ScheduleDelayedWorkInTimeDomain(now); On 2017/05/09 11:51:28, alex clarke wrote: > Should we notify here? No, time domain will call ScheduleDelayedWorkInTimeDomain and it will issue the notification. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:912: if (!scheduled_time_domain_wake_up) On 2017/05/09 12:50:50, alex clarke wrote: > nit: Stylistically we tend to collapse all these onto one if I think that in this case the conditions are different and they warrant separate ifs. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:180: void SetScheduledTimeDomainWakeUp( On 2017/05/09 12:50:50, alex clarke wrote: > Can you please add a brief comment mentioning that there is exactly one wake up, > calling this cancels any previous wake up and if |scheduled_time_domain_wake_up| > is nullopt then no wakeup will occur. Done. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:181: base::Optional<base::TimeTicks> scheduled_time_domain_wake_up); On 2017/05/09 12:50:50, alex clarke wrote: > Also please document which threads these two calls can be called from. Done. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:183: // Check for pending immediate work, but do not look in On 2017/05/09 11:51:28, alex clarke wrote: > s/but do not look in/but does not look in > > Also please reformat. Done. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:213: base::Optional<base::TimeTicks> scheduled_time_domain_wake_up() const { On 2017/05/09 11:51:28, alex clarke wrote: > Why do we need to use optional here? What's wrong with using zero as no wakeup? I prefer explicit base::nullopt to describe "no wake-up" situation. Helps with readability. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1842: std::unique_ptr<TimeDomain> mock_time_domain = On 2017/05/09 12:50:51, alex clarke wrote: > Seems like we're testing several things here, should we split this into two > tests? It's my understanding that we're testing only one thing here. The comment above explains why we need a time domain to achieve the desired effect.
lgtm
The CQ bit was checked by altimin@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:912: if (!scheduled_time_domain_wake_up) On 2017/05/09 13:15:03, altimin wrote: > On 2017/05/09 12:50:50, alex clarke wrote: > > nit: Stylistically we tend to collapse all these onto one if > > I think that in this case the conditions are different and they warrant separate > ifs. One of the style guide rules is to be consistent with other code. Elsewhere in the scheduler we tend to merge these into one line. I don't see anything special here that warrants them being on multiple lines. I don't think it helps readability ether. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1838: start_time + delay1s)); This is testing something that is kind of incidental to this test that I believe we already have sufficient coverage for. My suggestion is to change this to EXPECT_CALL(observer, OnQueueNextWakeUpChanged((_, _)); https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1848: EXPECT_CALL(observer, This is the thing we really care about right? Incidentally .Times(1) is I believe a nop although it's OK to leave it but please be consistent about using it or not.
So I would prefer if you make those changes but I won't block this patch on them. Let me know what you want to do.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
On 2017/05/10 10:26:39, alex clarke wrote: > So I would prefer if you make those changes but I won't block this patch on > them. Let me know what you want to do. Addressed all comments, PTAL.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:912: if (!scheduled_time_domain_wake_up) On 2017/05/09 19:18:42, alex clarke wrote: > On 2017/05/09 13:15:03, altimin wrote: > > On 2017/05/09 12:50:50, alex clarke wrote: > > > nit: Stylistically we tend to collapse all these onto one if > > > > I think that in this case the conditions are different and they warrant > separate > > ifs. > > One of the style guide rules is to be consistent with other code. Elsewhere in > the scheduler we tend to merge these into one line. I don't see anything > special here that warrants them being on multiple lines. I don't think it helps > readability ether. Done. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1838: start_time + delay1s)); On 2017/05/09 19:18:42, alex clarke wrote: > This is testing something that is kind of incidental to this test that I believe > we already have sufficient coverage for. My suggestion is to change this to > EXPECT_CALL(observer, OnQueueNextWakeUpChanged((_, _)); Yes, we don't really care about exact values in this test. https://codereview.chromium.org/2808843003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc:1848: EXPECT_CALL(observer, On 2017/05/09 19:18:42, alex clarke wrote: > This is the thing we really care about right? Incidentally .Times(1) is I > believe a nop although it's OK to leave it but please be consistent about using > it or not. Yes! The problem in the linked bug was caused by this notification not being issued. The underlying reason wasn't time domain change, but the mechanism issuing notification is the same and time domain change is the easiest way to test it.
Thanks, LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by altimin@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by altimin@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2808843003/#ps100001 (title: "Fix compilation")
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": 100001, "attempt_start_ts": 1494501674218420,
"parent_rev": "30af3e848371859a5cea265fac96e909f8cb6b5f", "commit_rev":
"0f67a0c1b7f0e461114771b4297f5fc0a0202cee"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Change TaskQueue observer call mechanism. Instead of manually calling TaskQueue observer we can rely on TimeDomain calling SetScheduledTimeDomainWakeUp and we can call the observer from there. This patch also fixes the problem where pages were stuck due to a missing notification. Test coverage is improved and HasPendingImmediateWork renamed to HasTaskToRunImmediately to better reflect its meaning. R=alexclarke@chromium.org,skyostil@chromium.org BUG=699541,710095 ========== to ========== [scheduler] Change TaskQueue observer call mechanism. Instead of manually calling TaskQueue observer we can rely on TimeDomain calling SetScheduledTimeDomainWakeUp and we can call the observer from there. This patch also fixes the problem where pages were stuck due to a missing notification. Test coverage is improved and HasPendingImmediateWork renamed to HasTaskToRunImmediately to better reflect its meaning. R=alexclarke@chromium.org,skyostil@chromium.org BUG=699541,710095 Review-Url: https://codereview.chromium.org/2808843003 Cr-Commit-Position: refs/heads/master@{#470957} Committed: https://chromium.googlesource.com/chromium/src/+/0f67a0c1b7f0e461114771b4297f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/0f67a0c1b7f0e461114771b4297f...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2915993003/ by shimazu@chromium.org. The reason for reverting is: This looks causing failure of loading pages when launching the process. For example, twitter lite cannot be open from the home screen because the hosting service worker probably cannot run on a process. See also https://crbug.com/725337.. |
