ServiceWorkerClients API spec changes

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nsm, Assigned: catalinb)

Tracking

({dev-doc-complete})

unspecified
mozilla38
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Andrea would you like to take this? Depending on issue 428 we may also need to dynamically obtain the list of open windows for an origin.
Blocks: 903441
Flags: needinfo?(amarchesini)
Created attachment 8478777 [details] [diff] [review]
ServiceWorkerClients API changes

This was the patch I had. Doesn't modify tests. Maybe we should hold on this until the issues are resolved, since otherwise it's not a good use of resources.
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
> This was the patch I had. Doesn't modify tests. Maybe we should hold on this
> until the issues are resolved, since otherwise it's not a good use of
> resources.

Sure. don't assignee it to yourself if you are not working on. I can take it in case these 2 issues are integrated in the spec.
Flags: needinfo?(amarchesini)
Assignee: nsm.nikhil → nobody
Keywords: dev-doc-needed
(Assignee)

Comment 4

4 years ago
Some of the changes discussed in 414 have been integrated in the spec:
getServiced  - renamed to getAll and now has an option parameter.
reloadAll - removed.

I think it's safe to switch to getAll without support for the uncontrolled option.
Assignee: nobody → catalin.badea392
(Assignee)

Comment 5

4 years ago
Created attachment 8565997 [details] [diff] [review]
Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll
(Assignee)

Comment 6

4 years ago
Created attachment 8565998 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.
(Assignee)

Comment 7

4 years ago
Created attachment 8566003 [details] [diff] [review]
Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll
Attachment #8478777 - Attachment is obsolete: true
Attachment #8565997 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Comment on attachment 8565998 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

Patch 1. Removes prefix renames getServiced to matchAll. Adds stubs for extra options in matchAll.
Attachment #8565998 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8565998 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8566003 - Flags: review?(amarchesini)
Comment on attachment 8566003 [details] [diff] [review]
Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll

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

lgtm

::: dom/workers/ServiceWorkerClients.h
@@ +9,5 @@
>  
>  #include "nsAutoPtr.h"
>  #include "nsWrapperCache.h"
>  #include "mozilla/dom/WorkerScope.h"
> +#include "mozilla/ErrorResult.h"

alphabetic order. Then, you can just forward ErrorResult.
Attachment #8566003 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 10

4 years ago
Created attachment 8567094 [details] [diff] [review]
Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll
Attachment #8566003 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Updated the patch to have includes in alphabetical order, but didn't used forward declaration because it broke the build in a bunch of ServiceWorker sources (e.g. ServiceWorkerEvents.cpp) that are beyond the scope of this patch.
(Assignee)

Comment 12

4 years ago
Comment on attachment 8567094 [details] [diff] [review]
Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll

https://hg.mozilla.org/integration/mozilla-inbound/rev/39354f436e2b
Attachment #8567094 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/39354f436e2b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 14

4 years ago
There is a second patch that needs to be landed first.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

4 years ago
Created attachment 8568833 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

This patch adds WindowClient and implements functionality for its getters.
Furthermore, I updated the Client and Clients webidl and added stubs.

I had difficulty testing frameType == top-level and visibilityState != visible.
For frameType == top-level, :bz suggested using a mochitest-browser, but I
would rather test it after implementing clients.claim.

Any suggestions on how I should cover these cases are welcomed.
Attachment #8565998 - Attachment is obsolete: true
Attachment #8568833 - Flags: review?(amarchesini)
(Assignee)

Comment 17

4 years ago
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #16)
> Catalin, is this a dupe? https://bugzilla.mozilla.org/show_bug.cgi?id=1130685

No, but it should be renamed to something like "Implement matchAll queryParam option".
Comment on attachment 8568833 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

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

It looks good except for the nsAutoPtr. Happy to review it again when this issue is solved. Thanks!

::: dom/base/nsGlobalWindow.h
@@ +678,5 @@
> +  IsTopLevelWindow()
> +  {
> +    MOZ_ASSERT(IsOuterWindow());
> +    nsCOMPtr<nsIDOMWindow> parentWindow;
> +    GetParent(getter_AddRefs(parentWindow));

GetScriptableTop.

::: dom/workers/ServiceWorkerClient.cpp
@@ +29,5 @@
> +{
> +  MOZ_ASSERT(aDoc);
> +  MOZ_ASSERT(aDoc->GetWindow());
> +
> +  nsRefPtr<nsGlobalWindow> outerWindow = static_cast<nsGlobalWindow*>(aDoc->GetWindow());

You don't need to case it to retrieve the WindowID(). Just use nsCOMPtr<nsPIDOMWindow>

@@ +40,5 @@
> +  if (NS_WARN_IF(result.Failed())) {
> +    NS_WARNING("Failed to get focus information.");
> +  }
> +
> +  if (outerWindow->IsTopLevelWindow()) {

wait... if is topLevel it should be FrameType::Top_Level, right?

::: dom/workers/ServiceWorkerClient.h
@@ +18,5 @@
>  namespace workers {
>  
> +// Used as a container object for information needed to create
> +// client objects.
> +class ServiceWorkerClientInfo MOZ_FINAL {

{ in a new line.

@@ +20,5 @@
> +// Used as a container object for information needed to create
> +// client objects.
> +class ServiceWorkerClientInfo MOZ_FINAL {
> +  friend class ServiceWorkerClient;
> +  friend class ServiceWorkerWindowClient;

forward declaration for these 2 classes, or move this class after those.

@@ +35,5 @@
> +  bool mFocused;
> +  FrameType mFrameType;
> +};
> +
> +class ServiceWorkerClient : public nsISupports,

MOZ_FINAL?

::: dom/workers/ServiceWorkerManager.cpp
@@ +2133,5 @@
>    nsMainThreadPtrHandle<nsIInterceptedChannel> mInterceptedChannel;
>    nsMainThreadPtrHandle<ServiceWorker> mServiceWorker;
>    nsTArray<nsCString> mHeaderNames;
>    nsTArray<nsCString> mHeaderValues;
> +  nsAutoPtr<ServiceWorkerClientInfo> mClientInfo;

It seems to me that you are usin nsAutoPtr as nsRefPtr. nsAutoPtr deletes the object when its dtor is called.
Probably you want make ServiceWorkerClientInfo refcnted and then use nsRefPtr everywhere here.

@@ +2141,5 @@
>  public:
>    FetchEventRunnable(WorkerPrivate* aWorkerPrivate,
>                       nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel,
>                       nsMainThreadPtrHandle<ServiceWorker>& aServiceWorker,
> +                     nsAutoPtr<ServiceWorkerClientInfo>& aClientInfo)

This clientInfo will be delete twice. mClientInfo, and in the caller.
Attachment #8568833 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 19

4 years ago
Created attachment 8572609 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.
Attachment #8568833 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
(In reply to Andrea Marchesini (:baku) from comment #18)
> Comment on attachment 8568833 [details] [diff] [review]
> Update client interface. Implement matchAll WindowClient.
> 
> Review of attachment 8568833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good except for the nsAutoPtr. Happy to review it again when this
> issue is solved. Thanks!
> 
> ::: dom/base/nsGlobalWindow.h
> @@ +678,5 @@
> > +  IsTopLevelWindow()
> > +  {
> > +    MOZ_ASSERT(IsOuterWindow());
> > +    nsCOMPtr<nsIDOMWindow> parentWindow;
> > +    GetParent(getter_AddRefs(parentWindow));
> 
> GetScriptableTop.
Fixed.

> ::: dom/workers/ServiceWorkerClient.cpp
> @@ +29,5 @@
> > +{
> > +  MOZ_ASSERT(aDoc);
> > +  MOZ_ASSERT(aDoc->GetWindow());
> > +
> > +  nsRefPtr<nsGlobalWindow> outerWindow = static_cast<nsGlobalWindow*>(aDoc->GetWindow());
>
> You don't need to case it to retrieve the WindowID(). Just use
> nsCOMPtr<nsPIDOMWindow>
I need the nsGlobalWindow cast for IsTopLevel and HadOriginalOpener.

> @@ +40,5 @@
> > +  if (NS_WARN_IF(result.Failed())) {
> > +    NS_WARNING("Failed to get focus information.");
> > +  }
> > +
> > +  if (outerWindow->IsTopLevelWindow()) {
> 
> wait... if is topLevel it should be FrameType::Top_Level, right?
You're right. This was actually doubled by an error in IsTopLevel which caused the test to pass.
Fixed.
> 
> ::: dom/workers/ServiceWorkerClient.h
> @@ +18,5 @@
> >  namespace workers {
> >  
> > +// Used as a container object for information needed to create
> > +// client objects.
> > +class ServiceWorkerClientInfo MOZ_FINAL {
> 
> { in a new line.
> 
> @@ +20,5 @@
> > +// Used as a container object for information needed to create
> > +// client objects.
> > +class ServiceWorkerClientInfo MOZ_FINAL {
> > +  friend class ServiceWorkerClient;
> > +  friend class ServiceWorkerWindowClient;
> 
> forward declaration for these 2 classes, or move this class after those.
Fixed.
> 
> @@ +35,5 @@
> > +  bool mFocused;
> > +  FrameType mFrameType;
> > +};
> > +
> > +class ServiceWorkerClient : public nsISupports,
> 
> MOZ_FINAL?
We now have ServiceWorkerWindowClient which inherits from ServiceWorkerClient.

> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +2133,5 @@
> >    nsMainThreadPtrHandle<nsIInterceptedChannel> mInterceptedChannel;
> >    nsMainThreadPtrHandle<ServiceWorker> mServiceWorker;
> >    nsTArray<nsCString> mHeaderNames;
> >    nsTArray<nsCString> mHeaderValues;
> > +  nsAutoPtr<ServiceWorkerClientInfo> mClientInfo;
> 
> It seems to me that you are usin nsAutoPtr as nsRefPtr. nsAutoPtr deletes
> the object when its dtor is called.
> Probably you want make ServiceWorkerClientInfo refcnted and then use
> nsRefPtr everywhere here.
> 
> @@ +2141,5 @@
> >  public:
> >    FetchEventRunnable(WorkerPrivate* aWorkerPrivate,
> >                       nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel,
> >                       nsMainThreadPtrHandle<ServiceWorker>& aServiceWorker,
> > +                     nsAutoPtr<ServiceWorkerClientInfo>& aClientInfo)
> 
> This clientInfo will be delete twice. mClientInfo, and in the caller.
My intention was to pass the ownership of the ClientInfo object and I believe it works as expected.
Please have another look and let me know if you still think it is wrong.

Thanks for the review!
(Assignee)

Updated

4 years ago
Attachment #8572609 - Flags: review?(amarchesini)
Comment on attachment 8572609 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

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

Thanks for sending this patch for a new review! It's good!
The only big comment is about the allocation of nsTArray. I guess you can just use a reference.

::: dom/base/nsGlobalWindow.h
@@ +682,5 @@
> +  {
> +    MOZ_ASSERT(IsOuterWindow());
> +    nsCOMPtr<nsIDOMWindow> parentWindow;
> +    nsresult rv = GetScriptableTop(getter_AddRefs(parentWindow));
> +    if (NS_FAILED(rv)) {

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

@@ +686,5 @@
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("Failed to get scriptable top.");
> +    }
> +
> +    return parentWindow.get() == static_cast<nsIDOMWindow*>(this);

do you really need .get() ?

::: dom/workers/ServiceWorkerClient.cpp
@@ +39,5 @@
> +  ErrorResult result;
> +  mFocused = aDoc->HasFocus(result);
> +  if (NS_WARN_IF(result.Failed())) {
> +    NS_WARNING("Failed to get focus information.");
> +  }

no double warning. remove NS_WARN_IF or NS_WARNING.

::: dom/workers/ServiceWorkerClients.cpp
@@ +127,5 @@
>  
>  public:
>    ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate,
>                                 PromiseHolder* aPromiseHolder,
> +                               nsAutoPtr<nsTArray<ServiceWorkerClientInfo>>& aValue)

What about if this is an NsTArray<ServiceWorkerClientInfo>& aValue and you do: mValue.SwapElements(aValue) ?

@@ +216,5 @@
>        return NS_OK;
>      }
>  
>      nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +    nsAutoPtr<nsTArray<ServiceWorkerClientInfo>> result(new nsTArray<ServiceWorkerClientInfo>());

What about:

nsTArray<ServiceWorkerClientInfo> result;
swm->GetAllClients(mScope, &result);

then you swap the elements in ResolvePromiseWorkerRunnable?

@@ +221,4 @@
>  
>      swm->GetAllClients(mScope, result);
>      nsRefPtr<ResolvePromiseWorkerRunnable> r =
>        new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result);

Test: this doesn't need to be allocated.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2565,5 @@
>  
>  class MOZ_STACK_CLASS FilterRegistrationData
>  {
>  public:
> +  FilterRegistrationData(nsTArray<ServiceWorkerClientInfo>* aDocuments,

nsTArray<ServiceWorkerClientInfo>& aDocuments

@@ +2572,5 @@
>      mRegistration(aRegistration)
>    {
>    }
>  
> +  nsTArray<ServiceWorkerClientInfo>* mDocuments;

nsTArray<ServiceWorkerClientInfo>& mDocuments;

@@ +2593,5 @@
>        return PL_DHASH_NEXT;
>    }
>  
> +  ServiceWorkerClientInfo clientInfo(document);
> +  data->mDocuments->AppendElement(clientInfo);

data->mDocuments.AppendElement(clientInfo);

@@ +2641,5 @@
>  } // anonymous namespace
>  
>  void
>  ServiceWorkerManager::GetAllClients(const nsCString& aScope,
> +                                    nsTArray<ServiceWorkerClientInfo>* aControlledDocuments)

nsTArray<ServiceWorkerClienTInfo>& aControlledDocuments)

::: dom/workers/ServiceWorkerManager.h
@@ +381,5 @@
>                uint32_t aFlags);
>  
>    void
>    GetAllClients(const nsCString& aScope,
> +                nsTArray<ServiceWorkerClientInfo>* aControlledDocuments);

nsTArray<ServiceWQorkerClientInfo>& aControlledDocuments);

::: dom/workers/ServiceWorkerWindowClient.cpp
@@ +14,5 @@
> +NS_IMPL_ADDREF_INHERITED(ServiceWorkerWindowClient, ServiceWorkerClient)
> +NS_IMPL_RELEASE_INHERITED(ServiceWorkerWindowClient, ServiceWorkerClient)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(ServiceWorkerWindowClient)
> +NS_INTERFACE_MAP_END_INHERITING(ServiceWorkerClient)

you can get rid of this.

::: dom/workers/ServiceWorkerWindowClient.h
@@ +15,5 @@
> +
> +class ServiceWorkerWindowClient MOZ_FINAL : public ServiceWorkerClient
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED

do you need this?

::: dom/workers/test/serviceworkers/test_match_all_client_properties.html
@@ +65,5 @@
> +  }
> +
> +  function testAuxiliaryWindow() {
> +    var p = getMessageListener();
> +    var w = window.open(clientURL);

have you tested this test with b2g emulator?
Window.open has to be handled differently, as far as I remember.
Attachment #8572609 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

4 years ago
Blocks: 1139431
Duplicate of this bug: 1139431
(Assignee)

Comment 23

4 years ago
Created attachment 8573783 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.
Attachment #8572609 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Comment on attachment 8573783 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

https://hg.mozilla.org/integration/mozilla-inbound/rev/16da34714117

Intermittent failures on mochitest-4 might spike, but the real cause is the worker per domain limitation applying on SW: bug 1053275.
Attachment #8573783 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/16da34714117
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.