Closed Bug 911676 Opened 11 years ago Closed 11 years ago

[system] check navigator.mozTelephony statements' robustness

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

Other
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

VERIFIED FIXED
blocking-b2g koi+
Tracking Status
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.
Blocks: flatfish
blocking-b2g: --- → koi?
Assignee: nobody → gduan
please also take mozMobileConnection and mozIccManager into consideration, thanks
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 .
blocking-b2g: koi? → koi+
blocking-b2g: koi+ → koi?
Depends on: 911693
Attached file PR to master
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 on attachment 800651 [details]
PR to master

The cost-control-widget modification looks invalid to me. Please check.
Attachment #800651 - Flags: review?(akuo)
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 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 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-
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)
Oh, please, don't ask me in private if the concerns are not private, let's discuss here. :)
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 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+
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)
Please, please, please, ensure this is not affecting the normal behaviour of Cost Control before merge.

But looks nice!
Flags: needinfo?(gduan)
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+
please dont merge until we figure out telephony API existence issues discussed in bug 911684
Depends on: 911684
Depends on: 920551
No longer depends on: 911684
blocking-b2g: koi? → koi+
Whiteboard: [Flatfish only]
With patch from bug 814556, Gaia may have a try for world without 'navigator.mozTelephony' on b2g-desktop.
George, could you help landing this patch?
Flags: needinfo?(gduan)
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
Uplifted b3769da0c9f5638f486a98b1d06cc68bfd9edd2d to:
v1.2: c9d2d307adf136ef266708b32a7aa472a5855e7c
Gaia:     4a0bb67204572affcbf1afb384d2ffdc8261e977
Gecko:    e42dac2ae79959eadd8d832c715e430ada8d64cc
BuildID   20131108064537
Version   28.0a1
Status: RESOLVED → VERIFIED
Attachment #802119 - Flags: feedback?(gduan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: