1198381 - Implement requestIdleCallback

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The proposed requestIdlCallback() looks useful.  Its been implemented in chrome canary so far.  Spec:

  https://w3c.github.io/requestidlecallback/

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This seems rather hard to implement somewhat interoperably. In any case it needs an Intent to Implement.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Why is this hard to implement interoperably?


The spec of course has bugs, like hinting that input handling happens at certain point, so
whoever starts to implement this, please review the spec first and file spec bugs ;)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ms2ger from comment #1)
> This seems rather hard to implement somewhat interoperably. In any case it
> needs an Intent to Implement.

Yea.  I don't know of anyone actively working this.  I filed it just to get it on our radar and have a place for discussion.  If/when someone starts it they should send an intent-to-implement.

Comment 4

[Ilya Grigorik]

Spec feedback would be very much appreciated, and the earlier the better!

Comment 5

[ross.mcilroy]

We have had some really positive web developer sentiment on the requestIdleCallback API [1] [2], and as such are keen to ship it to developers in Chrome soon. Before doing so we would really appreciate your thoughts on the spec (https://w3c.github.io/requestidlecallback/), particularly whether there are any parts which might make the spec difficult to implemented in Firefox down the road, or any API issues you foresee.

I realize it's difficult to get a full idea of how an implementation might look before it's started, but I'm hoping that if it's given a more in-depth look at this stage, then any future spec changes required for a future Firefox implementation (assuming you decide to implement the API) will be minor and not affect existing use case in the wild at that time.

Please feel free to file any bugs directly on the spec directly or continue the discussion in here. I'm happy to have chat in person over VC or teleconf if there are any questions you have. 

[1] https://developers.google.com/web/updates/2015/08/27/using-requestidlecallback?hl=en
[2] https://twitter.com/search?q=requestIdleCallback&src=typd

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll defer to the other people cc'ed.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Would be interested in servo's take on this too.

Comment 8

[Andreas Farre [:farre]]

Attachment: 0001-WIP-Implement-requestIdleCallback.patch

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8745894 [details] [diff] [review]
0001-WIP-Implement-requestIdleCallback.patch

Review of attachment 8745894 [details] [diff] [review]:
-----------------------------------------------------------------

It looks very good! Just some comments here and there.
The main comment is this one: we probably don't want to use a nsIObserver if the IdleRequest objects are controlled by the window. This means that the window can cancel the IdleRequest when everything is unlinked.

Second comment is: what about sync XHR ? We probably want to suspect the timers when this happens. Follow the pattern that we have for normal timers/intervals.

::: dom/base/IdleDeadline.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_IdleDeadline_h
> +#define mozilla_dom_IdleDeadline_h

oh yes! I like this :)

::: dom/base/IdleRequest.cpp
@@ +30,5 @@
> +  , mWindowID(0)
> +  , mCallback(&aCallback)
> +  , mTimeout(nullptr)
> +  , mHandle(aHandle)
> +{

MOZ_ASSERT(aWindow);

@@ +61,5 @@
> +NS_IMETHODIMP
> +IdleRequest::Notify(nsITimer* timer)
> +{
> +  RefPtr<nsGlobalWindow> window =
> +    nsGlobalWindow::GetInnerWindowWithId(mWindowID);

this window should be mWindow, right? You don't need this.

@@ +68,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsresult rv = RunIdleCallback(true);
> +  RefPtr<IdleRequest> requst = this;

1. request
2. why do you do this? you are not using request.

If you do this because you want to keep this object alive, then do:

RefPtr<IdleRequest> kungFuDeathGrip(this);

kungFuDeathGrip is how we call an object created for this reason.

@@ +73,5 @@
> +
> +  ErrorResult error;
> +  window->CancelIdleCallback(mHandle, error);
> +
> +  if (!NS_SUCCEEDED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv))) {
  error.SuppressException();
  return rv;
}

The reason why you want to suppress the exception is because ErrorResult::DTOR complains if it contains an non-handled error value.
Definitely we want a test about this.

@@ +85,5 @@
> +IdleRequest::Observe(nsISupports* aSubject, const char* aTopic,
> +                     const char16_t* aData)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +

MOZ_ASSERT(!strcmp(aTopic, "inner-window-destroyed"));

@@ +90,5 @@
> +  if (!mTimeout) {
> +    return NS_OK;
> +  }
> +
> +  if (!strcmp(aTopic, "inner-window-destroyed")) {

we just have observer for this particular topic. So yeah, no additional strcmp.

@@ +98,5 @@
> +    uint64_t windowID;
> +    nsresult rv = wrapper->GetData(&windowID);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (windowID == mWindowID) {

just because we have mWindow, we can do: if (mWindow->WindowID() == windowID) { ...
and you can get rid of mWindowID

@@ +128,5 @@
> +  rv = timer->InitWithCallback(this, timeout, nsITimer::TYPE_ONE_SHOT);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mTimeout = timer;
> +  mDeadline = mWindow->GetPerformance()->Timing()->TimeStampToDOMHighRes(

In theory, GetPrformance() can return nullptr.

@@ +130,5 @@
> +
> +  mTimeout = timer;
> +  mDeadline = mWindow->GetPerformance()->Timing()->TimeStampToDOMHighRes(
> +    TimeStamp::Now() + TimeDuration::FromMilliseconds(timeout));
> +  return rv;

return NS_OK;

::: dom/base/IdleRequest.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

new line after this comment block

@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef mozilla_dom_idlerequest_h__
> +#define mozilla_dom_idlerequest_h__

personally I don't like the __ at the end, but it's up to you :)

::: dom/base/nsGlobalWindow.cpp
@@ +598,4 @@
>    }
>  }
>  
> +int32_t

uint32_t

@@ +604,5 @@
> +                                    ErrorResult& aError)
> +{
> +  MOZ_RELEASE_ASSERT(IsInnerWindow());
> +
> +  if (mIdleRequestCallbackCounter == (INT32_MAX - 1)) {

1. UINT32_MAX
2. NS_WARN_IF

@@ +605,5 @@
> +{
> +  MOZ_RELEASE_ASSERT(IsInnerWindow());
> +
> +  if (mIdleRequestCallbackCounter == (INT32_MAX - 1)) {
> +    aError = NS_ERROR_NOT_AVAILABLE;

aError.Throw(NS_ERROR_NOT_AVAILABLE);
return 0;

plus... would be fun to see this error been thrown.

@@ +609,5 @@
> +    aError = NS_ERROR_NOT_AVAILABLE;
> +    return 0;
> +  }
> +
> +  int handle = ++mIdleRequestCallbackCounter;

I don't see this value to be initialized in the CTOR.
Maybe write a test checking that the first callback ID is 1.

@@ +615,5 @@
> +  bool needsScheduling = mIdleRequestCallbacks.IsEmpty();
> +
> +  RefPtr<IdleRequest> request = new IdleRequest(AsInner(), aCallback, handle);
> +
> +  mIdleRequestCallbacks.AppendElement(request);

move this after SetTimeout() just to have a full initialization of request, and then we store it.

@@ +635,5 @@
> +  MOZ_RELEASE_ASSERT(IsInnerWindow());
> +  nsresult rv;
> +  size_t index = mIdleRequestCallbacks.BinaryIndexOf(aHandle, IdleRequest::Comparator());
> +  if (index != nsTArray<RefPtr<IdleRequest>>::NoIndex) {
> +    rv = mIdleRequestCallbacks[index]->CancelTimeout();

aRv = mIdleRequestCallbacks[...] ...
NS_WARN_IF(aRv.Failed());

@@ +638,5 @@
> +  if (index != nsTArray<RefPtr<IdleRequest>>::NoIndex) {
> +    rv = mIdleRequestCallbacks[index]->CancelTimeout();
> +    mIdleRequestCallbacks.RemoveElementAt(index);
> +  }
> +  

extra spaces.

@@ +639,5 @@
> +    rv = mIdleRequestCallbacks[index]->CancelTimeout();
> +    mIdleRequestCallbacks.RemoveElementAt(index);
> +  }
> +  
> +  aError = rv;

remove this :)

::: dom/base/nsGlobalWindow.h
@@ +1063,5 @@
>    int32_t RequestAnimationFrame(mozilla::dom::FrameRequestCallback& aCallback,
>                                  mozilla::ErrorResult& aError);
>    void CancelAnimationFrame(int32_t aHandle, mozilla::ErrorResult& aError);
> +
> +  int32_t RequestIdleCallback(mozilla::dom::IdleRequestCallback& aCallback,

this should be uint32_t, right?

@@ +1066,5 @@
> +
> +  int32_t RequestIdleCallback(mozilla::dom::IdleRequestCallback& aCallback,
> +                              const mozilla::dom::IdleRequestOptions& aOptions,
> +                              mozilla::ErrorResult& aError);
> +  void CancelIdleCallback(int32_t aHandle, mozilla::ErrorResult& aError);

extra line after this

@@ +1824,5 @@
>  
>    uint32_t mSerial;
>  
> +  // The current idle request callback handle
> +  int32_t mIdleRequestCallbackCounter;

uint32_t ?

@@ +1826,5 @@
>  
> +  // The current idle request callback handle
> +  int32_t mIdleRequestCallbackCounter;
> +  nsTArray<RefPtr<mozilla::dom::IdleRequest>> mIdleRequestCallbacks;
> +  

remove these extra spaces.

::: modules/libpref/init/all.js
@@ +167,5 @@
>  // Enable notification of performance timing
>  pref("dom.performance.enable_notify_performance_timing", false);
>  
> +// Enable requestIdleCallback API
> +pref("dom.requestIdleCallback.enabled", false);

#ifdef RELEASE_BUILD
pref("dom.requestIdleCallback.enabled", false);
#else
 ..., true);
#end

Comment 10

[Andreas Farre [:farre]]

(In reply to Andrea Marchesini (:baku) from comment #9)
> Comment on attachment 8745894 [details] [diff] [review]
> 0001-WIP-Implement-requestIdleCallback.patch
> 
> @@ +61,5 @@
> > +NS_IMETHODIMP
> > +IdleRequest::Notify(nsITimer* timer)
> > +{
> > +  RefPtr<nsGlobalWindow> window =
> > +    nsGlobalWindow::GetInnerWindowWithId(mWindowID);
> 
> this window should be mWindow, right? You don't need this.
> 

The reason for this was that CancelIdleCallback is on nsGlobalWindow. So if we're not doing this, then we either need that mWindow is an nsGlobalWindow or add a method for cancelling idle callbacks on nsPIDOMWindow<T>? Which is better?

Comment 11

[Andreas Farre [:farre]]

(In reply to Andreas Farre [:farre] from comment #10)
> (In reply to Andrea Marchesini (:baku) from comment #9)
> > Comment on attachment 8745894 [details] [diff] [review]
> > 0001-WIP-Implement-requestIdleCallback.patch
> > 
> > @@ +61,5 @@
> > > +NS_IMETHODIMP
> > > +IdleRequest::Notify(nsITimer* timer)
> > > +{
> > > +  RefPtr<nsGlobalWindow> window =
> > > +    nsGlobalWindow::GetInnerWindowWithId(mWindowID);
> > 
> > this window should be mWindow, right? You don't need this.
> > 
> 
> The reason for this was that CancelIdleCallback is on nsGlobalWindow. So if
> we're not doing this, then we either need that mWindow is an nsGlobalWindow
> or add a method for cancelling idle callbacks on nsPIDOMWindow<T>? Which is
> better?

Or use nsGlobalWindow::Cast :)

Comment 12

[Andreas Farre [:farre]]

Attachment: requestIdleCallback-WIP.patch

Comment 13

[Andreas Farre [:farre]]

Comment on attachment 8748149 [details] [diff] [review]
requestIdleCallback-WIP.patch

>From d22e64489b97839f6ecea40b3f811a43489c0432 Mon Sep 17 00:00:00 2001
>From: Andreas Farre <[e-mail]>
>Date: Fri, 15 Apr 2016 13:44:24 -0400
>Subject: [PATCH 1/3] [WIP] Implement requestIdleCallback
>
>Add webidl for requestIdleCallback.
>
>MozReview-Commit-ID: B02RBG4wXW4
>---
> dom/base/IdleDeadline.cpp      |  64 +++++++++++++++++++
> dom/base/IdleDeadline.h        |  58 ++++++++++++++++++
> dom/base/IdleRequest.cpp       | 136 +++++++++++++++++++++++++++++++++++++++++
> dom/base/IdleRequest.h         |  63 +++++++++++++++++++
> dom/base/moz.build             |   4 ++
> dom/base/nsGlobalWindow.cpp    |  53 ++++++++++++++++
> dom/base/nsGlobalWindow.h      |  15 ++++-
> dom/webidl/IdleDeadline.webidl |  12 ++++
> dom/webidl/Window.webidl       |  14 +++++
> dom/webidl/moz.build           |   1 +
> modules/libpref/init/all.js    |   7 +++
> 11 files changed, 425 insertions(+), 2 deletions(-)
> create mode 100644 dom/base/IdleDeadline.cpp
> create mode 100644 dom/base/IdleDeadline.h
> create mode 100644 dom/base/IdleRequest.cpp
> create mode 100644 dom/base/IdleRequest.h
> create mode 100644 dom/webidl/IdleDeadline.webidl
>
>diff --git a/dom/base/IdleDeadline.cpp b/dom/base/IdleDeadline.cpp
>new file mode 100644
>index 0000000..8a8dc44
>--- /dev/null
>+++ b/dom/base/IdleDeadline.cpp
>@@ -0,0 +1,64 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et cindent: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include <algorithm>
>+
>+#include "mozilla/dom/IdleDeadline.h"
>+#include "mozilla/dom/IdleDeadlineBinding.h"
>+#include "nsCOMPtr.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsDOMNavigationTiming.h"
>+#include "nsPIDOMWindow.h"
>+#include "nsPerformance.h"
>+
>+namespace mozilla {
>+namespace dom {
>+
>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(IdleDeadline, mWindow)
>+NS_IMPL_CYCLE_COLLECTING_ADDREF(IdleDeadline)
>+NS_IMPL_CYCLE_COLLECTING_RELEASE(IdleDeadline)
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(IdleDeadline)
>+  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)
>+NS_INTERFACE_MAP_END
>+
>+IdleDeadline::IdleDeadline(nsPIDOMWindowInner* aWindow,
>+                           DOMHighResTimeStamp aDeadline, bool aDidTimeout)
>+  : mWindow(aWindow)
>+  , mDeadline(aDeadline)
>+  , mDidTimeout(aDidTimeout)
>+{
>+}
>+
>+IdleDeadline::~IdleDeadline()
>+{
>+}
>+
>+JSObject*
>+IdleDeadline::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
>+{
>+  return IdleDeadlineBinding::Wrap(aCx, this, aGivenProto);
>+}
>+
>+DOMHighResTimeStamp
>+IdleDeadline::TimeRemaining(ErrorResult& aError)
>+{
>+  RefPtr<nsPerformance> performance = mWindow->GetPerformance();
>+  if (!performance) {
>+    aError.Throw(NS_ERROR_NOT_AVAILABLE);
>+  }
>+
>+  return std::max(performance->Now() - mDeadline, 0.0);
>+}
>+
>+bool
>+IdleDeadline::DidTimeout() const
>+{
>+  return mDidTimeout;
>+}
>+
>+} // namespace dom
>+} // namespace mozilla
>diff --git a/dom/base/IdleDeadline.h b/dom/base/IdleDeadline.h
>new file mode 100644
>index 0000000..f36ee44
>--- /dev/null
>+++ b/dom/base/IdleDeadline.h
>@@ -0,0 +1,58 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et cindent: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_IdleDeadline_h
>+#define mozilla_dom_IdleDeadline_h
>+
>+#include "js/TypeDecls.h"
>+#include "mozilla/Attributes.h"
>+#include "mozilla/ErrorResult.h"
>+#include "mozilla/ErrorResult.h"
>+#include "mozilla/dom/BindingDeclarations.h"
>+#include "nsCOMPtr.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsDOMNavigationTiming.h"
>+#include "nsWrapperCache.h"
>+
>+class nsPIDOMWindowInner;
>+
>+namespace mozilla {
>+namespace dom {
>+
>+class IdleDeadline final
>+  : public nsISupports
>+  , public nsWrapperCache
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(IdleDeadline)
>+
>+public:
>+  IdleDeadline(nsPIDOMWindowInner* aWindow, DOMHighResTimeStamp aDeadline,
>+               bool aDidTimeout);
>+
>+protected:
>+  ~IdleDeadline();
>+
>+public:
>+  nsPIDOMWindowInner* GetParentObject() const { return mWindow; }
>+
>+  virtual JSObject* WrapObject(JSContext* aCx,
>+                               JS::Handle<JSObject*> aGivenProto) override;
>+
>+  DOMHighResTimeStamp TimeRemaining(ErrorResult& aError);
>+  bool DidTimeout() const;
>+
>+private:
>+  nsCOMPtr<nsPIDOMWindowInner> mWindow;
>+  DOMHighResTimeStamp mDeadline;
>+  bool mDidTimeout;
>+};
>+
>+} // namespace dom
>+} // namespace mozilla
>+
>+#endif // mozilla_dom_IdleDeadline_h
>diff --git a/dom/base/IdleRequest.cpp b/dom/base/IdleRequest.cpp
>new file mode 100644
>index 0000000..ba579de
>--- /dev/null
>+++ b/dom/base/IdleRequest.cpp
>@@ -0,0 +1,136 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "IdleRequest.h"
>+
>+#include "mozilla/TimeStamp.h"
>+#include "mozilla/dom/WindowBinding.h"
>+#include "nsComponentManagerUtils.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsDOMNavigationTiming.h"
>+#include "nsGlobalWindow.h"
>+#include "nsISupportsPrimitives.h"
>+#include "nsPIDOMWindow.h"
>+#include "nsPerformance.h"
>+
>+namespace mozilla {
>+namespace dom {
>+
>+IdleRequest::IdleRequest(nsPIDOMWindowInner* aWindow,
>+                         IdleRequestCallback& aCallback, uint32_t aHandle)
>+  : mWindow(aWindow)
>+  , mCallback(&aCallback)
>+  , mTimeout(nullptr)
>+  , mHandle(aHandle)
>+{
>+  MOZ_ASSERT(aWindow);
>+}
>+
>+IdleRequest::~IdleRequest()
>+{
>+  if (mTimeout) {
>+    mTimeout->Cancel();
>+    mTimeout = nullptr;
>+  }
>+}
>+
>+NS_IMPL_CYCLE_COLLECTION_CLASS(IdleRequest)
>+
>+NS_IMPL_CYCLE_COLLECTING_ADDREF(IdleRequest)
>+NS_IMPL_CYCLE_COLLECTING_RELEASE(IdleRequest)
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(IdleRequest)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mWindow)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mCallback)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mTimeout)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IdleRequest)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCallback)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTimeout)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>+
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(IdleRequest)
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)
>+  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
>+NS_INTERFACE_MAP_END
>+
>+NS_IMETHODIMP
>+IdleRequest::Notify(nsITimer* timer)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  RefPtr<IdleRequest> kungFuDeathGrip(this);
>+
>+  ErrorResult error;
>+  nsGlobalWindow::Cast(mWindow)->CancelIdleCallback(mHandle, error);
>+
>+  if (error.Failed()) {
>+    return error.StealNSResult();
>+  }
>+
>+  return RunIdleCallback(true);
>+}
>+
>+nsresult
>+IdleRequest::SetTimeout(uint32_t timeout)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(!mTimeout);
>+
>+  nsresult rv;
>+  nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  RefPtr<nsPerformance> performance = mWindow->GetPerformance();
>+  if (!performance) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  mDeadline = performance->Timing()->TimeStampToDOMHighRes(
>+    TimeStamp::Now() + TimeDuration::FromMilliseconds(timeout));
>+
>+  rv = timer->InitWithCallback(this, timeout, nsITimer::TYPE_ONE_SHOT);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  mTimeout = timer;
>+
>+  return NS_OK;
>+}
>+
>+nsresult
>+IdleRequest::RunIdleCallback(bool aDidTimeout)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  ErrorResult error;
>+  RefPtr<IdleDeadline> deadline =
>+    new IdleDeadline(mWindow, mDeadline, aDidTimeout);
>+  mCallback->Call(*deadline, error);
>+
>+  return error.StealNSResult();
>+}
>+
>+nsresult
>+IdleRequest::CancelTimeout()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  if (!mTimeout) {
>+    return NS_OK;
>+  }
>+
>+  nsresult rv;
>+  rv = mTimeout->Cancel();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  mTimeout = nullptr;
>+
>+  return rv;
>+}
>+
>+} // namespace dom
>+} // namespace mozilla
>diff --git a/dom/base/IdleRequest.h b/dom/base/IdleRequest.h
>new file mode 100644
>index 0000000..fcf11b6
>--- /dev/null
>+++ b/dom/base/IdleRequest.h
>@@ -0,0 +1,63 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_idlerequest_h
>+#define mozilla_dom_idlerequest_h
>+
>+#include "nsCOMPtr.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsDOMNavigationTiming.h"
>+#include "nsITimer.h"
>+
>+class nsPIDOMWindowInner;
>+
>+namespace mozilla {
>+namespace dom {
>+
>+class IdleRequestCallback;
>+
>+class IdleRequest final : public nsITimerCallback
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_CYCLE_COLLECTION_CLASS(IdleRequest)
>+  NS_DECL_NSITIMERCALLBACK
>+
>+public:
>+  IdleRequest(nsPIDOMWindowInner* aWindow, IdleRequestCallback& aCallback,
>+              uint32_t aHandle);
>+
>+  nsresult SetTimeout(uint32_t timout);
>+  nsresult RunIdleCallback(bool aDidTimeout);
>+  nsresult CancelTimeout();
>+
>+  struct Comparator
>+  {
>+    bool LessThan(RefPtr<IdleRequest> aRequest, uint32_t aHandle) const
>+    {
>+      return aRequest->mHandle < aHandle;
>+    }
>+
>+    bool Equals(RefPtr<IdleRequest> aRequest, uint32_t aHandle) const
>+    {
>+      return aRequest->mHandle == aHandle;
>+    }
>+  };
>+
>+private:
>+  virtual ~IdleRequest();
>+
>+  nsCOMPtr<nsPIDOMWindowInner> mWindow;
>+  RefPtr<IdleRequestCallback> mCallback;
>+  nsCOMPtr<nsITimer> mTimeout;
>+  DOMHighResTimeStamp mDeadline;
>+  uint32_t mHandle;
>+};
>+
>+} // namespace dom
>+} // namespace mozilla
>+
>+#endif // mozilla_dom_idlerequest_h
>diff --git a/dom/base/moz.build b/dom/base/moz.build
>index 3b2ebe0..3c2abb8 100644
>--- a/dom/base/moz.build
>+++ b/dom/base/moz.build
>@@ -185,6 +185,8 @@ EXPORTS.mozilla.dom += [
>     'FormData.h',
>     'FragmentOrElement.h',
>     'FromParser.h',
>+    'IdleDeadline.h',
>+    'IdleRequest.h',
>     'ImageEncoder.h',
>     'ImportManager.h',
>     'Link.h',
>@@ -253,6 +255,8 @@ UNIFIED_SOURCES += [
>     'FileReader.cpp',
>     'FormData.cpp',
>     'FragmentOrElement.cpp',
>+    'IdleDeadline.cpp',
>+    'IdleRequest.cpp',
>     'ImageEncoder.cpp',
>     'ImportManager.cpp',
>     'Link.cpp',
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>index 485d6de..d9b463b 100644
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -11,6 +11,7 @@
> #include "mozilla/MemoryReporting.h"
> 
> // Local Includes
>+#include "IdleRequest.h"
> #include "Navigator.h"
> #include "nsScreen.h"
> #include "nsHistory.h"
>@@ -597,6 +598,55 @@ DialogValueHolder::Get(JSContext* aCx, JS::Handle<JSObject*> aScope,
>   }
> }
> 
>+uint32_t
>+nsGlobalWindow::RequestIdleCallback(IdleRequestCallback& aCallback,
>+                                    const IdleRequestOptions& aOptions,
>+                                    ErrorResult& aError)
>+{
>+  MOZ_RELEASE_ASSERT(IsInnerWindow());
>+
>+  if (NS_WARN_IF((mIdleRequestCallbackCounter == (UINT32_MAX - 1)))) {
>+    aError.Throw(NS_ERROR_NOT_AVAILABLE);
>+    return 0;
>+  }
>+
>+  uint32_t handle = ++mIdleRequestCallbackCounter;
>+
>+  bool needsScheduling = mIdleRequestCallbacks.IsEmpty();
>+
>+  RefPtr<IdleRequest> request = new IdleRequest(AsInner(), aCallback, handle);
>+
>+  if (aOptions.mTimeout.WasPassed()) {
>+    aError = request->SetTimeout(aOptions.mTimeout.Value());
>+    if (NS_WARN_IF(aError.Failed())) {
>+      return 0;
>+    }
>+  }
>+
>+  mIdleRequestCallbacks.AppendElement(request);
>+
>+  if (needsScheduling) {
>+    // start listening for idle events
>+  }
>+
>+  return handle;
>+}
>+
>+void
>+nsGlobalWindow::CancelIdleCallback(int32_t aHandle,
>+                                   mozilla::ErrorResult& aError)
>+{
>+  MOZ_RELEASE_ASSERT(IsInnerWindow());
>+
>+  size_t index =
>+    mIdleRequestCallbacks.BinaryIndexOf(aHandle, IdleRequest::Comparator());
>+  if (index != nsTArray<RefPtr<IdleRequest>>::NoIndex) {
>+    aError = mIdleRequestCallbacks[index]->CancelTimeout();
>+    NS_WARN_IF(aError.Failed());
>+    mIdleRequestCallbacks.RemoveElementAt(index);
>+  }
>+}
>+
> namespace mozilla {
> namespace dom {
> extern uint64_t
>@@ -1889,6 +1939,7 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(nsGlobalWindow)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIdleService)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWakeLock)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingStorageEvents)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIdleRequestCallbacks)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIdleObservers)
> 
> #ifdef MOZ_GAMEPAD
>@@ -1949,6 +2000,7 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsGlobalWindow)
>     tmp->mListenerManager->Disconnect();
>     NS_IMPL_CYCLE_COLLECTION_UNLINK(mListenerManager)
>   }
>+
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mLocation)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mHistory)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mLocalStorage)
>@@ -1964,6 +2016,7 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsGlobalWindow)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mIdleService)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mWakeLock)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mPendingStorageEvents)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mIdleRequestCallbacks)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mIdleObservers)
> 
> #ifdef MOZ_GAMEPAD
>diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h
>index 0110771..cede178 100644
>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>@@ -106,6 +106,8 @@ class Crypto;
> class External;
> class Function;
> class Gamepad;
>+class IdleRequest;
>+class IdleRequestCallback;
> class MediaQueryList;
> class MozSelfSupport;
> class Navigator;
>@@ -201,7 +203,7 @@ public:
>   // stack depth at which timeout is firing
>   uint32_t mFiringDepth;
> 
>-  // 
>+  //
>   uint32_t mNestingLevel;
> 
>   // The popup state at timeout creation time if not created from
>@@ -1062,6 +1064,12 @@ public:
>   int32_t RequestAnimationFrame(mozilla::dom::FrameRequestCallback& aCallback,
>                                 mozilla::ErrorResult& aError);
>   void CancelAnimationFrame(int32_t aHandle, mozilla::ErrorResult& aError);
>+
>+  uint32_t RequestIdleCallback(mozilla::dom::IdleRequestCallback& aCallback,
>+                               const mozilla::dom::IdleRequestOptions& aOptions,
>+                               mozilla::ErrorResult& aError);
>+  void CancelIdleCallback(int32_t aHandle, mozilla::ErrorResult& aError);
>+
> #ifdef MOZ_WEBSPEECH
>   mozilla::dom::SpeechSynthesis*
>     GetSpeechSynthesis(mozilla::ErrorResult& aError);
>@@ -1218,7 +1226,6 @@ public:
>   already_AddRefed<nsWindowRoot> GetWindowRootOuter();
>   already_AddRefed<nsWindowRoot> GetWindowRoot(mozilla::ErrorResult& aError);
>   nsPerformance* GetPerformance();
>-
> protected:
>   // Web IDL helpers
> 
>@@ -1818,6 +1825,10 @@ protected:
> 
>   uint32_t mSerial;
> 
>+  // The current idle request callback handle
>+  uint32_t mIdleRequestCallbackCounter;
>+  nsTArray<RefPtr<mozilla::dom::IdleRequest>> mIdleRequestCallbacks;
>+
> #ifdef DEBUG
>   bool mSetOpenerWindowCalled;
>   nsCOMPtr<nsIURI> mLastOpenedURI;
>diff --git a/dom/webidl/IdleDeadline.webidl b/dom/webidl/IdleDeadline.webidl
>new file mode 100644
>index 0000000..bfadc4f
>--- /dev/null
>+++ b/dom/webidl/IdleDeadline.webidl
>@@ -0,0 +1,12 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/.
>+ */
>+
>+[Pref="dom.requestIdleCallback.enabled"]
>+interface IdleDeadline {
>+  [Throws]
>+  DOMHighResTimeStamp timeRemaining();
>+  readonly attribute boolean didTimeout;
>+};
>diff --git a/dom/webidl/Window.webidl b/dom/webidl/Window.webidl
>index 49fabef..5411006 100644
>--- a/dom/webidl/Window.webidl
>+++ b/dom/webidl/Window.webidl
>@@ -498,3 +498,17 @@ interface ChromeWindow {
> Window implements ChromeWindow;
> Window implements GlobalFetch;
> Window implements ImageBitmapFactories;
>+
>+partial interface Window {
>+  [Pref="dom.requestIdleCallback.enabled", Throws]
>+  unsigned long requestIdleCallback(IdleRequestCallback callback,
>+                                    optional IdleRequestOptions options);
>+  [Pref="dom.requestIdleCallback.enabled", Throws]
>+  void          cancelIdleCallback(unsigned long handle);
>+};
>+
>+dictionary IdleRequestOptions {
>+  unsigned long timeout;
>+};
>+
>+callback IdleRequestCallback = void (IdleDeadline deadline);
>diff --git a/dom/webidl/moz.build b/dom/webidl/moz.build
>index efea229..81e6f46 100644
>--- a/dom/webidl/moz.build
>+++ b/dom/webidl/moz.build
>@@ -267,6 +267,7 @@ WEBIDL_FILES = [
>     'IDBRequest.webidl',
>     'IDBTransaction.webidl',
>     'IDBVersionChangeEvent.webidl',
>+    'IdleDeadline.webidl',
>     'ImageBitmap.webidl',
>     'ImageBitmapRenderingContext.webidl',
>     'ImageCapture.webidl',
>diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js
>index 8db0e05..a7574b4 100644
>--- a/modules/libpref/init/all.js
>+++ b/modules/libpref/init/all.js
>@@ -167,6 +167,13 @@ pref("dom.performance.enable_user_timing_logging", false);
> // Enable notification of performance timing
> pref("dom.performance.enable_notify_performance_timing", false);
> 
>+// Enable requestIdleCallback API
>+#ifdef RELEASE_BUILD
>+pref("dom.requestIdleCallback.enabled", false);
>+#else
>+pref("dom.requestIdleCallback.enabled", true);
>+#endif
>+
> // Whether the Gamepad API is enabled
> pref("dom.gamepad.enabled", true);
> #ifdef RELEASE_BUILD
>-- 
>2.5.0
>
>
>From 063a845ef0e6cb43225cbe279fbdcb8c2c027ecf Mon Sep 17 00:00:00 2001
>From: Andreas Farre <[e-mail]>
>Date: Fri, 29 Apr 2016 14:35:09 +0200
>Subject: [PATCH 2/3] [WIP] Move nsTimeout to mozilla::dom::Timeout
>
>Also move it from nsGlobalWindow.h to its own Timeout.h.
>
>MozReview-Commit-ID: 47kB7CCE33o
>---
> dom/base/Timeout.cpp        |  83 ++++++++++++++++++++++++++++++
> dom/base/Timeout.h          |  93 ++++++++++++++++++++++++++++++++++
> dom/base/moz.build          |   2 +
> dom/base/nsGlobalWindow.cpp | 120 +++++++++++---------------------------------
> dom/base/nsGlobalWindow.h   |  19 +++----
> dom/base/nsPIDOMWindow.h    |   4 +-
> 6 files changed, 218 insertions(+), 103 deletions(-)
> create mode 100644 dom/base/Timeout.cpp
> create mode 100644 dom/base/Timeout.h
>
>diff --git a/dom/base/Timeout.cpp b/dom/base/Timeout.cpp
>new file mode 100644
>index 0000000..ee05039
>--- /dev/null
>+++ b/dom/base/Timeout.cpp
>@@ -0,0 +1,83 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et cindent: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "mozilla/dom/Timeout.h"
>+
>+#include "mozilla/LinkedList.h"
>+#include "mozilla/TimeStamp.h"
>+#include "nsCOMPtr.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsGlobalWindow.h"
>+#include "nsIPrincipal.h"
>+#include "nsIScriptTimeoutHandler.h"
>+#include "nsITimer.h"
>+#include "nsPIDOMWindow.h"
>+
>+Timeout::Timeout()
>+  : mCleared(false),
>+    mRunning(false),
>+    mIsInterval(false),
>+    mPublicId(0),
>+    mInterval(0),
>+    mFiringDepth(0),
>+    mNestingLevel(0),
>+    mPopupState(openAllowed)
>+{
>+#ifdef DEBUG_jst
>+  {
>+    extern int gTimeoutCnt;
>+
>+    ++gTimeoutCnt;
>+  }
>+#endif
>+
>+  MOZ_COUNT_CTOR(Timeout);
>+}
>+
>+Timeout::~Timeout()
>+{
>+#ifdef DEBUG_jst
>+  {
>+    extern int gTimeoutCnt;
>+
>+    --gTimeoutCnt;
>+  }
>+#endif
>+
>+  if (mTimer) {
>+    mTimer->Cancel();
>+    mTimer = nullptr;
>+  }
>+
>+  MOZ_COUNT_DTOR(Timeout);
>+}
>+
>+NS_IMPL_CYCLE_COLLECTION_CLASS(Timeout)
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_0(Timeout)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(Timeout)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPrincipal)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mScriptHandler)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>+NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(Timeout, AddRef)
>+NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(Timeout, Release)
>+
>+nsresult
>+Timeout::InitTimer(uint32_t aDelay)
>+{
>+  return mTimer->InitWithNameableFuncCallback(
>+    nsGlobalWindow::TimerCallback, this, aDelay,
>+    nsITimer::TYPE_ONE_SHOT, nsGlobalWindow::TimerNameCallback);
>+}
>+
>+// Return true if this timeout has a refcount of 1. This is used to check
>+// that dummy_timeout doesn't leak from nsGlobalWindow::RunTimeout.
>+bool
>+Timeout::HasRefCntOne()
>+{
>+  return mRefCnt.get() == 1;
>+}
>diff --git a/dom/base/Timeout.h b/dom/base/Timeout.h
>new file mode 100644
>index 0000000..acebf7e
>--- /dev/null
>+++ b/dom/base/Timeout.h
>@@ -0,0 +1,93 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et cindent: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_timeout_h
>+#define mozilla_dom_timeout_h
>+
>+#include "mozilla/LinkedList.h"
>+#include "mozilla/TimeStamp.h"
>+#include "nsCOMPtr.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsIPrincipal.h"
>+
>+class nsGlobalWindow;
>+class nsIScriptTimeoutHandler;
>+class nsITimer;
>+
>+namespace mozilla {
>+namespace dom {
>+
>+/*
>+ * Timeout struct that holds information about each script
>+ * timeout.  Holds a strong reference to an nsIScriptTimeoutHandler, which
>+ * abstracts the language specific cruft.
>+ */
>+class Timeout final
>+  : public LinkedListElement<Timeout>
>+{
>+public:
>+  Timeout();
>+
>+  NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(Timeout)
>+  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(Timeout)
>+
>+  nsresult InitTimer(uint32_t aDelay);
>+
>+  bool HasRefCntOne();
>+
>+  // Window for which this timeout fires
>+  RefPtr<nsGlobalWindow> mWindow;
>+
>+  // The actual timer object
>+  nsCOMPtr<nsITimer> mTimer;
>+
>+  // True if the timeout was cleared
>+  bool mCleared;
>+
>+  // True if this is one of the timeouts that are currently running
>+  bool mRunning;
>+
>+  // True if this is a repeating/interval timer
>+  bool mIsInterval;
>+
>+  // Returned as value of setTimeout()
>+  uint32_t mPublicId;
>+
>+  // Interval in milliseconds
>+  uint32_t mInterval;
>+
>+  // mWhen and mTimeRemaining can't be in a union, sadly, because they
>+  // have constructors.
>+  // Nominal time to run this timeout.  Use only when timeouts are not
>+  // suspended.
>+  TimeStamp mWhen;
>+  // Remaining time to wait.  Used only when timeouts are suspended.
>+  TimeDuration mTimeRemaining;
>+
>+  // Principal with which to execute
>+  nsCOMPtr<nsIPrincipal> mPrincipal;
>+
>+  // stack depth at which timeout is firing
>+  uint32_t mFiringDepth;
>+
>+  //
>+  uint32_t mNestingLevel;
>+
>+  // The popup state at timeout creation time if not created from
>+  // another timeout
>+  PopupControlState mPopupState;
>+
>+  // The language-specific information about the callback.
>+  nsCOMPtr<nsIScriptTimeoutHandler> mScriptHandler;
>+
>+private:
>+  ~Timeout();
>+};
>+
>+} // namespace dom
>+} // namespace mozilla
>+
>+#endif // mozilla_dom_timeout_h
>diff --git a/dom/base/moz.build b/dom/base/moz.build
>index 3c2abb8..1550b9b 100644
>--- a/dom/base/moz.build
>+++ b/dom/base/moz.build
>@@ -214,6 +214,7 @@ EXPORTS.mozilla.dom += [
>     'StyleSheetList.h',
>     'SubtleCrypto.h',
>     'Text.h',
>+    'Timeout.h',
>     'TreeWalker.h',
>     'URL.h',
>     'URLSearchParams.h',
>@@ -363,6 +364,7 @@ UNIFIED_SOURCES += [
>     'Text.cpp',
>     'TextInputProcessor.cpp',
>     'ThirdPartyUtil.cpp',
>+    'Timeout.cpp',
>     'TreeWalker.cpp',
>     'URL.cpp',
>     'URLSearchParams.cpp',
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>index d9b463b..067b9e4 100644
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -19,8 +19,10 @@
> #include "nsDOMNavigationTiming.h"
> #include "nsIDOMStorageManager.h"
> #include "mozilla/dom/DOMStorage.h"
>+#include "mozilla/dom/IdleRequest.h"
> #include "mozilla/dom/StorageEvent.h"
> #include "mozilla/dom/StorageEventBinding.h"
>+#include "mozilla/dom/Timeout.h"
> #include "mozilla/IntegerPrintfMacros.h"
> #if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)
> #include "mozilla/dom/WindowOrientationObserver.h"
>@@ -497,72 +499,6 @@ private:
> 
> NS_IMPL_ISUPPORTS(nsGlobalWindowObserver, nsIObserver, nsIInterfaceRequestor)
> 
>-nsTimeout::nsTimeout()
>-  : mCleared(false),
>-    mRunning(false),
>-    mIsInterval(false),
>-    mPublicId(0),
>-    mInterval(0),
>-    mFiringDepth(0),
>-    mNestingLevel(0),
>-    mPopupState(openAllowed)
>-{
>-#ifdef DEBUG_jst
>-  {
>-    extern int gTimeoutCnt;
>-
>-    ++gTimeoutCnt;
>-  }
>-#endif
>-
>-  MOZ_COUNT_CTOR(nsTimeout);
>-}
>-
>-nsTimeout::~nsTimeout()
>-{
>-#ifdef DEBUG_jst
>-  {
>-    extern int gTimeoutCnt;
>-
>-    --gTimeoutCnt;
>-  }
>-#endif
>-
>-  if (mTimer) {
>-    mTimer->Cancel();
>-    mTimer = nullptr;
>-  }
>-
>-  MOZ_COUNT_DTOR(nsTimeout);
>-}
>-
>-NS_IMPL_CYCLE_COLLECTION_CLASS(nsTimeout)
>-
>-NS_IMPL_CYCLE_COLLECTION_UNLINK_0(nsTimeout)
>-NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsTimeout)
>-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
>-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPrincipal)
>-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mScriptHandler)
>-NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>-NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(nsTimeout, AddRef)
>-NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(nsTimeout, Release)
>-
>-nsresult
>-nsTimeout::InitTimer(uint32_t aDelay)
>-{
>-  return mTimer->InitWithNameableFuncCallback(
>-    nsGlobalWindow::TimerCallback, this, aDelay,
>-    nsITimer::TYPE_ONE_SHOT, nsGlobalWindow::TimerNameCallback);
>-}
>-
>-// Return true if this timeout has a refcount of 1. This is used to check
>-// that dummy_timeout doesn't leak from nsGlobalWindow::RunTimeout.
>-bool
>-nsTimeout::HasRefCntOne()
>-{
>-  return mRefCnt.get() == 1;
>-}
>-
> static already_AddRefed<nsIVariant>
> CreateVoidVariant()
> {
>@@ -1920,10 +1856,10 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(nsGlobalWindow)
> 
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mListenerManager)
> 
>-  for (nsTimeout* timeout = tmp->mTimeouts.getFirst();
>+  for (Timeout* timeout = tmp->mTimeouts.getFirst();
>        timeout;
>        timeout = timeout->getNext()) {
>-    cb.NoteNativeChild(timeout, NS_CYCLE_COLLECTION_PARTICIPANT(nsTimeout));
>+    cb.NoteNativeChild(timeout, NS_CYCLE_COLLECTION_PARTICIPANT(Timeout));
>   }
> 
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLocation)
>@@ -2085,7 +2021,7 @@ nsGlobalWindow::IsBlackForCC(bool aTracingNeeded)
> void
> nsGlobalWindow::UnmarkGrayTimers()
> {
>-  for (nsTimeout* timeout = mTimeouts.getFirst();
>+  for (Timeout* timeout = mTimeouts.getFirst();
>        timeout;
>        timeout = timeout->getNext()) {
>     if (timeout->mScriptHandler) {
>@@ -11861,7 +11797,7 @@ nsGlobalWindow::SetTimeoutOrInterval(nsIScriptTimeoutHandler *aHandler,
>     interval = maxTimeoutMs;
>   }
> 
>-  RefPtr<nsTimeout> timeout = new nsTimeout();
>+  RefPtr<Timeout> timeout = new Timeout();
>   timeout->mIsInterval = aIsInterval;
>   timeout->mInterval = interval;
>   timeout->mScriptHandler = aHandler;
>@@ -11908,7 +11844,7 @@ nsGlobalWindow::SetTimeoutOrInterval(nsIScriptTimeoutHandler *aHandler,
>       return rv;
>     }
> 
>-    RefPtr<nsTimeout> copy = timeout;
>+    RefPtr<Timeout> copy = timeout;
> 
>     rv = timeout->InitTimer(realInterval);
>     if (NS_FAILED(rv)) {
>@@ -12016,13 +11952,13 @@ nsGlobalWindow::SetTimeoutOrInterval(JSContext* aCx, const nsAString& aHandler,
> }
> 
> bool
>-nsGlobalWindow::RunTimeoutHandler(nsTimeout* aTimeout,
>+nsGlobalWindow::RunTimeoutHandler(Timeout* aTimeout,
>                                   nsIScriptContext* aScx)
> {
>   // Hold on to the timeout in case mExpr or mFunObj releases its
>   // doc.
>-  RefPtr<nsTimeout> timeout = aTimeout;
>-  nsTimeout* last_running_timeout = mRunningTimeout;
>+  RefPtr<Timeout> timeout = aTimeout;
>+  Timeout* last_running_timeout = mRunningTimeout;
>   mRunningTimeout = timeout;
>   timeout->mRunning = true;
> 
>@@ -12131,7 +12067,7 @@ nsGlobalWindow::RunTimeoutHandler(nsTimeout* aTimeout,
> }
> 
> bool
>-nsGlobalWindow::RescheduleTimeout(nsTimeout* aTimeout, const TimeStamp& now,
>+nsGlobalWindow::RescheduleTimeout(Timeout* aTimeout, const TimeStamp& now,
>                                   bool aRunningPendingTimeouts)
> {
>   if (!aTimeout->mIsInterval) {
>@@ -12210,7 +12146,7 @@ nsGlobalWindow::RescheduleTimeout(nsTimeout* aTimeout, const TimeStamp& now,
> }
> 
> void
>-nsGlobalWindow::RunTimeout(nsTimeout *aTimeout)
>+nsGlobalWindow::RunTimeout(Timeout *aTimeout)
> {
>   // If a modal dialog is open for this window, return early. Pending
>   // timeouts will run when the modal dialog is dismissed.
>@@ -12221,8 +12157,8 @@ nsGlobalWindow::RunTimeout(nsTimeout *aTimeout)
>   NS_ASSERTION(IsInnerWindow(), "Timeout running on outer window!");
>   NS_ASSERTION(!IsFrozen(), "Timeout running on a window in the bfcache!");
> 
>-  nsTimeout *nextTimeout;
>-  nsTimeout *last_expired_timeout, *last_insertion_point;
>+  Timeout *nextTimeout;
>+  Timeout *last_expired_timeout, *last_insertion_point;
>   uint32_t firingDepth = mTimeoutFiringDepth + 1;
> 
>   // Make sure that the window and the script context don't go away as
>@@ -12253,7 +12189,7 @@ nsGlobalWindow::RunTimeout(nsTimeout *aTimeout)
>   // whose mWhen is greater than deadline, since once that happens we know
>   // nothing past that point is expired.
>   last_expired_timeout = nullptr;
>-  for (nsTimeout *timeout = mTimeouts.getFirst();
>+  for (Timeout *timeout = mTimeouts.getFirst();
>        timeout && timeout->mWhen <= deadline;
>        timeout = timeout->getNext()) {
>     if (timeout->mFiringDepth == 0) {
>@@ -12286,11 +12222,11 @@ nsGlobalWindow::RunTimeout(nsTimeout *aTimeout)
>   // timeouts that will be processed in a future call to
>   // win_run_timeout(). This dummy timeout serves as the head of the
>   // list for any timeouts inserted as a result of running a timeout.
>-  RefPtr<nsTimeout> dummy_timeout = new nsTimeout();
>+  RefPtr<Timeout> dummy_timeout = new Timeout();
>   dummy_timeout->mFiringDepth = firingDepth;
>   dummy_timeout->mWhen = now;
>   last_expired_timeout->setNext(dummy_timeout);
>-  RefPtr<nsTimeout> timeoutExtraRef(dummy_timeout);
>+  RefPtr<Timeout> timeoutExtraRef(dummy_timeout);
> 
>   last_insertion_point = mTimeoutInsertionPoint;
>   // If we ever start setting mTimeoutInsertionPoint to a non-dummy timeout,
>@@ -12299,7 +12235,7 @@ nsGlobalWindow::RunTimeout(nsTimeout *aTimeout)
> 
>   Telemetry::AutoCounter<Telemetry::DOM_TIMERS_FIRED_PER_NATIVE_TIMEOUT> timeoutsRan;
> 
>-  for (nsTimeout *timeout = mTimeouts.getFirst();
>+  for (Timeout *timeout = mTimeouts.getFirst();
>        timeout != dummy_timeout && !IsFrozen();
>        timeout = nextTimeout) {
>     nextTimeout = timeout->getNext();
>@@ -12382,7 +12318,7 @@ nsGlobalWindow::ClearTimeoutOrInterval(int32_t aTimerID)
>   MOZ_RELEASE_ASSERT(IsInnerWindow());
> 
>   uint32_t public_id = (uint32_t)aTimerID;
>-  nsTimeout *timeout;
>+  Timeout *timeout;
> 
>   for (timeout = mTimeouts.getFirst(); timeout; timeout = timeout->getNext()) {
>     if (timeout->mPublicId == public_id) {
>@@ -12426,7 +12362,7 @@ nsresult nsGlobalWindow::ResetTimersForNonBackgroundWindow()
>   // anything with mTimeoutInsertionPoint or anything before it, so should
>   // start at the timer after mTimeoutInsertionPoint, if there is one.
>   // Otherwise, start at the beginning of the list.
>-  for (nsTimeout *timeout = mTimeoutInsertionPoint ?
>+  for (Timeout *timeout = mTimeoutInsertionPoint ?
>          mTimeoutInsertionPoint->getNext() : mTimeouts.getFirst();
>        timeout; ) {
>     // It's important that this check be <= so that we guarantee that
>@@ -12470,7 +12406,7 @@ nsresult nsGlobalWindow::ResetTimersForNonBackgroundWindow()
> 
>       // Get the pointer to the next timeout now, before we move the
>       // current timeout in the list.
>-      nsTimeout* nextTimeout = timeout->getNext();
>+      Timeout* nextTimeout = timeout->getNext();
> 
>       // It is safe to remove and re-insert because mWhen is now
>       // strictly smaller than it used to be, so we know we'll insert
>@@ -12504,7 +12440,7 @@ nsresult nsGlobalWindow::ResetTimersForNonBackgroundWindow()
> void
> nsGlobalWindow::ClearAllTimeouts()
> {
>-  nsTimeout *timeout, *nextTimeout;
>+  Timeout *timeout, *nextTimeout;
> 
>   for (timeout = mTimeouts.getFirst(); timeout; timeout = nextTimeout) {
>     /* If RunTimeout() is higher up on the stack for this
>@@ -12539,7 +12475,7 @@ nsGlobalWindow::ClearAllTimeouts()
> }
> 
> void
>-nsGlobalWindow::InsertTimeoutIntoList(nsTimeout *aTimeout)
>+nsGlobalWindow::InsertTimeoutIntoList(Timeout *aTimeout)
> {
>   NS_ASSERTION(IsInnerWindow(),
>                "InsertTimeoutIntoList() called on outer window!");
>@@ -12547,7 +12483,7 @@ nsGlobalWindow::InsertTimeoutIntoList(nsTimeout *aTimeout)
>   // Start at mLastTimeout and go backwards.  Don't go further than
>   // mTimeoutInsertionPoint, though.  This optimizes for the common case of
>   // insertion at the end.
>-  nsTimeout* prevSibling;
>+  Timeout* prevSibling;
>   for (prevSibling = mTimeouts.getLast();
>        prevSibling && prevSibling != mTimeoutInsertionPoint &&
>          // This condition needs to match the one in SetTimeoutOrInterval that
>@@ -12577,7 +12513,7 @@ nsGlobalWindow::InsertTimeoutIntoList(nsTimeout *aTimeout)
> void
> nsGlobalWindow::TimerCallback(nsITimer *aTimer, void *aClosure)
> {
>-  RefPtr<nsTimeout> timeout = (nsTimeout *)aClosure;
>+  RefPtr<Timeout> timeout = (Timeout *)aClosure;
> 
>   timeout->mWindow->RunTimeout(timeout);
> }
>@@ -12587,7 +12523,7 @@ void
> nsGlobalWindow::TimerNameCallback(nsITimer* aTimer, void* aClosure, char* aBuf,
>                                   size_t aLen)
> {
>-  RefPtr<nsTimeout> timeout = (nsTimeout*)aClosure;
>+  RefPtr<Timeout> timeout = (Timeout*)aClosure;
> 
>   const char* filename;
>   uint32_t lineNum, column;
>@@ -12820,7 +12756,7 @@ nsGlobalWindow::SuspendTimeouts(uint32_t aIncrease,
>     }
> 
>     TimeStamp now = TimeStamp::Now();
>-    for (nsTimeout *t = mTimeouts.getFirst(); t; t = t->getNext()) {
>+    for (Timeout *t = mTimeouts.getFirst(); t; t = t->getNext()) {
>       // Set mTimeRemaining to be the time remaining for this timer.
>       if (t->mWhen > now)
>         t->mTimeRemaining = t->mWhen - now;
>@@ -12920,7 +12856,7 @@ nsGlobalWindow::ResumeTimeouts(bool aThawChildren, bool aThawWorkers)
>     bool _seenDummyTimeout = false;
> #endif
> 
>-    for (nsTimeout *t = mTimeouts.getFirst(); t; t = t->getNext()) {
>+    for (Timeout *t = mTimeouts.getFirst(); t; t = t->getNext()) {
>       // There's a chance we're being called with RunTimeout on the stack in which
>       // case we have a dummy timeout in the list that *must not* be resumed. It
>       // can be identified by a null mWindow.
>diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h
>index cede178..9672582 100644
>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>@@ -118,6 +118,7 @@ struct RequestInit;
> class RequestOrUSVString;
> class Selection;
> class SpeechSynthesis;
>+class Timeout;
> class U2F;
> class VRDevice;
> class WakeLock;
>@@ -1449,18 +1450,18 @@ public:
>   nsresult ResetTimersForNonBackgroundWindow();
> 
>   // The timeout implementation functions.
>-  void RunTimeout(nsTimeout *aTimeout);
>+  void RunTimeout(mozilla::dom::Timeout *aTimeout);
>   void RunTimeout() { RunTimeout(nullptr); }
>   // Return true if |aTimeout| was cleared while its handler ran.
>-  bool RunTimeoutHandler(nsTimeout* aTimeout, nsIScriptContext* aScx);
>+  bool RunTimeoutHandler(mozilla::dom::Timeout* aTimeout, nsIScriptContext* aScx);
>   // Return true if |aTimeout| needs to be reinserted into the timeout list.
>-  bool RescheduleTimeout(nsTimeout* aTimeout, const TimeStamp& now,
>+  bool RescheduleTimeout(mozilla::dom::Timeout* aTimeout, const TimeStamp& now,
>                          bool aRunningPendingTimeouts);
> 
>   void ClearAllTimeouts();
>   // Insert aTimeout into the list, before all timeouts that would
>   // fire after it, but no earlier than mTimeoutInsertionPoint, if any.
>-  void InsertTimeoutIntoList(nsTimeout *aTimeout);
>+  void InsertTimeoutIntoList(mozilla::dom::Timeout *aTimeout);
>   static void TimerCallback(nsITimer *aTimer, void *aClosure);
>   static void TimerNameCallback(nsITimer* aTimer, void* aClosure, char* aBuf,
>                                 size_t aLen);
>@@ -1777,7 +1778,7 @@ protected:
>   RefPtr<mozilla::dom::BarProp> mPersonalbar;
>   RefPtr<mozilla::dom::BarProp> mStatusbar;
>   RefPtr<mozilla::dom::BarProp> mScrollbars;
>-  RefPtr<nsDOMWindowUtils>    mWindowUtils;
>+  RefPtr<nsDOMWindowUtils>      mWindowUtils;
>   nsString                      mStatus;
>   nsString                      mDefaultStatus;
>   RefPtr<nsGlobalWindowObserver> mObserver; // Inner windows only.
>@@ -1802,13 +1803,13 @@ protected:
>   // non-null.  In that case, the dummy timeout pointed to by
>   // mTimeoutInsertionPoint may have a later mWhen than some of the timeouts
>   // that come after it.
>-  mozilla::LinkedList<nsTimeout> mTimeouts;
>+  mozilla::LinkedList<mozilla::dom::Timeout> mTimeouts;
>   // If mTimeoutInsertionPoint is non-null, insertions should happen after it.
>   // This is a dummy timeout at the moment; if that ever changes, the logic in
>   // ResetTimersForNonBackgroundWindow needs to change.
>-  nsTimeout*                    mTimeoutInsertionPoint;
>-  uint32_t                      mTimeoutPublicIdCounter;
>-  uint32_t                      mTimeoutFiringDepth;
>+  mozilla::dom::Timeout*      mTimeoutInsertionPoint;
>+  uint32_t                    mTimeoutPublicIdCounter;
>+  uint32_t                    mTimeoutFiringDepth;
>   RefPtr<nsLocation>          mLocation;
>   RefPtr<nsHistory>           mHistory;
> 
>diff --git a/dom/base/nsPIDOMWindow.h b/dom/base/nsPIDOMWindow.h
>index bbf44c4..2679d1d 100644
>--- a/dom/base/nsPIDOMWindow.h
>+++ b/dom/base/nsPIDOMWindow.h
>@@ -36,13 +36,13 @@ class nsPIDOMWindowInner;
> class nsPIDOMWindowOuter;
> class nsPIWindowRoot;
> class nsXBLPrototypeHandler;
>-struct nsTimeout;
> 
> namespace mozilla {
> namespace dom {
> class AudioContext;
> class Element;
> class ServiceWorkerRegistrationMainThread;
>+class Timeout;
> } // namespace dom
> namespace gfx {
> class VRDeviceProxy;
>@@ -624,7 +624,7 @@ protected:
>   uint32_t               mModalStateDepth;
> 
>   // These variables are only used on inner windows.
>-  nsTimeout             *mRunningTimeout;
>+  mozilla::dom::Timeout *mRunningTimeout;
> 
>   uint32_t               mMutationBits;
> 
>-- 
>2.5.0
>
>
>From 7fccfd3aec1b60837fd7f973a7356d7bf9bc3eba Mon Sep 17 00:00:00 2001
>From: Andreas Farre <[e-mail]>
>Date: Sun, 1 May 2016 12:18:31 +0200
>Subject: [PATCH 3/3] [WIP] Make IdleRequest be a nsIScriptTimeoutHandler
>
>MozReview-Commit-ID: 5X1UUzTLrTS
>---
> dom/base/IdleRequest.cpp           | 109 ++++++++++++++++++++++---------------
> dom/base/IdleRequest.h             |  24 ++++++--
> dom/base/Timeout.cpp               |   3 +-
> dom/base/Timeout.h                 |   7 ++-
> dom/base/nsContentSink.cpp         |   2 +-
> dom/base/nsGlobalWindow.cpp        |  51 ++++++++++-------
> dom/base/nsGlobalWindow.h          |  16 ++++--
> dom/base/nsIScriptTimeoutHandler.h |  10 +++-
> dom/base/nsJSTimeoutHandler.cpp    |   8 ++-
> dom/webidl/Window.webidl           |   2 +-
> 10 files changed, 149 insertions(+), 83 deletions(-)
>
>diff --git a/dom/base/IdleRequest.cpp b/dom/base/IdleRequest.cpp
>index ba579de..7610145 100644
>--- a/dom/base/IdleRequest.cpp
>+++ b/dom/base/IdleRequest.cpp
>@@ -6,12 +6,15 @@
> 
> #include "IdleRequest.h"
> 
>+#include "mozilla/Function.h"
>+#include "mozilla/Maybe.h"
> #include "mozilla/TimeStamp.h"
> #include "mozilla/dom/WindowBinding.h"
> #include "nsComponentManagerUtils.h"
> #include "nsCycleCollectionParticipant.h"
> #include "nsDOMNavigationTiming.h"
> #include "nsGlobalWindow.h"
>+#include "nsIScriptTimeoutHandler.h"
> #include "nsISupportsPrimitives.h"
> #include "nsPIDOMWindow.h"
> #include "nsPerformance.h"
>@@ -23,18 +26,14 @@ IdleRequest::IdleRequest(nsPIDOMWindowInner* aWindow,
>                          IdleRequestCallback& aCallback, uint32_t aHandle)
>   : mWindow(aWindow)
>   , mCallback(&aCallback)
>-  , mTimeout(nullptr)
>   , mHandle(aHandle)
>+  , mTimeoutHandle(Nothing())
> {
>   MOZ_ASSERT(aWindow);
> }
> 
> IdleRequest::~IdleRequest()
> {
>-  if (mTimeout) {
>-    mTimeout->Cancel();
>-    mTimeout = nullptr;
>-  }
> }
> 
> NS_IMPL_CYCLE_COLLECTION_CLASS(IdleRequest)
>@@ -45,46 +44,22 @@ NS_IMPL_CYCLE_COLLECTING_RELEASE(IdleRequest)
> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(IdleRequest)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mWindow)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mCallback)
>-  NS_IMPL_CYCLE_COLLECTION_UNLINK(mTimeout)
> NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> 
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IdleRequest)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCallback)
>-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTimeout)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(IdleRequest)
>+NS_INTERFACE_MAP_ENTRY(nsIScriptTimeoutHandler)
>   NS_INTERFACE_MAP_ENTRY(nsISupports)
>-  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
> NS_INTERFACE_MAP_END
> 
>-NS_IMETHODIMP
>-IdleRequest::Notify(nsITimer* timer)
>-{
>-  MOZ_ASSERT(NS_IsMainThread());
>-
>-  RefPtr<IdleRequest> kungFuDeathGrip(this);
>-
>-  ErrorResult error;
>-  nsGlobalWindow::Cast(mWindow)->CancelIdleCallback(mHandle, error);
>-
>-  if (error.Failed()) {
>-    return error.StealNSResult();
>-  }
>-
>-  return RunIdleCallback(true);
>-}
>-
> nsresult
> IdleRequest::SetTimeout(uint32_t timeout)
> {
>   MOZ_ASSERT(NS_IsMainThread());
>-  MOZ_ASSERT(!mTimeout);
>-
>-  nsresult rv;
>-  nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
>-  NS_ENSURE_SUCCESS(rv, rv);
> 
>   RefPtr<nsPerformance> performance = mWindow->GetPerformance();
>   if (!performance) {
>@@ -94,18 +69,23 @@ IdleRequest::SetTimeout(uint32_t timeout)
>   mDeadline = performance->Timing()->TimeStampToDOMHighRes(
>     TimeStamp::Now() + TimeDuration::FromMilliseconds(timeout));
> 
>-  rv = timer->InitWithCallback(this, timeout, nsITimer::TYPE_ONE_SHOT);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  mTimeout = timer;
>+  int32_t handle;
>+  nsresult rv = nsGlobalWindow::Cast(mWindow)->SetTimeoutOrInterval(
>+    this, timeout, false, Timeout::Access::Private, &handle);
>+  mTimeoutHandle = Some(handle);
> 
>-  return NS_OK;
>+  return rv;
> }
> 
> nsresult
>-IdleRequest::RunIdleCallback(bool aDidTimeout)
>+IdleRequest::RunIdleRequestCallback(bool aDidTimeout)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>+  if (!aDidTimeout) {
>+    CancelTimeout();
>+  }
>+
>   ErrorResult error;
>   RefPtr<IdleDeadline> deadline =
>     new IdleDeadline(mWindow, mDeadline, aDidTimeout);
>@@ -114,22 +94,61 @@ IdleRequest::RunIdleCallback(bool aDidTimeout)
>   return error.StealNSResult();
> }
> 
>-nsresult
>+void
> IdleRequest::CancelTimeout()
> {
>-  MOZ_ASSERT(NS_IsMainThread());
>-
>-  if (!mTimeout) {
>-    return NS_OK;
>+  if (mTimeoutHandle.isSome()) {
>+    nsGlobalWindow::Cast(mWindow)->ClearTimeoutOrInterval(
>+      mTimeoutHandle.value(), Timeout::Access::Private);
>   }
>+}
> 
>-  nsresult rv;
>-  rv = mTimeout->Cancel();
>-  NS_ENSURE_SUCCESS(rv, rv);
>+nsresult
>+IdleRequest::DoRunIdleRequestTimeout(nsIScriptTimeoutHandler* aHandler)
>+{
>+  RefPtr<IdleRequest> handler = static_cast<IdleRequest*>(aHandler);
>+  return handler->RunIdleRequestTimeout();
>+}
> 
>-  mTimeout = nullptr;
>+Maybe<IdleRequest::NativeCallback>
>+IdleRequest::GetNativeCallback()
>+{
>+  return Some(
>+    function<nsresult(nsIScriptTimeoutHandler*)>(&DoRunIdleRequestTimeout));
>+}
> 
>-  return rv;
>+Function*
>+IdleRequest::GetCallback()
>+{
>+  return nullptr;
>+}
>+
>+const char16_t*
>+IdleRequest::GetHandlerText()
>+{
>+  return nullptr;
>+}
>+
>+void
>+IdleRequest::GetLocation(const char** aFileName, uint32_t* aLineNo,
>+                         uint32_t* aColumn)
>+{
>+  MOZ_ASSERT(false, "should not be reached");
>+}
>+
>+const nsTArray<JS::Value>&
>+IdleRequest::GetArgs()
>+{
>+  MOZ_ASSERT(false, "should not be reached");
>+  static nsTArray<JS::Heap<JS::Value>> args;
>+  return args;
>+}
>+
>+nsresult
>+IdleRequest::RunIdleRequestTimeout()
>+{
>+  nsGlobalWindow::Cast(mWindow)->CancelIdleCallback(mHandle);
>+  return RunIdleRequestCallback(true);
> }
> 
> } // namespace dom
>diff --git a/dom/base/IdleRequest.h b/dom/base/IdleRequest.h
>index fcf11b6..69fd2c8 100644
>--- a/dom/base/IdleRequest.h
>+++ b/dom/base/IdleRequest.h
>@@ -7,10 +7,11 @@
> #ifndef mozilla_dom_idlerequest_h
> #define mozilla_dom_idlerequest_h
> 
>+#include "mozilla/Maybe.h"
> #include "nsCOMPtr.h"
> #include "nsCycleCollectionParticipant.h"
> #include "nsDOMNavigationTiming.h"
>-#include "nsITimer.h"
>+#include "nsIScriptTimeoutHandler.h"
> 
> class nsPIDOMWindowInner;
> 
>@@ -19,20 +20,26 @@ namespace dom {
> 
> class IdleRequestCallback;
> 
>-class IdleRequest final : public nsITimerCallback
>+class IdleRequest final : public nsIScriptTimeoutHandler
> {
> public:
>   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>   NS_DECL_CYCLE_COLLECTION_CLASS(IdleRequest)
>-  NS_DECL_NSITIMERCALLBACK
>+
>+  virtual Maybe<NativeCallback> GetNativeCallback() override;
>+  virtual Function* GetCallback() override;
>+  virtual const char16_t* GetHandlerText() override;
>+  virtual void GetLocation(const char** aFileName, uint32_t* aLineNo,
>+                           uint32_t* aColumn) override;
>+  virtual const nsTArray<JS::Value>& GetArgs() override;
> 
> public:
>   IdleRequest(nsPIDOMWindowInner* aWindow, IdleRequestCallback& aCallback,
>               uint32_t aHandle);
> 
>   nsresult SetTimeout(uint32_t timout);
>-  nsresult RunIdleCallback(bool aDidTimeout);
>-  nsresult CancelTimeout();
>+  nsresult RunIdleRequestCallback(bool aDidTimeout);
>+  void     CancelTimeout();
> 
>   struct Comparator
>   {
>@@ -47,14 +54,19 @@ public:
>     }
>   };
> 
>+  static nsresult DoRunIdleRequestTimeout(nsIScriptTimeoutHandler* aHandler);
>+
>+protected:
>+  nsresult RunIdleRequestTimeout();
>+
> private:
>   virtual ~IdleRequest();
> 
>   nsCOMPtr<nsPIDOMWindowInner> mWindow;
>   RefPtr<IdleRequestCallback> mCallback;
>-  nsCOMPtr<nsITimer> mTimeout;
>   DOMHighResTimeStamp mDeadline;
>   uint32_t mHandle;
>+  mozilla::Maybe<int32_t> mTimeoutHandle;
> };
> 
> } // namespace dom
>diff --git a/dom/base/Timeout.cpp b/dom/base/Timeout.cpp
>index ee05039..d698303 100644
>--- a/dom/base/Timeout.cpp
>+++ b/dom/base/Timeout.cpp
>@@ -20,7 +20,8 @@ Timeout::Timeout()
>   : mCleared(false),
>     mRunning(false),
>     mIsInterval(false),
>-    mPublicId(0),
>+    mAccess(Access::Public),
>+    mTimeoutId(0),
>     mInterval(0),
>     mFiringDepth(0),
>     mNestingLevel(0),
>diff --git a/dom/base/Timeout.h b/dom/base/Timeout.h
>index acebf7e..670f28e 100644
>--- a/dom/base/Timeout.h
>+++ b/dom/base/Timeout.h
>@@ -14,6 +14,7 @@
> #include "nsIPrincipal.h"
> 
> class nsGlobalWindow;
>+class nsIPrincipal;
> class nsIScriptTimeoutHandler;
> class nsITimer;
> 
>@@ -36,6 +37,8 @@ public:
> 
>   nsresult InitTimer(uint32_t aDelay);
> 
>+  enum class Access { Public, Private };
>+
>   bool HasRefCntOne();
> 
>   // Window for which this timeout fires
>@@ -53,8 +56,10 @@ public:
>   // True if this is a repeating/interval timer
>   bool mIsInterval;
> 
>+  Access mAccess;
>+
>   // Returned as value of setTimeout()
>-  uint32_t mPublicId;
>+  uint32_t mTimeoutId;
> 
>   // Interval in milliseconds
>   uint32_t mInterval;
>diff --git a/dom/base/nsContentSink.cpp b/dom/base/nsContentSink.cpp
>index 1996f4b..cf5dbc0 100644
>--- a/dom/base/nsContentSink.cpp
>+++ b/dom/base/nsContentSink.cpp
>@@ -756,7 +756,7 @@ nsContentSink::ProcessStyleLink(nsIContent* aElement,
>     aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);
>   }
>   if (!integrity.IsEmpty()) {
>-    MOZ_LOG(SRILogHelper::GetSriLog(), mozilla::LogLevel::Debug,
>+    MOZ_LOG(dom::SRILogHelper::GetSriLog(), mozilla::LogLevel::Debug,
>             ("nsContentSink::ProcessStyleLink, integrity=%s",
>              NS_ConvertUTF16toUTF8(integrity).get()));
>   }
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>index 067b9e4..43af8e8 100644
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -11,7 +11,6 @@
> #include "mozilla/MemoryReporting.h"
> 
> // Local Includes
>-#include "IdleRequest.h"
> #include "Navigator.h"
> #include "nsScreen.h"
> #include "nsHistory.h"
>@@ -569,18 +568,23 @@ nsGlobalWindow::RequestIdleCallback(IdleRequestCallback& aCallback,
> }
> 
> void
>-nsGlobalWindow::CancelIdleCallback(int32_t aHandle,
>-                                   mozilla::ErrorResult& aError)
>+nsGlobalWindow::CancelIdleCallback(uint32_t aHandle)
> {
>   MOZ_RELEASE_ASSERT(IsInnerWindow());
> 
>   size_t index =
>     mIdleRequestCallbacks.BinaryIndexOf(aHandle, IdleRequest::Comparator());
>   if (index != nsTArray<RefPtr<IdleRequest>>::NoIndex) {
>-    aError = mIdleRequestCallbacks[index]->CancelTimeout();
>-    NS_WARN_IF(aError.Failed());
>+    mIdleRequestCallbacks[index]->CancelTimeout();
>     mIdleRequestCallbacks.RemoveElementAt(index);
>   }
>+
>+  index = mRunnableIdleRequestCallbacks.BinaryIndexOf(
>+    aHandle, IdleRequest::Comparator());
>+  if (index != nsTArray<RefPtr<IdleRequest>>::NoIndex) {
>+    mRunnableIdleRequestCallbacks[index]->CancelTimeout();
>+    mRunnableIdleRequestCallbacks.RemoveElementAt(index);
>+  }
> }
> 
> namespace mozilla {
>@@ -1192,7 +1196,7 @@ nsGlobalWindow::nsGlobalWindow(nsGlobalWindow *aOuterWindow)
>     mNotifiedIDDestroyed(false),
>     mAllowScriptsToClose(false),
>     mTimeoutInsertionPoint(nullptr),
>-    mTimeoutPublicIdCounter(1),
>+    mTimeoutIdCounter(1),
>     mTimeoutFiringDepth(0),
>     mTimeoutsSuspendDepth(0),
>     mFocusMethod(0),
>@@ -7627,7 +7631,7 @@ nsGlobalWindow::ClearTimeout(int32_t aHandle)
>   MOZ_RELEASE_ASSERT(IsInnerWindow());
> 
>   if (aHandle > 0) {
>-    ClearTimeoutOrInterval(aHandle);
>+    ClearTimeoutOrInterval(aHandle, Timeout::Access::Public);
>   }
> }
> 
>@@ -7635,7 +7639,7 @@ void
> nsGlobalWindow::ClearInterval(int32_t aHandle)
> {
>   if (aHandle > 0) {
>-    ClearTimeoutOrInterval(aHandle);
>+    ClearTimeoutOrInterval(aHandle, Timeout::Access::Public);
>   }
> }
> 
>@@ -11773,9 +11777,9 @@ nsGlobalWindow::SetInterval(JSContext* aCx, const nsAString& aHandler,
> }
> 
> nsresult
>-nsGlobalWindow::SetTimeoutOrInterval(nsIScriptTimeoutHandler *aHandler,
>-                                     int32_t interval,
>-                                     bool aIsInterval, int32_t *aReturn)
>+nsGlobalWindow::SetTimeoutOrInterval(nsIScriptTimeoutHandler* aHandler,
>+                                     int32_t interval, bool aIsInterval,
>+                                     Timeout::Access aAccess, int32_t* aReturn)
> {
>   MOZ_ASSERT(IsInnerWindow());
> 
>@@ -11801,6 +11805,7 @@ nsGlobalWindow::SetTimeoutOrInterval(nsIScriptTimeoutHandler *aHandler,
>   timeout->mIsInterval = aIsInterval;
>   timeout->mInterval = interval;
>   timeout->mScriptHandler = aHandler;
>+  timeout->mAccess = aAccess;
> 
>   // Now clamp the actual interval we will use for the timer based on
>   uint32_t nestingLevel = sNestingLevel + 1;
>@@ -11891,8 +11896,8 @@ nsGlobalWindow::SetTimeoutOrInterval(nsIScriptTimeoutHandler *aHandler,
> 
>   InsertTimeoutIntoList(timeout);
> 
>-  timeout->mPublicId = ++mTimeoutPublicIdCounter;
>-  *aReturn = timeout->mPublicId;
>+  timeout->mTimeoutId = ++mTimeoutIdCounter;
>+  *aReturn = timeout->mTimeoutId;
> 
>   return NS_OK;
> 
>@@ -11921,7 +11926,8 @@ nsGlobalWindow::SetTimeoutOrInterval(JSContext *aCx, Function& aFunction,
>   }
> 
>   int32_t result;
>-  aError = SetTimeoutOrInterval(handler, aTimeout, aIsInterval, &result);
>+  aError = SetTimeoutOrInterval(handler, aTimeout, aIsInterval,
>+                                Timeout::Access::Public, &result);
>   return result;
> }
> 
>@@ -11947,7 +11953,8 @@ nsGlobalWindow::SetTimeoutOrInterval(JSContext* aCx, const nsAString& aHandler,
>   }
> 
>   int32_t result;
>-  aError = SetTimeoutOrInterval(handler, aTimeout, aIsInterval, &result);
>+  aError = SetTimeoutOrInterval(handler, aTimeout, aIsInterval,
>+                                Timeout::Access::Public, &result);
>   return result;
> }
> 
>@@ -11992,7 +11999,13 @@ nsGlobalWindow::RunTimeoutHandler(Timeout* aTimeout,
>   bool abortIntervalHandler = false;
>   nsCOMPtr<nsIScriptTimeoutHandler> handler(timeout->mScriptHandler);
>   RefPtr<Function> callback = handler->GetCallback();
>-  if (!callback) {
>+  const Maybe<function<nsresult(nsIScriptTimeoutHandler*)>> nativeCallback =
>+    handler->GetNativeCallback();
>+
>+  if (nativeCallback.isSome()) {
>+    nsCOMPtr<nsISupports> kungFuDeathGrip(static_cast<nsIDOMWindow *>(this));
>+    nativeCallback.value()(handler);
>+  } else if (!callback) {
>     // Evaluate the timeout expression.
>     const char16_t* script = handler->GetHandlerText();
>     NS_ASSERTION(script, "timeout has no script nor handler text!");
>@@ -12313,15 +12326,15 @@ nsGlobalWindow::RunTimeout(Timeout *aTimeout)
> }
> 
> void
>-nsGlobalWindow::ClearTimeoutOrInterval(int32_t aTimerID)
>+nsGlobalWindow::ClearTimeoutOrInterval(int32_t aTimerID, Timeout::Access aAccess)
> {
>   MOZ_RELEASE_ASSERT(IsInnerWindow());
> 
>-  uint32_t public_id = (uint32_t)aTimerID;
>+  uint32_t timer_id = (uint32_t)aTimerID;
>   Timeout *timeout;
> 
>   for (timeout = mTimeouts.getFirst(); timeout; timeout = timeout->getNext()) {
>-    if (timeout->mPublicId == public_id) {
>+    if (timeout->mTimeoutId == timer_id && timeout->mAccess == aAccess) {
>       if (timeout->mRunning) {
>         /* We're running from inside the timeout. Mark this
>            timeout for deferred deletion by the code in
>diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h
>index 9672582..3b2b48a6 100644
>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>@@ -54,6 +54,7 @@
> #include "nsSize.h"
> #include "nsCheapSets.h"
> #include "mozilla/dom/ImageBitmapSource.h"
>+#include "mozilla/dom/Timeout.h"
> 
> #define DEFAULT_HOME_PAGE "www.mozilla.org"
> #define PREF_BROWSER_STARTUP_HOMEPAGE "browser.startup.homepage"
>@@ -1069,7 +1070,7 @@ public:
>   uint32_t RequestIdleCallback(mozilla::dom::IdleRequestCallback& aCallback,
>                                const mozilla::dom::IdleRequestOptions& aOptions,
>                                mozilla::ErrorResult& aError);
>-  void CancelIdleCallback(int32_t aHandle, mozilla::ErrorResult& aError);
>+  void CancelIdleCallback(uint32_t aHandle);
> 
> #ifdef MOZ_WEBSPEECH
>   mozilla::dom::SpeechSynthesis*
>@@ -1433,9 +1434,10 @@ public:
>   // Timeout Functions
>   // Language agnostic timeout function (all args passed).
>   // |interval| is in milliseconds.
>-  nsresult SetTimeoutOrInterval(nsIScriptTimeoutHandler *aHandler,
>-                                int32_t interval,
>-                                bool aIsInterval, int32_t* aReturn);
>+  nsresult SetTimeoutOrInterval(nsIScriptTimeoutHandler* aHandler,
>+                                int32_t interval, bool aIsInterval,
>+                                mozilla::dom::Timeout::Access aAccess,
>+                                int32_t* aReturn);
>   int32_t SetTimeoutOrInterval(JSContext* aCx,
>                                mozilla::dom::Function& aFunction,
>                                int32_t aTimeout,
>@@ -1444,7 +1446,8 @@ public:
>   int32_t SetTimeoutOrInterval(JSContext* aCx, const nsAString& aHandler,
>                                int32_t aTimeout, bool aIsInterval,
>                                mozilla::ErrorResult& aError);
>-  void ClearTimeoutOrInterval(int32_t aTimerID);
>+  void ClearTimeoutOrInterval(int32_t aTimerID,
>+                              mozilla::dom::Timeout::Access aAccess);
> 
>   // JS specific timeout functions (JS args grabbed from context).
>   nsresult ResetTimersForNonBackgroundWindow();
>@@ -1808,7 +1811,7 @@ protected:
>   // This is a dummy timeout at the moment; if that ever changes, the logic in
>   // ResetTimersForNonBackgroundWindow needs to change.
>   mozilla::dom::Timeout*      mTimeoutInsertionPoint;
>-  uint32_t                    mTimeoutPublicIdCounter;
>+  uint32_t                    mTimeoutIdCounter;
>   uint32_t                    mTimeoutFiringDepth;
>   RefPtr<nsLocation>          mLocation;
>   RefPtr<nsHistory>           mHistory;
>@@ -1829,6 +1832,7 @@ protected:
>   // The current idle request callback handle
>   uint32_t mIdleRequestCallbackCounter;
>   nsTArray<RefPtr<mozilla::dom::IdleRequest>> mIdleRequestCallbacks;
>+  nsTArray<RefPtr<mozilla::dom::IdleRequest>> mRunnableIdleRequestCallbacks;
> 
> #ifdef DEBUG
>   bool mSetOpenerWindowCalled;
>diff --git a/dom/base/nsIScriptTimeoutHandler.h b/dom/base/nsIScriptTimeoutHandler.h
>index 99a20a86..cbacc56 100644
>--- a/dom/base/nsIScriptTimeoutHandler.h
>+++ b/dom/base/nsIScriptTimeoutHandler.h
>@@ -8,6 +8,8 @@
> 
> #include "nsTArray.h"
> #include "js/TypeDecls.h"
>+#include "mozilla/Function.h"
>+#include "mozilla/Maybe.h"
> 
> namespace mozilla {
> namespace dom {
>@@ -29,9 +31,15 @@ class nsIScriptTimeoutHandler : public nsISupports
> public:
>   NS_DECLARE_STATIC_IID_ACCESSOR(NS_ISCRIPTTIMEOUTHANDLER_IID)
> 
>+  typedef mozilla::function<nsresult(nsIScriptTimeoutHandler*)> NativeCallback;
>+
>+  // Get the C++ callback to call. If this returns Nothing, GetGallback() will
>+  // be called to get the Function.
>+  virtual mozilla::Maybe<NativeCallback> GetNativeCallback() = 0;
>+
>   // Get the Function to call.  If this returns nullptr, GetHandlerText() will
>   // be called to get the string.
>-  virtual mozilla::dom::Function *GetCallback() = 0;
>+  virtual mozilla::dom::Function* GetCallback() = 0;
> 
>   // Get the handler text of not a compiled object.
>   virtual const char16_t *GetHandlerText() = 0;
>diff --git a/dom/base/nsJSTimeoutHandler.cpp b/dom/base/nsJSTimeoutHandler.cpp
>index 7c4d18b..7ebdd14 100644
>--- a/dom/base/nsJSTimeoutHandler.cpp
>+++ b/dom/base/nsJSTimeoutHandler.cpp
>@@ -18,6 +18,8 @@
> #include <algorithm>
> #include "mozilla/dom/FunctionBinding.h"
> #include "nsAXPCNativeCallContext.h"
>+#include "mozilla/Maybe.h"
>+#include "mozilla/Function.h"
> 
> static const char kSetIntervalStr[] = "setInterval";
> static const char kSetTimeoutStr[] = "setTimeout";
>@@ -44,10 +46,12 @@ public:
>                            ErrorResult& aError);
> 
>   virtual const char16_t* GetHandlerText() override;
>-  virtual Function* GetCallback() override
>+  virtual Function* GetCallback() override { return mFunction; }
>+  virtual Maybe<NativeCallback> GetNativeCallback() override
>   {
>-    return mFunction;
>+    return Nothing();
>   }
>+
>   virtual void GetLocation(const char** aFileName, uint32_t* aLineNo,
>                            uint32_t* aColumn) override
>   {
>diff --git a/dom/webidl/Window.webidl b/dom/webidl/Window.webidl
>index 5411006..3078108 100644
>--- a/dom/webidl/Window.webidl
>+++ b/dom/webidl/Window.webidl
>@@ -503,7 +503,7 @@ partial interface Window {
>   [Pref="dom.requestIdleCallback.enabled", Throws]
>   unsigned long requestIdleCallback(IdleRequestCallback callback,
>                                     optional IdleRequestOptions options);
>-  [Pref="dom.requestIdleCallback.enabled", Throws]
>+  [Pref="dom.requestIdleCallback.enabled"]
>   void          cancelIdleCallback(unsigned long handle);
> };
> 
>-- 
>2.5.0
>

Comment 14

[Andreas Farre [:farre]]

Attachment: 0001-WIP-Implement-requestIdleCallback.patch

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8748173 [details] [diff] [review]
0001-WIP-Implement-requestIdleCallback.patch

Review of attachment 8748173 [details] [diff] [review]:
-----------------------------------------------------------------

I really like this Timeout class!

::: dom/base/IdleRequest.cpp
@@ +10,5 @@
> +#include "mozilla/Maybe.h"
> +#include "mozilla/TimeStamp.h"
> +#include "mozilla/dom/WindowBinding.h"
> +#include "nsComponentManagerUtils.h"
> +#include "nsCycleCollectionParticipant.h"

you can remove this and probably other headers.

::: dom/base/IdleRequest.h
@@ +40,5 @@
> +  nsresult SetTimeout(uint32_t timout);
> +  nsresult RunIdleRequestCallback(bool aDidTimeout);
> +  void     CancelTimeout();
> +
> +  struct Comparator

Maybe, move this in an anonymous namespace in nsGlobalWindow.cpp and expose mHandle with a uint32_t Handle() const { return mHandle; }

::: dom/base/Timeout.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/Timeout.h"

just "Timeout.h"

@@ +8,5 @@
> +
> +#include "mozilla/LinkedList.h"
> +#include "mozilla/TimeStamp.h"
> +#include "nsCOMPtr.h"
> +#include "nsCycleCollectionParticipant.h"

remove all the included headers that are already in Timeout.h

@@ +26,5 @@
> +    mFiringDepth(0),
> +    mNestingLevel(0),
> +    mPopupState(openAllowed)
> +{
> +#ifdef DEBUG_jst

maybe follow up, or separate patch, remove this and ask jst to review the patch.

@@ +57,5 @@
> +}
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(Timeout)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_0(Timeout)

You should unlink mWindow, mPrincipal and mScriptHandler

@@ +77,5 @@
> +
> +// Return true if this timeout has a refcount of 1. This is used to check
> +// that dummy_timeout doesn't leak from nsGlobalWindow::RunTimeout.
> +bool
> +Timeout::HasRefCntOne()

This is used only in debug builds. Do:

#ifdef DEBUG
bool
Timeout::HasRefCntOne() ....
#endif

same in the header file.

::: dom/base/Timeout.h
@@ +13,5 @@
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsIPrincipal.h"
> +
> +class nsGlobalWindow;
> +class nsIPrincipal;

remove this or remove #include "nsIPrincipal.h"

@@ +36,5 @@
> +  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(Timeout)
> +
> +  nsresult InitTimer(uint32_t aDelay);
> +
> +  enum class Access { Public, Private };

ePublic ePrivate

@@ +38,5 @@
> +  nsresult InitTimer(uint32_t aDelay);
> +
> +  enum class Access { Public, Private };
> +
> +  bool HasRefCntOne();

bool HasRefCntOne() const ?
And add a comment about why we need it.

@@ +77,5 @@
> +
> +  // stack depth at which timeout is firing
> +  uint32_t mFiringDepth;
> +
> +  //

remove this line.

::: dom/base/nsContentSink.cpp
@@ +756,4 @@
>      aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);
>    }
>    if (!integrity.IsEmpty()) {
> +    MOZ_LOG(dom::SRILogHelper::GetSriLog(), mozilla::LogLevel::Debug,

Should it be part of the previous patch?

::: dom/base/nsGlobalWindow.cpp
@@ +11993,5 @@
>    nsCOMPtr<nsIScriptTimeoutHandler> handler(timeout->mScriptHandler);
>    RefPtr<Function> callback = handler->GetCallback();
> +  const Maybe<function<nsresult(nsIScriptTimeoutHandler*)>> nativeCallback =
> +    handler->GetNativeCallback();
> +

Can we assert this?

MOZ_ASSERT_IF(nativeCallback.isSome(), !callback);
MOZ_ASSERT_IF(callback, nativeCallback.IsNothing());

@@ +12323,4 @@
>  {
>    MOZ_RELEASE_ASSERT(IsInnerWindow());
>  
> +  uint32_t timer_id = (uint32_t)aTimerID;

I know that you are renaming/touching existing code, but can we try to improve the consistency of it having ID (or Id, or id) everywhere? In this method we have aTimerID, timer_id and mTimeoutId :)

::: dom/base/nsGlobalWindow.h
@@ +58,1 @@
>  

I guess you want to get rid of struct nsTimeout from this file as well.

@@ +205,4 @@
>    // stack depth at which timeout is firing
>    uint32_t mFiringDepth;
>  
> +  //

just simply remove this line.

@@ +1452,5 @@
>    // JS specific timeout functions (JS args grabbed from context).
>    nsresult ResetTimersForNonBackgroundWindow();
>  
>    // The timeout implementation functions.
> +  void RunTimeout(mozilla::dom::Timeout *aTimeout);

'*' next to the type. Here and everywhere else in your patch.

::: dom/base/nsJSTimeoutHandler.cpp
@@ +18,4 @@
>  #include <algorithm>
>  #include "mozilla/dom/FunctionBinding.h"
>  #include "nsAXPCNativeCallContext.h"
> +#include "mozilla/Maybe.h"

alphabetic order.

@@ +46,4 @@
>                             ErrorResult& aError);
>  
>    virtual const char16_t* GetHandlerText() override;
> +  virtual Function* GetCallback() override { return mFunction; }

I prefer the previous indentation.

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Netflix uses this API in Chrome so if we are going to do it, then we want to do it now.

Comment 17

[Andrew Overholt [:overholt]]

Can Netflix feature-detect the availability of this API?

Andreas is working on this but has remaining work to do like writing a scheduler for the various requested idle callbacks.

When you say "now", do you mean we need this "yesterday" or is having someone working on it sufficient? Do we need to see if we can parallelize the work?

Comment 18

[Chris Peterson [:cpeterson]]

Netflix could feature-detect this API, but we're trying to minimize the work we're asking them to do to port their Widevine HTML5 video player from Chrome to Firefox on OS X.

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Andrew Overholt [:overholt] from comment #17)
> Can Netflix feature-detect the availability of this API?

Lets take a step back and figure out the current priority and size. It seems like this will take a while so now isn't a realistic expectation. Our best approach is to ask Netflix to feature detect this (rather than sniffing) and give them an approximate time line. We want to bring Firefox as close to existing sites that use Widevine as possible but we do have other channels to use here.

Comment 20

[Chris Peterson [:cpeterson]]

This bug does not need to block Widevine. It only affects Netflix when spoofing Chrome's UA in Firefox. cpearce verified that bkelly's attached WIP patch allows him to test Netflix when spoofing Chrome's UA, so his Netflix testing is not blocked.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Chris Peterson [:cpeterson] from comment #20)
> cpearce verified that bkelly's attached WIP
> patch allows him to test Netflix when spoofing Chrome's UA, so his Netflix
> testing is not blocked.

You mean Andreas's patch. :-)

Comment 22

[Andreas Farre [:farre]]

Attachment: 0001-WIP-Implement-requestIdleCallback.patch

Comment 23

[Andreas Farre [:farre]]

Attachment: 0002-WIP-Run-idle-callbacks-after-paints.patch

Comment 24

[Andreas Farre [:farre]]

Attachment: 0002-WIP-Run-idle-callbacks-after-paints.patch

Implemented short period idle time detection and callback execution using the refresh driver.

Comment 25

[Andreas Farre [:farre]]

Attachment: 0002-WIP-Run-idle-callbacks-after-paints.patch

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8773801 [details] [diff] [review]
0002-WIP-Run-idle-callbacks-after-paints.patch

Review of attachment 8773801 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/moz.build
@@ +241,4 @@
>      'FileReader.cpp',
>      'FormData.cpp',
>      'FragmentOrElement.cpp',
> +    'IdleCallbackScheduler.cpp',

This file is not present in this patch. So the patch doesn't build. :(

Comment 27

[Andreas Farre [:farre]]

Attachment: 0002-WIP-Run-idle-callbacks-after-paints.patch

Yeah, sorry about that. I removed that file and created a new patch, but attached the old one instead of the new one. This one should be correct though.

With this there is some semblance of a functioning implementation of rIC, but I'm not sure of potential problems with my solution. Will badger people about feedback.

Comment 28

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Andrew - can you help with badgering people and/or landing a something-is-better-than-nothing implementation?

Comment 29

[Andrew Overholt [:overholt]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #28)
> Andrew - can you help with badgering people and/or landing a
> something-is-better-than-nothing implementation?

Obviously :) It's progressing.

Comment 30

[Patrick Walton (:pcwalton)]

No objections here from Servo's side, from a quick glance over the spec. Seems like a nice addition that's easy to implement minimally.

Comment 31

[Andreas Farre [:farre]]

Attachment: 0001-WIP-Implement-requestIdleCallback.patch

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8780096 [details] [diff] [review]
0001-WIP-Implement-requestIdleCallback.patch


>-/*
>- * Timeout struct that holds information about each script
>- * timeout.  Holds a strong reference to an nsIScriptTimeoutHandler, which
>- * abstracts the language specific cruft.
>- */
>-struct nsTimeout final
renaming this really belongs to a separate patch.



>+
>+    // We've just finished painting a frame, and using the tick timer
>+    // interval we can guess when the next tick is going to
>+    // happen. Use this to try to squeeze in a short idle period
>+    // in between frames.
>+    TimeStamp now = TimeStamp::Now();
>+    TimeStamp nextTick =
>+      (now + mShortIdlePeriod) -
>+      (now - GetTransactionStart());
Why mShortIdlePeriod affects to nextTick?
I guess I don't understand what nextTick means here.

>+  while (requestWindows.Length()) {
>+    uint64_t windowID = requestWindows[0];
>+    requestWindows.RemoveElementAt(0);
>+    nsGlobalWindow* window = nsGlobalWindow::GetInnerWindowWithId(windowID);
>+
>+    // RunIdleRequestCallbacks returns true to indicate that there are
>+    // callbacks left to run
>+    if(window && window->RunIdleRequestCallbacks(aIdleEnd)) {
What keeps window object alive while RunIdleRequestCallbacks is called?
Should it be RefPtr<nsGlobalWindow>



>+  // If the time for the last scheduled idle period hasn't passed we
>+  // should wait for that, i.e. we're waiting to process idle
>+  // callbacks as it is.
>+  if (TimeStamp::Now() < mLastIdlePeriodDeadline) {
>+    return;
>+  }
>+
>+  // You can't accomplish anything in less than 3 milliseconds!
>+  if (aIdleEnd < TimeStamp::Now() + TimeDuration::FromMilliseconds(3)) {
>+    return;
>+  }
>+
>+  // Record when we think we'll be done processing this round of idle callbacks.
>+  mLastIdlePeriodDeadline = aIdleEnd;
>+
>+  // Post a callback on the message loops deferred queue. It will call
>+  // back when the message loop either is idle, or the timeout has
>+  // expired. This may seem similar to the timeout option of
>+  // requestIdleCallback, but it is intended to be used to ensure that
>+  // we don't get backed up on idle callbacks.
>+  nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod<TimeStamp>(
>+    this, &nsRefreshDriver::RunIdleRequestCallbacks, aIdleEnd);
>+  nsCOMPtr<nsIMessageLoop> loop = mozilla::services::GetMessageLoop();
>+  loop->PostIdleTask(
>+    runnable,
>+    static_cast<uint32_t>((aIdleEnd - TimeStamp::Now()).ToMilliseconds()));
I wish nsIMessageLoop wasn't used here. 


>+
>+/* static */ void
>+nsRefreshDriver::ScheduleLongIdlePeriod(nsITimer* aTimer, void* aClosure)
>+{
>+  // This is a big hack, unfortunately. The long idle period is
>+  // currently the same as 1/fps.
What does 1/fps mean hear? Is long idle period normally 0.016ms or what? or 16ms?

 This is because we don't implement
>+  // real idleness detection at the moment.
Not sure what "real idleness" means.


>+++ b/xpcom/build/ServiceList.h
>@@ -33,6 +33,8 @@ MOZ_SERVICE(UUIDGenerator, nsIUUIDGenerator,
>             "@mozilla.org/uuid-generator;1");
> MOZ_SERVICE(GfxInfo, nsIGfxInfo,
>             "@mozilla.org/gfx/info;1");
>+MOZ_SERVICE(MessageLoop, nsIMessageLoop,
>+            "@mozilla.org/message-loop;1");

We should really use xpcom main loop, and not chromium loop, so this shouldn't be needed.
chromium loop might be rather empty occasionally when main loop isn't.
I think we could rename nsIMessageLoop and make it use normal event loop. Or even add
postIdleRunnable to nsIThread. That might be easiest, and useful also elsewhere.


I don't understand why the need to schedule any specific long idle periods. Spec doesn't have such. Spec just lets UA to guess deadline (max now+50ms).


Does something use the information about scheduled setTimeout and setInterval to schedule rIC?
I think ::ScheduleIdlePeriod(TimeStamp aIdleEnd) should do that.
Would be perhaps good to move next tick calculation to that method and take into account also settimeout/interval

Comment 33

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1198381-Move-rename-nsTimeout-to-Timeout.patch

Comment 34

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1198381-Extend-setTimeout-handling-in-nsGlobalWi.patch

Comment 35

[Andreas Farre [:farre]]

Attachment: 0003-Bug-1198381-Extend-nsIEventTarget-with-idleDispatch.patch

Comment 36

[Andreas Farre [:farre]]

Attachment: 0004-Bug-1198381-Implement-the-requestIdleCallback-featur.patch

Comment 37

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hi Andreas. Did you mean to ask for review on these patches?

Comment 38

[Andreas Farre [:farre]]

(In reply to Bill McCloskey (:billm) from comment #37)
> Hi Andreas. Did you mean to ask for review on these patches?

Not really, just in case I would request more feedback. Will in moments submit a review request to mozreview.

Comment 39

[Andreas Farre [:farre]]

Attachment: Bug 1198381 - Move/rename nsTimeout to Timeout,

Review commit: https://reviewboard.mozilla.org/r/77208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77208/

Comment 40

[Andreas Farre [:farre]]

Attachment: Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

The requestIdleCallback feature behaves in many ways as setTimeout
since it takes an optional timout when the idle callback will be
called regardless of the user agent being idle or not. This means that
the same mechanisms controlling setTimeout are needed for
requestIdleCallback.

Review commit: https://reviewboard.mozilla.org/r/77210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77210/

Comment 41

[Andreas Farre [:farre]]

Attachment: Bug 1198381 - Extend nsIThread with idleDispatch,

The intent of idleDispatch is the possibility to have a runnable
executed when the event target is idle. This only makes sense for
nsThread, so the implementation for other event targets are
intentionally left unimplemented.

Review commit: https://reviewboard.mozilla.org/r/77212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77212/

Comment 42

[Andreas Farre [:farre]]

Attachment: Bug 1198381 - Implement the requestIdleCallback feature,

Expose requestIdleCallback on Window and implement idle detection
using nsRefreshdriver. Idle period scheduling is accomplished by
either estimating when the next tick will happen or when the last tick
occured after a sufficiently long time ago that we can say that the
current refresh driver won't refresh as if it were animating.

Review commit: https://reviewboard.mozilla.org/r/77214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77214/

Comment 43

[Andreas Farre [:farre]]

Attachment: Bug 1198381 - Add tests for requestIdleCallback,

Review commit: https://reviewboard.mozilla.org/r/77216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77216/

Comment 44

[Andreas Farre [:farre]]

Attachment: Empty file that obsolets old, non-mozreview patches

Comment 45

[Nathan Froyd [:froydnj]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review75500

I've read through part 4 and AFAICT, the idleness your patches care about is for the refresh driver, not the main thread event loop.  Therefore, all of this infrastructure for `nsIEventTarget` doesn't seem particularly appropriate.  Are you envisioning some later time when the main thread's event loop and the refresh driver are more closely intertwined?  Can you explain your reasoning here, and re-request review if you think it's appropriate?

::: netwerk/base/nsSocketTransportService2.cpp:255
(Diff revision 1)
>          RemoveFromPollList(sock);
>      else
>          RemoveFromIdleList(sock);
>  
>      // NOTE: sock is now an invalid pointer
> -    
> +

Note: we prefer that you not modify whitespace in code that you're not directly changing; please revert the whitespace changes in this patch.

::: xpcom/glue/nsThreadUtils.cpp:213
(Diff revision 1)
> +    // NOTE: if you stop leaking here, adjust Promise::MaybeReportRejected(),
> +    // which assumes a leak here, or split into leaks and no-leaks versions

This comment isn't appropriate here, as `NS_IdleDispatchToMainThread` isn't being called in a place where it might possibly leak.

::: xpcom/threads/nsIEventTarget.idl:121
(Diff revision 1)
>     * events, so this event would never run and has not been dispatched, or
>     * that delay is zero.
>     */
>    [noscript] void delayedDispatch(in alreadyAddRefed_nsIRunnable event, in unsigned long delay);
> +
> +  [noscript] void idleDispatch(in alreadyAddRefed_nsIRunnable event);

You said this really only makes sense for `nsIThread` event targets, but given the current implementation, why not just make this function a `%{C++ %}` bit that forwards to `Dispatch`?  Then you wouldn't have to add all the boilerplate to every `nsIEventTarget` implementation, and you avoid footguns where people call `IdleDispatch` assuming that it succeeds and it silently fails because their `nsIEventTarget` isn't the right kind of event target under the hood.

Ideally, I think, we'd make `IdleDispatch` forward to `Dispatch` for everything except `nsIThread`, and then `nsThread` would provide its own, thread-aware implementation of `IdleDispatch`.

Comment 46

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review75500

You're right, the idea of adding IdleDispatch to nsIEventTarget is really the next step. I could've limited myself to just call Dispatch instead of adding IdleDispatch, but I wanted to show my intentions.

What the current patch is lacking is the ability of avoiding more than just the refresh driver's execution. I want to be able to also inspect the main loop to see if there are pending runnables on the queue and also query the timer thread to be able to determine if a timer is due soon. The former is something that I could see happen in nsThread::IdleDispatch, by e.g. having a separate queue for lower priority 'run-when-idle' kind of tasks.

Ideally the IdleDispatch backend would handle all of avoiding higher priority work, and have some way of from several sources gather the next time they're bound to execute and figuring out idleness that way.

The more I explain, the more I think that this is too pre-mature. I think I'll revert to calling the regular Dispatch and drop part 4. I'll probably come back with something along these lines further on though, when it would actually do something.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Andreas, in bug 1300659 I am proposing to add a main thread TaskQueue for each window.  I'm wondering if that could help you here.

For example, idle-ness could be defined as:

1) Inspect main thread TaskQueue to see if its empty
  a) If its empty, then execute idle stuff now
2) If its not empty then insert a CheckForIdleRunnable into the TaskQueue
3) When CheckForIdleRunnable executes, go back to (1)

Anyway, I just wanted to throw that out there as an alternative to trying to bake this into the nsIEventTarget and all its implementations.

What do you think?

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

I don't think TaskQueue can help here, since we want thread level idleness, if possible, not any connected-windows-taskqueue idleness.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review75548

This a is a good start, but I think there is at least one spec problem (timeRemaining attribute) and I have concerns about the scheduling of the callbacks.

::: dom/base/IdleDeadline.h:13
(Diff revision 1)
> +#define mozilla_dom_IdleDeadline_h
> +
> +#include "js/TypeDecls.h"
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/ErrorResult.h"

nit: duplicate header include

::: dom/base/IdleDeadline.h:32
(Diff revision 1)
> +  : public nsISupports
> +  , public nsWrapperCache
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(IdleDeadline)

If you just put the NS_DECL_ lines at the bottom of the class you can drop the extra public: line.  A lot of code follows this convention.

::: dom/base/IdleDeadline.h:38
(Diff revision 1)
> +
> +public:
> +  IdleDeadline(nsPIDOMWindowInner* aWindow, Maybe<DOMHighResTimeStamp> aDeadline);
> +
> +protected:
> +  ~IdleDeadline();

The destructor should be private since the class is final.

::: dom/base/IdleDeadline.h:51
(Diff revision 1)
> +  DOMHighResTimeStamp TimeRemaining(ErrorResult& aError);
> +  bool DidTimeout() const;
> +
> +private:
> +  nsCOMPtr<nsPIDOMWindowInner> mWindow;
> +  Maybe<DOMHighResTimeStamp> mDeadline;

Can you make mDeadline const?

::: dom/base/IdleDeadline.cpp:54
(Diff revision 1)
> +    return 0.0;
> +  }
> +
> +  RefPtr<Performance> performance = mWindow->GetPerformance();
> +  if (!performance) {
> +    aError.Throw(NS_ERROR_NOT_AVAILABLE);

Could we just return 0.0 (immediate deadline) if the Performance object is no longer available?  I think this means the window is going inactive, etc.

::: dom/base/IdleDeadline.cpp:63
(Diff revision 1)
> +}
> +
> +bool
> +IdleDeadline::DidTimeout() const
> +{
> +  return mDeadline.isNothing();

This seems wrong to me.  The spec in section 4.3 says:

"The didTimeout attribute on an IdleDeadline object MUST return true if the callback was invoked by the invoke idle callback timeout algorithm, and false otherwise."

And section 5.3 step 2.3 says:

"Let deadlineArg be an IdleDeadline constructed with a deadline of now and the didTimeout attribute set to true."

As fas as I can tell we're just returning true here if we have any deadline at all.

::: dom/base/nsGlobalWindow.h:1050
(Diff revision 1)
> +  uint32_t RequestIdleCallback(mozilla::dom::IdleRequestCallback& aCallback,
> +                               const mozilla::dom::IdleRequestOptions& aOptions,
> +                               mozilla::ErrorResult& aError);
> +  void CancelIdleCallback(uint32_t aHandle);
> +
> +  bool RunIdleRequestCallbacks(mozilla::TimeStamp aIdleEnd);

Please add some documentation for this method.

::: dom/base/nsGlobalWindow.h:1052
(Diff revision 1)
> +                               mozilla::ErrorResult& aError);
> +  void CancelIdleCallback(uint32_t aHandle);
> +
> +  bool RunIdleRequestCallbacks(mozilla::TimeStamp aIdleEnd);
> +
> +  typedef nsTArray<RefPtr<mozilla::dom::IdleRequest>> IdleRequests;

It doesn't seem this typedef is used outside of nsGlobalWindow.  Can you move it to the private: part of the header where the object members are defined?

::: dom/base/nsGlobalWindow.h:1826
(Diff revision 1)
> +
>     // The current idle request callback timeout handle
>    uint32_t mIdleCallbackTimeoutCounter;
> +  // The current idle request callback handle
> +  uint32_t mIdleRequestCallbackCounter;
> +  IdleRequests mIdleRequestCallbacks;

Move the IdleRequests typedef to here please.

::: dom/base/nsGlobalWindow.h:1827
(Diff revision 1)
>     // The current idle request callback timeout handle
>    uint32_t mIdleCallbackTimeoutCounter;
> +  // The current idle request callback handle
> +  uint32_t mIdleRequestCallbackCounter;
> +  IdleRequests mIdleRequestCallbacks;
> +  IdleRequests mPendingIdleRequestCallbacks;

Please add a comment that these are sorted by callback handle.

Also, should these lists be LinkedList types instead of nsTArray?  It seems like if script passes any kind of timeout value we are going to end up processing these lists out of order and removing from the middle of the list.

::: dom/base/nsGlobalWindow.cpp:629
(Diff revision 1)
> +    RefPtr<IdleRequest> request = mPendingIdleRequestCallbacks[0];
> +    mPendingIdleRequestCallbacks.RemoveElementAt(0);
> +    request->RunIdleRequestCallback(Some(timeStamp));
> +
> +    if (aIdleEnd < TimeStamp::Now()) {
> +      mIdleRequestCallbacks.InsertElementsAt(0, mPendingIdleRequestCallbacks);

For example, this is expensive with nsTArray but would be cheap with LinkedList.

::: dom/base/nsGlobalWindow.cpp:672
(Diff revision 1)
> +    return;
> +  }
> +
> +  nsRefreshDriver* refreshDriver = presContext->RefreshDriver();
> +  if (NS_WARN_IF(!refreshDriver)) {
> +    return;

Should these ever be able to fail?  We only call this method at explicit times.  It seems like we should be able to assert here or call this method at the appropriate time to avoid triggering any of these failures.

::: dom/base/nsGlobalWindow.cpp:709
(Diff revision 1)
> +    return;
> +  }
> +
> +  nsRefreshDriver* refreshDriver = presContext->RefreshDriver();
> +  if (NS_WARN_IF(!refreshDriver)) {
> +    return;

Any of these early returns will effectively leave the callbacks orphaned in the window, no?  It seems like this is only called from ::RequestIdleCallback().  Any reason not to propagate an error code back and clean up the resources?

::: dom/base/nsInProcessTabChildGlobal.h:13
(Diff revision 1)
>  #define nsInProcessTabChildGlobal_h
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/DOMEventTargetHelper.h"
>  #include "nsCOMPtr.h"
> +#include "nsContentUtils.h"

What is this change for?

::: dom/webidl/IdleDeadline.webidl:9
(Diff revision 1)
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Pref="dom.requestIdleCallback.enabled"]
> +interface IdleDeadline {
> +  [Throws]

See my question in the TimeRemaining() method impl.  I wonder if we can get rid of this [Throws].

::: dom/webidl/Window.webidl:520
(Diff revision 1)
>  Window implements ChromeWindow;
>  Window implements GlobalFetch;
>  Window implements ImageBitmapFactories;
> +
> +partial interface Window {
> +  [Pref="dom.requestIdleCallback.enabled", Throws]

Is the implementation incomplete?  What is the reason to hide this behind a pref?

::: layout/base/nsRefreshDriver.cpp:2253
(Diff revision 1)
> +  MOZ_ASSERT(mIdleTimer);
> +  mIdleTimer->Cancel();
> +
> +  // This timer will be reset in ScheduleIdlePeriod, hence it should
> +  // only trigger when we haven't painted recently.
> +  TimeDuration delay = TimeDuration::FromSeconds(10);

Why 10 seconds here?  When would two refresh tick periods ever exceed 10 seconds? Can you explain the rationale for this algorithm a bit more?

::: layout/base/nsRefreshDriver.cpp:2271
(Diff revision 1)
> +  MOZ_ASSERT(NS_IsMainThread());
> +  NS_ASSERTION(mIdleRequestCallbackWindows.IndexOf(aWindowID) ==
> +               mIdleRequestCallbackWindows.NoIndex,
> +               "Don't schedule the same window multiple times");
> +
> +  mIdleRequestCallbackWindows.AppendElement(aWindowID);

This gets called for every window.requestIdlCallback() method AFAICT.  This means you can end up with duplicates here.  It seems like we should check to see if the window ID is already in the list before appending.

Alternatively, maybe we should be storing an array of requests instead to handle the fairness questions I raised in other comments.

::: layout/base/nsRefreshDriver.cpp:2309
(Diff revision 1)
> +  if (aMostRecentRefresh != sRegularRateTimer->MostRecentRefresh()) {
> +    aIdleEnd = GetIdleEnd(true, TimeStamp::Now());
> +  }
> +
> +  // You can't accomplish anything in less than 3 milliseconds!
> +  if (aIdleEnd < TimeStamp::Now() + TimeDuration::FromMilliseconds(3)) {

Again, this seems like a magic number.  Should we define these in constants or even prefs so we can tune things?

::: layout/base/nsRefreshDriver.cpp:2313
(Diff revision 1)
> +  // You can't accomplish anything in less than 3 milliseconds!
> +  if (aIdleEnd < TimeStamp::Now() + TimeDuration::FromMilliseconds(3)) {
> +    return;
> +  }
> +
> +  // Take a copy of and clear mIdleRequestCallbackWindows. We will

I wonder if there is a fairness issue here.  We run all idle callbacks for a single window before moving to the next.  This could re-order idle callbacks to favor a particular window.

Consider:

  Window 1 rIC
  Window 2 rIC
  Window 1 rIC
  Window 1 rIC
  Window 1 rIC
  Window 1 rIC
  
This will run all 5 Window 1 idle callbacks before Window 2 gets a shot at running.

::: layout/base/nsRefreshDriver.cpp:2325
(Diff revision 1)
> +    RefPtr<nsGlobalWindow> window =
> +      nsGlobalWindow::GetInnerWindowWithId(windowID);
> +
> +    // RunIdleRequestCallbacks returns true to indicate that there are
> +    // callbacks left to run, but that the idle time ran out.
> +    if(window && window->RunIdleRequestCallbacks(aIdleEnd)) {

In addition to the previous fairness question, I'm not sure its a good idea to run multiple idle callbacks in a single runnable like this.  The idle callbacks would have less impact to responsiveness if we yielded back to the main thread after each one.  So effectively dispatch a new Runnable and re-check idleness after each callback.

::: layout/base/nsRefreshDriver.cpp:2342
(Diff revision 1)
> +  // didn't manage to run to keep the scheduled order.
> +  mIdleRequestCallbackWindows.InsertElementsAt(0, requestWindows);
> +
> +  // If there are no scheduled callbacks, make sure to turn off the
> +  // long idle period timer.
> +  if (!mIdleRequestCallbackWindows.Length()) {

IsEmpty() is prefered to !Length()

::: layout/base/nsRefreshDriver.cpp:2356
(Diff revision 1)
> +{
> +  TimeStamp mostRecentRefresh = sRegularRateTimer->MostRecentRefresh();
> +  TimeDuration refreshRate = sRegularRateTimer->GetTimerRate();
> +  TimeStamp idleEnd = mostRecentRefresh + refreshRate;
> +
> +  if (!aDidPaint && (idleEnd + refreshRate * 2 < aNowTime)) {

Can this calc be shared with the calculation in ResetIdleTimer?

::: layout/base/nsRefreshDriver.cpp:2358
(Diff revision 1)
> +  TimeDuration refreshRate = sRegularRateTimer->GetTimerRate();
> +  TimeStamp idleEnd = mostRecentRefresh + refreshRate;
> +
> +  if (!aDidPaint && (idleEnd + refreshRate * 2 < aNowTime)) {
> +    // It was a long time since we painted
> +    idleEnd = aNowTime + TimeDuration::FromMilliseconds(50);

Another magic number that could be extracted.

::: layout/base/nsRefreshDriver.cpp:2394
(Diff revision 1)
> +    return;
> +  }
> +
> +  TimeStamp idleEnd = GetIdleEnd(aDidPaint, now);
> +
> +  // You can't accomplish anything in less than 3 milliseconds!

This code seems duplicated with RunIdleCallbacks above.  Can we refactor it to remove duplication?

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788893 [details]
Bug 1198381 - Add tests for requestIdleCallback,

https://reviewboard.mozilla.org/r/77216/#review75632

Looks good, but I'd like to see it again after the other patch comments are addressed.

Also, could you make a test that spams the event queue and verify rIC does not fire while its busy?

::: testing/web-platform/tests/html/webappapis/idle-callbacks/callback-multiple-calls.html:22
(Diff revision 1)
> +
> +      ++counter;
> +    }
> +    window.requestIdleCallback(t.step_func(function () { f(0) }));
> +    window.requestIdleCallback(t.step_func(function () { f(1) }));
> +  }, "requestIdleCallback callbacks should be invoked in order");

Can you add another test that uses two different iframes and interleaves idle callbacks?  I don't think the current code will maintain ordering in that case.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Andreas, do you want me to review all of https://reviewboard.mozilla.org/r/77214/diff/1#index_header or just some specific part of it?

Comment 52

[Andreas Farre [:farre]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review75858

::: dom/base/IdleDeadline.cpp:63
(Diff revision 1)
> +}
> +
> +bool
> +IdleDeadline::DidTimeout() const
> +{
> +  return mDeadline.isNothing();

It's not wrong, but it is confusing and I'll fix that. The trouble is that I joined the values of didTimeout and the end of the idle period. The Maybe is really the inverse of didTimeout, i.e. Nothing == didTimeout:true and Some(\_) == didTimeout:false, and the value of the Maybe is the point in time where timeRemaining will start to return 0.0.

It could as easily be rewritten to:
bool mDidTimeout;
DOMHighResTimeStamp mDeadline;

where TimeRemaining starts with:

if (mDidTimeout) \{ return 0.0; \}

and DidTimeout is implemented as just return mDidTimeout.

::: dom/base/nsInProcessTabChildGlobal.h:13
(Diff revision 1)
>  #define nsInProcessTabChildGlobal_h
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/DOMEventTargetHelper.h"
>  #include "nsCOMPtr.h"
> +#include "nsContentUtils.h"

This was a unified compilation compile fix that for some reason got sucked into this commit instead of being on its own.

::: layout/base/nsRefreshDriver.cpp:2253
(Diff revision 1)
> +  MOZ_ASSERT(mIdleTimer);
> +  mIdleTimer->Cancel();
> +
> +  // This timer will be reset in ScheduleIdlePeriod, hence it should
> +  // only trigger when we haven't painted recently.
> +  TimeDuration delay = TimeDuration::FromSeconds(10);

The larger than 10s tick period happens only when the refresh driver is throttled, and then it can be way larger than 10s, but we still want to call idle callbacks on throttled tabs on a regular basis. I'll add a comment explaining this.

::: layout/base/nsRefreshDriver.cpp:2394
(Diff revision 1)
> +    return;
> +  }
> +
> +  TimeStamp idleEnd = GetIdleEnd(aDidPaint, now);
> +
> +  // You can't accomplish anything in less than 3 milliseconds!

I actually think that it could be removed entirely. The point of this was to not get too many idle callback runnables on the main loop, but since we have the now < mLastIdleDeadline above we'll only ever get at most one per refresh driver, which is acceptable.

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Also, after thinking about this a bit over the evening I'm not sure we need to maintain strict ordering across windows.  For example, if a window is in the background we could/should stop firing idle callbacks similar to how we throttle timers.  We still probably want to fix the fairness issue to prevent one window from monopolizing the idle callback time, though.

Comment 54

[Andreas Farre [:farre]]

(In reply to Boris Zbarsky [:bz] from comment #51)
> Andreas, do you want me to review all of
> https://reviewboard.mozilla.org/r/77214/diff/1#index_header or just some
> specific part of it?

The layout stuff primarily, I looked for a layout/base peer that also had dom knowledge, and you stood out :)

Comment 55

[Andreas Farre [:farre]]

(In reply to Ben Kelly [:bkelly] from comment #47)
> Andreas, in bug 1300659 I am proposing to add a main thread TaskQueue for
> each window.  I'm wondering if that could help you here.
> 
> For example, idle-ness could be defined as:
> 
> 1) Inspect main thread TaskQueue to see if its empty
>   a) If its empty, then execute idle stuff now
> 2) If its not empty then insert a CheckForIdleRunnable into the TaskQueue
> 3) When CheckForIdleRunnable executes, go back to (1)
> 
> Anyway, I just wanted to throw that out there as an alternative to trying to
> bake this into the nsIEventTarget and all its implementations.
> 
> What do you think?

The TaskQueue could help with the parts where rIC is similar to setTimeout (throttling, preventing callback flooding), but I'm starting to get second thoughts about the parts concerning that as well at this point in time (that would be parts 1 and 2, the extensions to nsTimeout) because I now think that they might not be enough to begin with.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review76386

I did not review the details of the DOM stuff or the scheduling bits very carefully, esp. since there seems to be pending feedback from bkelly to handle.  But I did have some general questions....

::: dom/base/IdleDeadline.cpp:54
(Diff revision 1)
> +    return 0.0;
> +  }
> +
> +  RefPtr<Performance> performance = mWindow->GetPerformance();
> +  if (!performance) {
> +    aError.Throw(NS_ERROR_NOT_AVAILABLE);

In any case, you need to return if !performance instead of pressing on to use it below.

::: dom/base/IdleRequest.h:23
(Diff revision 1)
> +namespace mozilla {
> +namespace dom {
> +
> +class IdleRequestCallback;
> +
> +class IdleRequest final : public nsIScriptTimeoutHandler

This seems like overkill if we never have to support a string version...  Of course we also never have to support the Function* version.

It would probably be better to have a superclass or discriminated union or something in the window code...

::: dom/base/IdleRequest.cpp:102
(Diff revision 1)
> +  RefPtr<IdleRequest> handler = static_cast<IdleRequest*>(aHandler);
> +  return handler->RunIdleRequestTimeout();
> +}
> +
> +Maybe<IdleRequest::NativeCallback>
> +IdleRequest::GetNativeCallback()

If this never returns null, then you should MOZ_RELEASE_ASSERT or equivalent (MOZ_ASSERT plus some notreached annotations) that GetCallback/GetHandlerText/GetLocation are not called.

In particular, returning an NS_LITERAL_STRING as a reference is wrong; it'll give a reference to a destroyed object, afaict.

::: dom/webidl/IdleDeadline.webidl:5
(Diff revision 1)
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

This file needs to link to the relevant spec.

::: dom/webidl/Window.webidl:519
(Diff revision 1)
>  
>  Window implements ChromeWindow;
>  Window implements GlobalFetch;
>  Window implements ImageBitmapFactories;
> +
> +partial interface Window {

This needs to link to the relevant spec.

::: layout/base/nsRefreshDriver.h:245
(Diff revision 1)
>     * Schedule a frame visibility update "soon", subject to the heuristics and
>     * throttling we apply to visibility updates.
>     */
>    void ScheduleFrameVisibilityUpdate() { mNeedToRecomputeVisibility = true; }
>  
> +  void ScheduleIdleRequestCallbacks(uint64_t aWindowID);

All the additions to this header need documentation....

::: layout/base/nsRefreshDriver.cpp:1951
(Diff revision 1)
>      if (nsContentUtils::XPConnect()) {
>        nsContentUtils::XPConnect()->NotifyDidPaint();
>        nsJSContext::NotifyDidPaint();
>      }
> +
> +    ScheduleIdlePeriod(true);

So why is the refresh driver the right place to hook this in?  That doesn't even exist in some display:none-like scenarios, iirc....

::: layout/base/nsRefreshDriver.cpp:2394
(Diff revision 1)
> +    return;
> +  }
> +
> +  TimeStamp idleEnd = GetIdleEnd(aDidPaint, now);
> +
> +  // You can't accomplish anything in less than 3 milliseconds!

Why can't you accomplish anything in less than 3ms?

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

https://reviewboard.mozilla.org/r/77208/#review76502

A bit hard to review, but I assume the latter patches explain all the changes.

::: dom/base/Timeout.h:73
(Diff revision 1)
> +  TimeStamp mWhen;
> +  // Remaining time to wait.  Used only when timeouts are suspended.
> +  TimeDuration mTimeRemaining;
> +
> +  // Principal with which to execute
> +  nsCOMPtr<nsIPrincipal> mPrincipal;

hmm, you add a new member variable. I have no idea why. This patch is hard to review.

::: dom/base/Timeout.cpp:42
(Diff revision 1)
> +  MOZ_COUNT_DTOR(Timeout);
> +}
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(Timeout)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Timeout)

Hmm, you change unlinking in this rename/move patch.

::: dom/base/nsIScriptTimeoutHandler.h:38
(Diff revision 1)
>  
> +  typedef mozilla::function<nsresult(nsIScriptTimeoutHandler*)> NativeCallback;
> +
> +  // Get the C++ callback to call. If this returns Nothing, GetGallback() will
> +  // be called to get the Function.
> +  virtual mozilla::Maybe<NativeCallback> GetNativeCallback() = 0;

ok, this patch doesn't yet use this new method for anything. In general this kind of code-move patches shouldn't add anything to the code, just move/rename

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

https://reviewboard.mozilla.org/r/77210/#review76504

r+, assuming I understand the following patches which will actually use this stuff :)

::: dom/base/nsGlobalWindow.cpp:12156
(Diff revision 1)
>    RefPtr<Function> callback = handler->GetCallback();
> +  const Maybe<function<nsresult(nsIScriptTimeoutHandler*)>> nativeCallback =
> +    handler->GetNativeCallback();
>  
> -  if (!callback) {
> +  if (nativeCallback.isSome()) {
> +    nsCOMPtr<nsISupports> kungFuDeathGrip(static_cast<nsIDOMWindow *>(this));

extra space before *

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review76506

So, I think adding the new method to nsIThread would make more sense and would make the code simpler.
If we later need it in other nsIEventTargets too, then we can move it here.

::: xpcom/threads/nsIEventTarget.idl:121
(Diff revision 1)
>     * events, so this event would never run and has not been dispatched, or
>     * that delay is zero.
>     */
>    [noscript] void delayedDispatch(in alreadyAddRefed_nsIRunnable event, in unsigned long delay);
> +
> +  [noscript] void idleDispatch(in alreadyAddRefed_nsIRunnable event);

I would put idleDispatch to nsIThread.
Looks like delayedDispatch is for some bizarre reason in nsIEventTarget and not in nsIThread, yet nsThread is after all its only implementation.
froydnj, why did  delayedDispatch end up to nsIEventTarget?

Comment 60

[Andreas Farre [:farre]]

Attachment: Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review commit: https://reviewboard.mozilla.org/r/83666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83666/

Comment 61

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/1-2/

Comment 62

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/1-2/

Comment 63

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/1-2/

Comment 64

[Andreas Farre [:farre]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77214/diff/1-2/

Comment 65

[Andreas Farre [:farre]]

Comment on attachment 8788893 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77216/diff/1-2/

Comment 66

[Andreas Farre [:farre]]

I've pushed a substantial re-write to mozreview. Sorry for the big changes. I think that the relevant issues from the review so far have been addressed, but I guess that by introducing entirely new code other issues might pop up. Sorry for that as well.

On the other hand, the new version is way more well separated. The changes to nsThread to be able to handle events that should run when idle is completely isolated from the implementation of requestIdleCallback making it possible to use nsIThread::idleDispatch to run a host of other things that could execute when idle.

I've also switched around with reviewers, sorry for that as well, but hopefully I haven't bothered anyone too much.

Comment 67

[Nathan Froyd [:froydnj]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review82478

I have a few comments.  The intent of the previous patch is much clearer after reading through this patch.  I expect smaug will have more comments on the DOM stuff than I would.

::: dom/base/nsGlobalWindow.cpp:582
(Diff revision 2)
> +
> +  nsGlobalWindow* outer = GetOuterWindowInternal();
> +  if (outer && outer->AsOuter()->IsBackground()) {
> +    // mThrottledIdleRequestCallbacks now owns request
> +    mThrottledIdleRequestCallbacks.insertBack(request);
> +    request->AddRef();

I'm pretty sure this is going to fail in static analysis builds; we don't permit calling `AddRef` or `Release` on smart pointers.  Here and other places.

::: dom/base/nsGlobalWindow.cpp:584
(Diff revision 2)
> +    NS_DelayedDispatchToMainThread(
> +      NewRunnableMethod(this, &nsGlobalWindow::PostThrottledIdleCallback),
> +      10000);

Why 10000 here?  Could we at least derive that constant from some other constants?

I have a slight preference for adding only `NS_DelayedDispatchToCurrentThread`, as I think DOM code usually uses `*CurrentThread` unless it's actually running off-thread.  (This rule is probably not consistently enforced.)

::: dom/base/nsGlobalWindow.cpp:620
(Diff revision 2)
> +  for (RefPtr<IdleRequest> request = mIdleRequestCallbacks.popFirst(); request;
> +       request = request->getNext()) {
> +    request->Cancel();

How does this loop work?  You pop the request off, and then it is no longer in the list, so it has no access to the rest of the list anymore.

And even if it did, when you `Cancel` it, you remove it from the list, so it no longer has access to the rest of the list when you try to go around the loop again.

The loops you added from `nsGlobalWindow::FreeInnerObjects` seem more appropriate here.  And what happens to throttled callback requests, shouldn't we pop those as well?

::: xpcom/threads/nsThread.h:201
(Diff revision 2)
>      {
>      }
>    };
>  
> -  // This lock protects access to mObserver, mEvents and mEventsAreDoomed.
> -  // All of those fields are only modified on the thread itself (never from
> +  // This lock protects access to mObserver, mEvents, mIdleQueue and
> +  // mEventsAreDoomed.  All of those fields are only modified on the

...and `mIdlePeriod` from the previous patch, yes?

Comment 68

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #67)
> Comment on attachment 8788892 [details]
> Bug 1198381 - Implement the requestIdleCallback feature,
> 
> https://reviewboard.mozilla.org/r/77214/#review82478
> 
> I have a few comments.  The intent of the previous patch is much clearer
> after reading through this patch.  I expect smaug will have more comments on
> the DOM stuff than I would.

Excellent comments, thanks. It's definitely the xpcom/threads part that I wanted your help with the most.
 
> ::: dom/base/nsGlobalWindow.cpp:582
> (Diff revision 2)
> > +
> > +  nsGlobalWindow* outer = GetOuterWindowInternal();
> > +  if (outer && outer->AsOuter()->IsBackground()) {
> > +    // mThrottledIdleRequestCallbacks now owns request
> > +    mThrottledIdleRequestCallbacks.insertBack(request);
> > +    request->AddRef();
> 
> I'm pretty sure this is going to fail in static analysis builds; we don't
> permit calling `AddRef` or `Release` on smart pointers.  Here and other
> places.

So, this seems risky to me as well, but it models itself against how the timeouts are handled in the global window. Those are also held in a LinkedList and are also manually addref'ed and released. See nsGlobalWindow::InsertTimeoutIntoList.


> ::: dom/base/nsGlobalWindow.cpp:584
> (Diff revision 2)
> > +    NS_DelayedDispatchToMainThread(
> > +      NewRunnableMethod(this, &nsGlobalWindow::PostThrottledIdleCallback),
> > +      10000);
> 
> Why 10000 here?  Could we at least derive that constant from some other
> constants?

Yes, I should probably even pull that out into a pref.

> I have a slight preference for adding only
> `NS_DelayedDispatchToCurrentThread`, as I think DOM code usually uses
> `*CurrentThread` unless it's actually running off-thread.  (This rule is
> probably not consistently enforced.)

I agree, will make it so.

> ::: dom/base/nsGlobalWindow.cpp:620
> (Diff revision 2)
> > +  for (RefPtr<IdleRequest> request = mIdleRequestCallbacks.popFirst(); request;
> > +       request = request->getNext()) {
> > +    request->Cancel();
> 
> How does this loop work?  You pop the request off, and then it is no longer
> in the list, so it has no access to the rest of the list anymore.

Right. That shouldn't work. I'll fix that.

> And even if it did, when you `Cancel` it, you remove it from the list, so it
> no longer has access to the rest of the list when you try to go around the
> loop again.
> 
> The loops you added from `nsGlobalWindow::FreeInnerObjects` seem more
> appropriate here.  And what happens to throttled callback requests,
> shouldn't we pop those as well?



> ::: xpcom/threads/nsThread.h:201
> (Diff revision 2)
> >      {
> >      }
> >    };
> >  
> > -  // This lock protects access to mObserver, mEvents and mEventsAreDoomed.
> > -  // All of those fields are only modified on the thread itself (never from
> > +  // This lock protects access to mObserver, mEvents, mIdleQueue and
> > +  // mEventsAreDoomed.  All of those fields are only modified on the
> 
> ...and `mIdlePeriod` from the previous patch, yes?

Yes.

Comment 69

[Nathan Froyd [:froydnj]]

(In reply to Andreas Farre [:farre] from comment #68)
> (In reply to Nathan Froyd [:froydnj] from comment #67)
> > Comment on attachment 8788892 [details]
> > Bug 1198381 - Implement the requestIdleCallback feature,
> > 
> > https://reviewboard.mozilla.org/r/77214/#review82478
> > 
> > I have a few comments.  The intent of the previous patch is much clearer
> > after reading through this patch.  I expect smaug will have more comments on
> > the DOM stuff than I would.
> 
> Excellent comments, thanks. It's definitely the xpcom/threads part that I
> wanted your help with the most.
>  
> > ::: dom/base/nsGlobalWindow.cpp:582
> > (Diff revision 2)
> > > +
> > > +  nsGlobalWindow* outer = GetOuterWindowInternal();
> > > +  if (outer && outer->AsOuter()->IsBackground()) {
> > > +    // mThrottledIdleRequestCallbacks now owns request
> > > +    mThrottledIdleRequestCallbacks.insertBack(request);
> > > +    request->AddRef();
> > 
> > I'm pretty sure this is going to fail in static analysis builds; we don't
> > permit calling `AddRef` or `Release` on smart pointers.  Here and other
> > places.
> 
> So, this seems risky to me as well, but it models itself against how the
> timeouts are handled in the global window. Those are also held in a
> LinkedList and are also manually addref'ed and released. See
> nsGlobalWindow::InsertTimeoutIntoList.

Indeed.  The refcounting thing is a little confusing, but I agree that it should work.

What won't work because we have static analysis specifically to reject it is this pattern:

  RefPtr<Klass> thing = ...
  thing->AddRef();

due to:

http://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#303
http://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#431

Comment 70

[Nathan Froyd [:froydnj]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review82468

I think this is OK.  I'd like to see the documentation revisions before granting final review, though.

::: xpcom/glue/nsThreadUtils.h:289
(Diff revision 2)
> +class IncrementalRunnable : public CancelableRunnable,
> +                            public nsIIncrementalRunnable
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  // nsIIncrementalrunnable

Nit: `nsIIncrementalRunnable` -- capital R.

::: xpcom/threads/nsIIdlePeriod.idl:17
(Diff revision 2)
> +%}
> +
> +native TimeStamp(mozilla::TimeStamp);
> +
> +[scriptable, function, uuid(21dd35a2-eae9-4bd8-b470-0dfa35a0e3b9)]
> +interface nsIIdlePeriod : nsISupports

Some description of this interface would be good.

::: xpcom/threads/nsIIdlePeriod.idl:20
(Diff revision 2)
> +     * Return the number of milliseconds until the current idle period
> +     * will end.
> +     */
> +    TimeStamp getIdlePeriodHint();

A `TimeStamp` is a point in time, not a number of milliseconds, so either the documentation is incorrect, or the interface needs to be changed.

Shouldn't the comment string here say "an estimate" somewhere?

::: xpcom/threads/nsIIncrementalRunnable.h:23
(Diff revision 2)
> +class nsIIncrementalRunnable : public nsISupports
> +{
> +public:
> +  NS_DECLARE_STATIC_IID_ACCESSOR(NS_IINCREMENTALRUNNABLE_IID)
> +
> +  virtual void SetDeadline(mozilla::TimeStamp aDeadline) = 0;

Documentation here would be most helpful.  Documentation for the class, too, as I have no idea what this it supposed to do.  (How does "incremental" and "deadline" relate to each other, for instance?)

::: xpcom/threads/nsIThread.idl:31
(Diff revision 2)
>     * @returns
>     *   The NSPR thread object corresponding to this nsIThread.
>     */
>    [noscript] readonly attribute PRThread PRThread;
>  
> +  attribute nsIIdlePeriod idlePeriod;

Documentation for this attribute would be good.  I think in the next patch you edit a comment which says that the internal lock on nsIThread should protect the internal class variable underlying this.  So we should also include documentation and assertions that this attribute is only read and written on the current thread (through this method, perhaps), yes?

::: xpcom/threads/nsThread.cpp:1061
(Diff revision 2)
> +void
> +nsThread::GetIdleEvent(nsIRunnable** aEvent)
> +{
> +  MOZ_ASSERT(aEvent);
> +  TimeStamp idleDeadline;
> +  mIdlePeriod->GetIdlePeriodHint(&idleDeadline);

For consistency, I think this `mIdlePeriod` access should be under a lock.

::: xpcom/threads/nsThread.cpp:1141
(Diff revision 2)
> +    if (!event) {
> +      GetIdleEvent(getter_AddRefs(event));
> +    }

Assuming I understand this correctly, a comment indicating that we don't have to care about `reallyWait` here, because we never block for idle events, and we'd only not have an event if we didn't block earlier--said better than I just did--would be good.

Comment 71

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #69)
>
> Indeed.  The refcounting thing is a little confusing, but I agree that it
> should work.
> 
> What won't work because we have static analysis specifically to reject it is
> this pattern:
> 
>   RefPtr<Klass> thing = ...
>   thing->AddRef();
> 
> due to:
> 
> http://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#303
> http://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#431

Ok. So making sure to call AddRef on Klass* instead of RefPtr<Klass> should do it, right?

Comment 72

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #70)
> Comment on attachment 8788891 [details]
> Bug 1198381 - Extend nsIThread with idleDispatch,
> 

I agree with everything, will make it so.

Comment 73

[Nathan Froyd [:froydnj]]

(In reply to Andreas Farre [:farre] from comment #71)
> (In reply to Nathan Froyd [:froydnj] from comment #69)
> >
> > Indeed.  The refcounting thing is a little confusing, but I agree that it
> > should work.
> > 
> > What won't work because we have static analysis specifically to reject it is
> > this pattern:
> > 
> >   RefPtr<Klass> thing = ...
> >   thing->AddRef();
> > 
> > due to:
> > 
> > http://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#303
> > http://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#431
> 
> Ok. So making sure to call AddRef on Klass* instead of RefPtr<Klass> should
> do it, right?

Right.  Or you can play games with an extra RefPtr variable and .forget().take() or similar.

Comment 74

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #67)
> Comment on attachment 8788892 [details]
> Bug 1198381 - Implement the requestIdleCallback feature,
> 
> I have a slight preference for adding only
> `NS_DelayedDispatchToCurrentThread`, as I think DOM code usually uses
> `*CurrentThread` unless it's actually running off-thread.  (This rule is
> probably not consistently enforced.)
> 

I'll change NS_IdleDispatchToMainThread to NS_IdleDispatchToCurrentThread as well in the previous patch, for the same reason.

Comment 75

[Nathan Froyd [:froydnj]]

(In reply to Andreas Farre [:farre] from comment #74)
> (In reply to Nathan Froyd [:froydnj] from comment #67)
> > I have a slight preference for adding only
> > `NS_DelayedDispatchToCurrentThread`, as I think DOM code usually uses
> > `*CurrentThread` unless it's actually running off-thread.  (This rule is
> > probably not consistently enforced.)
> 
> I'll change NS_IdleDispatchToMainThread to NS_IdleDispatchToCurrentThread as
> well in the previous patch, for the same reason.

Thank you for catching that!

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

https://reviewboard.mozilla.org/r/83666/#review82588

::: dom/base/nsGlobalWindow.cpp:12213
(Diff revision 1)
>    } else {
>      reason = "setTimeout handler";
>    }
>  
>    bool abortIntervalHandler = false;
> -  nsCOMPtr<nsIScriptTimeoutHandler> handler(timeout->mScriptHandler);
> +  nsCOMPtr<nsITimeoutHandler> basicHandler(timeout->mScriptHandler);

Could you perhaps have this strong nsCOMPtr<nsITimeoutHandler> only in ase handler is null. So, put
nsCOMPtr<nsITimeoutHandler> basicHandler to the 'else' where you have kungFuDeathGrip for the window too.

::: dom/base/nsGlobalWindow.cpp:12214
(Diff revision 1)
>      reason = "setTimeout handler";
>    }
>  
>    bool abortIntervalHandler = false;
> -  nsCOMPtr<nsIScriptTimeoutHandler> handler(timeout->mScriptHandler);
> +  nsCOMPtr<nsITimeoutHandler> basicHandler(timeout->mScriptHandler);
> +  nsCOMPtr<nsIScriptTimeoutHandler> handler(do_QueryInterface(basicHandler));

this could then just QI timeout->mScriptHandler

::: dom/base/nsITimeoutHandler.h:12
(Diff revision 1)
> +#define nsITimeoutHandler_h___
> +
> +#include "nsISupports.h"
> +
> +#define NS_ITIMEOUTHANDLER_IID \
> +{ 0xb071a1d3, 0xfd54, 0x40a8,                                 \

\ is at odd place. Perhaps put it under the \ in the previous line.

::: dom/base/nsITimeoutHandler.h:22
(Diff revision 1)
> +public:
> +  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ITIMEOUTHANDLER_IID)
> +
> +  virtual nsresult Call() = 0;
> +
> +  virtual void GetLocation(const char **aFileName, uint32_t *aLineNo,

über-nit, since you're just moving this method, but * and ** go with the type, not with the argument name. So const char** aFileName and so on.

Comment 77

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review82600

I'd like to see the comments fixed, and I'll review again.

::: xpcom/glue/nsThreadUtils.cpp:38
(Diff revision 2)
>  #ifndef XPCOM_GLUE_AVOID_NSPR
>  
> +NS_IMPL_ISUPPORTS(IdlePeriod, nsIIdlePeriod)
> +
> +NS_IMETHODIMP
> +IdlePeriod::GetIdlePeriodHint(TimeStamp *aIdleDeadline)

nit,
TimeStamp* aIdleDeadline

::: xpcom/threads/nsIThread.idl:121
(Diff revision 2)
>     *   on the thread object.
>     */
>    void asyncShutdown();
> +
> +  /**
> +   * Dispatch an event to the threads idle queue.  This function may be called

thread's

::: xpcom/threads/nsThread.h:216
(Diff revision 2)
>    nsAutoTObserverArray<NotNull<nsCOMPtr<nsIThreadObserver>>, 2> mEventObservers;
>  
>    NotNull<nsChainedEventQueue*> mEvents;  // never null
>    nsChainedEventQueue mEventsRoot;
>  
> +  nsCOMPtr<nsIIdlePeriod> mIdlePeriod;

So what does mIdlePeriod here mean? Is it the end of the current idle period, or expected start time of the next idle period or what?
Some comment would be good, and perhaps even rename the member variable to something easier to understand.

::: xpcom/threads/nsThread.cpp:994
(Diff revision 2)
> +
> +NS_IMETHODIMP
> +nsThread::IdleDispatch(already_AddRefed<nsIRunnable> aEvent)
> +{
> +  MutexAutoLock lock(mLock);
> +  LeakRefPtr<nsIRunnable> event(Move(aEvent));

I don't understand the use of LeakRefPtr here. Either you don't have any aEvent and return early or you pass already_AddRefed to PutEvent.

I guess you're missing check for mEventsAreDoomed here, in which case LeakRefPtr would make sense.

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review82618

mostly nits, but I think I should re-read this anyhow, so if you could fix the nits and ask review again.

::: dom/base/IdleRequest.h:37
(Diff revision 2)
> +  virtual nsresult Call() override;
> +  virtual void GetLocation(const char **aFileName, uint32_t *aLineNo,
> +                           uint32_t *aColumn) override;
> +  virtual void MarkForCC() override;
> +
> +  nsresult SetTimeout(uint32_t timout);

nit, aTimeout

::: dom/base/IdleRequest.cpp:64
(Diff revision 2)
> +  NS_INTERFACE_MAP_ENTRY(nsITimeoutHandler)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsITimeoutHandler)
> +NS_INTERFACE_MAP_END
> +
> +nsresult
> +IdleRequest::SetTimeout(uint32_t timeout)

nit, aTimeout

::: dom/ipc/ContentChild.cpp:587
(Diff revision 2)
>  
> +  {
> +    nsCOMPtr<nsIThread> thread;
> +    rv = NS_GetMainThread(getter_AddRefs(thread));
> +    if (!NS_WARN_IF(NS_FAILED(rv))) {
> +      thread->SetIdlePeriod(new MainThreadIdlePeriod());

This looks odd. Do we end up setting MainThreadIdlePeriod only on child processes?
I think you should move the SetIdlePeriod to ::Init method of ThreadManager

::: layout/base/nsRefreshDriver.h:346
(Diff revision 2)
> +   *
> +   * Otherwise returns Some(TimeStamp(t)), where t is the time of the next tick.
> +   */
> +  static mozilla::Maybe<mozilla::TimeStamp> GetIdleDeadlineHint();
> +
> +  bool SkippedPaints() const {

Nit, { to its own line.

::: layout/base/nsRefreshDriver.cpp:88
(Diff revision 2)
> +// The time we estimate is needed between the end of an idle time and
> +// the next Tick.
> +#define DEFAULT_IDLE_PERIOD_TIME_LIMIT 1.0f

is this in milliseconds? 1ms feels quite small. Would 2 or 3 perhaps?

::: layout/base/nsRefreshDriver.cpp:228
(Diff revision 2)
>      aNewTimer->mLastFireTime = mLastFireTime;
>    }
>  
> +  virtual TimeDuration GetTimerRate() = 0;
> +
> +  bool LastTickSkipped() const {

Nit, { to its own line

::: layout/base/nsRefreshDriver.cpp:2270
(Diff revision 2)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!sRegularRateTimer) {
> +    return Nothing();
> +  }
> +
> +  if (sRegularRateTimer->LastTickSkipped()) {
> +    return Some(TimeStamp());
> +  }
> +
> +  TimeStamp mostRecentRefresh = sRegularRateTimer->MostRecentRefresh();
> +  TimeDuration refreshRate = sRegularRateTimer->GetTimerRate();
> +  TimeStamp idleEnd = mostRecentRefresh + refreshRate;
> +
> +  uint32_t quiescentFrames = Preferences::GetUint(

Could you use pref var cache for these preferences so that we don't need to do a slow-ish pref check all the time.

::: modules/libpref/init/all.js:188
(Diff revision 2)
>  pref("dom.enable_performance_observer", true);
>  #else
>  pref("dom.enable_performance_observer", false);
>  #endif
>  
> +// Enable requestIdleCallback API

perhaps enable first only on Nightly, and keep disabled elsewhere. We could change that later, but just that we don't accidentally ship this to aurora.

::: modules/libpref/init/all.js:2716
(Diff revision 2)
> +// pass without painting used to guess that we'll not paint again soon
> +pref("layout.idle_period.required_quiescent_frames", 2);
> +
> +// The amount of time (milliseconds) needed between an idle period's
> +// end and the start of the next tick to avoid jank.
> +pref("layout.idle_period.time_limit", 1);

also here, I think 1 is quite small value. 2 or 3 perhaps? or does that make us to never run the callbacks?

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788893 [details]
Bug 1198381 - Add tests for requestIdleCallback,

https://reviewboard.mozilla.org/r/77216/#review82630

::: testing/web-platform/tests/html/webappapis/idle-callbacks/callback-multiple-calls.html:21
(Diff revision 2)
> +      }
> +
> +      ++counter;
> +    }
> +    window.requestIdleCallback(t.step_func(function () { f(0) }));
> +    window.requestIdleCallback(t.step_func(function () { f(1) }));

maybe you could add some more these to stress the system a bit more. Maybe do it even in a loop and request 100 callbacks or so.

One thing to test is to rIC inside another rIC.

Comment 80

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/1-2/

Comment 81

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/2-3/

Comment 82

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/2-3/

Comment 83

[Andreas Farre [:farre]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77214/diff/2-3/

Comment 84

[Andreas Farre [:farre]]

Comment on attachment 8788893 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77216/diff/2-3/

Comment 85

[Andreas Farre [:farre]]

I'm fairly certain that I've fixed the outstanding review issues, with two exceptions. I haven't moved setting MainThreadIdlePeriod to nsThreadManager, and I missed only exposing rIC on nightly through prefs. 

The former of these, I'm a bit uncertain about. The reason for me wanting to put it in dom/ipc/ContentChild.cpp was to limit where MainThreadIdlePeriod actually had an effect. For all other threads there is a default nsIIdlePeriod set, that never claims to be idle. The MainThreadIdlePeriod is supposed to be unique to the main thread of the content process, i.e. the one doing painting, handling events etc. Other threads should be unaffected, and if idle period utilization is desired, a custom tailored nsIIdlePeriod should be created for that purpose. But I could be missing something, of course. 

With regard to the prefs, bkelly argued in Comment #49 that the pref should be removed since the feature is complete. That is the reason for removing the pref guard in Window.webidl, but I forgot modules/libpref/init/all.js. So I just thought I'd make sure which way is better. Pref or no pref?

The only change I made was that I changed the idle period on a thread from being an attribute to a being registered instead.

Comment 86

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review83614

::: layout/base/nsRefreshDriver.cpp:2270
(Diff revision 3)
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!sRegularRateTimer) {
> +    return Nothing();
> +  }
> +
> +  if (sRegularRateTimer->LastTickSkipped()) {

What if this driver isn't currently being controlled by the regular rate timer?

LastTickSkipped() on the timer returns true if any of the drivers attached to it skipped a paint.

Even if we are controlled by that timer, we might not have been the driver that skipped a paint, how accurate do we need this to be?

::: layout/base/nsRefreshDriver.cpp:2271
(Diff revision 3)
> +  if (!sRegularRateTimer) {
> +    return Nothing();
> +  }
> +
> +  if (sRegularRateTimer->LastTickSkipped()) {
> +    return Some(TimeStamp());

Rather than returning an emtpy timestamp when we're late, why can't we just return the time in the past that it was expected at?

That way we avoid needing the LastTickSkipped logic entirely

Comment 87

[Andreas Farre [:farre]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #86)
> Comment on attachment 8788892 [details]
> Bug 1198381 - Implement the requestIdleCallback feature,
> 
> https://reviewboard.mozilla.org/r/77214/#review83614

Many thanks for the review! Good stuff.

I just thought that I'd try to explain the context for when GetIdleDeadlineHint is intended to be used to keep track of my answers below. In short; it's all about figuring out when to run lower priority stuff that can run for a given amount of time and make sure to stop before a tick/paint is about to happen.

> ::: layout/base/nsRefreshDriver.cpp:2270
> (Diff revision 3)
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  if (!sRegularRateTimer) {
> > +    return Nothing();
> > +  }
> > +
> > +  if (sRegularRateTimer->LastTickSkipped()) {
> 
> What if this driver isn't currently being controlled by the regular rate
> timer?

If a driver isn't currently controlled by the regular timer, then it should be a driver for a hidden page, right? In that case I've made the call that that tick is really also idle work and doesn't need to be avoided.

> LastTickSkipped() on the timer returns true if any of the drivers attached
> to it skipped a paint.
> 
> Even if we are controlled by that timer, we might not have been the driver
> that skipped a paint, how accurate do we need this to be?

Since what we're trying to accomplish is to compute an estimate of how long we won't be doing any work related to painting, anything that can end up running on the main thread before the end of that estimate is an indication that the estimate was wrong. The question is then how skipped paints are handled. I've guessed that nsRefreshDriver::NotifyTransactionCompleted could come at any time, which if a refresh driver has skipped paints should result in a tick on that driver as soon as possible to catch up and we wouldn't really be idle.

If my reasoning is correct, then I think that we need to be as perfect as possible to allow that catching up to have higher priority than possible idle work.

> ::: layout/base/nsRefreshDriver.cpp:2271
> (Diff revision 3)
> > +  if (!sRegularRateTimer) {
> > +    return Nothing();
> > +  }
> > +
> > +  if (sRegularRateTimer->LastTickSkipped()) {
> > +    return Some(TimeStamp());
> 
> Rather than returning an emtpy timestamp when we're late, why can't we just
> return the time in the past that it was expected at?
> 
> That way we avoid needing the LastTickSkipped logic entirely

The information that we try to return by having three possible types of return values is three separate forms of idleness status. Some(TimeStamp()) means that we're busy. Some(aNonNullTimestamp) means that we will be idle until aNonNullTimestamp. Nothing means that we are idle until so far in the future that we cannot give a reasonable estimate.

In the context of requestIdleCallback Nothing means that we could schedule a long idle period instead of the short inter-frame idle period, and that is really the whole reason of returning Nothing. So what I'm trying to avoid by actually providing perfect knowledge of ticks skipped on the regular timer is that a skipped paint inadvertently triggers a long period. 

Given that we set nsLayoutUtils::QuiescentFramesBeforeIdlePeriod() to a high enough value, then this might be avoided and we could remove LastTickSkipped, but then we might become less aware of long idle periods. This is the trade off that I've tried to balance.

I'm starting to feel that the biggest trouble with my patch is that the documentation of GetIdleDeadlineHint is still not good enough.

Does this sound reasonable to you?

Comment 88

[Andreas Farre [:farre]]

Forgot to add need info for Comment #85

Comment 89

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review83902

::: xpcom/threads/nsIThread.idl:126
(Diff revision 3)
>     */
>    void asyncShutdown();
> +
> +  /**
> +   * Register an instance of nsIIdlePeriod which works as a facade of
> +   * the abstract notion of a ""current idle period". The

s/""/"/

::: xpcom/threads/nsThread.cpp:1143
(Diff revision 3)
>      {
>        MutexAutoLock lock(mLock);
>        mEvents->GetEvent(reallyWait, getter_AddRefs(event), lock);
>      }
>  
> +    if (!event) {

Hmm, but what if reallyWait is just, we may never end up here.
Don't we want to basically have reallyWait false if we have idle tasks.


Could we perhaps change the code above a bit, something like:

MutexAutoLock lock(mLock);
if (reallyWait) {
  reallyWait = mIdleQueue.Count(mLock) == 0;

}
mEvents->GetEvent(reallyWait, getter_AddRefs(event), lock);



IsEmpty() would be faster, but atm it is private in EventQueue. Perhaps that could be made public and use that and not Count(). (IsEmpty should also take MutexAutoLock& as param)

Comment 90

[Olli Pettay [:smaug][[e-mail]]]

> 
> Hmm, but what if reallyWait is just, we may never end up here.
s/just/true/

Comment 91

[Matt Woodrow (:mattwoodrow)]

(In reply to Andreas Farre [:farre] from comment #87)
 
> If a driver isn't currently controlled by the regular timer, then it should
> be a driver for a hidden page, right? In that case I've made the call that
> that tick is really also idle work and doesn't need to be avoided.
> 
> > LastTickSkipped() on the timer returns true if any of the drivers attached
> > to it skipped a paint.
> > 
> > Even if we are controlled by that timer, we might not have been the driver
> > that skipped a paint, how accurate do we need this to be?
> 
> Since what we're trying to accomplish is to compute an estimate of how long
> we won't be doing any work related to painting, anything that can end up
> running on the main thread before the end of that estimate is an indication
> that the estimate was wrong. The question is then how skipped paints are
> handled. I've guessed that nsRefreshDriver::NotifyTransactionCompleted could
> come at any time, which if a refresh driver has skipped paints should result
> in a tick on that driver as soon as possible to catch up and we wouldn't
> really be idle.
> 
> If my reasoning is correct, then I think that we need to be as perfect as
> possible to allow that catching up to have higher priority than possible
> idle work.
> 
> The information that we try to return by having three possible types of
> return values is three separate forms of idleness status. Some(TimeStamp())
> means that we're busy. Some(aNonNullTimestamp) means that we will be idle
> until aNonNullTimestamp. Nothing means that we are idle until so far in the
> future that we cannot give a reasonable estimate.
> 
> In the context of requestIdleCallback Nothing means that we could schedule a
> long idle period instead of the short inter-frame idle period, and that is
> really the whole reason of returning Nothing. So what I'm trying to avoid by
> actually providing perfect knowledge of ticks skipped on the regular timer
> is that a skipped paint inadvertently triggers a long period. 
> 
> Given that we set nsLayoutUtils::QuiescentFramesBeforeIdlePeriod() to a high
> enough value, then this might be avoided and we could remove
> LastTickSkipped, but then we might become less aware of long idle periods.
> This is the trade off that I've tried to balance.
> 
> I'm starting to feel that the biggest trouble with my patch is that the
> documentation of GetIdleDeadlineHint is still not good enough.
> 
> Does this sound reasonable to you?

Thanks for the full explanation, it does indeed seem much more reasonable now.

It might be a bit clearer if GetIdleDeadlineHint was a function on the timer, and the static nsRefreshDriver version just called that (along with the above explanation of why the regular rate timer is the only thing we care about).

Maybe also change LastTickSkipped() on the timer to LastTickSkippedAnyPaints().

Adding the above explanation about why we want the various return value types and what we use them for would be a really helpful comment.

Comment 92

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andreas Farre [:farre] from comment #85)
> I'm fairly certain that I've fixed the outstanding review issues, with two
> exceptions. I haven't moved setting MainThreadIdlePeriod to nsThreadManager,
> and I missed only exposing rIC on nightly through prefs. 
> 
> The former of these, I'm a bit uncertain about. The reason for me wanting to
> put it in dom/ipc/ContentChild.cpp was to limit where MainThreadIdlePeriod
> actually had an effect. For all other threads there is a default
> nsIIdlePeriod set, that never claims to be idle. The MainThreadIdlePeriod is
> supposed to be unique to the main thread of the content process, i.e. the
> one doing painting, handling events etc. Other threads should be unaffected,
> and if idle period utilization is desired, a custom tailored nsIIdlePeriod
> should be created for that purpose. But I could be missing something, of
> course. 
But we want the idle stuff to work also in the parent process (main thread there), not only in child process.


> With regard to the prefs, bkelly argued in Comment #49 that the pref should
> be removed since the feature is complete.
I don't understand bkelly's argument. We want some experience on how the implementation works, and once we've decided it is good enough, then explicitly enable on non-Nightlies too. And still keep the pref so that if we later, say during the first release with the feature, we notice some major issue, we could easily disable it.
Then later remove the pref. 
(I think this is large and complicated enough feature to be a bit careful there.)

Comment 93

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review83908

::: dom/base/nsGlobalWindow.cpp:579
(Diff revision 3)
> +                                    ErrorResult& aError)
> +{
> +  MOZ_RELEASE_ASSERT(IsInnerWindow());
> +  AssertIsOnMainThread();
> +
> +  if (NS_WARN_IF((mIdleRequestCallbackCounter == (UINT32_MAX - 1)))) {

The spec doesn't have this kind of check. Why is it needed? I know there is the risk of overflowing, but does that really matter much?

::: dom/base/nsGlobalWindow.cpp:1967
(Diff revision 3)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDoc)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIdleService)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWakeLock)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingStorageEvents)
> +
> +  for (IdleRequest* request : tmp->mIdleRequestCallbacks) {

Why you don't need to traverse mThrottledIdleRequestCallbacks?

Why mThrottledIdleRequestCallbacks nor mIdleRequestCallbacks is unlinked?

::: dom/ipc/ContentChild.cpp:584
(Diff revision 3)
>    nsresult rv = nsThreadManager::get().Init();
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return false;
>    }
>  
> +  {

So, I still think this isn't good place for this. We want similar MainThreadIdlePeriod on parent process too so that chrome JS can use the API.

::: dom/webidl/IdleDeadline.webidl:10
(Diff revision 3)
> + *
> + * The origin of this IDL file is:
> + * https://w3c.github.io/requestidlecallback/
> + */
> +
> +interface IdleDeadline {

pref

::: dom/webidl/Window.webidl:526
(Diff revision 3)
>  Window implements GlobalFetch;
>  Window implements ImageBitmapFactories;
> +
> +partial interface Window {
> +  [Throws]
> +  unsigned long requestIdleCallback(IdleRequestCallback callback,

So I would expect still prefs here, or let bkelly to explain why he wants that unusual non-pref handling here.

Comment 94

[Andreas Farre [:farre]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #91)
> 
> Thanks for the full explanation, it does indeed seem much more reasonable
> now.
> 
> It might be a bit clearer if GetIdleDeadlineHint was a function on the
> timer, and the static nsRefreshDriver version just called that (along with
> the above explanation of why the regular rate timer is the only thing we
> care about).
> 
> Maybe also change LastTickSkipped() on the timer to
> LastTickSkippedAnyPaints().
> 
> Adding the above explanation about why we want the various return value
> types and what we use them for would be a really helpful comment.

Will do.

Comment 95

[Andreas Farre [:farre]]

(In reply to Olli Pettay [:smaug] from comment #92)
> (In reply to Andreas Farre [:farre] from comment #85)
> > I'm fairly certain that I've fixed the outstanding review issues, with two
> > exceptions. I haven't moved setting MainThreadIdlePeriod to nsThreadManager,
> > and I missed only exposing rIC on nightly through prefs. 
> > 
> > The former of these, I'm a bit uncertain about. The reason for me wanting to
> > put it in dom/ipc/ContentChild.cpp was to limit where MainThreadIdlePeriod
> > actually had an effect. For all other threads there is a default
> > nsIIdlePeriod set, that never claims to be idle. The MainThreadIdlePeriod is
> > supposed to be unique to the main thread of the content process, i.e. the
> > one doing painting, handling events etc. Other threads should be unaffected,
> > and if idle period utilization is desired, a custom tailored nsIIdlePeriod
> > should be created for that purpose. But I could be missing something, of
> > course. 
> But we want the idle stuff to work also in the parent process (main thread
> there), not only in child process.
> 
> 
> > With regard to the prefs, bkelly argued in Comment #49 that the pref should
> > be removed since the feature is complete.
> I don't understand bkelly's argument. We want some experience on how the
> implementation works, and once we've decided it is good enough, then
> explicitly enable on non-Nightlies too. And still keep the pref so that if
> we later, say during the first release with the feature, we notice some
> major issue, we could easily disable it.
> Then later remove the pref. 
> (I think this is large and complicated enough feature to be a bit careful
> there.)

Great, will do.

Comment 96

[Andreas Farre [:farre]]

(In reply to Olli Pettay [:smaug] from comment #93)
> Comment on attachment 8788892 [details]
> Bug 1198381 - Implement the requestIdleCallback feature,
> 
> https://reviewboard.mozilla.org/r/77214/#review83908
> 
> ::: dom/base/nsGlobalWindow.cpp:579
> (Diff revision 3)
> > +                                    ErrorResult& aError)
> > +{
> > +  MOZ_RELEASE_ASSERT(IsInnerWindow());
> > +  AssertIsOnMainThread();
> > +
> > +  if (NS_WARN_IF((mIdleRequestCallbackCounter == (UINT32_MAX - 1)))) {
> 
> The spec doesn't have this kind of check. Why is it needed? I know there is
> the risk of overflowing, but does that really matter much?
> 
> ::: dom/base/nsGlobalWindow.cpp:1967
> (Diff revision 3)
> >    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDoc)
> >    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIdleService)
> >    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWakeLock)
> >    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingStorageEvents)
> > +
> > +  for (IdleRequest* request : tmp->mIdleRequestCallbacks) {
> 
> Why you don't need to traverse mThrottledIdleRequestCallbacks?
> 
> Why mThrottledIdleRequestCallbacks nor mIdleRequestCallbacks is unlinked?
> 
> ::: dom/ipc/ContentChild.cpp:584
> (Diff revision 3)
> >    nsresult rv = nsThreadManager::get().Init();
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> >      return false;
> >    }
> >  
> > +  {
> 
> So, I still think this isn't good place for this. We want similar
> MainThreadIdlePeriod on parent process too so that chrome JS can use the API.
> 
> ::: dom/webidl/IdleDeadline.webidl:10
> (Diff revision 3)
> > + *
> > + * The origin of this IDL file is:
> > + * https://w3c.github.io/requestidlecallback/
> > + */
> > +
> > +interface IdleDeadline {
> 
> pref
> 
> ::: dom/webidl/Window.webidl:526
> (Diff revision 3)
> >  Window implements GlobalFetch;
> >  Window implements ImageBitmapFactories;
> > +
> > +partial interface Window {
> > +  [Throws]
> > +  unsigned long requestIdleCallback(IdleRequestCallback callback,
> 
> So I would expect still prefs here, or let bkelly to explain why he wants
> that unusual non-pref handling here.

Ok, good. This all sounds reasonable to me. Will make it happen.

Comment 97

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/2-3/

Comment 98

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/2-3/

Comment 99

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/3-4/

Comment 100

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/3-4/

Comment 101

[Andreas Farre [:farre]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77214/diff/3-4/

Comment 102

[Andreas Farre [:farre]]

Comment on attachment 8788893 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77216/diff/3-4/

Comment 103

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review84196

Comment 104

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review84204

::: dom/base/IdleRequest.h:25
(Diff revision 4)
> +namespace dom {
> +
> +class IdleRequestCallback;
> +
> +class IdleRequest final : public nsITimeoutHandler
> +                        , public IncrementalRunnable

So here you inherit threadsafe refcounting from IncrementalRunnable...

::: dom/base/IdleRequest.h:50
(Diff revision 4)
> +  uint32_t Handle() const
> +  {
> +    return mHandle;
> +  }
> +
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS

..and here define cycle collectable refcounting (which isn't thread safe).

I think you should support only cyclecollectable, which means you should not inherit IncrementalRunnable but nsIIncrementalRunnable.

(and should nsIIncrementalRunnable perhaps inherit nsICancelableRunnable, or nsIRunnable, or both?)

Comment 105

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review84396

Comment 106

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/3-4/

Comment 107

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/3-4/

Comment 108

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/4-5/

Comment 109

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/4-5/

Comment 110

[Andreas Farre [:farre]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77214/diff/4-5/

Comment 111

[Andreas Farre [:farre]]

Comment on attachment 8788893 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77216/diff/4-5/

Comment 112

[Andreas Farre [:farre]]

(In reply to Olli Pettay [:smaug] from comment #104)
> Comment on attachment 8788892 [details]
> Bug 1198381 - Implement the requestIdleCallback feature,
> 
> https://reviewboard.mozilla.org/r/77214/#review84204
> 
> ::: dom/base/IdleRequest.h:25
> (Diff revision 4)
> > +namespace dom {
> > +
> > +class IdleRequestCallback;
> > +
> > +class IdleRequest final : public nsITimeoutHandler
> > +                        , public IncrementalRunnable
> 
> So here you inherit threadsafe refcounting from IncrementalRunnable...
> 
> ::: dom/base/IdleRequest.h:50
> (Diff revision 4)
> > +  uint32_t Handle() const
> > +  {
> > +    return mHandle;
> > +  }
> > +
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> 
> ..and here define cycle collectable refcounting (which isn't thread safe).
> 
> I think you should support only cyclecollectable, which means you should not
> inherit IncrementalRunnable but nsIIncrementalRunnable.
> 
> (and should nsIIncrementalRunnable perhaps inherit nsICancelableRunnable, or
> nsIRunnable, or both?)

I did the former but not the latter. I want to keep the existing pattern of the concrete Runnable variants.

Also, I added a last var cache in MainThreadIdlePeriod.

Comment 113

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review84584

::: dom/base/IdleRequest.h:27
(Diff revision 5)
> +class IdleRequestCallback;
> +
> +class IdleRequest final : public nsITimeoutHandler
> +                        , public nsIRunnable
> +                        , public nsICancelableRunnable
> +                        , public nsIIncrementalRunnable

(So IncrementalRunnable could be removed in the other patch, right?)

::: dom/base/nsGlobalWindow.cpp:2002
(Diff revision 5)
>    tmp->TraverseHostObjectURIs(cb);
>  
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsGlobalWindow)

Shouldn't you unlink idlerequests.
Perhaps call tmp->DisableIdleCallbackRequests() somewhere in Unlink?

Comment 114

[Andreas Farre [:farre]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77214/diff/5-6/

Comment 115

[Andreas Farre [:farre]]

Comment on attachment 8788893 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77216/diff/5-6/

Comment 116

[Andreas Farre [:farre]]

Nathan, what do you think about how I addressed the xpcom/threads/* issues?

Comment 117

[Nathan Froyd [:froydnj]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review85200

Thanks for writing all the documentation!  A few small things noted below.

::: xpcom/threads/nsIIdlePeriod.idl:18
(Diff revisions 2 - 5)
>  
>  native TimeStamp(mozilla::TimeStamp);
>  
> +/**
> + * An instance implementing nsIIdlePeriod is used by an associated
> + * nsIThread, that currently has an empty event queue, to estimate

Nit: please remove the "that currently has an empty event queue"; we just need to know that it's used for estimation.  The details of when it's used for estimation are up to the implementation.

::: xpcom/threads/nsIIdlePeriod.idl:21
(Diff revisions 2 - 5)
> +/**
> + * An instance implementing nsIIdlePeriod is used by an associated
> + * nsIThread, that currently has an empty event queue, to estimate
> + * when it is likely that it will receive an event.
> + */
>  [scriptable, function, uuid(21dd35a2-eae9-4bd8-b470-0dfa35a0e3b9)]

This should not be scriptable, since I think the lone instance of it in other IDL files is in `[noscript]` methods.

::: xpcom/threads/nsIThread.idl:11
(Diff revisions 2 - 5)
>  
>  #include "nsIEventTarget.idl"
> +#include "nsIIdlePeriod.idl"
> +
> +%{C++
> +#include "nsCOMPtr.h"

Nit: I don't think we need this include.

::: xpcom/threads/nsThread.h:221
(Diff revisions 2 - 5)
> +  // mIdlePeriod keeps track of the current idle period. If at any
> +  // time the main event queue is empty, calling
> +  // mIdlePeriod->GetIdlePeriodHint() will give an estimate of when
> +  // the current idle period will end.
>    nsCOMPtr<nsIIdlePeriod> mIdlePeriod;
>    nsEventQueue mIdleQueue;

It occurs to me that we might want to call this `mIdleEvents`, in continuity with `mEvents`.  WDYT?

::: xpcom/threads/nsThread.cpp:1139
(Diff revisions 2 - 5)
>      {
>        MutexAutoLock lock(mLock);
> -      mEvents->GetEvent(reallyWait, getter_AddRefs(event), lock);
> +      mEvents->GetEvent(false, getter_AddRefs(event), lock);
>      }
>  
> +    // [2] If we didn't get an event from the regular queue, try to
> +    // get one from the idle queue
>      if (!event) {
> +      // Since events in mEvents have higher priority than idle
> +      // events, we will only consider idle events when there are no
> +      // pending events in mEvents. We will for the same reason never
> +      // wait for an idle event, since a higher priority event might
> +      // appear at any time.
>        GetIdleEvent(getter_AddRefs(event));
>      }
>  
> +    // [3] If we neither got an event from the regular queue nor the
> +    // idle queue, then if we should wait for events we block on the
> +    // main queue until an event is available.
> +    // If we are shutting down, then do not wait for new events.
> +    if (!event && reallyWait) {
> +      MutexAutoLock lock(mLock);
> +      mEvents->GetEvent(reallyWait, getter_AddRefs(event), lock);
> +    }

I think all of this should live under a single `MutexAutoLock`; compare the case where a thread needs to wait for an event before:

1. Lock `mLock`, and wait.

versus after your patch:

1. Lock `mLock`, check `mEvents`, find nothing.
2. Lock `mLock` for calling into `mIdlePeriod`.
3. Lock `mLock` for checking `mIdleQueue`.
4. Lock `mLock`, and wait.

Doesn't seem great to add all that overhead, and we shouldn't be locking for that long.  The only wrinkle is calling out to `GetIdleDeadlineHint` might want to be done outside the lock, but I don't think we need to worry about that.

::: xpcom/threads/MainThreadIdlePeriod.h:15
(Diff revision 5)
> +#include "mozilla/TimeStamp.h"
> +#include "nsThreadUtils.h"
> +
> +namespace mozilla {
> +
> +class MainThreadIdlePeriod final : public mozilla::IdlePeriod

No `mozilla::` qualification needed here, since you're already in the `mozilla` namespace.

Do we have plans for idle periods off the main thread?  It seems a bit silly to have an `IdlePeriod` class who only has one subclass.

::: xpcom/threads/MainThreadIdlePeriod.cpp:15
(Diff revision 5)
> +float sLongIdlePeriod = DEFAULT_LONG_IDLE_PERIOD;
> +bool sInitialized = false;

Nit: these should be declared `static`.  You could even declare them as `static` variables inside of `GetLongIdlePeriod`.

Comment 118

[Nathan Froyd [:froydnj]]

(In reply to Andreas Farre [:farre] from comment #116)
> Nathan, what do you think about how I addressed the xpcom/threads/* issues?

I assume my review answered your question?  The other review doesn't appear to have meaty thread issues in it.

Apologies for the review lag, I was out the latter half of last week.

Comment 119

[Nathan Froyd [:froydnj]]

Comment on attachment 8788892 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/77214/#review85250

All the XPCOM bits look good to me.

Comment 120

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #118)
> (In reply to Andreas Farre [:farre] from comment #116)
> > Nathan, what do you think about how I addressed the xpcom/threads/* issues?
> 
> I assume my review answered your question?  The other review doesn't appear
> to have meaty thread issues in it.
> 
> Apologies for the review lag, I was out the latter half of last week.

No worries, I just wanted to make sure that I hadn't missed notifying you of the changes. Unsure what mozreview sends out wrt updated patches.

Comment 121

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #117)
> Comment on attachment 8788891 [details]
> Bug 1198381 - Extend nsIThread with idleDispatch,
> 
> https://reviewboard.mozilla.org/r/77212/#review85200
> 
> Thanks for writing all the documentation!  A few small things noted below.
> 
> ::: xpcom/threads/nsIIdlePeriod.idl:18
> (Diff revisions 2 - 5)
> >  
> >  native TimeStamp(mozilla::TimeStamp);
> >  
> > +/**
> > + * An instance implementing nsIIdlePeriod is used by an associated
> > + * nsIThread, that currently has an empty event queue, to estimate
> 
> Nit: please remove the "that currently has an empty event queue"; we just
> need to know that it's used for estimation.  The details of when it's used
> for estimation are up to the implementation.
> 
> ::: xpcom/threads/nsIIdlePeriod.idl:21
> (Diff revisions 2 - 5)
> > +/**
> > + * An instance implementing nsIIdlePeriod is used by an associated
> > + * nsIThread, that currently has an empty event queue, to estimate
> > + * when it is likely that it will receive an event.
> > + */
> >  [scriptable, function, uuid(21dd35a2-eae9-4bd8-b470-0dfa35a0e3b9)]
> 
> This should not be scriptable, since I think the lone instance of it in
> other IDL files is in `[noscript]` methods.
> 
> ::: xpcom/threads/nsIThread.idl:11
> (Diff revisions 2 - 5)
> >  
> >  #include "nsIEventTarget.idl"
> > +#include "nsIIdlePeriod.idl"
> > +
> > +%{C++
> > +#include "nsCOMPtr.h"
> 
> Nit: I don't think we need this include.
> 
> ::: xpcom/threads/nsThread.h:221
> (Diff revisions 2 - 5)
> > +  // mIdlePeriod keeps track of the current idle period. If at any
> > +  // time the main event queue is empty, calling
> > +  // mIdlePeriod->GetIdlePeriodHint() will give an estimate of when
> > +  // the current idle period will end.
> >    nsCOMPtr<nsIIdlePeriod> mIdlePeriod;
> >    nsEventQueue mIdleQueue;
> 
> It occurs to me that we might want to call this `mIdleEvents`, in continuity
> with `mEvents`.  WDYT?
> 
> ::: xpcom/threads/nsThread.cpp:1139
> (Diff revisions 2 - 5)
> >      {
> >        MutexAutoLock lock(mLock);
> > -      mEvents->GetEvent(reallyWait, getter_AddRefs(event), lock);
> > +      mEvents->GetEvent(false, getter_AddRefs(event), lock);
> >      }
> >  
> > +    // [2] If we didn't get an event from the regular queue, try to
> > +    // get one from the idle queue
> >      if (!event) {
> > +      // Since events in mEvents have higher priority than idle
> > +      // events, we will only consider idle events when there are no
> > +      // pending events in mEvents. We will for the same reason never
> > +      // wait for an idle event, since a higher priority event might
> > +      // appear at any time.
> >        GetIdleEvent(getter_AddRefs(event));
> >      }
> >  
> > +    // [3] If we neither got an event from the regular queue nor the
> > +    // idle queue, then if we should wait for events we block on the
> > +    // main queue until an event is available.
> > +    // If we are shutting down, then do not wait for new events.
> > +    if (!event && reallyWait) {
> > +      MutexAutoLock lock(mLock);
> > +      mEvents->GetEvent(reallyWait, getter_AddRefs(event), lock);
> > +    }
> 
> I think all of this should live under a single `MutexAutoLock`; compare the
> case where a thread needs to wait for an event before:
> 
> 1. Lock `mLock`, and wait.
> 
> versus after your patch:
> 
> 1. Lock `mLock`, check `mEvents`, find nothing.
> 2. Lock `mLock` for calling into `mIdlePeriod`.
> 3. Lock `mLock` for checking `mIdleQueue`.
> 4. Lock `mLock`, and wait.
> 
> Doesn't seem great to add all that overhead, and we shouldn't be locking for
> that long.  The only wrinkle is calling out to `GetIdleDeadlineHint` might
> want to be done outside the lock, but I don't think we need to worry about
> that.
> 
> ::: xpcom/threads/MainThreadIdlePeriod.h:15
> (Diff revision 5)
> > +#include "mozilla/TimeStamp.h"
> > +#include "nsThreadUtils.h"
> > +
> > +namespace mozilla {
> > +
> > +class MainThreadIdlePeriod final : public mozilla::IdlePeriod
> 
> No `mozilla::` qualification needed here, since you're already in the
> `mozilla` namespace.
> 
> Do we have plans for idle periods off the main thread?  It seems a bit silly
> to have an `IdlePeriod` class who only has one subclass.
> 
> ::: xpcom/threads/MainThreadIdlePeriod.cpp:15
> (Diff revision 5)
> > +float sLongIdlePeriod = DEFAULT_LONG_IDLE_PERIOD;
> > +bool sInitialized = false;
> 
> Nit: these should be declared `static`.  You could even declare them as
> `static` variables inside of `GetLongIdlePeriod`.

I agree, need to check the locking, but you're most certainly right. We can add a documentation nibble about GetIdleDeadlineHint happening when locked and should take as little amount of time as possible. Good thing is that in the case of GetIdleDeadlineHint being called, we are most likely idle as it is.

Comment 122

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #117)
> Comment on attachment 8788891 [details]
> Bug 1198381 - Extend nsIThread with idleDispatch,
> 
> Do we have plans for idle periods off the main thread?  It seems a bit silly
> to have an `IdlePeriod` class who only has one subclass.
> 

That was my reasoning at least. Since we're adding idle handling to the general thread it's tempting to allow having different idle period sources depending on what busy means in the context of that particular thread.

I.e. no plan, but I think it's a good idea.

Comment 123

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/4-5/

Comment 124

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/4-5/

Comment 125

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/5-6/

Comment 126

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/5-6/

Comment 127

[Andreas Farre [:farre]]

Ignore that last commit. Pushed in the middle of a rebase.

Comment 128

[Andreas Farre [:farre]]

Attachment: Bug 1198381 - Implement the requestIdleCallback feature,

Expose requestIdleCallback on Window and implement running callbacks
in idle periods by posting rICs to the main threads idle queue.

Review commit: https://reviewboard.mozilla.org/r/86606/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86606/

Comment 129

[Andreas Farre [:farre]]

Attachment: Bug 1198381 - Add tests for requestIdleCallback,

Review commit: https://reviewboard.mozilla.org/r/86610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86610/

Comment 130

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/6-7/

Comment 131

[Andreas Farre [:farre]]

Ok, that's better. There are no changes in https://bugzilla.mozilla.org/attachment.cgi?id=8802062 and https://bugzilla.mozilla.org/attachment.cgi?id=8802061, but mozreview probably didn't catch that.

Comment 132

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

https://reviewboard.mozilla.org/r/86610/#review85504

::: testing/web-platform/tests/html/webappapis/idle-callbacks/callback-iframe.html:12
(Diff revision 1)
> +<iframe style="display:none" id="frame"></iframe>
> +<script>
> +  async_test(function (t) {
> +    let frame = document.getElementById("frame");
> +    frame.contentWindow.test = function() {
> +      this.window.requestIdleCallback(t.step_func_done());

I'm pretty sure you don't have this.window.requestIdleCallback but frame.contentWindow.requestIdleCallback

Comment 133

[Andreas Farre [:farre]]

Attachment: rb77214-rb86606.diff

Diff for "Implement the requestIdleCallback feature"

Comment 134

[Andreas Farre [:farre]]

Attachment: rb77216-rb86610.diff

Diff for Add tests for requestIdleCallback

Comment 135

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/86606/#review85510

Based on the interdiff in the bug, this should be fine.

Comment 136

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/1-2/

Comment 137

[Nathan Froyd [:froydnj]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/86606/#review85588

r=me assuming nothing has changed--I think the re-ask for review was just mozreview confusion?

Comment 138

[Nathan Froyd [:froydnj]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

https://reviewboard.mozilla.org/r/77212/#review85590

Thanks for pulling out `nsThread::GetEvent`; that helps makes things clearer.

Comment 139

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

https://reviewboard.mozilla.org/r/86606/#review85592

Comment 140

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #137)
> Comment on attachment 8802061 [details]
> Bug 1198381 - Implement the requestIdleCallback feature,
> 
> https://reviewboard.mozilla.org/r/86606/#review85588
> 
> r=me assuming nothing has changed--I think the re-ask for review was just
> mozreview confusion?

Correct, thanks.

Comment 141

[Andreas Farre [:farre]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86606/diff/1-2/

Comment 142

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/2-3/

Comment 143

[Andreas Farre [:farre]]

Just pushed some missing overrides to nsRefreshDriver

Comment 144

[Andreas Farre [:farre]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86606/diff/2-3/

Comment 145

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/3-4/

Comment 146

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/4-5/

Comment 147

[Andreas Farre [:farre]]

Just pushed some small fixes to the test cases patch (IdlePeriod was missing from dom/test/mochitest/general/test_interfaces.html and testing/web-platform/meta/MANIFEST.json failed lint because of ordering)

Comment 148

[Andreas Farre [:farre]]

Talos looks ok: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9eef773f6b4b&newProject=try&newRevision=ea70830ba3ab&framework=1

Comment 149

[Andreas Farre [:farre]]

With the exception of two pgo builds this also looks good on try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=90d6a782b906&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&filter-resultStatus=success

I'm retriggering those (again), but I'm fairly sure that it isn't due to these patches (the builds are timing out, they're not busted in any other way).

Should I checkin-needed this?

Comment 150

[Olli Pettay [:smaug][[e-mail]]]

Assuming the tryserver results look good, then yes.
Make sure the patches apply cleanly to current up-to-date m-i.

(You don't have rights to push to m-i?)

Comment 151

[Andreas Farre [:farre]]

Except the pgo builds that time out there were some intermittent failures that didn't repeat when re-triggered. So it looks good.

Comment 152

[Andreas Farre [:farre]]

And it applies cleanly.

And, no I don't got rights to push yet.

Comment 153

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/228cc133fe6b
Move/rename nsTimeout to Timeout, r=smaug
https://hg.mozilla.org/integration/autoland/rev/24839edcf0ef
Extract nsITimeoutHandler from nsIScriptTimeoutHandler. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ffaf3d371130
Extend setTimeout handling in nsGlobalWindow, r=smaug
https://hg.mozilla.org/integration/autoland/rev/333a899fb5e6
Extend nsIThread with idleDispatch, r=froydnj,smaug
https://hg.mozilla.org/integration/autoland/rev/eb2606332cb8
Implement the requestIdleCallback feature, r=froydnj,mattwoodrow,smaug
https://hg.mozilla.org/integration/autoland/rev/377601350abc
Add tests for requestIdleCallback, r=smaug

Comment 154

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/228cc133fe6b
https://hg.mozilla.org/mozilla-central/rev/24839edcf0ef
https://hg.mozilla.org/mozilla-central/rev/ffaf3d371130
https://hg.mozilla.org/mozilla-central/rev/333a899fb5e6
https://hg.mozilla.org/mozilla-central/rev/eb2606332cb8
https://hg.mozilla.org/mozilla-central/rev/377601350abc

Comment 155

[Carsten Book [:Tomcat]]

backed out after talking to andreas and whimboo for causing bug 1312683

Comment 156

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/ced3b02ea694
Backed out changeset 377601350abc 
https://hg.mozilla.org/mozilla-central/rev/8a58ea129976
Backed out changeset eb2606332cb8 
https://hg.mozilla.org/mozilla-central/rev/186fcc0dd237
Backed out changeset 333a899fb5e6 
https://hg.mozilla.org/mozilla-central/rev/393de96f724c
Backed out changeset ffaf3d371130 
https://hg.mozilla.org/mozilla-central/rev/fb1e01469d7a
Backed out changeset 24839edcf0ef 
https://hg.mozilla.org/mozilla-central/rev/c6ccd71126ff
Backed out changeset 228cc133fe6b for causing increased number of hangs in navigate() in marionette tests

Comment 157

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/5-6/

Comment 158

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/5-6/

Comment 159

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/6-7/

Comment 160

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/7-8/

Comment 161

[Andreas Farre [:farre]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86606/diff/3-4/

Comment 162

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/5-6/

Comment 163

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/8-9/

Comment 164

[Andreas Farre [:farre]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86606/diff/4-5/

Comment 165

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/6-7/

Comment 166

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/6-7/

Comment 167

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/6-7/

Comment 168

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/7-8/

Comment 169

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/9-10/

Comment 170

[Andreas Farre [:farre]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86606/diff/5-6/

Comment 171

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/7-8/

Comment 172

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/7-8/

Comment 173

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/7-8/

Comment 174

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/8-9/

Comment 175

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/10-11/

Comment 176

[Andreas Farre [:farre]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86606/diff/6-7/

Comment 177

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/8-9/

Comment 178

[Andreas Farre [:farre]]

Nathan: I had trouble with deadlocks on nsThread::mLock, so I did this: https://reviewboard.mozilla.org/r/77212/diff/8-11/

MozReview don't want to revert r+ to r?, can you just check it? Trouble is that nsRefreshDriver::GetIdleTimer eventually ends up in VsyncChild::GetTimerRate, which does ipc and that caused deadlocks. I'll try to do something about that in a followup.

Comment 179

[Andreas Farre [:farre]]

Markus: something like this for bug 1312937 problems? https://reviewboard.mozilla.org/r/86606/diff/6-7/

Comment 180

[Nathan Froyd [:froydnj]]

(In reply to Andreas Farre [:farre] from comment #178)
> Nathan: I had trouble with deadlocks on nsThread::mLock, so I did this:
> https://reviewboard.mozilla.org/r/77212/diff/8-11/
> 
> MozReview don't want to revert r+ to r?, can you just check it? Trouble is
> that nsRefreshDriver::GetIdleTimer eventually ends up in
> VsyncChild::GetTimerRate, which does ipc and that caused deadlocks. I'll try
> to do something about that in a followup.

Bleh, thought that might be a problem.

OK, let's go with it, but please do investigate trying to remove that MutexAutoUnlock.  Or, failing that, perhaps we could just skip the idle event queue on non-main threads at the very least.

Comment 181

[Olli Pettay [:smaug][[e-mail]]]

FWIW, as I said on IRC, using MutexAutoUnlock feels scary. What guarantees the state is still sane when execution returns to the caller?
If the issue happens because we end up sending async message to parent via GetTimerRate(), we really should just send that message at different time.

Comment 182

[Markus Stange [:mstange]]

(In reply to Andreas Farre [:farre] from comment #179)
> Markus: something like this for bug 1312937 problems?
> https://reviewboard.mozilla.org/r/86606/diff/6-7/

Yes, that looks good to me.

Comment 183

[Andreas Farre [:farre]]

Still waiting on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=600a634010803c1c8cc1d1af237562a8afe61796

If that looks good we should try to commit again.

Comment 184

[Ben Kelly [:bkelly, not reviewing]]

Note, I just pushed bug 1303167.  I hope that does not conflict too much here.

Comment 185

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/8-9/

Comment 186

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/8-9/

Comment 187

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/9-10/

Comment 188

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/11-12/

Comment 189

[Andreas Farre [:farre]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86606/diff/7-8/

Comment 190

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/9-10/

Comment 191

[Andreas Farre [:farre]]

(In reply to Ben Kelly [:bkelly] from comment #184)
> Note, I just pushed bug 1303167.  I hope that does not conflict too much
> here.

I rebased and there were only a couple of easily resolved conflicts. If you can have a look at it I'd be happy, the changes are here https://reviewboard.mozilla.org/r/77208/diff/8-9/

As you see, there were some unfortunate leakage from your patch into this review. Not sure why MozReview does that. 

Olli: n-i you as well due to that you've r+ it, but Ben might be able to tell the difference more easily.

Comment 192

[Olli Pettay [:smaug][[e-mail]]]

ok, mozreview is playing really badly here. The changes in nsGlobalWindow are from bkelly's patches and look ok.

Comment 193

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a day or two) from comment #192)
> ok, mozreview is playing really badly here. The changes in nsGlobalWindow
> are from bkelly's patches and look ok.

This usually happens when you rebase your branch and include updates of your code at the same time. It should help to do separate pushes afair.

Comment 194

[Andreas Farre [:farre]]

(In reply to Henrik Skupin (:whimboo) from comment #193)
> (In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a
> day or two) from comment #192)
> > ok, mozreview is playing really badly here. The changes in nsGlobalWindow
> > are from bkelly's patches and look ok.
> 
> This usually happens when you rebase your branch and include updates of your
> code at the same time. It should help to do separate pushes afair.

Yep, I've seen it work in those cases. For this case the changes happened due to resolving a conflict of the rebase, so not really sure what I could've done about it.

Comment 195

[Ben Kelly [:bkelly, not reviewing]]

Yea, those changes just look like what I pushed.  I don't see any differences or other conflicts.  Sorry for the trouble.  I use mercurial mq still, so I'm not familiar with how to fix mozreview.

Comment 196

[Andreas Farre [:farre]]

Right, new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c9e6f01edd&group_state=expanded&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=30015284

With exceptions of a bunch of (from what I could tell) intermittent failures, it looked good. 

I merges cleanly with inbound/default. Let's try committing it again.

Comment 197

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Olli Pettay [:smaug] from comment #181)
> FWIW, as I said on IRC, using MutexAutoUnlock feels scary. What guarantees
> the state is still sane when execution returns to the caller?
> If the issue happens because we end up sending async message to parent via
> GetTimerRate(), we really should just send that message at different time.

I'm also a little worried about this. Someone could post a normal priority event while we're unlocked, but then we would dispatch an idle event instead. Why can't we just call GetIdlePeriodHint before we take the lock for taking the lock for GetEvent?

I also noticed a problem. If we go to sleep waiting for a normal event, it seems like we won't wake up if someone dispatches an idle event.

Comment 198

[Ryan VanderMeulen [:RyanVM]]

Autoland couldn't rebase for landing.

Comment 199

[Andreas Farre [:farre]]

(In reply to Bill McCloskey (:billm) from comment #197)
> (In reply to Olli Pettay [:smaug] from comment #181)
> > FWIW, as I said on IRC, using MutexAutoUnlock feels scary. What guarantees
> > the state is still sane when execution returns to the caller?
> > If the issue happens because we end up sending async message to parent via
> > GetTimerRate(), we really should just send that message at different time.
> 
> I'm also a little worried about this. Someone could post a normal priority
> event while we're unlocked, but then we would dispatch an idle event
> instead. Why can't we just call GetIdlePeriodHint before we take the lock
> for taking the lock for GetEvent?

The reason for doing the operations:

1. GetEvent
2. GetIdlePeriodHint/GetIdleEvent
3. GetEvent

in this particular order is twofold. The first is of course that after step 1 we know whether we are idle or not. The second is that it keeps a similar performance behaviour as before the introduction of the idle queue. Not doing the extra work needed for idle queue dispatch until we actually know we are idle means that the time we spend in step 2 will not affect anything else. And that goes for the extra GetEvent in 3 as well.

I agree that GetIdlePeriodHint should happen while locked, but I don't think that that the appearance of a new event on the regular event queue, while unlocked, is that worrisome. Since we are on the thread executing the events we cannot be responsible for posting it, unless it is due to GetIdlePeriodHint, and then it is a bug if GetIdlePeriodHint needs sync event processing. If another thread posts events to us, then sure, the posting thread can continue and we miss that event and process an idle event instead, but the alternative is that the posting thread would block and we missed processing a normal priority event anyhow. And in any case, posting an event to a thread doesn't mean for certain that that event will be the next one processed, e.g. in the case of a non-empty normal priority queue. 

So what needs to be done in the follow-up bug that intends to address that GetIdlePeriodHint should be called when locked is also to document that GetIdlePeriodHint cannot post events.

> I also noticed a problem. If we go to sleep waiting for a normal event, it
> seems like we won't wake up if someone dispatches an idle event.

Yes, this might be a problem. We could consider it to be the responsibility of the poster of an idle event to have a normal priority event on a timer for the cases when the thread waits for normal priority events? Otherwise we need to find a way to share cond vars between queues, and that feels a bit tricky. I'll see what Olli does for the high priority queues.

Comment 200

[Andreas Farre [:farre]]

(In reply to Andreas Farre [:farre] from comment #199)

> doing the extra work needed for idle queue dispatch until we actually know

And I of course mean idle queue processing.

Comment 201

[Andreas Farre [:farre]]

Comment on attachment 8788889 [details]
Bug 1198381 - Move/rename nsTimeout to Timeout,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77208/diff/9-10/

Comment 202

[Andreas Farre [:farre]]

Comment on attachment 8798146 [details]
Bug 1198381 - Extract nsITimeoutHandler from nsIScriptTimeoutHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83666/diff/9-10/

Comment 203

[Andreas Farre [:farre]]

Comment on attachment 8788890 [details]
Bug 1198381 - Extend setTimeout handling in nsGlobalWindow,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77210/diff/10-11/

Comment 204

[Andreas Farre [:farre]]

Comment on attachment 8788891 [details]
Bug 1198381 - Extend nsIThread with idleDispatch,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77212/diff/12-13/

Comment 205

[Andreas Farre [:farre]]

Comment on attachment 8802061 [details]
Bug 1198381 - Implement the requestIdleCallback feature,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86606/diff/8-9/

Comment 206

[Andreas Farre [:farre]]

Comment on attachment 8802062 [details]
Bug 1198381 - Add tests for requestIdleCallback,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86610/diff/10-11/

Comment 207

[Andreas Farre [:farre]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #198)
> Autoland couldn't rebase for landing.

Thanks for trying. Rebased, resolved conflicts, and pushed to MozReview. Is still blocked on bug 1313590 though. Will try again after that lands.

Comment 208

[Nathan Froyd [:froydnj]]

(In reply to Andreas Farre [:farre] from comment #199)
> (In reply to Bill McCloskey (:billm) from comment #197)
> > I also noticed a problem. If we go to sleep waiting for a normal event, it
> > seems like we won't wake up if someone dispatches an idle event.
> 
> Yes, this might be a problem. We could consider it to be the responsibility
> of the poster of an idle event to have a normal priority event on a timer
> for the cases when the thread waits for normal priority events? Otherwise we
> need to find a way to share cond vars between queues, and that feels a bit
> tricky. I'll see what Olli does for the high priority queues.

I vote that we make nsEventQueue parameterizable on the number of priority levels it supports, and then add logic for indicating the priority level of events we put in, and a priority level of the events being returned.  That way we don't need to bother trying to work out sharing issues between multiple queues.  WDYT?

Comment 209

[Olli Pettay [:smaug][[e-mail]]]

In principle that is quite close to what I have in the high priority patch atm, though it is not 
nsEventQueue which deals with multiple queues, but nsChainedEventQueue.

However different queues are handled in quite different ways.
High priority queue should be processed asap, but in such way that normal queue is processed occasionally (otherwise normal queue will starve, and for example devtool tests won't pass). idle queue doesn't have similar requirements (if one must ensure the idle task is run, pass some timeout value when dispatching idle task [1]), and idle queue scheduling is more tightly connected to what else is happening (refreshdriver, timer etc.).

[1] I guess we don't have a good C++ API for this yet.

Comment 210

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Bill McCloskey (:billm) from comment #197)
> I also noticed a problem. If we go to sleep waiting for a normal event, it
> seems like we won't wake up if someone dispatches an idle event.

FWIW, I'd be fine to land the patch and fixing the issue in a followup, since initially this stuff is used only by
window.requestIdleCallback, so no other random thread can dispatch idle runnables when main thread is waiting for more runnables.

Comment 211

[Olli Pettay [:smaug][[e-mail]]]

Before the issue is fixed, there could be a MOZ_ASSERT(isMainThread) in nsThread::IdleDispatch

Comment 212

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8739d8f536d0
Move/rename nsTimeout to Timeout, r=smaug
https://hg.mozilla.org/integration/autoland/rev/030e1cc3b1c1
Extract nsITimeoutHandler from nsIScriptTimeoutHandler. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ed2eac576e47
Extend setTimeout handling in nsGlobalWindow, r=smaug
https://hg.mozilla.org/integration/autoland/rev/17b19eb241db
Extend nsIThread with idleDispatch, r=froydnj,smaug
https://hg.mozilla.org/integration/autoland/rev/9b8cb1537fbe
Implement the requestIdleCallback feature, r=froydnj,mattwoodrow,smaug
https://hg.mozilla.org/integration/autoland/rev/e239269e0900
Add tests for requestIdleCallback, r=smaug

Comment 213

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8739d8f536d0
https://hg.mozilla.org/mozilla-central/rev/030e1cc3b1c1
https://hg.mozilla.org/mozilla-central/rev/ed2eac576e47
https://hg.mozilla.org/mozilla-central/rev/17b19eb241db
https://hg.mozilla.org/mozilla-central/rev/9b8cb1537fbe
https://hg.mozilla.org/mozilla-central/rev/e239269e0900

Comment 214

[Chris Peterson [:cpeterson]]

Release Note Request (optional, but appreciated)
[Why is this notable]: We're adding a new API.
[Affects Firefox for Android]:
[Suggested wording]: Introduced requestIdleCallback API
[Links (documentation, blog post, etc)]: API documentation: https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback

Comment 215

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx52 Aurora release notes

Comment 216

[Eric Shepherd [:sheppy]]

I’ve updated the documentation for requestIdleCallback() and cancelIdleCallback(), added documentation for the IdleDeadline, and updated Firefox 52 for developers:

https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback
https://developer.mozilla.org/en-US/docs/Web/API/Window/cancelIdleCallback
https://developer.mozilla.org/en-US/docs/Web/API/IdleDeadline (and its children)

I also created an overview document with a lengthy example that can be tried out in-place. There are some gaps on the page where content needs to be added later.

https://developer.mozilla.org/en-US/docs/Web/API/Background_Tasks_API