WebSMS: Move SmsFilter to WebIDL dictionary

RESOLVED FIXED in 2.1 S3 (29aug)

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Ms2ger, Assigned: vicamo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

Trunk
2.1 S3 (29aug)
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:ril][p=3])

Attachments

(4 attachments, 15 obsolete attachments)

47 bytes, text/x-github-pull-request
steveck
: review+
Details | Review | Splinter Review
42.97 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
41.45 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
13.09 KB, patch
blassey
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 757084 [details] [diff] [review]
WIP

I tried to do this, but it's a pretty poorly designed API, so I hit

 1:11.15 WebIDL.WebIDLError: error: An attribute cannot be of a sequence type, MozSmsFilter.webidl line 9:2
 1:11.15   attribute sequence<DOMString>? numbers;
<shrug>.  Someone who designed/reviewed this API should explain why exactly returning a new object on each call to an attribute getter is desirable here...  unless it's just 
"that's all XPIDL will let us do"?

Needinfo on code author and reviewers, I guess.
Flags: needinfo?(mrbkap)
Flags: needinfo?(mounir)
Flags: needinfo?(bugs)
I'd say just a bug in the API. The API was designed way before we had much experience on webidlification.
Flags: needinfo?(bugs)
This isn't about the "webidlification" aspect.  This is about what the API _should_ be.  Once we figure that out, then we worry about how to express it in WebIDL.
It is about webidlification. WebIDL prevents this kinds of mistakes in APIs ;)
Well, sure.  The question remains: what API do we _want_ here?
I believe attribute DOMString[]? numbers, and similar for other attributes.
But mounir should comment.
Which of the three kinds of DOMString[]?
I guess fixed length, readonly.
In that case, let's use DOMStringList for now.
I don't have much to add here. I think Mounir is the one who really reviewed the API.
Flags: needinfo?(mrbkap)
(In reply to Boris Zbarsky (:bz) from comment #1)
> <shrug>.  Someone who designed/reviewed this API should explain why exactly
> returning a new object on each call to an attribute getter is desirable
> here...  unless it's just 
> "that's all XPIDL will let us do"?
> 
> Needinfo on code author and reviewers, I guess.

Returning a new object or not isn't something I thought about much. I guess that if we wanted to always return a new object, we should expect it to return like a passed by reference object which isn't what we want to do here.

So, intuitively, I would say that the best solution is to always return a new DOMString[] that is read-only and of fixed length.
Flags: needinfo?(mounir)
>So, intuitively, I would say that the best solution is to always return a new DOMString[] >that is read-only and of fixed length.

You mean always return the same DOMString[]?
Flags: needinfo?(mounir)
No, I meant a different DOMString[]. I mean not the same object but the same content. Would that make sense in WebIDL world and conventions?
Flags: needinfo?(mounir)
(Reporter)

Comment 14

5 years ago
You mean filter.numbers !== filter.numbers? People don't like that, aiui
(In reply to :Ms2ger from comment #14)
> You mean filter.numbers !== filter.numbers? People don't like that, aiui

The equal operation between filter.numbers would be like an equal operation between two arrays.

Though, if developers really don't like that, we should return always the same object but in that case, it would sound more convenient to me to have some kind of "live" object. Unless returning always the same object but not making it live is considered fine?
Also, FWIW, we shouldn't worry too much about this. This API is for certified applications only (aka built-in applications) and the standard is going in another direction. I am mostly asking those questions to understand what's the conventions.
> Would that make sense in WebIDL world and conventions?

No.  Returning a different object each time from a property access is an anti-pattern that WebIDL goes out of its way to make difficult.  For example, this:

  foo.bar.myExpando = "foo";
  alert(foo.bar.myExpando);

should alert "foo" as far as web developers are concerned.  If that's not what your thing does, it should be a method, not a property.
(Assignee)

Updated

5 years ago
Depends on: 859764
(Assignee)

Updated

5 years ago
Blocks: 859764
No longer depends on: 859764
Summary: Move SmsFilter to WebIDL → WebSMS: Move SmsFilter to WebIDL
(Assignee)

Comment 18

5 years ago
If I may, I'm trying to convert all MobileMessage interfaces to WebIDL in bug 859764.  It seems retype the |filter.numbers| to |(DOMString or DOMStringList)?| may fix above problems except code generator doesn't create correct code for union types with DOMStringList.  See more there.
No longer blocks: 859764
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 859764
> except code generator doesn't create correct code for union types with DOMStringList.

Details?  I see none in bug 859764.
Flags: needinfo?(vyang)
(Assignee)

Comment 20

5 years ago
Sorry for the late reply.  When we have an interface method that accepts a (DOMString or DOMStringList) and also returns a (DOMString or DOMStringList), the code generator gives two variables of the same name 'length' in the same scope, which is a compile time error.  I think the original problem could be simplified as DOMStringList alone.  Will file a bug with an example tomorrow, so I'll keep that ni flag.
Hmm.  I can't reproduce that problem with a function like so:

  (DOMString or DOMStringList) foo((DOMString or DOMStringList) arg);

(though I see a possible header include issue with it).  Please do file with your example IDL!
(Assignee)

Comment 22

4 years ago
Reopen to make some progress here.  Depending on bug 958782 because dom/webidl/MobileMessageManager.webidl is to be renamed to MozMobileMessageManager.webidl in that bug.

Convert MozSmsFilter into a WebIDL dictionary instead because we never pass a MozSmsFilter instance to content page.  MozSmsFilter is always used as an input parameter for MobileMessageManager::GetMessages().
Assignee: nobody → vyang
Status: RESOLVED → REOPENED
Depends on: 958782
Flags: needinfo?(vyang)
Resolution: DUPLICATE → ---
Summary: WebSMS: Move SmsFilter to WebIDL → WebSMS: Move SmsFilter to WebIDL dictionary
(Assignee)

Comment 23

4 years ago
Created attachment 8436942 [details] [diff] [review]
part 1/2: the conversion.

Ask for reviews from all participants in the thread.  Feel free to cancel it if you feel irrelevant.

1) rename MozSmsFilter to MobileMessageFilter.  This also solves bug 856971.

2) re-type SmsFilterData::startDate and endDate to double because the underlying data type of a Date object is really a double.  This helps encapsulate epoch 0 second correctly.

3) accept a MobileMessageFilter dictionary in nsIMobileMessageDatabaseService::CreateMessageCursor() as well.
Attachment #757084 - Attachment is obsolete: true
Attachment #8436942 - Flags: review?(mounir)
Attachment #8436942 - Flags: review?(bzbarsky)
Attachment #8436942 - Flags: review?(bugs)
Attachment #8436942 - Flags: review?(Ms2ger)
(Assignee)

Comment 24

4 years ago
Created attachment 8436943 [details] [diff] [review]
part 2/2: fix test cases : WIP
(Assignee)

Updated

4 years ago
Whiteboard: [ft:ril][p=3]
Target Milestone: --- → 2.0 S4 (20june)
Comment on attachment 8436942 [details] [diff] [review]
part 1/2: the conversion.

Tiny bit fewer reviewers is enough ;)
Attachment #8436942 - Flags: review?(mounir)
Attachment #8436942 - Flags: review?(bzbarsky)
Attachment #8436942 - Flags: review?(Ms2ger)
Comment on attachment 8436942 [details] [diff] [review]
part 1/2: the conversion.

>+  AutoSafeJSContext cx;
At some point AutoJSAPI should be used here, but that API is still a bit
developer hostile, at least in this kind of simple case.

Are the member variables of SmsFilterData automatically initialized?

>+dictionary MobileMessageFilter
>+{
>+  // A date that can return null.
>+  Date? startDate;
>+
>+  // A date that can return null.
>+  Date? endDate;

Not too useful comments. '?' already means the value can be null.
Perhaps comment what start/endDates are, and whether they are inclusive in the filter.


>+
>+  // An array of DOMString that can return null.
>+  sequence<DOMString>? numbers;
Again, not too useful comment. What is numbers about?


>+
>+  // A DOMString that can return and be set to "sent", "received" or null.
>+  DOMString? delivery;
Sounds like this should be an enum value, and the enum would then have "sent" and "received"

>+  // A read flag that can return and be set to a boolean or null.
>+  boolean? read;
boolean? already tells that the value can be a boolean value or null, so no need to
say that in the comment


>+
>+  // A thread id that can return and be set to a numeric value or null.
>+  unsigned long long? threadId;
similar here.


Could take another look with enum in the .webidl
Attachment #8436942 - Flags: review?(bugs) → review-
Why do you need Date in this dictionary?  Seems like unrestricted double would do just as well, if not better..
(Assignee)

Comment 28

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #27)
> Why do you need Date in this dictionary?  Seems like unrestricted double
> would do just as well, if not better..

I don't.  It's just what it originally was.  A pull request for Gaia as a corresponding change to the Gecko part has become a must because we're removing global interface MozSmsFilter now.  So I will buy everything you consider good or even better, and that's why I asked for all of your review in comment 23.  Just want to make sure everyone agrees with the new interface.
(Assignee)

Comment 29

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #27)
> Why do you need Date in this dictionary?  Seems like unrestricted double
> would do just as well, if not better..

What about DOMTimestamp? We use DOMTimestamp for message timestamps.  Maybe it will be a pretty nature move to do "getMessages({ startDate: message.timestamp });".

http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl#42
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 31

4 years ago
Created attachment 8437464 [details] [diff] [review]
part 2/2: fix test cases

@smaug, this touches |test_interfaces.html| so I need DOM peers' review.

@gene, for others, I just replace all MozSmsFilter instantiation with a plain js object.  "test_smsfilter.html" has been removed as well because MobileMessageFilter is now a automatically generated type and its validation belongs to WebIDL binding generator in my point of view.
Attachment #8436943 - Attachment is obsolete: true
Attachment #8437464 - Flags: review?(gene.lian)
Attachment #8437464 - Flags: review?(bugs)
(Assignee)

Comment 32

4 years ago
Created attachment 8437466 [details] [diff] [review]
part 1/2: the conversion : v2

Address comment 26 -- be more descriptive in MobileMessageFilter comments.  Waiting for feedbacks for the data type of startDate & endDate.
Attachment #8436942 - Attachment is obsolete: true
Comment on attachment 8437464 [details] [diff] [review]
part 2/2: fix test cases

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

Nice! r=gene

::: dom/mobilemessage/tests/marionette/test_filter_date_notfound.js
@@ +23,2 @@
>  
> +  let cursor = manager.getMessages();

I assume calling getMessages() without passing parameters will have the same effect as the original codes.

::: dom/mobilemessage/tests/marionette/test_filter_number.js
@@ +27,5 @@
>  
>  function checkMessage(aNeedle, aValidNumbers) {
>    log("  Verifying " + aNeedle);
>  
> +  return getMessages({numbers: [aNeedle]})

nit: add spaces around "{"/"}" to have the consistent coding style in all tests.
Attachment #8437464 - Flags: review?(gene.lian) → review+
(In reply to Boris Zbarsky [:bz] from comment #27)
> Why do you need Date in this dictionary?  Seems like unrestricted double
> would do just as well, if not better..

I really would prefer to not change APIs when webidlfying them.
API changes should happen in different bugs, if possible.
> I don't.  It's just what it originally was

OK.  So I suggest filing a followup to switch to a numeric type in the IDL here.

> I really would prefer to not change APIs when webidlfying them.

This patch is totally doing that.

For example, the old code treated an invalid date (as in new Date(NaN)) as 0 but the new code treats it as NaN, afaict.  And then adds some new code that does special things for NaN.

We should decide what we actually want the API to be.  I'm fine with a followup to get us there, but random/partial API changes without an end goal in mind don't seem right.
Flags: needinfo?(bzbarsky)
A few code comments also:

1)  If dict.ToObject() fails in DoRequest, you need to report the exception on the JSContext, not just leave it hanging there.

2)  If you want to treat missing and null the same way, just make null the default value in IDL, and then you don't need to worry about !WasPassed() in the C++.
> What about DOMTimestamp?

That would mostly match the old behavior (except would deterministically convert NaN Date to 0; the old code actually relied on undefined behavior in C++).

Again, the real question is what you want the API behavior to be.  That needs to be decided before writing the IDL, instead of randomly picking IDL and hence API behavior.
(In reply to Boris Zbarsky [:bz] from comment #35)

> This patch is totally doing that.
> 
> For example, the old code treated an invalid date (as in new Date(NaN)) as 0
> but the new code treats it as NaN, afaict.  And then adds some new code that
> does special things for NaN.

You're right. We should not add any special NaN handling in this bug.
(In reply to Boris Zbarsky [:bz] from comment #36)
> 1)  If dict.ToObject() fails in DoRequest, you need to report the exception
> on the JSContext, not just leave it hanging there.
If that is the case, I'd say ToObject is rather broken API and should be fixed.
People shouldn't be using ToObject anyway.  They should be using ToJSValue.  That said, that has the same caveat: exception handling is the caller's responsibility.
(Assignee)

Comment 41

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #35)
> > I don't.  It's just what it originally was
> 
> OK.  So I suggest filing a followup to switch to a numeric type in the IDL
> here.
> 
> > I really would prefer to not change APIs when webidlfying them.
> 
> This patch is totally doing that.
> 
> For example, the old code treated an invalid date (as in new Date(NaN)) as 0
> but the new code treats it as NaN, afaict.  And then adds some new code that
> does special things for NaN.
> 
> We should decide what we actually want the API to be.  I'm fine with a
> followup to get us there, but random/partial API changes without an end goal
> in mind don't seem right.

Passing |new Date(NaN)| was an undefined behaviour, so it should work as null. And |new Date(0)| should be acceptable as other normal date for me. So I changed underlying data type to double.
Comment on attachment 8437464 [details] [diff] [review]
part 2/2: fix test cases

r+ for the test_interfaces.

(But I wonder why you change test_getmessages.js so much. Does it really still test the same set of features?)
Attachment #8437464 - Flags: review?(bugs) → review+
(Assignee)

Comment 43

4 years ago
(In reply to Olli Pettay [:smaug] from comment #42)
> (But I wonder why you change test_getmessages.js so much. Does it really
> still test the same set of features?)

Yes, it still tests getMessages() twice with reverse={true,false}. We've created a lot utility functions using Promise, so you see many lines get removed, but you can still find the verifyFoundMsgs() remains mostly untouched.
(Assignee)

Comment 44

4 years ago
Created attachment 8445828 [details] [diff] [review]
part 1/2: the conversion : v3

1) Rebase.
2) Revert the changes to timestamp types. Stick with what it is now.
3) Add exception handling after |ToJSValue()|.
Attachment #8437466 - Attachment is obsolete: true
Attachment #8445828 - Flags: review?(bzbarsky)
(Assignee)

Comment 45

4 years ago
Created attachment 8445829 [details] [diff] [review]
part 2/2: fix test cases : v2

Rebase & address review comment 33.
Attachment #8437464 - Attachment is obsolete: true
Attachment #8445829 - Flags: review+
Comment on attachment 8445828 [details] [diff] [review]
part 1/2: the conversion : v3

>+    // Use SmsIPCService-only overload to skip wraping/unwraping aFilter again.

wrapping/unwapping

>+    AutoSafeJSContext cx;

This is deprecated, I thought.  Should check with Bobby what the right thing to do here is, both places.

>+      JS_ClearPendingException(cx);

Why, exactly?

>+  data.startDate() = startDate.IsNull() ? 0 : startDate.Value().TimeStamp();

Given this, why not make your IDL be:

  long startDate = 0;

and then here you'd just do:

  data.startData() = aFilter.mStartDate;

Similar for endDate.  Or does the callee expect to see actual JS Date objects here?  Generally speaking, Date is deprecated in IDL; we shouldn't be adding new instances of it if we can avoid it.

>+  data.threadId() = threadId.IsNull() ? 0 : threadId.Value();

And here, if you had:

  unsigned long long threadId = 0;

then you wouldn't need the IsNull() checking.

>+    if (eReadState_Unknown != filter.read()) {
>+      dict.mRead.SetValue(filter.read());
>+    }

This is depending on the exact numeric values in the enum; why does it need to do that?

>+  sequence<DOMString>? numbers = null;

This, and the other thing on this interface, seem to be using null when they just want "not passed".  Do we have actual consumers passing null and expecting non-throwing behavior?  If not, and if we think this will ever get standardized, we should not be allowing null here.
Attachment #8445828 - Flags: review?(bzbarsky)
Attachment #8445828 - Flags: review?(bobbyholley)
Attachment #8445828 - Flags: review+
Comment on attachment 8445828 [details] [diff] [review]
part 1/2: the conversion : v3

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +384,5 @@
> +    SmsIPCService* ipcDbService = static_cast<SmsIPCService*>(dbService.get());
> +    rv = ipcDbService->CreateMessageCursor(aFilter, aReverse, cursorCallback,
> +                                           getter_AddRefs(continueCallback));
> +  } else {
> +    AutoSafeJSContext cx;

Please use AutoJSAPI here, presumably initialized with the result of GetOwner().
Attachment #8445828 - Flags: review?(bobbyholley) → review-
(Assignee)

Comment 48

4 years ago
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #46)
> Comment on attachment 8445828 [details] [diff] [review]
> part 1/2: the conversion : v3
> 
> >+    // Use SmsIPCService-only overload to skip wraping/unwraping aFilter again.
> 
> wrapping/unwapping
> 
> >+    AutoSafeJSContext cx;
> 
> This is deprecated, I thought.  Should check with Bobby what the right thing
> to do here is, both places.
> 
> >+      JS_ClearPendingException(cx);
> 
> Why, exactly?

For all above, that's a result of a jsval-typed filter argument in nsIMobileMessageDatabaseService::createMessageCursor(). I'll remove them by a list of arguments of native types.

> >+  data.startDate() = startDate.IsNull() ? 0 : startDate.Value().TimeStamp();
> 
> Given this, why not make your IDL be:
> 
>   long startDate = 0;
> 
> and then here you'd just do:
> 
>   data.startData() = aFilter.mStartDate;
> 
> Similar for endDate.  Or does the callee expect to see actual JS Date
> objects here?  Generally speaking, Date is deprecated in IDL; we shouldn't
> be adding new instances of it if we can avoid it.

I've re-typed |startDate| and |endDate| as DOMTimeStamp in my WIP branch. And a DOMTimeStamp 0 is a valid value here, so I'll keep the default null rather than default 0.

> >+  data.threadId() = threadId.IsNull() ? 0 : threadId.Value();
> 
> And here, if you had:
> 
>   unsigned long long threadId = 0;
> 
> then you wouldn't need the IsNull() checking.
> 
> >+    if (eReadState_Unknown != filter.read()) {
> >+      dict.mRead.SetValue(filter.read());
> >+    }
> 
> This is depending on the exact numeric values in the enum; why does it need
> to do that?

Both addressed in WIP.

> >+  sequence<DOMString>? numbers = null;
> 
> This, and the other thing on this interface, seem to be using null when they
> just want "not passed".  Do we have actual consumers passing null and
> expecting non-throwing behavior?  If not, and if we think this will ever get
> standardized, we should not be allowing null here.

The original implementation allows setting as null here and takes it as a way to reset its previous value. A T? already implies a nullable attribute. A null default value is only for the convenience in the implementation. It's treated as not being passed in the first place.
(Assignee)

Comment 49

4 years ago
(In reply to Bobby Holley (:bholley) from comment #47)
> Comment on attachment 8445828 [details] [diff] [review]
> part 1/2: the conversion : v3
> 
> Review of attachment 8445828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/MobileMessageManager.cpp
> @@ +384,5 @@
> > +    SmsIPCService* ipcDbService = static_cast<SmsIPCService*>(dbService.get());
> > +    rv = ipcDbService->CreateMessageCursor(aFilter, aReverse, cursorCallback,
> > +                                           getter_AddRefs(continueCallback));
> > +  } else {
> > +    AutoSafeJSContext cx;
> 
> Please use AutoJSAPI here, presumably initialized with the result of
> GetOwner().

Thank you. I'll remove that jsval argument so that we don't need any JS API here.
(Assignee)

Comment 50

4 years ago
Created attachment 8468271 [details] [diff] [review]
part 1/2: the conversion : v4

1) There is no more jsval argument in nsIMobileMessageDatabaseService::CreateMessageCursor(), so no more JS API in need.

2) MobileMessageFilterParameters::{start,end}Date are now DOMTimeStamp-typed attributes. Numeric 0 is a valid value for both of them.

3) IPDL data type SmsFilterData was also modified to reflect the changes.
Attachment #8445828 - Attachment is obsolete: true
(Assignee)

Comment 51

4 years ago
Comment on attachment 8468271 [details] [diff] [review]
part 1/2: the conversion : v4

May I have your review since Boris is on vacation?
Attachment #8468271 - Flags: review?(bugs)
(Assignee)

Comment 52

4 years ago
Created attachment 8468273 [details] [diff] [review]
part 2/2: fix test cases : v3

MobileMessageFilterParameters::{start,end}Date have become DOMTimeStamp.
Attachment #8445829 - Attachment is obsolete: true
Attachment #8468273 - Flags: review+
Comment on attachment 8468271 [details] [diff] [review]
part 1/2: the conversion : v4

>-  nsICursorContinueCallback createMessageCursor(in nsIDOMMozSmsFilter filter,
>+  nsICursorContinueCallback createMessageCursor(in boolean hasStartDate,
>+                                                in unsigned long long startDate,
>+                                                in boolean hasEndDate,
>+                                                in unsigned long long endDate,
>+                                                [array, size_is(numbersCount)] in wstring numbers,
>+                                                in uint32_t numbersCount,
It is not clear to me why we want to pass wstring array here. Why not AString array?
Try to avoid manual char(16_t) arrays whenever possible.
Please explain and ask review again, or new patch.



>+  bool hasStartDate, hasEndDate, hasRead, read = false;
>+  uint64_t startDate = 0, endDate = 0, threadId = 0;
Please define variables when you use them.
Attachment #8468271 - Flags: review?(bugs) → review-
Hmm, or does idl have a limitation to not accept AString arrays. Trying to recall.
(arrays are very rarely used with idl.)
(Assignee)

Comment 55

4 years ago
(In reply to Olli Pettay [:smaug] from comment #54)
> Hmm, or does idl have a limitation to not accept AString arrays. Trying to
> recall.
> (arrays are very rarely used with idl.)

`find . -type f -name \*.idl | xargs cat | grep array.\*in.\*ring` reveals only "wstring" and "string" is ever used in similar cases. If I use "string", which maps to char**, then I have to convert that in MozMobileMessageManager::GetMessages() before passing that array into nsIMobileMessageDatabaseService::CreateMessageCursor(). That's one-time extra work and I'd like to avoid that if possible.

May I know what's the reason we have to avoid manual char(16_t) arrays?
Flags: needinfo?(bugs)
I didn't suggest string, but AString.

Using raw types are just error prone, and if the idl interface was converted to webidl at some point, it would anyway use an array of ns*Strings
Flags: needinfo?(bugs)
(Assignee)

Comment 57

4 years ago
(In reply to Olli Pettay [:smaug] from comment #56)
> I didn't suggest string, but AString.
> 
> Using raw types are just error prone, and if the idl interface was converted
> to webidl at some point, it would anyway use an array of ns*Strings

XPIDL translates AString/DOMString to nsAString, and you have the interface:

  void Foo(..., const nsAString& *numbers, uint32_t numbersCount, ...)

It follows you have to create an array of reference to objects, and that's illegal in C++.
Flags: needinfo?(bugs)
I doubt that is illegal, but ok, let's use char16_t then.
Flags: needinfo?(bugs)
(Assignee)

Comment 59

4 years ago
Created attachment 8471283 [details] [diff] [review]
part 1/2: the conversion : v5

Address review comment 53 -- declare variables right above being referenced.
Attachment #8468271 - Attachment is obsolete: true
Attachment #8471283 - Flags: review?(bugs)
(Assignee)

Comment 60

4 years ago
Created attachment 8471284 [details] [diff] [review]
part 2/2: fix test cases : v4

Rebase after bug 1046026.
Attachment #8468273 - Attachment is obsolete: true
Attachment #8471284 - Flags: review+
Attachment #8471283 - Flags: review?(bugs) → review+
(Assignee)

Comment 61

4 years ago
Preparing Gaia pull request for this change ...
(Assignee)

Comment 62

4 years ago
Created attachment 8472795 [details] [review]
Github pull request for Gaia

This pull request remove all MozSmsFilter usages except on in message_manager.js for backward compatibility. Since the Gecko patches remove symbol MozSmsFilter from window, this Gaia pull request has to be landed before Gecko ones. May I have your review?
Attachment #8472795 - Flags: review?(schung)
Comment on attachment 8472795 [details] [review]
Github pull request for Gaia

Patch works great! Thanks.

However I noticed that you mention that replacing 'MozSmsFilter' with 'MobileMessageFilter',it looks like simply a js object in IDL. Or we will have another type of filter object in the future? Sorry for the stupid question :p
Attachment #8472795 - Flags: review?(schung) → review+
(Assignee)

Comment 64

4 years ago
(In reply to Steve Chung [:steveck] from comment #63)
> Comment on attachment 8472795 [details] [review]
> Github pull request for Gaia
> 
> Patch works great! Thanks.
> 
> However I noticed that you mention that replacing 'MozSmsFilter' with
> 'MobileMessageFilter',it looks like simply a js object in IDL. Or we will
> have another type of filter object in the future? Sorry for the stupid
> question :p

No, there won't be other interface, hopefully.
(Assignee)

Comment 67

4 years ago
Created attachment 8474315 [details] [diff] [review]
part 1/2: the conversion : v6

1) Rebase after bug 1052052.
2) s/MobileMessageFilterParameters/MobileMessageFilter/
Attachment #8471283 - Attachment is obsolete: true
Attachment #8474315 - Flags: review+
(Assignee)

Comment 68

4 years ago
Created attachment 8474316 [details] [diff] [review]
part 2/2: fix test cases : v5

s/MobileMessageFilterParameters/MobileMessageFilter/
Attachment #8471284 - Attachment is obsolete: true
Attachment #8474316 - Flags: review+
(Assignee)

Updated

4 years ago
Target Milestone: 2.0 S4 (20june) → 2.1 S3 (29aug)
Hello, you seem to have broken the builds with this push. Can you push a fix in-place or would you like me to backout?
(Assignee)

Comment 72

4 years ago
(In reply to Nigel Babu [:nigelb] from comment #71)
> Backed out for build bustage
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/bd870278aaaf
> https://hg.mozilla.org/integration/b2g-inbound/rev/bf890e2223d0

Need an explicit |static_cast<uint32_t>(...)| in MobileMessageManager::GetMessages() on platforms other that B2G emulator. Probably other places.

Will only re-land after a full try. Sorry for the inconveniences.
(Assignee)

Comment 73

4 years ago
Created attachment 8474349 [details] [diff] [review]
part 1/2: the conversion : v7

Fix build bustage on Mac OS X. Full try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=17be0d21aed9
Attachment #8474315 - Attachment is obsolete: true
Attachment #8474349 - Flags: review+
(Assignee)

Comment 75

4 years ago
Created attachment 8479604 [details] [diff] [review]
part 1/3: the conversion : v8

Rebase & update commit summary.
Attachment #8474349 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8479604 - Attachment description: part 1/2: the conversion : v8 → part 1/3: the conversion : v8
(Assignee)

Comment 76

4 years ago
Created attachment 8479606 [details] [diff] [review]
part 2/3: fix test cases : v6

Update commit summary only.
Attachment #8474316 - Attachment is obsolete: true
Attachment #8479606 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8479604 - Flags: review+
(Assignee)

Comment 77

4 years ago
Created attachment 8479608 [details] [diff] [review]
part 3/3: fix Fennec build bustage : v2

Update "GeneratedJNIWrappers.cpp".
Attachment #8474501 - Attachment is obsolete: true
(Assignee)

Comment 79

4 years ago
Comment on attachment 8479608 [details] [diff] [review]
part 3/3: fix Fennec build bustage : v2

This patch updates argument list of GeckoSmsManager::createMessageList only.
Attachment #8479608 - Flags: review?(blassey.bugs)
Attachment #8479608 - Flags: review?(blassey.bugs) → review+
dev-doc-needed to be sure to check that our doc is still up-to-date.
Keywords: dev-doc-needed
Depends on: 1060865
You need to log in before you can comment on or make changes to this bug.