Implement WindowClient.navigate()

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: farre)

Tracking

({dev-doc-complete})

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments)

No description provided.
Looks like we may already have docs (if not an implementation), Jean-Yves: https://developer.mozilla.org/en-US/docs/Web/API/WindowClient/navigate
Flags: needinfo?(jypenator)
Assignee: nobody → afarre
Status: NEW → ASSIGNED
Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

https://reviewboard.mozilla.org/r/58520/#review55636

It looks good! The only thing I would like to see improved is this ResolveOrRejectPromiseRunnable.
Currently it doing too many things and it's hard to follow them via CTORs.
What about if:

if mRV is error => reject with error.
otherwise if client == null -> resolve with null
otherwise -> resolve with the client

And we make 2 CTORs:

with aRV: and MOZ_ASSERT(NS_FAILED(aRv));
with Client: and it can be null. Add a comment about it.

no mNullResolve (because we can use !mClient).

::: dom/workers/ServiceWorkerWindowClient.cpp:75
(Diff revision 1)
> +  ResolveOrRejectPromiseRunnable(WorkerPrivate* aWorkerPrivate,
> +                                 PromiseWorkerProxy* aPromiseProxy)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> +    , mPromiseProxy(aPromiseProxy)
> +    , mClientInfo(nullptr)
> +    , mNullResolve(true)

What's the default value for mRV here?

::: dom/workers/ServiceWorkerWindowClient.cpp:87
(Diff revision 1)
> +                                 nsresult aRv)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> +    , mPromiseProxy(aPromiseProxy)
> +    , mClientInfo(nullptr)
> +    , mRv(aRv)
> +    , mNullResolve(false)

This is quite confusing. What about if you do:

ResolveOrRejectPromiseRunnable(..., nsresult aRv, bool aNullResolve) ?

::: dom/workers/ServiceWorkerWindowClient.cpp:240
(Diff revision 1)
> +      return NS_OK;
> +    }
> +
> +    aWebProgress->RemoveProgressListener(this);
> +
> +    MutexAutoLock lock(mPromiseProxy->Lock());

Also here {}

::: dom/workers/ServiceWorkerWindowClient.cpp:262
(Diff revision 1)
> +        clientInfo.reset(new ServiceWorkerClientInfo(doc));
> +        resolveRunnable = new ResolveOrRejectPromiseRunnable(
> +          mPromiseProxy->GetWorkerPrivate(), mPromiseProxy, Move(clientInfo));
> +      } else {
> +        resolveRunnable = new ResolveOrRejectPromiseRunnable(
> +          mPromiseProxy->GetWorkerPrivate(), mPromiseProxy);

It's very confusing the fact if you don't pass arguments, then we resolve it as null.

::: dom/workers/ServiceWorkerWindowClient.cpp:349
(Diff revision 1)
> +  NS_IMETHOD
> +  Run() override
> +  {
> +    AssertIsOnMainThread();
> +
> +    MutexAutoLock lock(mPromiseProxy->Lock());

put this into {} so that you don't lock mProxyProxy for the all Run().

::: dom/workers/ServiceWorkerWindowClient.cpp:354
(Diff revision 1)
> +    MutexAutoLock lock(mPromiseProxy->Lock());
> +    if (mPromiseProxy->CleanedUp()) {
> +      return NS_OK;
> +    }
> +
> +    nsresult rv = Navigate();

Do a quick return here:

if (NS_FAILED(rv)) {
  RejectPromise(rv);
  return;
}

where RejectPromise is:

RefPtr<ResolveOrRejectPromiseRunnable> runnable = new ResolveOrRejectPromiseRunnable(...., rv);
runnable->Dispatch();

::: dom/workers/ServiceWorkerWindowClient.cpp:366
(Diff revision 1)
> +      nsresult rv = NS_NewURI(getter_AddRefs(baseURI), info.mHref);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return NS_ERROR_FAILURE;
> +      }
> +
> +      nsGlobalWindow* window = nsGlobalWindow::GetInnerWindowWithId(mWindowId);

Don't do this step twice. Store mWindow as argument. Or return it from Navigate().

::: dom/workers/ServiceWorkerWindowClient.cpp:387
(Diff revision 1)
> +
> +    RefPtr<ResolveOrRejectPromiseRunnable> resolveRunnable;
> +    if (rv != NS_ERROR_DOM_BAD_URI) {
> +      resolveRunnable = new ResolveOrRejectPromiseRunnable(
> +        mPromiseProxy->GetWorkerPrivate(), mPromiseProxy, rv);
> +    } else {

This would be:

new ResolveOrRjectPromiseRunnable(..., rv, rv == NS_ERROR_DOM_BAD_URI)

::: dom/workers/ServiceWorkerWindowClient.cpp:407
(Diff revision 1)
> +    WorkerPrivate::LocationInfo& info = workerPrivate->GetLocationInfo();
> +
> +    nsCOMPtr<nsIURI> baseUrl;
> +    nsresult rv = NS_NewURI(getter_AddRefs(baseUrl), info.mHref);
> +    if (NS_FAILED(rv)) {
> +      return NS_ERROR_FAILURE;

if (NS_WARN_IF(NS_FAILED(rv))) {
  return NS_ERROR_TYPE_ERR;
}

::: dom/workers/ServiceWorkerWindowClient.cpp:456
(Diff revision 1)
> +    loadInfo->SetReferrerPolicy(doc->GetReferrerPolicy());
> +    loadInfo->SetLoadType(nsIDocShellLoadInfo::loadStopContentAndReplace);
> +    loadInfo->SetSourceDocShell(docShell);
> +    rv = docShell->LoadURI(url, loadInfo, nsIWebNavigation::LOAD_FLAGS_NONE, true);
> +
> +    if (NS_FAILED(rv)) {

NS_WARN_IF
Attachment #8761222 - Flags: review?(amarchesini) → review-
Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

https://reviewboard.mozilla.org/r/58522/#review55638

::: testing/web-platform/meta/MANIFEST.json
(Diff revision 1)
>          "path": "dom/nodes/Node-isSameNode.html",
>          "url": "/dom/nodes/Node-isSameNode.html"
>        },
>        {
> -        "path": "dom/nodes/Node-isConnected.html",
> -        "url": "/dom/nodes/Node-isConnected.html"

Why this change?

::: testing/web-platform/tests/service-workers/service-worker/resources/client-navigate-worker.js:54
(Diff revision 1)
> +
> +    promise_test(function(t) {
> +      this.add_cleanup(function() { port.postMessage(pass(test, "")); });
> +      return self.clients.get(clientId)
> +                 .then(client => promise_rejects(t, new TypeError(), client.navigate("about:blank")))
> +                 .catch(unreached_rejection(t));

add a test where we reject because the URL is not valid.
Attachment #8761223 - Flags: review?(amarchesini) → review+
https://reviewboard.mozilla.org/r/58522/#review55638

> Why this change?

I ran the script for adding a test, which made this change. I guess it's a manual change, which the script has now fixed (it's only a move right?).
Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58520/diff/1-2/
Attachment #8761222 - Flags: review- → review?(amarchesini)
Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58522/diff/1-2/
Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

https://reviewboard.mozilla.org/r/58520/#review56040

::: dom/workers/ServiceWorkerWindowClient.cpp:278
(Diff revision 2)
> +  OnProgressChange(nsIWebProgress* aWebProgress, nsIRequest* aRequest,
> +                   int32_t aCurSelfProgress, int32_t aMaxSelfProgress,
> +                   int32_t aCurTotalProgress,
> +                   int32_t aMaxTotalProgress) override
> +  {
> +    MOZ_ASSERT(false, "Unexpected notification.");

MOZ_CRASH

::: dom/workers/ServiceWorkerWindowClient.cpp:366
(Diff revision 2)
> +    }
> +
> +    nsCOMPtr<nsIURI> baseUrl;
> +    nsCOMPtr<nsIURI> url;
> +    nsresult rv = ParseUrl(getter_AddRefs(baseUrl), getter_AddRefs(url));
> +    NS_ENSURE_SUCCESS(rv, rv);

remove this

::: dom/workers/ServiceWorkerWindowClient.cpp:384
(Diff revision 2)
> +      return RejectPromise(rv);
> +    }
> +
> +    nsCOMPtr<nsIDocShell> docShell = window->GetDocShell();
> +    nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(docShell);
> +    if (!webProgress) {

NS_WARN_IF

::: dom/workers/ServiceWorkerWindowClient.cpp:393
(Diff revision 2)
> +    nsCOMPtr<nsIWebProgressListener> listener =
> +      new WebProgressListener(mPromiseProxy, window->GetOuterWindow(), baseUrl);
> +
> +    rv = webProgress->AddProgressListener(
> +      listener, nsIWebProgress::NOTIFY_STATE_DOCUMENT);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

This can fail. Probably it's better to a simple: NS_WARN_IF + FAILED + RejectPromise(rv);

::: dom/workers/ServiceWorkerWindowClient.cpp:398
(Diff revision 2)
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +    return NS_OK;
> +  }
> +
> +private:
> +  nsresult RejectPromise(nsresult aRv)

nsresult
RejectPromise(nsresult aRv)

::: dom/workers/ServiceWorkerWindowClient.cpp:408
(Diff revision 2)
> +
> +    resolveRunnable->Dispatch();
> +    return NS_OK;
> +  }
> +
> +  nsresult ResolvePromise(UniquePtr<ServiceWorkerClientInfo>&& aClientInfo)

same indentation here.

::: dom/workers/ServiceWorkerWindowClient.cpp:421
(Diff revision 2)
> +    return NS_OK;
> +  }
> +
> +  nsresult
> +  ParseUrl(nsIURI** aBaseUrl, nsIURI** aUrl)
> +  {

MOZ_ASSERT(NS_IsOnMainThread());
MOZ_ASSERT(aBaseUrl)
MOZ_ASSERT(aUrl)

::: dom/workers/ServiceWorkerWindowClient.cpp:438
(Diff revision 2)
> +    return NS_OK;
> +  }
> +
> +  nsresult
> +  Navigate(nsIURI* aUrl, nsIPrincipal* aPrincipal, nsGlobalWindow** aWindow)
> +  {

MOZ_ASSERT(aWindow);

::: dom/workers/ServiceWorkerWindowClient.cpp:460
(Diff revision 2)
> +      return NS_ERROR_TYPE_ERR;
> +    }
> +
> +    nsCOMPtr<nsIDocShellLoadInfo> loadInfo;
> +    nsresult rv = docShell->CreateLoadInfo(getter_AddRefs(loadInfo));
> +

extra line?
Attachment #8761222 - Flags: review?(amarchesini) → review+
Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58520/diff/2-3/
Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58522/diff/2-3/
Note, there is some additional spec discussion here:

https://github.com/slightlyoff/ServiceWorker/issues/918
I'll fix that.
Actually, it seems like maybe you already do that?  It seems like the patch is using nsIWebNavigation::LOAD_FLAGS_NONE and not nsIWebNavigation::LOAD_FLAGS_REPLACE_HISTORY.
Ah, I had it replacing in an intermediary version. Still, I'll make sure to await and see how the discussion the issue turns out.
Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58520/diff/3-4/
Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58522/diff/3-4/
Keywords: checkin-needed
has problems to apply:

patching file dom/workers/ServiceWorkerWindowClient.cpp
Hunk #2 FAILED at 40
1 out of 4 hunks FAILED -- saving rejects to file dom/workers/ServiceWorkerWindowClient.cpp.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(afarre)
Keywords: checkin-needed
Comment on attachment 8761222 [details]
Bug 1218148 - Implement WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58520/diff/4-5/
Comment on attachment 8761223 [details]
Bug 1218148 - Web Platform tests for WindowClient.navigate()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58522/diff/4-5/
Rebased and pushed to mozreview. Now the patch should apply cleanly.
Flags: needinfo?(afarre)
Keywords: checkin-needed
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2088d0085a
Implement WindowClient.navigate() r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d290ccf62a7
Web Platform tests for WindowClient.navigate() r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa2088d0085a
https://hg.mozilla.org/mozilla-central/rev/7d290ccf62a7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(In reply to Andrew Overholt [:overholt] from comment #1)
> Looks like we may already have docs (if not an implementation), Jean-Yves:
> https://developer.mozilla.org/en-US/docs/Web/API/WindowClient/navigate

Yes, but we need to be sure it's on Firefox X for developers. Which it now is: https://developer.mozilla.org/en-US/Firefox/Releases/50#Service_Workers
Flags: needinfo?(jypenator)
You need to log in before you can comment on or make changes to this bug.