Closed
Bug 1409761
Opened 7 years ago
Closed 7 years ago
consider increasing dom.disable_open_click_delay on fennec
Categories
(Core :: DOM: Service Workers, defect, P3)
Core
DOM: Service Workers
Tracking
()
VERIFIED
FIXED
mozilla58
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
bkelly
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We've been struggling with some intermittent issues where notification clicks don't open windows on fennec. We have also seen cases where doing things on fennec takes multiple seconds, particularly when we have to launch the app in response to the notification click.
One possibility is that our dom.disable_open_click_delay time limit is kicking in and preventing the window from opening. Since the overhead on fennec on some devices is quite high we should consider increasing this limit. Perhaps from 1 second to 5 seconds.
Comment 1•7 years ago
|
||
Has anyone tried increasing the pref value to see whether it helps?
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
I'm asking a user to try it. I'll test it on my device as well. Since the issue is intermittent it can be difficult to make a conclusion.
Assignee | ||
Comment 4•7 years ago
|
||
So far touching twitter push notifications have worked 100% reliably since updating the pref locally on my device. Going to keep testing for a while since the problem was intermittent.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•7 years ago
|
||
After playing with the pref I'm fairly certain this is a big part of why notification clicks often seem to fail on android. We launch the app, start the service worker, but then the SW script can openWindow() because launching the SW took so long. It likely takes longer to launch the thread in this cases since the entire app is starting up clogging the main thread.
While we could just bump the pref up, I'm going to see if I can just make the time measurement start after the SW thread is running and we are actually firing the click event.
Summary: consider increasing dom.disable_open_click_delay on fennec → start interaction allowed time period after service worker thread starts
Assignee | ||
Comment 6•7 years ago
|
||
Actually, we already do what I describe in comment 5. We start the nsITimer in the WorkerRun() method just before dispatching the runnable.
I guess the js and async code just runs too slowly on these mobile devices.
Summary: start interaction allowed time period after service worker thread starts → consider increasing dom.disable_open_click_delay on fennec
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8919771 [details] [diff] [review]
Increase dom.disable_open_click_delay on fennec to account for slow devices. r=smaug
Olli, my local testing suggests that this fixes the problem where clicking an android notification often doesn't seem to work. It seems fennec runs very slow when its being launched from a notification and still in the background.
I think just allowing more time on fennec might be the easiest approach here.
If you prefer, I could make a separate pref so that we don't change the main window delay as well.
Attachment #8919771 -
Flags: review?(bugs)
Comment 8•7 years ago
|
||
Comment on attachment 8919771 [details] [diff] [review]
Increase dom.disable_open_click_delay on fennec to account for slow devices. r=smaug
I think service worker + notification use case is different enough, that we should have a separate pref for that.
Attachment #8919771 -
Flags: review?(bugs)
Assignee | ||
Comment 9•7 years ago
|
||
This adds a new preference that we can increase to only effect service worker window interaction delays on mobile.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba8011bc0f71b32f043980aefcc81c2bb15c83bc
Attachment #8919771 -
Attachment is obsolete: true
Attachment #8921077 -
Flags: review?(bugs)
Updated•7 years ago
|
Attachment #8921077 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•7 years ago
|
||
Updated some tests to use the new pref. It didn't really matter, though, since they were already using the default value.
Attachment #8921077 -
Attachment is obsolete: true
Attachment #8921117 -
Flags: review+
Assignee | ||
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ea0df0b154
Create dom.serviceWorkers.disable_open_click_delay and set it to a larger value on fennec. r=smaug
Comment 13•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Comment 14•7 years ago
|
||
Comment on attachment 8921117 [details] [diff] [review]
Create dom.serviceWorkers.disable_open_click_delay and set it to a larger value on fennec. r=smaug
Approval Request Comment
[Feature/Bug causing the regression]: Push notifications on firefox for android.
[User impact if declined]: Clicking a notification on android will often appear to have no effect. Its a glaring UX problem and happens somewhat frequently. Normally I would not request uplift for this patch, but I'd like to get this into beta since its such a bad experience for users and we might have more users trying firefox in 57.
[Is this code covered by automated tests?]: Unfortunately we don't have the ability to automate tests for android push notification testing.
[Has the fix been verified in Nightly?]: Its been in nightly for a couple days and I have manually verified it. I haven't had a single push notification over that time fail to launch FF.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. To test please:
1. Login in to twitter.com
2. Enable twitter.com push notifications in the twitter settings.
3. Accept the permissions request.
4. Close firefox.
5. Use another test account on twitter to trigger a notification (@mention or DM) with the first account. Twitter doesn't show all notifications due to its algorithm, so you may need to experiment to see the best way to manually trigger notifications.
6. When the notification appears, make sure firefox is closed.
7. Click the notification.
8. Verify that firefox launches and opens a twitter window to the event being notified. (Note there is sometimes a delay in opening, which is bug 1358178.)
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: This patch adds a new preference and sets it to a higher value on fennec.
[String changes made/needed]: None
Attachment #8921117 -
Flags: approval-mozilla-beta?
status-firefox57:
--- → affected
Comment on attachment 8921117 [details] [diff] [review]
Create dom.serviceWorkers.disable_open_click_delay and set it to a larger value on fennec. r=smaug
Improving the reliability of push notification user interaction on Fennec, seems low risk and includes automated tests!!, Beta57+
Attachment #8921117 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 16•7 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #15)
> Improving the reliability of push notification user interaction on Fennec,
> seems low risk and includes automated tests!!, Beta57+
Just to clarify, this does not include automated tests but does have a set of manual QA tests that can be run. I still think its low risk, though.
Comment 18•7 years ago
|
||
Device:
- Samsung Galaxy Tab 3 (Android 7.0)
- Samsung Galaxy Note 4 (Android 5.0.1)
- Huawei Honor 5X (Android 5.1.1)
Hello,
I have verified this following the steps in Comment 14. Regarding step 4, by close do you mean send the app into the background [1] or end the process entirely [2]?
If [1] > push notifications are received.
If [2] > no push notifications are received.
If [1] is the desired scenario, please mark this as verified for both beta and nightly as it works as expected without any kind of issues.
Thanks!
Flags: needinfo?(bkelly)
Assignee | ||
Comment 19•7 years ago
|
||
Hmm, I actually meant kill firefox via the android task switcher. So I think that is [2] in comment 18.
Do you mean you don't get the notification at all? Or when clicking the notification the window does not open?
Thanks.
Flags: needinfo?(bkelly) → needinfo?(bogdan.surd)
Assignee | ||
Comment 20•7 years ago
|
||
The issue here is more about what happens after clicking the notification. So if killing firefox first prevents the notification from being shown, then perhaps wait for the notification to appear and then kill firefox. The important part here is to click the notification while firefox is killed to verify its restarted.
Comment 21•7 years ago
|
||
Hello and thank you for the clarification in Comment 20:
- Checked again and everything works as expected. If I close the app after receiving the notification and tap on it, [1] the app opens a twitter window with the event. This works the same in Nightly & Beta.
Regarding Comment 19:
- Yes, that is what I meant by no notifications are receives. If I kill the app from the task switcher I don't receive any kind of push notifications.
- If the app is sent into the background I receive a push notifications and tapping on it > [1].
Status: RESOLVED → VERIFIED
Flags: needinfo?(bogdan.surd)
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Bogdan Surd, QA [:BogdanS, NI] from comment #21)
> Regarding Comment 19:
> - Yes, that is what I meant by no notifications are receives. If I kill the
> app from the task switcher I don't receive any kind of push notifications.
Ok, this sounds like a different bug. I'll write a separate issue.
FWIW, though, I do get notifications when firefox is closed in everyday use, but its hard for me to tell if they are delayed or sometimes missed.
Assignee | ||
Updated•7 years ago
|
Component: DOM → DOM: Service Workers
Assignee | ||
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•