:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 11 years ago

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;

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 11 years ago

I'd say just a bug in the API. The API was designed way before we had much experience on webidlification.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 11 years ago

It is about webidlification. WebIDL prevents this kinds of mistakes in APIs ;)

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 11 years ago

Well, sure.  The question remains: what API do we _want_ here?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 11 years ago

I believe attribute DOMString[]? numbers, and similar for other attributes.
But mounir should comment.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 11 years ago

Which of the three kinds of DOMString[]?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 11 years ago

I guess fixed length, readonly.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

In that case, let's use DOMStringList for now.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 10 • 11 years ago

I don't have much to add here. I think Mounir is the one who really reviewed the API.

	Mounir Lamouri (:mounir)
	Comment 11 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 11 years ago

>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[]?

	Mounir Lamouri (:mounir)
	Comment 13 • 11 years ago

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?

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 14 • 11 years ago

You mean filter.numbers !== filter.numbers? People don't like that, aiui

	Mounir Lamouri (:mounir)
	Comment 15 • 11 years ago

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

	Mounir Lamouri (:mounir)
	Comment 16 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 18 • 11 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.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 11 years ago

> except code generator doesn't create correct code for union types with DOMStringList.

Details?  I see none in bug 859764.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 20 • 11 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.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 11 years ago

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!

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 22 • 10 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().

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

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 10 years ago

Comment on attachment 8436942 [details] [diff] [review]
part 1/2: the conversion.

Tiny bit fewer reviewers is enough ;)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 26 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 10 years ago

Why do you need Date in this dictionary?  Seems like unrestricted double would do just as well, if not better..

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 28 • 10 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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 29 • 10 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

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

Full try with comment 26 addressed: https://tbpl.mozilla.org/?tree=Try&rev=724a9edc4e90

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

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

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

Address comment 26 -- be more descriptive in MobileMessageFilter comments.  Waiting for feedbacks for the data type of startDate & endDate.

	Gene Lian (I already quit Mozilla)
	Comment 33 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 34 • 10 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 really would prefer to not change APIs when webidlfying them.
API changes should happen in different bugs, if possible.

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 10 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 38 • 10 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 39 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 40 • 10 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 41 • 10 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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 42 • 10 years ago

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?)

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 43 • 10 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.

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

1) Rebase.
2) Revert the changes to timestamp types. Stick with what it is now.
3) Add exception handling after |ToJSValue()|.

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

Rebase & address review comment 33.

	Boris Zbarsky [:bzbarsky]
	Comment 46 • 10 years ago

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.

	Bobby Holley (:bholley)
	Comment 47 • 10 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 48 • 10 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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 49 • 10 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.

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

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 51 • 10 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?

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

MobileMessageFilterParameters::{start,end}Date have become DOMTimeStamp.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 53 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 54 • 10 years ago

Hmm, or does idl have a limitation to not accept AString arrays. Trying to recall.
(arrays are very rarely used with idl.)

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 55 • 10 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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 56 • 10 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 57 • 10 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++.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 58 • 10 years ago

I doubt that is illegal, but ok, let's use char16_t then.

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

Address review comment 53 -- declare variables right above being referenced.

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

Rebase after bug 1046026.

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

Preparing Gaia pull request for this change ...

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

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?

	Steve Chung [:steveck]
	Comment 63 • 10 years ago

Comment on attachment 8472795 [details]
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

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

(In reply to Steve Chung [:steveck] from comment #63)
> Comment on attachment 8472795 [details]
> 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.

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

Will merge if https://tbpl.mozilla.org/?rev=f399ff56d3b650c0784f10d87e5079d29a89ead7&tree=Gaia-Try turn green.

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

https://github.com/mozilla-b2g/gaia/commit/10e4b59fa2baba999e9d607acc244037c740fa55

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

1) Rebase after bug 1052052.
2) s/MobileMessageFilterParameters/MobileMessageFilter/

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

s/MobileMessageFilterParameters/MobileMessageFilter/

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

https://hg.mozilla.org/integration/b2g-inbound/rev/080926480632
https://hg.mozilla.org/integration/b2g-inbound/rev/b658d7efa3b4

	Nigel Babu [:nigelb]
	Comment 70 • 10 years ago

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?

	Nigel Babu [:nigelb]
	Comment 71 • 10 years ago

Backed out for build bustage

https://hg.mozilla.org/integration/b2g-inbound/rev/bd870278aaaf
https://hg.mozilla.org/integration/b2g-inbound/rev/bf890e2223d0

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 72 • 10 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.

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

Fix build bustage on Mac OS X. Full try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=17be0d21aed9

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

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8e5ea345e1b8

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

Rebase & update commit summary.

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

Update commit summary only.

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

Update "GeneratedJNIWrappers.cpp".

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

https://tbpl.mozilla.org/?tree=Try&rev=097b02e26352 or https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=097b02e26352

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 79 • 10 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.

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

https://hg.mozilla.org/integration/b2g-inbound/rev/74bfb70bcefd
https://hg.mozilla.org/integration/b2g-inbound/rev/843e68ec798d
https://hg.mozilla.org/integration/b2g-inbound/rev/1dd9d6ebb7a9

	Carsten Book [:Tomcat]
	Comment 81 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/74bfb70bcefd
https://hg.mozilla.org/mozilla-central/rev/843e68ec798d
https://hg.mozilla.org/mozilla-central/rev/1dd9d6ebb7a9

	Jean-Yves Perrier [:teoli]
	Comment 82 • 10 years ago

dev-doc-needed to be sure to check that our doc is still up-to-date.