Vicamo Yang [:vicamo][:vyang] Assignee
	Description • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 1 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=00d1a948f321

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 2 • 11 years ago

Depends on bug 859616 to skip building MobileMessage sources on unsupported platforms.

	Mounir Lamouri (:mounir)
	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.

	Vicamo Yang [:vicamo][:vyang] 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.

	Vicamo Yang [:vicamo][:vyang] 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.

	Mounir Lamouri (: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?

	Vicamo Yang [:vicamo][:vyang] 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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 8 • 11 years ago

Rebase.  Removed fallback/*, android/MmsService, nsISmsService::HasSupport.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 9 • 11 years ago

Full try: https://tbpl.mozilla.org/?tree=Try&rev=3b0720f37ab9

	Vicamo Yang [:vicamo][:vyang] 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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 11 • 11 years ago

s/bail-out/backed out/

	Gene Lian (I already quit Mozilla)
	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?

	Vicamo Yang [:vicamo][:vyang] 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.

	Gene Lian (I already quit Mozilla)
	Comment 14 • 11 years ago

Yeap, I guess so. Thanks! Please go ahead to land.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 15 • 11 years ago

Rebase.  Full try before landing https://tbpl.mozilla.org/?tree=Try&rev=7f63afbeafe7

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 16 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/0226eba0e10b

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/0226eba0e10b

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a040227d9c1e

	Vicamo Yang [:vicamo][:vyang] 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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 10 years ago

(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.