Closed Bug 1262251 Opened 8 years ago Closed 8 years ago

Implement Clients.openWindow() for when Fennec is running in background

Categories

(Core :: DOM: Notifications, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
fennec 52+ ---
firefox52 --- fixed

People

(Reporter: jchen, Assigned: droeh)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(2 files, 2 obsolete files)

When Fennec is running in the background (e.g. after receiving a GCM message), we still need support for openWindow(), which will launch the Fennec activity and open a tab.
Just curious, does that indicate a push notification can cause a pop-up?
(In reply to Samael Wang [:freesamael][:sawang] from comment #1)
> Just curious, does that indicate a push notification can cause a pop-up?

Yes, but in Firefox, openWindow is permitted only when the user clicks a notification.
(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> Yes, but in Firefox, openWindow is permitted only when the user clicks a
> notification.

Sounds reasonable. Thank you!
Catalin, are you able/interested to take this?
Flags: needinfo?(catalin.badea392)
Whiteboard: btpp-followup-2016-04-14
Yes, though I'm not very familiar with android.
Flags: needinfo?(catalin.badea392)
(In reply to Cătălin Badea (:catalinb) from comment #5)
> Yes, though I'm not very familiar with android.

Thanks! If you have questions, I'm sure snorp or Margaret can help.
Assignee: nobody → catalin.badea392
Whiteboard: btpp-followup-2016-04-14 → btpp-active
After debugging this a bit, it turns out clicking a notification while fennec is not running will not launch the associated service worker.
Depends on: 1264815
Any update on this?
Flags: needinfo?(catalin.badea392)
tracking-fennec: --- → ?
tracking-fennec: ? → 48+
Depends on: 1281940
Very rough patch, but I'm uploading it since I won't be able to work on this until next week
and someone else might take it.
The problem with openWindow on android is that when gecko is running as a result of push
notification click there's no browser window active to use for a new window (which is what
the SW code is trying to use). To fix this, I'm making a call from gecko to java
to launch the main activity and then wait for the browser window to be active
using the observer service.
There are two cases that need to be addressed:
1. fennec is running, but in the background: here we need to just foreground the application
2. fennec is not running: launch the main activity and wait until we can open the window.
Assignee: catalin.badea392 → nobody
Flags: needinfo?(catalin.badea392)
Assignee: nobody → afarre
Cătălin, any update? Are you still working on it? Andreas, do you have this now?
Flags: needinfo?(catalin.badea392)
Flags: needinfo?(afarre)
I have this, and I've started working on it, but basically the patch from Cătălin is the way to go. Unfortunately it crashes, and I haven't been able to sort that out yet. I'm glad to hand this over to someone else if you want?
Flags: needinfo?(afarre)
OK. Dylan, please take this one and see if you can get the patch into a landable state.
Assignee: afarre → droeh
Assignee: droeh → esawin
tracking-fennec: 48+ → 49+
tracking-fennec: 49+ → 50+
Assignee: esawin → droeh
Flags: needinfo?(catalin.badea392)
In this patch, if there is no browser window, we launch Fennec, wait for the "BrowserChrome:Ready" message, and try to open the window once again.
Attachment #8799051 - Flags: review?(overholt)
Attachment #8799051 - Flags: review?(overholt) → review?(catalin.badea392)
Comment on attachment 8799051 [details] [diff] [review]
Launch Fennec if necessary before opening a window

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

Thank you for working on this!

The main issue is that ServiceWorkerClients shouldn't be used on the main thread.
Also, there's a second case that should be handled, with Fennec running in the background -
openWindow will successfully open the tab, but the app will still be hidden in the background.

Could you please also add a test for this?

::: dom/workers/ServiceWorkerClients.cpp
@@ +474,5 @@
>  {
>    RefPtr<PromiseWorkerProxy> mPromiseProxy;
>    nsString mUrl;
>    nsString mScope;
> +  ServiceWorkerClients* mServiceWorkerClients;

ServiceWorkerClients is ref-counted on the worker thread and shouldn't be passed to the main thread. You should use ServiceWorkerPrivate on the main thread instead.

My preference would be to make the runnable an Observer and just do ServiceWorkerPrivate->StoreISupports() to add-ref it while it waits for "Browser:Ready", but it's up to you.

@@ +566,5 @@
> +      java::GeckoAppShell::OpenWindowForNotification();
> +      mServiceWorkerClients->AddPendingWindow(this);
> +      return NS_OK;
> +    }
> +#endif

Please also handle the situation where Fennec is running in the background. In this case, the openWindow operation will succeed but Fennec would continue to stay in the background.

@@ +809,5 @@
> +  pendingWindows.AppendElement(aPendingWindow);
> +}
> +
> +nsresult
> +ServiceWorkerClients::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData)

nit coding style: nsISupports* aSubject, const char* aTopic etc.
Attachment #8799051 - Flags: review?(catalin.badea392) → review-
Alright, I think this addresses everything you brought up aside from your request for a test. I talked to snorp a bit about this and it looks like trying to figure out how to do a good job of testing push notifications on Android might be quite difficult and probably outside the scope of this bug.
Attachment #8799051 - Attachment is obsolete: true
Attachment #8804459 - Flags: review?(catalin.badea392)
Blocks: 1313096
tracking-fennec: 50+ → 52+
Comment on attachment 8804459 [details] [diff] [review]
Launch Fennec if necessary before opening a window v2

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

Looks good. Thanks!

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +2019,5 @@
> +  pendingWindows.AppendElement(aPendingWindow);
> +}
> +
> +nsresult
> +ServiceWorkerPrivate::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData)

coding style: nsISupports* aSubject, const char* aTopic, const char16_t* aData
Attachment #8804459 - Flags: review?(catalin.badea392) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0349a81229e7
Make openWindow() launch Fennec if it isn't already running. r=catalinb
Ugh, sorry about that. I just need to wrap the AndroidBridge.h include in #ifdef MOZ_WIDGET_ANDROID.
Flags: needinfo?(droeh)
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/582e5f96a727
Make openWindow() launch Fennec if it isn't already running. r=catalinb
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1145a10a2fd5
Backed out changeset 582e5f96a727 for Linux serviceworker bustage
Oh, and not just Linux, I just hadn't looked again since we finally got around to running some tests on Mac and Windows, both of which were also affected.
Alright, this doesn't seem like it should affect anything other than Android, so that's strange. I'll look into it.
So it looks like the test failures were related to cycle collection, which is not an area I'm super familiar with. I was able to fix it (verified on a local OSX build and on try) by changing NS_INTERFACE_MAP_ENTRY(nsIObserver) with NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver) in ServiceWorkerPrivate.cpp, but as I said I'm not too familiar with this and am not 100% sure this is the correct way to go. Catalin, do you have any thoughts about this? (The rest of the patch is identical to the previous version.)
Attachment #8804459 - Attachment is obsolete: true
Attachment #8805678 - Flags: review?(catalin.badea392)
(In reply to Dylan Roeh (:droeh) from comment #25)
> Created attachment 8805678 [details] [diff] [review]
> Launch Fennec if necessary before opening a window v2.1
> 
> So it looks like the test failures were related to cycle collection, which
> is not an area I'm super familiar with. I was able to fix it (verified on a
> local OSX build and on try) by changing NS_INTERFACE_MAP_ENTRY(nsIObserver)
> with NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver) in
> ServiceWorkerPrivate.cpp, but as I said I'm not too familiar with this and
> am not 100% sure this is the correct way to go. Catalin, do you have any
> thoughts about this? (The rest of the patch is identical to the previous
> version.)

It doesn't look like ServiceWorkerPrivate leaked, just that the QI to nsISupports failed.
test_service_worker_liftime.html is the only test which uses the QI to nsISupports, when dispatching
a test only event for service worker shutdown.
Attachment #8805678 - Flags: review?(catalin.badea392) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5abd7301134e
Make openWindow() launch Fennec if it isn't already running. r=catalinb
https://hg.mozilla.org/mozilla-central/rev/5abd7301134e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.