Closed
Bug 911676
Opened 11 years ago
Closed 11 years ago
[system] check navigator.mozTelephony statements' robustness
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(blocking-b2g:koi+, b2g-v1.2 fixed)
People
(Reporter: gasolin, Assigned: gduan)
References
Details
(Whiteboard: [Flatfish only])
Attachments
(2 files)
we'll have non telephony devices soon, need to check if we proper use the navigator.mozTelephony API In non telephony devices, the navigator.mozTelephony is not exist.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → gduan
Reporter | ||
Comment 1•11 years ago
|
||
please also take mozMobileConnection and mozIccManager into consideration, thanks
Assignee | ||
Comment 2•11 years ago
|
||
cost_control.js - widgetFrame.setVisible(false) may cause error if iframe is not created QuickSettings - init function should not check mozMobileConnection and jump out - need to modify elf.data.dataset.enabled value properly For the rest of things, I think they can be filtered by returning value of window.navigator.mozMobileConnection/mozIccManager/mozTelephony .
Updated•11 years ago
|
blocking-b2g: koi? → koi+
Assignee | ||
Comment 3•11 years ago
|
||
Hi Alive, Since we're going to support device that has no mozMobileConnection & mozTelephony, I went through system app's code and found some items may need to modify as below. 1. hide data icon from quicksettings when mozMobileConnection not exist. 2. prevent js error in costcontrol.js if trying to call |widgetFrame.setVisible(true)| but widgetFrame is undefined yet. Please kindly check, thanks!
Attachment #800651 -
Flags: review?(akuo)
Comment 4•11 years ago
|
||
Comment on attachment 800651 [details]
PR to master
The cost-control-widget modification looks invalid to me. Please check.
Attachment #800651 -
Flags: review?(akuo)
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 800651 [details]
PR to master
Thanks you to point this out. Patch is updated, please kindly check again!
Attachment #800651 -
Flags: review?(akuo)
Comment 6•11 years ago
|
||
Comment on attachment 800651 [details]
PR to master
A little concern to have setVisible everywhere....
But don't want to be picky so r+ first.
Salva please check cost_control.js change again.
Thanks.
Attachment #800651 -
Flags: review?(akuo)
Attachment #800651 -
Flags: review+
Attachment #800651 -
Flags: feedback?(salva)
Comment 7•11 years ago
|
||
Comment on attachment 800651 [details]
PR to master
The is no code in cost_control.js that could throw an error when mozMobileConnection in unavailable. If you want Cost Control to support no mobile devices I think it deserves a specific bug and the Cost Control application should be adapted.
Attachment #800651 -
Flags: feedback?(salva) → feedback-
Comment 8•11 years ago
|
||
Assuming you are receiving an error because there is code making use of the widget, the solution is as simple as checking if Cost Control is installed BEFORE any other code moving the early return at the beginning of the script. Nothing more. I'm preparing a PR for :gduan with the proper solution. Then ask for my review / feedback again. Ok?
Flags: needinfo?(gduan)
Comment 9•11 years ago
|
||
Oh, please, don't ask me in private if the concerns are not private, let's discuss here. :)
Comment 10•11 years ago
|
||
I think this is the best solution not involving waiting for application ready event which complicates a little bit the trivial approach.
Attachment #802119 -
Flags: review?(alive)
Attachment #802119 -
Flags: feedback?(gduan)
Comment 11•11 years ago
|
||
Comment on attachment 802119 [details]
Avoiding the use of an undefined iframe.
r+ for short term solution.
To me long term solution is lazy load the cost_control.js on demand.
Attachment #802119 -
Flags: review?(alive) → review+
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 800651 [details]
PR to master
Hi Alive,
This patch uses lazyloader for cost_control.js in utilitytray.js.
Please kindly review.
Attachment #800651 -
Flags: review+ → review?(alive)
Comment 13•11 years ago
|
||
Please, please, please, ensure this is not affecting the normal behaviour of Cost Control before merge. But looks nice!
Updated•11 years ago
|
Flags: needinfo?(gduan)
Comment 15•11 years ago
|
||
Comment on attachment 800651 [details]
PR to master
Though we don't have a 'wifi' only cost control now, we shall consider this case in the future. Please comment on the line of lazy load.
Attachment #800651 -
Flags: review?(alive) → review+
Reporter | ||
Comment 16•11 years ago
|
||
please dont merge until we figure out telephony API existence issues discussed in bug 911684
Depends on: 911684
Reporter | ||
Updated•11 years ago
|
Updated•11 years ago
|
blocking-b2g: koi? → koi+
Updated•11 years ago
|
Whiteboard: [Flatfish only]
Comment 17•11 years ago
|
||
With patch from bug 814556, Gaia may have a try for world without 'navigator.mozTelephony' on b2g-desktop.
Reporter | ||
Comment 18•11 years ago
|
||
George, could you help landing this patch?
Flags: needinfo?(gduan)
Assignee | ||
Comment 19•11 years ago
|
||
Tested on flatfish device that Viral flashed for us and mobile, it works fine. Thanks, Merged into master, https://github.com/mozilla-b2g/gaia/commit/b3769da0c9f5638f486a98b1d06cc68bfd9edd2d
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(gduan)
Resolution: --- → FIXED
Comment 21•11 years ago
|
||
Uplifted b3769da0c9f5638f486a98b1d06cc68bfd9edd2d to: v1.2: c9d2d307adf136ef266708b32a7aa472a5855e7c
status-b2g-v1.2:
--- → fixed
Comment 22•11 years ago
|
||
Gaia: 4a0bb67204572affcbf1afb384d2ffdc8261e977 Gecko: e42dac2ae79959eadd8d832c715e430ada8d64cc BuildID 20131108064537 Version 28.0a1
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•11 years ago
|
Attachment #802119 -
Flags: feedback?(gduan)
You need to log in
before you can comment on or make changes to this bug.
Description
•