Closed Bug 831683 Opened 7 years ago Closed 7 years ago

B2G SMS & B2G MMS: make SMS database more generic for MMS

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g leo+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

(Whiteboard: [RIL_INTERFACE] )

Attachments

(4 files, 8 obsolete files)

83.94 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
25.71 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
84.08 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
25.77 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
We need to rename every single line for the SMS database codes like:
  
  s/Sms/MobileMessage

and reallocate all the related files into a more generic folder from:

  dom/sms/interfaces/nsISmsDatabaseService.idl
  dom/sms/src/ril/SmsDatabaseService.js

to:

  dom/mobilemessage/interfaces/nsIMobileMessageDatabaseService.idl
  dom/mobilemessage/src/ril/MobileMessageDatabaseService.js

This is a big work and needs to be merged fast after reviewed, since lots of engineers are working on the SMS database things and it's easy to produce conflicts.

After this is done, at bug 811252 we need to further redesign the database APIs for MMS-specific functions (also keep it backward-compatible to SMS at the same time).

Cc'ing some guys used to be involved in the SMS database design.
Is it really worth the hassle to move/rename all this code?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> Is it really worth the hassle to move/rename all this code?

Honestly, I don't have strong opinion on that. After discussing with Vicamo, we're just hoping to make the naming things and architecture more generic before we really start to include the MMS database APIs. Indeed, It's a bit dangerous to rename/move all the codes since our SMS database APIs have already been there for a long time and used in lots of place.

Another happy solution might be providing another |nsIMmsDatabaseService| for MMS database and all its API implementation will be redirected to (or constructed by) the current |nsISmsDatabaseService| APIs. Or even directly use |nsISmsDatabaseService| in MMS if we can tolerate to see the SMS keyword in the MMS codes.

Do others have any preferences? I'm still open to doing this or not.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> Is it really worth the hassle to move/rename all this code?

For me, yes. If there will be only one mobilemessage API in the end as defined in bug 760065, then mobilemessage/ should be its final home and MobileMessageDatabaseService should be its final name.

If your concern is mostly based on naming differences from b2g18 or other stable branches, it will be something we can't really afford all the time. More and more breaking changes are going to be landed in m-c and MobileMessage is no exception.
Attachment #707563 - Flags: feedback?(vyang)
(In reply to Gene Lian [:gene] from comment #5)
> Created attachment 707563 [details] [diff] [review]
> Part 2, create dom/mobilemessage to put DB codes

Btw, the Bugzilla Diff reviewer cannot parse and show the renaming change very well like:

diff --git a/dom/sms/interfaces/nsISmsDatabaseService.idl b/dom/mobilemessage/interfaces/nsIMobileMessageDatabaseService.idl
rename from dom/sms/interfaces/nsISmsDatabaseService.idl
rename to dom/mobilemessage/interfaces/nsIMobileMessageDatabaseService.idl
diff --git a/dom/sms/interfaces/nsIRilSmsDatabaseService.idl b/dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
rename from dom/sms/interfaces/nsIRilSmsDatabaseService.idl
rename to dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl

Have to check it in the raw diff file.
Attachment #707562 - Flags: feedback?(vyang) → feedback+
Comment on attachment 707563 [details] [diff] [review]
Part 2, create dom/mobilemessage to put DB codes

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

Gene, can we also move/rename SmsDatabaseService.*?

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

Do we really need it now?
Attachment #707563 - Flags: feedback?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> Comment on attachment 707563 [details] [diff] [review]
> Part 2, create dom/mobilemessage to put DB codes
> 
> Review of attachment 707563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Gene, can we also move/rename SmsDatabaseService.*?

I don't quite understand. I've already moved/renamed all the files. Maybe comment #6 makes you missed the changes. ;)

> ::: dom/dom-config.mk
> @@ +12,5 @@
> >    dom/media \
> >    dom/network/src \
> >    dom/settings \
> >    dom/phonenumberutils \
> > +  dom/mobilemessage/src \
> 
> Do we really need it now?

Why not? All the SmsDatabaseService.* have been moved to dom/mobilemessage/src (into the sub-catogories android/fallback/ril respectively) and renamed as MobileMessageDatabaseService.*. I can talk to you in person later. ;)
(In reply to Gene Lian [:gene] from comment #8)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> > Gene, can we also move/rename SmsDatabaseService.*?
> 
> I don't quite understand. I've already moved/renamed all the
> files. Maybe comment #6 makes you missed the changes. ;)

Ah, it does. :)

> > ::: dom/dom-config.mk
> > @@ +12,5 @@
> > >    dom/media \
> > >    dom/network/src \
> > >    dom/settings \
> > >    dom/phonenumberutils \
> > > +  dom/mobilemessage/src \
> > 
> > Do we really need it now?
> 
> Why not? All the SmsDatabaseService.* have been moved to
> dom/mobilemessage/src (into the sub-catogories
> android/fallback/ril respectively) and renamed as
> MobileMessageDatabaseService.*. I can talk to you in person
> later. ;)

Just don't pollute dom-config if not necessary. See bug 778093 comment #33.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #9)
> Just don't pollute dom-config if not necessary. See bug 778093 comment #33.

Indeed, we don't this. :) Thanks for the feedback. I'll upload formal patches for your reviews after having more tests to make sure I wouldn't mess up the existing SMS functions.
Attachment #707562 - Attachment is obsolete: true
Attachment #708475 - Flags: review?(vyang)
Attachment #707563 - Attachment is obsolete: true
Attachment #708477 - Flags: review?(vyang)
Fixed compiling errors on android/fallback plarforms.
Attachment #708477 - Attachment is obsolete: true
Attachment #708477 - Flags: review?(vyang)
Attachment #708503 - Flags: review?(vyang)
Wrong patch. Sorry for the noise.
Attachment #708503 - Attachment is obsolete: true
Attachment #708503 - Flags: review?(vyang)
Attachment #708509 - Flags: review?(vyang)
Summary: B2G SMS & B2G MMS: Reallocate/Rename SMS database to make it generic for MMS → B2G SMS & B2G MMS: make SMS database more generic for MMS
Comment on attachment 708509 [details] [diff] [review]
Part 2, create dom/mobilemessage to put DB codes, V2.1

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

::: dom/mobilemessage/src/Makefile.in
@@ +62,5 @@
> +  $(NULL)
> +
> +CPPSRCS += \
> +  MobileMessageDatabaseService.cpp \
> +  $(NULL)  

trailing space

::: dom/sms/src/android/SmsDatabaseService.cpp
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/dom/sms/SmsFilter.h"

Please add an additional entry in LOCAL_INCLUDES in Makefile instead.

@@ +8,3 @@
>  #include "AndroidBridge.h"
>  
> +using namespace mozilla::dom::sms;

As :mounir suggested, we should have mozilla::dom only, but this can be another follow-up.

::: dom/sms/src/SmsServicesFactory.cpp
@@ +7,5 @@
>  #include "nsXULAppAPI.h"
>  #include "SmsService.h"
>  #include "SmsIPCService.h"
>  #ifndef MOZ_B2G_RIL
> +#include "mozilla/dom/mobilemessage/MobileMessageDatabaseService.h"

ditto.
Attachment #708509 - Flags: review?(vyang) → review+
Attachment #708475 - Flags: review?(vyang) → review+
Rebased. r=vicamo
Attachment #708475 - Attachment is obsolete: true
Attachment #710130 - Flags: review+
Rebased. r=vicamo
Attachment #708509 - Attachment is obsolete: true
Attachment #710131 - Flags: review+
Rebased. r=vicamo
Attachment #710131 - Attachment is obsolete: true
Attachment #710141 - Flags: review+
Rebased. r=vicamo

Sorry for all the noises...
Attachment #710141 - Attachment is obsolete: true
Attachment #710146 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/97a64853f10c
https://hg.mozilla.org/mozilla-central/rev/1eab3e0d7f53
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
This bug relates to MMS features and needs to be tagged as leo+ so that we can uplift it into the b2g-18 branch.
blocking-b2g: --- → leo?
Leo triage: leo+ for MMS blockers
blocking-b2g: leo? → leo+
Blocks: 839352
This is a b2g-18-specific patch for part 1.
Attachment #719842 - Flags: review+
This is a b2g-18-specific patch for part 2.
Attachment #719844 - Flags: review+
Whiteboard: [RIL_INTERFACE]
Flags: in-moztrap-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.