1172869 - Implement a single API to deal with window.open() scenarios

Status: NEW

(Core :: DOM: Service Workers, defect, P3)

Description

[Noemí Freire (:noemi)]

As raised in https://bugzilla.mozilla.org/show_bug.cgi?id=1130687#c36 we need to set up some service that exposes an API to be able to deal with the different window.open() scenarios (Desktop, FirefoxOS, Android)

Comment 1

[Albert [:albert]]

Attachment: Patch v1

The patch implements the openWindow algorithm from W3C spec and leave the underlying open for a new 'WorkerWindow' component.

Comment 2

[Albert [:albert]]

Attachment: Patch v2

Added IPC

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8625154 [details] [diff] [review]
Patch v2

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

r- mainly for the sync method in PBackground.

::: dom/interfaces/base/nsIWorkerWindow.idl
@@ +8,5 @@
> +interface nsIDOMWindow;
> +interface nsIURI;
> +
> +[scriptable, uuid(e703acfa-439f-43b8-ab6a-99a226802e90)]
> +interface nsIWorkerWindow : nsISupports 

extra space

::: dom/workers/IpcTypes.h
@@ +13,5 @@
> +namespace mozilla {
> +namespace dom {
> +namespace workers {
> +
> +struct WorkerClientInfoData

final

@@ +15,5 @@
> +namespace workers {
> +
> +struct WorkerClientInfoData
> +{
> +  WorkerClientInfoData() : windowId(-1)

WorkerClientInfoData()
   : windowId(-1)
   , ...

@@ +21,5 @@
> +                         , visibilityState(VisibilityState::Hidden)
> +                         , frameType(FrameType::None)
> +  {}
> +
> +  uint64_t windowId;

mWindowId, mClientId, mUrl, mFocused, ...

::: dom/workers/IpcUtils.h
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_ipcutils_h
> +#define mozilla_dom_ipcutils_h
> +

can you unify this header file and the other one?

@@ +18,5 @@
> +template <>
> +struct ParamTraits<WorkerClientInfoData>
> +{
> +  typedef WorkerClientInfoData paramType;
> +  

extra space

::: dom/workers/PServiceWorkerManager.ipdl
@@ +10,5 @@
>  
>  using mozilla::OriginAttributes from "mozilla/ipc/BackgroundUtils.h";
>  
> +using struct WorkerClientInfoData from "mozilla/dom/workers/IpcUtils.h";
> +include "mozilla/dom/workers/IpcUtils.h";

why this extra include?

@@ +30,5 @@
>    PropagateUnregister(PrincipalInfo principalInfo, nsString scope);
>  
>    Shutdown();
>  
> +  sync ServiceWorkerOpenWindow(URIParams uri)

rename it to "OpenWindow". This protocol is just about ServiceWorkers :)

@@ +31,5 @@
>  
>    Shutdown();
>  
> +  sync ServiceWorkerOpenWindow(URIParams uri)
> +      returns (nsresult aResult, WorkerClientInfoData clientInfo);

We should not do it sync. Can you change this to async?

::: dom/workers/ServiceWorkerManagerParent.cpp
@@ +9,5 @@
>  #include "mozilla/AppProcessChecker.h"
>  #include "mozilla/ipc/BackgroundParent.h"
>  #include "mozilla/ipc/BackgroundUtils.h"
>  #include "mozilla/unused.h"
> +#include "URIUtils.h"

Where is it from?

@@ +263,5 @@
> +                                                        nsresult* aResult,
> +                                                        WorkerClientInfoData* aClientInfo)
> +{
> +  AssertIsInMainProcess();
> +  AssertIsOnBackgroundThread();

MOZ_ASSERT(aResult);
MOZ_ASSERT(aClientInfo);

@@ +268,5 @@
> +
> +  nsRefPtr<WorkerWindowOpen> runnable =
> +    new WorkerWindowOpen(aUri, aClientInfo);
> +
> +  *aResult = NS_DispatchToMainThread(runnable, NS_DISPATCH_SYNC);

I don't know if I like this. I have to discuss with bent about this.

Comment 4

[Noemí Freire (:noemi)]

Per comment https://bugzilla.mozilla.org/show_bug.cgi?id=1172870#c11, removing the target milestone for now.

Comment 5

[Albert [:albert]]

Releasing it, please ni if you need some feedback.

Comment 6

[Cătălin Badea (:catalinb)]

Unblocking serviceWorker.openWindow. While this is a nice thing to have at some point, it is not worth blocking on it for SWs.

Comment 7

[Matt Chambers]

Attachment: same problem

Thanks from exexerar1970.