Closed
Bug 965139
Opened 11 years ago
Closed 11 years ago
[Email] Acquiring a wifi wakelock while sending an email
Categories
(Firefox OS Graveyard :: Gaia::E-Mail, defect)
Tracking
(b2g-v1.3 affected, b2g-v1.3T affected, b2g-v1.4 fixed, b2g-v2.0 unaffected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
b2g-v1.3 | --- | affected |
b2g-v1.3T | --- | affected |
b2g-v1.4 | --- | fixed |
b2g-v2.0 | --- | unaffected |
People
(Reporter: psingapati, Assigned: psingapati)
Details
(Whiteboard: [TD-42468])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/html
|
jrburke
:
review+
|
Details |
Issue:
This issue is about sending a large email (4 ~ 5 MB) using WiFi was failed and not stored on any of email box in Leo device.
Bug 871897 was referred for this issue, but wakelocks consideration in this scenario is pushed to later(earlier discussion with :asuth). Some related bug on email for WiFi wake lock is 907028. But it is about sync.
So need to consider the acquiring WiFi wakelock, while sending an email.
Background:
Discussed with :jrburke on IRC, raising the issue and also attaching the patch.
Assignee | ||
Comment 1•11 years ago
|
||
This patch is to avoid wifi disconnection(on screen off)while sending a large email from FxOS email client.
Patch :
Acquiring WiFi wakelock onSend and unlocking it on getting the response.
Attachment #8367129 -
Flags: review?(jrburke)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → psingapati
Comment 2•11 years ago
|
||
Comment on attachment 8367129 [details] [diff] [review]
WiFi_WakeLock.patch
I apologize for the delay in reviewing. Some initial comments:
1) I am waiting to get confirmation from someone on the network team about if it is wise to always ask for a wifi wake lock even when not on wifi. That person will be back after a Chinese New Year holiday, Feb 8, so I do not expect to resolve this fully before then. However, I sent email to him, in the context of other network detection questions, that include asking about when a wake lock should be grabbed.
2) In the meantime, I feel like we should do a timeout here to clear any wakelock from a bad send case, similar to how wake_locks.js does for the sync wake locks:
https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/wake_locks.js#L9
to avoid a bad case from retaining a wake lock.
It should be a per-compose card instance state to do this, in case a compose card is somehow destroyed and a new one created. The wake lock on the card should also be cleared in the die: method in the case of card destruction.
So I'm going to clear review for this now but feel free to flip it back to me with the above changes, and then we can lock down the final wake lock logic after hearing back from network team.
Otherwise, we can wait to get the final answer, and I will post to this bug about what code changes need to be done.
Also note that background send story in bug 921050 will likely change how this work is done. I am fine doing this change for now though, as background send will not happen in 1.4, and it may be possible to get this to land for 1.4.
Attachment #8367129 -
Flags: review?(jrburke)
Comment 3•11 years ago
|
||
I just heard back early from the network person, and he confirmed it is fine to just always ask for the wifi wake lock when doing the connection, does not matter what the actual state of the network is. So we can proceed along the review feedback above for a final fix.
Assignee | ||
Comment 4•11 years ago
|
||
hi :jrburke,
I have added the check for "lock" and clearing it in the die:function().
is it required to have the timer in this case? Since sending of the email is happening in the foreground and it is blocking to the user. I feel, timer may not be required in this case, if the application is killed lock will be released automatically.[https://developer.mozilla.org/en-US/docs/Web/API/Navigator.requestWakeLock - Note].
Please review the attached patch and suggest me if any thing needs to be added.
Thanks.
Attachment #8367129 -
Attachment is obsolete: true
Attachment #8373187 -
Flags: review?(jrburke)
Comment 5•11 years ago
|
||
Comment on attachment 8373187 [details] [diff] [review]
Bug_965139_WiFi_WakeLock.patch
I would prefer to see the `if (this.wifiLock)` work just in a method on this class instead of having that duplicated multiple times, and just call that method (releaseLocks?) to release the locks. Having this change as a github pull request would also help for merge/tracking purposes.
Thinking more about this though: do we also need a 'cpu' lock for this work? Furthermore, maybe a 'high-priority' lock makes more sense, as discussed here:
https://groups.google.com/d/topic/mozilla.dev.gaia/PhnzYwu2lxU/discussion
If you want to do some testing/research to confirm which locks are actually needed and allow the send to complete, that would be useful to know. I suspect though that just like with sync, we may need a 'cpu' lock.
Attachment #8373187 -
Flags: review?(jrburke)
Assignee | ||
Comment 6•11 years ago
|
||
Hi :jrburke
Have done the changes as per comment#5.
Following are the tests done with the attached PR.
1. Open Email -> Compose -> attach 4.9MB content to the email composer -> send
-- Screen went to sleep after some time and the email send is successful.
2. Open Email -> Compose -> attach 4.9MB content to the email composer -> send -> press "Power" button
-- Screen went to sleep immediately and the email send is successful.
3. Open Email -> Compose -> attach 4.9MB content to the email composer -> send -> press "Home" button
-- Email application went to the background and after some time screen went to sleep mode and the email send is successful.
All the above tests are carried out 3 times for each scenario.
For each send it took 4~5 minutes.
I did not see any failure case.
Please review the attached PR.
Thanks.
Attachment #8378204 -
Flags: review?(jrburke)
Comment 7•11 years ago
|
||
Comment on attachment 8378204 [details]
Pointer to Pull Request
This is looking good!
I do have one piece of capability detect feedback, mentioned in the pull request. If you can get that change in, the rebase over latest master (I had a hard time testing on latest b2g due to a change that is only in a more recent version of master), then flip it back to me and I should be able to turn around an r+ very quickly.
I apologize for taking so long with the review, but I think it is really close now, just down to the last change.
Attachment #8378204 -
Flags: review?(jrburke)
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 8378204 [details]
Pointer to Pull Request
hi,
PR is updated with the capability check for navigator.requestWakeLock
Please check the PR and review
Thanks
Attachment #8378204 -
Flags: review?(jrburke)
Comment 9•11 years ago
|
||
Comment on attachment 8378204 [details]
Pointer to Pull Request
r+ but with a nit I did not see before: comments in bodies should ideally just be the double slash with a space between the slashes, like so:
// releasing the wake lock on send response
same for favoring that style instead of /* */ inside a function.
Fix/rebase with that change and should be good to land.
Attachment #8378204 -
Flags: review?(jrburke) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Updated the PR,
Please check and land it
Thanks
Flags: needinfo?(jrburke)
Comment 11•11 years ago
|
||
Merged to gaia master:
https://github.com/mozilla-b2g/gaia/commit/cd90096bd9448efeef08eae32acd7074ba4b6c20
from pull request:
https://github.com/mozilla-b2g/gaia/pull/16446
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(jrburke)
Resolution: --- → FIXED
Updated•11 years ago
|
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•