Closed
Bug 889898
Opened 11 years ago
Closed 11 years ago
WebSMS: remove fallback backend
Categories
(Firefox OS Graveyard :: RIL, defect)
Firefox OS Graveyard
RIL
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: vicamo, Assigned: vicamo)
References
Details
Attachments
(1 file, 4 obsolete files)
21.53 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
From bug 859616, we should return undefined rather then null on platforms have no SMS support. From SmsManager::CreateInstanceIfAllowed()[1], we don't even instantiate a SmsService unless all conditions are met.
From above two facts, having a fallback backend and a SmsService::HasSupport() method are not very meaningful because they're either never referenced or returning a always-true value.
[1]: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/SmsManager.cpp#58
Assignee | ||
Comment 1•11 years ago
|
||
Assignee: nobody → vyang
Assignee | ||
Comment 2•11 years ago
|
||
Depends on bug 859616 to skip building MobileMessage sources on unsupported platforms.
Depends on: 859616
Comment 3•11 years ago
|
||
Vicamo, could you explain what you are doing here. I think the correct solution would be to use WebIDL: if Navigator is using WebIDL, we could easily say that .mozSMS or whatever the property isn't exposed if SMS isn't supported by the platform.
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #3)
> Vicamo, could you explain what you are doing here. I think the correct
> solution would be to use WebIDL: if Navigator is using WebIDL, we could
> easily say that .mozSMS or whatever the property isn't exposed if SMS isn't
> supported by the platform.
I think we can just guard all WebSMS code with MOZ_WEBSMS_BACKENDS first. Other functions like Bluetooth, Telephony, FM, etc. all have their own global switches, but we build WebSMS code on all platforms that are unlikely to have a working SMS port. I think we may take old fashion method first, in order to solve bug 859616 as well as to stop building unnecessary code, before WebIDL-based Navigator.
Assignee | ||
Comment 5•11 years ago
|
||
1) Remove nsISmsService::hasSupport. Support for WebSMS is actually controlled be MOZ_WEBSMS_BACKENDS, hasSupport() call doesn't really mean something and may be never called at all.
2) Remove "fallback" backend. Same reason here. Fallback SMS Service is never instantiated if MOZ_WEBSMS_BACKENDS is undefined.
Attachment #770874 -
Attachment is obsolete: true
Attachment #773027 -
Flags: review?(mounir)
Comment 6•11 years ago
|
||
Comment on attachment 773027 [details] [diff] [review]
patch
Review of attachment 773027 [details] [diff] [review]:
-----------------------------------------------------------------
What's the purpose of this?
Attachment #773027 -
Flags: review?(mounir)
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #6)
> What's the purpose of this?
Since bug 859616, guarding all WebSMS codes with MOZ_WEBSMS_BACKENDS, is not going to happen, I have to rewrite the patch here.
Basically we don't really need a nsISmsService::hasSupport() call and a fallback nsISmsService instance. They're redundant in current implementation.
nsISmsService::hasSupport() always returns the same boolean value, and that call is actually shadowed by MOZ_WEBSMS_BACKENDS. If MOZ_WEBSMS_BACKENDS is not defined, it's never called and the return value is meaningless[1][2]; otherwise, it's called and always returns true. We can simply remove this function without any side effect. Actually, after bug 891235 is landed, this call is never referenced even in Navigator::GetMozMobileMessage()[3].
About the fallback SMS service, if MOZ_WEBSMS_BACKENDS is not defined, then it's never instantiated[1][4]; otherwise, either gonk/SmsService or android/SmsService is instantiated, not the fallback one.
[1]: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/SmsManager.cpp#62
[2]: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/SmsManager.cpp#86
[3]: http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1219
[4]: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/SmsManager.cpp#82
No longer depends on: 859616
Assignee | ||
Comment 8•11 years ago
|
||
Rebase. Removed fallback/*, android/MmsService, nsISmsService::HasSupport.
Attachment #773027 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #821689 -
Attachment is obsolete: true
Attachment #821802 -
Flags: feedback?
Assignee | ||
Updated•11 years ago
|
Attachment #821802 -
Flags: feedback? → feedback?(gene.lian)
Assignee | ||
Updated•11 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Assignee | ||
Comment 10•11 years ago
|
||
Have another bail-out for code that is never used. That's absolutely wrong! https://hg.mozilla.org/integration/b2g-inbound/rev/b95c97c9617d
Assignee | ||
Comment 11•11 years ago
|
||
s/bail-out/backed out/
Assignee | ||
Updated•11 years ago
|
Attachment #821802 -
Flags: feedback?(gene.lian) → review?(gene.lian)
Comment 12•11 years ago
|
||
Comment on attachment 821802 [details] [diff] [review]
patch
Review of attachment 821802 [details] [diff] [review]:
-----------------------------------------------------------------
Looks nice! I assume all the fallback directly will complete go away.
r=gene
::: dom/mobilemessage/src/SmsServicesFactory.cpp
@@ +33,3 @@
> smsService = new SmsService();
> +#elif defined(MOZ_WIDGET_GONK) && defined(MOZ_B2G_RIL)
> + smsService = new SmsService();
Can we merge this one with the above one?
Attachment #821802 -
Flags: review?(gene.lian) → review+
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #12)
> ::: dom/mobilemessage/src/SmsServicesFactory.cpp
> @@ +33,3 @@
> > smsService = new SmsService();
> > +#elif defined(MOZ_WIDGET_GONK) && defined(MOZ_B2G_RIL)
> > + smsService = new SmsService();
>
> Can we merge this one with the above one?
I would prefer not because that keeps preprocessor conditions clear and one for each platform. Besides, bug 873351 is going to move SmsService out of RadioInterfaceLayer, and this will separate the two yet again if we merge them now.
Comment 14•11 years ago
|
||
Yeap, I guess so. Thanks! Please go ahead to land.
Assignee | ||
Comment 15•11 years ago
|
||
Rebase. Full try before landing https://tbpl.mozilla.org/?tree=Try&rev=7f63afbeafe7
Attachment #821802 -
Attachment is obsolete: true
Attachment #832725 -
Flags: review+
Assignee | ||
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #18)
> https://hg.mozilla.org/mozilla-central/rev/a040227d9c1e
That must be something wrong.
Flags: needinfo?(kwierso)
Flags: needinfo?(jorendorff)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
> (In reply to Wes Kocher (:KWierso) from comment #18)
> > https://hg.mozilla.org/mozilla-central/rev/a040227d9c1e
>
> That must be something wrong.
That patch should be for bug 889897.
Flags: needinfo?(kwierso)
Flags: needinfo?(jorendorff)
You need to log in
before you can comment on or make changes to this bug.
Description
•