Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 10 years ago

      No description provided.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 1 • 10 years ago

Folded:
Allow file: serviceworkers
Registration fixes WIP
Queue updatefound instead of immediately firing
Initial "atomically" steps of registration should also be a part of the job
Fix some compiler errors
Be sure not to null out various workers too early during activation
Integrated ServiceWorkerGlobalScope::Update into the ServiceWorkerRegisterJob.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 2 • 10 years ago

The major changes it does are that calls to register() and internal calls to SWM::Update() are queued up in a job queue, all of which runs in the main thread. This allows multiple calls to have predictable behaviour. See https://github.com/slightlyoff/ServiceWorker/issues/396

I'll file a followup to have unregister() also use the queue.

	Andrea Marchesini [:baku]
	Comment 3 • 10 years ago

Comment on attachment 8539190 [details] [diff] [review]
Update ServiceWorker registration lifecycle

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

looks good. In particular because I reviewed all the other patches based on this one.
Many of these comments are already fixed in the following patches, but I already had them in my pending unfinished review...

::: dom/workers/ServiceWorkerManager.cpp
@@ +109,5 @@
> +  nsRefPtr<ServiceWorkerRegistrationInfo> mRegistration;
> +public:
> +  explicit QueueFireUpdateFoundRunnable(ServiceWorkerRegistrationInfo* aReg)
> +    : mRegistration(aReg)
> +  { }

MOZ_ASSERT(aReg);

@@ +168,5 @@
> +public:
> +  explicit FinishInstallRunnable(
> +    const nsMainThreadPtrHandle<nsISupports>& aJob,
> +    bool aSuccess,
> +    bool aActivateImmediately)

Also here the indentation is strange.

@@ +173,5 @@
> +    : mJob(aJob)
> +    , mSuccess(aSuccess)
> +    , mActivateImmediately(aActivateImmediately)
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());

Sometimes you use MOZ_ASSERT

@@ +195,5 @@
>  public:
> +  InstallEventRunnable(
> +    WorkerPrivate* aWorkerPrivate,
> +    const nsMainThreadPtrHandle<nsISupports>& aJob,
> +    const nsCString& aScope)

I don't know why you used this indentation... I saw it in all the other patches.
Do me it's ok but it's unusual.

@@ +227,5 @@
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(ServiceWorkerUpdateFinishCallback)
> +
> +  virtual
> +  void UpdateSucceeded(ServiceWorkerRegistrationInfo* aInfo)

= 0 ?

@@ +231,5 @@
> +  void UpdateSucceeded(ServiceWorkerRegistrationInfo* aInfo)
> +  { }
> +
> +  virtual
> +  void UpdateFailed(nsresult aStatus)

= 0 ?

@@ +253,5 @@
>    {
> +  }
> +
> +  void
> +  UpdateSucceeded(ServiceWorkerRegistrationInfo* aInfo)

MOZ_OVERRIDE

@@ +262,5 @@
> +    mPromise->MaybeResolve(swr);
> +  }
> +
> +  void
> +  UpdateFailed(nsresult aStatus)

MOZ_OVERRIDE

@@ +395,5 @@
> +  // Aspects of (actually the whole algorithm) of [[Update]] after
> +  // "Run the following steps in parallel."
> +  void
> +  ContinueUpdate()
> +  {

MOZ_ASSERT(NS_IsMainThread()), right?

@@ +585,5 @@
> +  ErrorResult result;
> +  if (!authenticatedOrigin) {
> +    bool isHttps, isFile, isApp;
> +    result = documentURI->SchemeIs("https", &isHttps);
> +    if (result.Failed()) {

if (NS_WARN_IF(result.Failed())) {
  return result.ErrorCode();
}

@@ +590,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    result = documentURI->SchemeIs("file", &isFile);
> +    if (result.Failed()) {

here too

@@ +595,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    result = documentURI->SchemeIs("app", &isApp);
> +    if (result.Failed()) {

here to

@@ +598,5 @@
> +    result = documentURI->SchemeIs("app", &isApp);
> +    if (result.Failed()) {
> +      return NS_ERROR_FAILURE;
> +    }
> +

This seems a bit expensive. Can you return immediately or get the scheme and do a string cmp?

@@ +621,5 @@
> +    bool isFile;
> +    result = documentURI->SchemeIs("file", &isFile);
> +    if (result.Failed() || !isFile) {
> +      bool isHttps;
> +      result = documentURI->SchemeIs("https", &isHttps);

maybe here we should return the correct result.ErrorCode()

@@ +662,5 @@
>    if (NS_WARN_IF(result.Failed())) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsCString spec;

nsAutoCString

@@ +663,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsCString spec;
> +  nsresult rv = scriptURI->GetSpec(spec);

can you continue using result?
You used an ErrorResult for all the previous methods, and not you start using nsresult...

@@ +671,5 @@
>  
> +
> +  nsRefPtr<ServiceWorkerManager::ServiceWorkerDomainInfo> domainInfo = GetDomainInfo(cleanedScope);
> +  if (!domainInfo) {
> +    nsCString domain;

nsAutoCString

@@ +672,5 @@
> +
> +  nsRefPtr<ServiceWorkerManager::ServiceWorkerDomainInfo> domainInfo = GetDomainInfo(cleanedScope);
> +  if (!domainInfo) {
> +    nsCString domain;
> +    nsresult rv = scriptURI->GetHost(domain);

error

@@ +693,5 @@
> +
> +  nsRefPtr<ServiceWorkerResolveWindowPromiseOnUpdateCallback> cb =
> +    new ServiceWorkerResolveWindowPromiseOnUpdateCallback(window, promise);
> +
> +  nsRefPtr<ServiceWorkerRegisterJob> job

= in the previous line

@@ +725,5 @@
> +  ~FinishInstallHandler()
> +  { }
> +
> +public:
> +  explicit FinishInstallHandler(

remove explicit when you have 2 arguments.

@@ +782,5 @@
> +    if (!waitUntilPromise) {
> +      ErrorResult rv;
> +      waitUntilPromise =
> +        Promise::Resolve(sgo,
> +                         aCx, JS::UndefinedHandleValue, rv);

this can stay in the same line

at least do a warningif rv failed

@@ +788,5 @@
> +  } else {
> +    ErrorResult rv;
> +    // Continue with a canceled install.
> +    waitUntilPromise = Promise::Reject(sgo, aCx,
> +                                       JS::UndefinedHandleValue, rv);

at least do a warningif rv failed

@@ +798,5 @@
> +  waitUntilPromise->AppendNativeHandler(handler);
> +  return true;
> +}
> +
> +class FinishActivationRunnable : public nsRunnable

MOZ_FINAL ?

@@ +804,5 @@
> +  bool mSuccess;
> +  nsMainThreadPtrHandle<ServiceWorkerRegistrationInfo> mRegistration;
> +
> +public:
> +  explicit FinishActivationRunnable(

remove explicit

@@ +823,5 @@
> +    return NS_OK;
> +  }
> +};
> +
> +class FinishActivateHandler : public PromiseNativeHandler

MOZ_FINAL

@@ +853,5 @@
> +    NS_DispatchToMainThread(r);
> +  }
> +};
> +
> +class ActivateEventRunnable : public WorkerRunnable

MOZ_FINAL

@@ +901,5 @@
> +        nsCOMPtr<nsIGlobalObject> global =
> +          do_QueryObject(aWorkerPrivate->GlobalScope());
> +        waitUntilPromise =
> +          Promise::Resolve(global,
> +                           aCx, JS::UndefinedHandleValue, rv);

warning...

@@ +909,5 @@
> +      nsCOMPtr<nsIGlobalObject> global =
> +        do_QueryObject(aWorkerPrivate->GlobalScope());
> +      // Continue with a canceled install.
> +      waitUntilPromise = Promise::Reject(global, aCx,
> +                                         JS::UndefinedHandleValue, rv);

warning...

@@ +981,5 @@
> +
> +  nsRefPtr<ActivateEventRunnable> r =
> +    new ActivateEventRunnable(serviceWorker->GetWorkerPrivate(), handle);
> +
> +  AutoSafeJSContext cx;

can you use AUtoJSAPI?

@@ +1592,5 @@
>  {
>    AssertIsOnMainThread();
>    MOZ_ASSERT(aURI);
>  
>    nsCString domain;

nsAutoCString

::: dom/workers/ServiceWorkerManager.h
@@ +83,5 @@
> +};
> +
> +class ServiceWorkerJobQueue;
> +
> +class ServiceWorkerJob : public nsISupports

MOZ_FINAL, right?

@@ +85,5 @@
> +class ServiceWorkerJobQueue;
> +
> +class ServiceWorkerJob : public nsISupports
> +{
> +  ServiceWorkerJobQueue* mQueue;

Can you write a comment saying that is the queue to keep alive the jobs, so this can be a raw pointer.

@@ +105,5 @@
> +  void
> +  Done(nsresult aStatus);
> +};
> +
> +class ServiceWorkerJobQueue

MOZ_FINAL

@@ +120,5 @@
> +  }
> +
> +  void
> +  Append(ServiceWorkerJob* aJob)
> +  {

MOZ_ASSERT(aJob);
MOZ_ASSERT(!mJobs.Contains(aJob));

@@ +122,5 @@
> +  void
> +  Append(ServiceWorkerJob* aJob)
> +  {
> +    mJobs.AppendElement(aJob);
> +    if (mJobs.Length() == 1) {

What about:

bool wasEmpty = mJobs.IsEmpty();
mJobs.AppendElemnet(aJob);
if (wasEmpty) {
...

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +236,5 @@
>      default:
>        MOZ_CRASH("Invalid enum value");
>    }
>  
> +  NS_WARN_IF_FALSE(rv == NS_OK || rv == NS_ERROR_DOM_NOT_FOUND_ERR, "Unexpected error getting service worker instance from ServiceWorkerManager");

NS_SUCCEDDED(rv) || rv == NS_ERROR_DOM_NOT_FOUND_ERR

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 4 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/08a776d630fb

	Carsten Book [:Tomcat]
	Comment 5 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/08a776d630fb