844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

As title. After discussing with Vicamo, we're hoping to move the SMS codes into dom/mobilemessage to make it more generic for MMS. This is a precondition to implement the MMS DOM API in the future.

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Seems working well. Please take this review.

Run the try at the same time: https://tbpl.mozilla.org/?tree=Try&rev=fd6b1506bafa

Comment 2

[Gene Lian (I already quit Mozilla)]

Comment on attachment 717781 [details] [diff] [review]
Patch

Hi Mounir,

Just a quick question needing your feedback. In this bug, we need to deprecate the |dom_sms.xpt| in:

  b2g/installer/package-manifest.in
  browser/installer/package-manifest.in
  mobile/android/installer/package-manifest.in
  mobile/xul/installer/package-manifest.in

We're wondering do we need to add the corresponding codes to indicate the |dom_sms.xpt| has been removed?

  b2g/installer/removed-files.in
  browser/installer/removed-files.in
  mobile/android/installer/removed-files.in
  mobile/xul/installer/removed-files.in

If yes, could you please give us a quick example for how to do so? Thanks!

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

leo+, this bug is needed to fulfill MMS user stories for v1.1

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 717781 [details] [diff] [review]
Patch

Review of attachment 717781 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +68,5 @@
>  
>  #include "nsIDOMGlobalPropertyInitializer.h"
>  
>  using namespace mozilla::dom::power;
> +using namespace mozilla::dom::mobilemessage;

If we had to change this line, maybe just move all dom-related code to mozilla::dom instead and keep ipc-related in mozilla::dom::mobilemessage.

::: dom/dom-config.mk
@@ +12,5 @@
>    dom/media \
>    dom/network/src \
>    dom/settings \
>    dom/phonenumberutils \
> +  dom/mobilemessage/src \

Please don't add such entry to dom-config. We don't really need it. Just export necessary header files instead.

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Hi Mounir,

Could you please double check the |removed-files.in| parts (please see the comment #2 for the purpose)? Thanks!

Hi Vicamo,

This new patch addresses comment #4. Please feel free to let me know if you think it's better to put *internal* services/types/constants under |mozilla::dom| as well (instead of |mozilla::dom::mobilemessage|). Btw, we have to add |dom/mobilemessage/src| to dom-config.mk; otherwise, we would see compiling errors. Thanks for your review again!

See the try at the same time: https://tbpl.mozilla.org/?tree=Try&rev=05e00fb01cd8

Comment 6

[Gene Lian (I already quit Mozilla)]

> See the try at the same time:
> https://tbpl.mozilla.org/?tree=Try&rev=05e00fb01cd8

Oops, some compiling errors in Android but seems easy to fix. Please go ahead to review. Thanks!

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 718922 [details] [diff] [review]
Patch, V2

Review of attachment 718922 [details] [diff] [review]:
-----------------------------------------------------------------

Great work! Thank you :)

::: dom/dom-config.mk
@@ +12,5 @@
>    dom/media \
>    dom/network/src \
>    dom/settings \
>    dom/phonenumberutils \
> +  dom/mobilemessage/src \

Can't we just remove dom/sms/src from the list?

Comment 8

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3

Addressing comment #7 and rebasing everything. r=vicamo

Run the try at the same time: https://tbpl.mozilla.org/?tree=Try&rev=68dce758e3f0

Comment 9

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.1

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

Comment 10

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af40e0f0ce26

Comment 11

[Gene Lian (I already quit Mozilla)]

It might be difficult to check this in into b2g18 due to lots of conflicts with the recent makefile change.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Mounir Lamouri (:mounir)]

So this has been pushed without a DOM peer review?

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Mounir Lamouri (:mounir) from comment #13)
> So this has been pushed without a DOM peer review?

Ah, I apologize for this careless mistake. We don't really mean it :(

Comment 15

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> (In reply to Mounir Lamouri (:mounir) from comment #13)
> > So this has been pushed without a DOM peer review?
> 
> Ah, I apologize for this careless mistake. We don't really mean it :(

Sorry! Mounir! We are not aware of that because it didn't touch any public DOM API changes. So... it seems we also need to ask for a DOM-peer review for the directory changes regarding the DOM API codes. We'll watch out next time. Sorry again!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This is going to need a branch-specific patch for uplift.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> This is going to need a branch-specific patch for uplift.

We'll take care of it.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [b2g18]

Verified locally. Will push after last tbpl commit turns (nearly) all green.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [b2g18]

Re-gen with hg. Landing ...

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ac03e8749fa9

Comment 21

[Gene Lian (I already quit Mozilla)]

Added [NO_UPLIFT][RIL_INTERFACE] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out.

------------------------------
If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible:

Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643)
Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages
Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS
Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments.
Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741).
Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS
Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix)
Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix)
Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event
Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead().
Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete().
Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). 
Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Bug 845643 - B2G MMS: Save retrieved MMS into database.
Bug 792321 - Check max values of MMS parameters in sendRequest.
Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS
Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS
Bug 839436 - B2G MMS: make DB be able to save MMS messages.

Comment 22

[Gene Lian (I already quit Mozilla)]

Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.

Comment 23

[Michael Vines [:m1] [:evilmachines]]

Lets keep it in please, no need to churn more.