WebSMS: Update method "Delete" in WebSMS for receiving a array as input

RESOLVED FIXED in Firefox 23

Status

()

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

People

(Reporter: borjasalguero, Assigned: chucklee)

Tracking

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -

Firefox Tracking Flags

(blocking-kilimanjaro:-, blocking-b2g:leo+, blocking-basecamp:-, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(11 attachments, 22 obsolete attachments)

5.71 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
1.57 KB, patch
Details | Diff | Splinter Review
8.35 KB, patch
Details | Diff | Splinter Review
7.60 KB, patch
Details | Diff | Splinter Review
2.53 KB, patch
Details | Diff | Splinter Review
10.24 KB, patch
Details | Diff | Splinter Review
10.34 KB, patch
Details | Diff | Splinter Review
8.35 KB, patch
Details | Diff | Splinter Review
5.72 KB, patch
Details | Diff | Splinter Review
7.61 KB, patch
Details | Diff | Splinter Review
3.22 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.11 (KHTML, like Gecko) Chrome/20.0.1132.47 Safari/536.11

Steps to reproduce:

Delete an array of message IDs increase performance against deleting one by one. It would be interesting have same feature for "Mark as read/unread"
Blocks: 674725
OS: Mac OS X → All
Hardware: x86 → All
Status: UNCONFIRMED → NEW
blocking-basecamp: --- → ?
Ever confirmed: true
(In reply to Borja Salguero from comment #0)
> It would be interesting have same feature for "Mark as read/unread"

Can you open another bug for that?
(Reporter)

Comment 2

6 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=771463
Nice to have, but doesn't block the release.
blocking-basecamp: ? → -
blocking-kilimanjaro: --- → -
(Reporter)

Updated

5 years ago
Blocks: 806592
Summary: Update method "Delete" in WebSMS for receiving a array as input → WebSMS: Update method "Delete" in WebSMS for receiving a array as input
Depends on SmsFilter refactoring code in bug 838467.
Depends on: 838467
Julien, do you want this to accept a SmsFilter as well?
Flags: needinfo?(felash)
No.

Basically, what we need is :
* delete one or several message ids
* delete one or several thread ids

in one call.

AFAIK SmsFilter does accept neither an array of ids neither nor an array of thread ids.
Flags: needinfo?(felash)
The current delete operation is very slow, even when there is only one sms to delete. I don't know if that's the gaia code's fault or the gecko's code fault, but clearly making this API better won't hurt.
Blocks: 860607
asking leo+ here because bug 860607 is leo+.
blocking-b2g: --- → leo?

Comment 9

5 years ago
Blocks a blocker, so leo+ for now.
blocking-b2g: leo? → leo+

Updated

5 years ago
Assignee: nobody → chulee
Created attachment 740639 [details] [diff] [review]
0001. Accept array as argument for delete() API.

Handle array object in delete() API.

delete() accepts jsval as argument, so no API change is required.
Attachment #740639 - Flags: review?(vyang)
Created attachment 740640 [details] [diff] [review]
0002. Change IPDL to support delete() by array.

Change argument type of delete message in IPDL to array, and make corresponding modification.
Attachment #740640 - Flags: review?(vyang)
Created attachment 740641 [details] [diff] [review]
0003. delete message in DB by array.

Delete every message contains in array.
Attachment #740641 - Flags: review?(vyang)
Created attachment 740642 [details] [diff] [review]
0004. Test case for deleting multiple SMS. (WIP)

Test case is terminated with error:
JavascriptException: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.isDeadWrapper]

Still trying to find out why.
Attachment #740642 - Flags: feedback?(vyang)
Created attachment 740698 [details] [diff] [review]
Simple Gaia Patch for testing

Let SMS App delete SMS by array
Comment on attachment 740639 [details] [diff] [review]
0001. Accept array as argument for delete() API.

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +217,5 @@
> +  nsCOMPtr<nsIDOMMozSmsMessage> smsMessage =
> +    do_QueryInterface(nsContentUtils::XPConnect()->GetNativeOfWrapper(aCx, &aMessage.toObject()));
> +  if (smsMessage) {
> +    smsMessage->GetId(&aId);
> +  } else {

Bail out early please.
Attachment #740639 - Flags: review?(vyang) → review+
Comment on attachment 740641 [details] [diff] [review]
0003. delete message in DB by array.

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

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +12,5 @@
>  
>  const RIL_MOBILEMESSAGEDATABASESERVICE_CONTRACTID =
>    "@mozilla.org/mobilemessage/rilmobilemessagedatabaseservice;1";
>  const RIL_MOBILEMESSAGEDATABASESERVICE_CID =
> +  Components.ID("{ea6f49ae-3a4c-47eb-a489-15578e634100}");

You don't need this :)

@@ +1310,5 @@
>        };
>  
> +      for (let i = 0; i < length; i++) {
> +        let messageId = messageIds[i];
> +        messageStore.get(messageId).onsuccess = function(event) {

We have had refactored this code piece in bug 853752. So you'll probably have to rebase on it :)

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.manifest
@@ +1,2 @@
> +component {ea6f49ae-3a4c-47eb-a489-15578e634100} MobileMessageDatabaseService.js
> +contract @mozilla.org/mobilemessage/rilmobilemessagedatabaseservice;1 {ea6f49ae-3a4c-47eb-a489-15578e634100}

Ditto.
Attachment #740641 - Flags: review?(vyang)
Created attachment 741641 [details] [diff] [review]
0001. Accept array as argument for delete() API. V1.1

Address comment 15
Attachment #741641 - Flags: review?(vyang)
Attachment #740639 - Attachment is obsolete: true
Created attachment 741642 [details] [diff] [review]
0003. delete message in DB by array. V1.1

Address comment 16 including rebase.
Attachment #740641 - Attachment is obsolete: true
Attachment #741642 - Flags: review?(vyang)
Comment on attachment 741641 [details] [diff] [review]
0001. Accept array as argument for delete() API. V1.1

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +211,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +MobileMessageManager::GetMessageId(AutoPushJSContext &aCx, const JS::Value &aMessage, int32_t &aId)

nit: Wrap long lines please.

@@ +217,5 @@
> +  nsCOMPtr<nsIDOMMozSmsMessage> smsMessage =
> +    do_QueryInterface(nsContentUtils::XPConnect()->GetNativeOfWrapper(aCx, &aMessage.toObject()));
> +  if (smsMessage) {
> +    smsMessage->GetId(&aId);
> +    return NS_OK;

nit: return smsMessage->GetId(&aId);

@@ +224,5 @@
> +  nsCOMPtr<nsIDOMMozMmsMessage> mmsMessage =
> +    do_QueryInterface(nsContentUtils::XPConnect()->GetNativeOfWrapper(aCx, &aMessage.toObject()));
> +  if (mmsMessage) {
> +    mmsMessage->GetId(&aId);
> +    return NS_OK;

ditto.

::: dom/mobilemessage/src/MobileMessageManager.h
@@ +43,5 @@
> +
> +  /**
> +   * Helper to get message ID from SMS/MMS Message object
> +   */
> +  nsresult GetMessageId(AutoPushJSContext &aCx, const JS::Value &aMessage, int32_t &aId);

nit: line wrap

::: dom/mobilemessage/src/SmsManager.cpp
@@ +234,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +SmsManager::GetSmsMessageId(AutoPushJSContext &aCx, const JS::Value &aSmsMessage, int32_t &aId)

ditto

@@ +242,5 @@
> +  NS_ENSURE_TRUE(message, NS_ERROR_INVALID_ARG);
> +
> +  message->GetId(&aId);
> +
> +  return NS_OK;

nit: return message->GetId(&aId);

::: dom/mobilemessage/src/SmsManager.h
@@ +44,5 @@
> +
> +  /**
> +   * Helper to get message ID from SMS Message object
> +   */
> +  nsresult GetSmsMessageId(AutoPushJSContext &aCx, const JS::Value &aSmsMessage, int32_t &aId);

nit: line wrap
Attachment #741641 - Flags: review?(vyang) → review+
Comment on attachment 740640 [details] [diff] [review]
0002. Change IPDL to support delete() by array.

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

Please fix ReplyMessageDelete as well.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +486,5 @@
> +
> +    nsAutoArrayPtr<int32_t> idArray(new int32_t[size]);
> +    for (uint32_t i = 0; i < size; i++) {
> +      idArray[i] = (int32_t)messageIds[i];
> +    }

can't we use nsTArray::Elements() instead?
Attachment #740640 - Flags: review?(vyang)
Comment on attachment 741642 [details] [diff] [review]
0003. delete message in DB by array. V1.1

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

The original semantics of message deletion notifications are:

  1) Either the message specified by messageId is not found in the database, or the message specified is successfully deleted, notify content with success and whether has the message been deleted;
  2) Notify content with error code otherwise.

So, when we're going to support deletion of multiple messages at once, we should still follow this semantics. That is, notify content with success only when all the messages specified by ID array are either not found or have been deleted successfully.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1377,5 @@
> +        let messageId = messageIds[i];
> +        messageStore.get(messageId).onsuccess = function(event) {
> +          let messageRecord = event.target.result;
> +          if (messageRecord) {
> +            if (DEBUG) debug("Deleting message id " + messageId);

messageId here is captured outside the callback. Since it's in a for-loop, it's probably assigned to another value at the time this callback is executed. Please use either |messageRecord.id| or |bind(null, messageId)| instead.
Attachment #741642 - Flags: review?(vyang)
Comment on attachment 740642 [details] [diff] [review]
0004. Test case for deleting multiple SMS. (WIP)

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

::: dom/mobilemessage/tests/marionette/test_massive_incoming_delete.js
@@ +10,5 @@
> +const RECEIVER = "15555215554"; // the emulator's number
> +
> +let sms = window.navigator.mozSms;
> +let msgText = "Mozilla Firefox OS!";
> +let SmsNumber = 100;

constant value names should be all upper case.

@@ +39,5 @@
> +
> +    verifySmsExists(incomingSms);
> +
> +    waitFor(simulateIncomingSms, function() {
> +      return (checkDone && emulatorReady);

This doesn't guarantee all emulator command transactions are completed before finish() is invoked. Considering you call |deleteAllSms()| in |simulateIncomingSms()|, no matter the last emulator command transaction is ended or not, further processing has already began. Please do as test_filter_mixed.js does -- check |pendingEmulatorCmdCount| in |cleanUp()|.

@@ +43,5 @@
> +      return (checkDone && emulatorReady);
> +    });
> +  };
> +
> +  simulateIncomingSms();

Please be task based. See test_filter_mixed.js.
Attachment #740642 - Flags: feedback?(vyang)
Created attachment 741762 [details] [diff] [review]
0001. Accept array as argument for delete() API. V1.2

Address comment 19.
Attachment #741641 - Attachment is obsolete: true
Created attachment 741764 [details] [diff] [review]
0002. Change IPDL to support delete() by array. V2

Address comment 20.
Mainly provide boolean array of message deleting status in onsuccess(). If delete only one message, the return status becomes boolean to provide API backward compatibility.
Attachment #740640 - Attachment is obsolete: true
Attachment #741764 - Flags: review?(vyang)
Created attachment 741767 [details] [diff] [review]
0003. delete message in DB by array. V2

Address comment 21, record and return delete status array wrt message id array if delete is success.
Bind delete function with index of message id to make sure every delete status is recorded in correct index.
Attachment #741642 - Attachment is obsolete: true
Attachment #741767 - Flags: review?(vyang)
Created attachment 741769 [details] [diff] [review]
Simple Gaia Patch for testing

Add Calculation of execution time of message delete API.
Attachment #740698 - Attachment is obsolete: true
Comment on attachment 741764 [details] [diff] [review]
0002. Change IPDL to support delete() by array. V2

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

::: dom/mobilemessage/src/MobileMessageCallback.cpp
@@ +135,5 @@
> +                  aDeleted[i] ? &jsValTrue : &jsValFalse);
> +  }
> +
> +  JS::Value deleteArray;
> +  deleteArray.setObjectOrNull(deleteArrayObj);

We'd better convert it to nsTArray and apply nsTArrayToJSArray() here, so that we don't have to write JS code ourselves.

::: dom/mobilemessage/src/ipc/SmsChild.cpp
@@ +177,5 @@
>      case MessageReply::TReplyGetMessageFail:
>        mReplyRequest->NotifyGetMessageFailed(aReply.get_ReplyGetMessageFail().error());
>        break;
> +    case MessageReply::TReplyMessageDelete: {
> +        nsTArray<bool> deletedResult(aReply.get_ReplyMessageDelete().deleted());

const InfallibleTArray<bool>& deleted = aReply.get_ReplyMessageDelete().deleted();

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +133,5 @@
>                               nsIMobileMessageCallback* aRequest)
>  {
> +  nsTArray<int> idArray(aSize);
> +
> +  idArray.AppendElements(aMessageIds, aSize);

DeleteMessageRequest data;
data.messageIds.AppendElements(aMessageIds, aSize);

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +482,5 @@
>      do_GetService(MOBILE_MESSAGE_DATABASE_SERVICE_CONTRACTID);
>    if (dbService) {
> +    nsTArray<int> messageIds(aRequest.messageIds());
> +
> +    rv = dbService->DeleteMessage(messageIds.Elements(), messageIds.Length(), this);

const InfallibleTArray<int32_t>& messageIds= aRequest.messageIds();
rv = dbService->DeleteMessage(messageIds.Elements(), messageIds.Length(), this);

@@ +589,5 @@
> +  nsTArray<bool> deleteResultArray(aSize);
> +
> +  deleteResultArray.AppendElements(aDeleted, aSize);
> +
> +  return SendReply(ReplyMessageDelete(deleteResultArray));

ReplyMessageDelete data;
data.deleted().AppendElements(aDeleted, aSize);
return SendReply(data);
Attachment #741764 - Flags: review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #27)
> Comment on attachment 741764 [details] [diff] [review]
> 0002. Change IPDL to support delete() by array. V2
> 
> Review of attachment 741764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/ipc/SmsParent.cpp
> @@ +482,5 @@
> >      do_GetService(MOBILE_MESSAGE_DATABASE_SERVICE_CONTRACTID);
> >    if (dbService) {
> > +    nsTArray<int> messageIds(aRequest.messageIds());
> > +
> > +    rv = dbService->DeleteMessage(messageIds.Elements(), messageIds.Length(), this);
> 
> const InfallibleTArray<int32_t>& messageIds= aRequest.messageIds();
> rv = dbService->DeleteMessage(messageIds.Elements(), messageIds.Length(),
> this);
> 

This will cause compiler error, also
InfallibleTArray<int32_t>& messageIds= aRequest.messageIds();
rv = dbService->DeleteMessage(messageIds.Elements(), messageIds.Length(), this);

If we wan't to prevent construct new object here, the only way seems to be using const_cast.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #27)
> Comment on attachment 741764 [details] [diff] [review]
> 0002. Change IPDL to support delete() by array. V2
> 
> Review of attachment 741764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/MobileMessageCallback.cpp
> @@ +135,5 @@
> > +                  aDeleted[i] ? &jsValTrue : &jsValFalse);
> > +  }
> > +
> > +  JS::Value deleteArray;
> > +  deleteArray.setObjectOrNull(deleteArrayObj);
> 
> We'd better convert it to nsTArray and apply nsTArrayToJSArray() here, so
> that we don't have to write JS code ourselves.

Based on [1], |nsTArrayToJSArray()| is designed for class supporting |QueryInterface()|.
In this case, the type in nsTArray should be |bool *| or |JS::Value *|, and neither one can be handled in |nsTArrayToJSArray()|.
I think we can't use |nsTArrayToJSArray()| here.

[1] https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/public/nsTArrayHelpers.h#27
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #22)
> Comment on attachment 740642 [details] [diff] [review]
> 0004. Test case for deleting multiple SMS. (WIP)
> 
> Review of attachment 740642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/tests/marionette/test_massive_incoming_delete.js
> @@ +39,5 @@
> > +
> > +    verifySmsExists(incomingSms);
> > +
> > +    waitFor(simulateIncomingSms, function() {
> > +      return (checkDone && emulatorReady);
> 
> This doesn't guarantee all emulator command transactions are completed
> before finish() is invoked. Considering you call |deleteAllSms()| in
> |simulateIncomingSms()|, no matter the last emulator command transaction is
> ended or not, further processing has already began. Please do as
> test_filter_mixed.js does -- check |pendingEmulatorCmdCount| in |cleanUp()|.
> 

|waitFor()| runs |simulateIncomingSms()| only if the function returns true, so mentioned situation won't happen because |deleteAllSms()| won't be executed unless last emulator command is done and set |emulatorReady| to true.

> @@ +43,5 @@
> > +      return (checkDone && emulatorReady);
> > +    });
> > +  };
> > +
> > +  simulateIncomingSms();
> 
> Please be task based. See test_filter_mixed.js.

I ran marionette test on test_filter_mixed.js with today's Mozilla central and Gaia master, it also breaks with error message:

JavascriptException: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.isDeadWrapper]
stacktrace:
TEST-UNEXPECTED-FAIL | test_filter_mixed.js | 	@resource://gre/modules/BrowserElementParent.jsm, line 204

and logcat:

E/GeckoConsole(   42): [JavaScript Error: "NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.isDeadWrapper]" {file: "resource://gre/modules/BrowserElementParent.jsm" line: 204}]
E/GeckoConsole(  155): [JavaScript Error: "infos is null" {file: "chrome://global/content/BrowserElementChild.js" line: 22}]
E/GeckoConsole(   42): [JavaScript Error: "NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.isDeadWrapper]" {file: "resource://gre/modules/BrowserElementParent.jsm" line: 204}]
E/GeckoConsole(  153): [JavaScript Error: "infos is null" {file: "chrome://global/content/BrowserElementChild.js" line: 22}]


But there's no such error running marionette test test_incoming_delete.js, which is the structure I used.

So I don't think change to task-based style can solve the error.
Created attachment 742290 [details] [diff] [review]
0002. Change IPDL to support delete() by array. V2.1

Address comment 27 to 29
Attachment #741764 - Attachment is obsolete: true
Attachment #742290 - Flags: review?(vyang)
Created attachment 742898 [details] [diff] [review]
Simple Gaia Patch for testing. V1.1

Use mozMobileMessage instead of mozSms as gaia updated.
Attachment #741769 - Attachment is obsolete: true
Created attachment 742901 [details] [diff] [review]
0004. Test case for deleting multiple SMS.

Rewrite test case into task-based form.
Attachment #742901 - Flags: review?(vyang)
Attachment #740642 - Attachment is obsolete: true
Comment on attachment 742290 [details] [diff] [review]
0002. Change IPDL to support delete() by array. V2.1

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

::: dom/mobilemessage/src/ipc/SmsChild.cpp
@@ +177,5 @@
>      case MessageReply::TReplyGetMessageFail:
>        mReplyRequest->NotifyGetMessageFailed(aReply.get_ReplyGetMessageFail().error());
>        break;
> +    case MessageReply::TReplyMessageDelete: {
> +        const InfallibleTArray<bool>& deletedResult(aReply.get_ReplyMessageDelete().deleted());

nit: please just use operator= because it's certainly not constructing a new object.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +480,5 @@
>  
>    nsCOMPtr<nsIMobileMessageDatabaseService> dbService =
>      do_GetService(MOBILE_MESSAGE_DATABASE_SERVICE_CONTRACTID);
>    if (dbService) {
> +    const InfallibleTArray<int32_t>& messageIds= aRequest.messageIds();

nit: space before '='
Attachment #742290 - Flags: review?(vyang) → review+
Created attachment 742978 [details] [diff] [review]
0002. Change IPDL to support delete() by array. V2.2

Fix nit per comment 34.
Attachment #742290 - Attachment is obsolete: true
Created attachment 745069 [details] [diff] [review]
0004. Test case for deleting multiple SMS. V1.1

Remove debug message
Attachment #742901 - Attachment is obsolete: true
Attachment #742901 - Flags: review?(vyang)
Attachment #745069 - Flags: review?(vyang)
Attachment #741767 - Flags: review?(vyang) → review+
Comment on attachment 745069 [details] [diff] [review]
0004. Test case for deleting multiple SMS. V1.1

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

::: dom/mobilemessage/tests/marionette/test_massive_incoming_delete.js
@@ +175,5 @@
> +    let deleteDone = Date.now();
> +    log("Delete " + SMS_NUMBER + " SMS takes " + (deleteDone - deleteStart) + " ms.");
> +    log("Received 'onsuccess' smsrequest event.");
> +    if(event.target.result){
> +      for (let i = 0; i < SmsList.length; i++ ) {

nit: spaces before left parenthesis & after right parenthesis.

@@ +197,5 @@
> +    return verifDeletedCount === SMS_NUMBER;
> +  });
> +});
> +
> +tasks.push(function cleanUp() {

Please include "// WARNING: All tasks should be pushed before this!!!" before the last task push. And still check pendingEmulatorCmdCount again as we usually do. This way, when somebody is going to add new tasks, they don't step into the pendingEmulatorCmdCount trap accidentally.
Attachment #745069 - Flags: review?(vyang) → review+
Whiteboard: [NO_UPLIFT]
Created attachment 745774 [details] [diff] [review]
0004. Test case for deleting multiple SMS. V1.2

Address comment 37.
Attachment #745069 - Attachment is obsolete: true
Try : https://tbpl.mozilla.org/?tree=Try&rev=6edce5f24ae0

Comment 40

5 years ago
Removing NO_UPLIFT, as no change required in commercial RIL.
Whiteboard: [NO_UPLIFT]
(In reply to Chuck Lee [:chucklee] from comment #39)
> Try : https://tbpl.mozilla.org/?tree=Try&rev=6edce5f24ae0

It's strange that try stock in building for almost two days, I decide to cancel it and run another one
https://tbpl.mozilla.org/?tree=Try&rev=cd466f6f1025
How can we move this one forward so that we can unblock Bug 860607?
Flags: needinfo?(chulee)
It's already r+ but platform build on try server doesn't progress except B2G, and I have no idea why.
Flags: needinfo?(chulee)
(In reply to Chuck Lee [:chucklee] from comment #43)
> It's already r+ but platform build on try server doesn't progress except
> B2G, and I have no idea why.

Actually you have to fix other backends like fallback & android as well.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #44)
> (In reply to Chuck Lee [:chucklee] from comment #43)
> > It's already r+ but platform build on try server doesn't progress except
> > B2G, and I have no idea why.
> 
> Actually you have to fix other backends like fallback & android as well.

Oh... I see.
I am working on it.
Created attachment 747337 [details] [diff] [review]
0005. Support API change on other platform

Pilot run build looks good https://tbpl.mozilla.org/?tree=Try&rev=92362eb6c515


Running full try : https://tbpl.mozilla.org/?tree=Try&rev=5c664d7e0880
Attachment #747337 - Flags: review?(vyang)
Comment on attachment 747337 [details] [diff] [review]
0005. Support API change on other platform

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

Thank you :)
Attachment #747337 - Flags: review?(vyang) → review+
Comment on attachment 747337 [details] [diff] [review]
0005. Support API change on other platform

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

::: dom/mobilemessage/src/android/MobileMessageDatabaseService.cpp
@@ +38,5 @@
> +    return NS_OK;
> +  }
> +
> +  for (uint32_t i = 0; i < aLength; i++) {
> +    AndroidBridge::Bridge()->DeleteMessage(aMessageIds[i], aRequest);

Sorry, please return error when there is more than one message id.
Created attachment 747736 [details] [diff] [review]
0005. Support API change on other platform. V1.1

Address comment 48.
Attachment #747337 - Attachment is obsolete: true
Keywords: checkin-needed
rebase needed?
Keywords: checkin-needed
Created attachment 747860 [details] [diff] [review]
0001. Accept array as argument for delete() API. V1.3

Rebase
Attachment #741762 - Attachment is obsolete: true
https://hg.mozilla.org/projects/birch/rev/f6d65d0d6c10
https://hg.mozilla.org/projects/birch/rev/9faf137cf39f
https://hg.mozilla.org/projects/birch/rev/669735383491
https://hg.mozilla.org/projects/birch/rev/90002330772e
https://hg.mozilla.org/projects/birch/rev/8581d14c264c
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/f6d65d0d6c10
https://hg.mozilla.org/mozilla-central/rev/9faf137cf39f
https://hg.mozilla.org/mozilla-central/rev/669735383491
https://hg.mozilla.org/mozilla-central/rev/90002330772e
https://hg.mozilla.org/mozilla-central/rev/8581d14c264c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Created attachment 748709 [details] [diff] [review]
(b2g18) 0001. Accept array as argument for delete() API.

Rebase
Created attachment 748710 [details] [diff] [review]
(b2g18) 0002. Change IPDL to support delete() by array.
Created attachment 748711 [details] [diff] [review]
(b2g18) 0003. delete message in DB by array.

Rebase
Created attachment 748712 [details] [diff] [review]
(b2g18) 0004. Test case for deleting multiple SMS.

Rebase
Created attachment 748713 [details] [diff] [review]
(b2g18) 0005. Support API change on other platform.

Rebase
It seems AutoPushJSContext is not yet supported in b2g18....
https://tbpl.mozilla.org/?tree=Try&rev=f6c00fcb403c
(In reply to Chuck Lee [:chucklee] from comment #59)
> It seems AutoPushJSContext is not yet supported in b2g18....
> https://tbpl.mozilla.org/?tree=Try&rev=f6c00fcb403c

Yeap! It used to be a pain for me, too. You have to generate a b2g18 specific patch that doesn't use that.
Blocks: 871905
Created attachment 749822 [details] [diff] [review]
(b2g18) 0004. Test case for deleting multiple SMS.

Rebase
Attachment #748712 - Attachment is obsolete: true
Attachment #748712 - Attachment is obsolete: false
Attachment #749822 - Attachment is obsolete: true
Created attachment 749824 [details] [diff] [review]
(b2g18) 0001. Accept array as argument for delete() API. V1.1

Fix compile error due to data type.
Attachment #748709 - Attachment is obsolete: true
Created attachment 749828 [details] [diff] [review]
(b2g18) 0002. Change IPDL to support delete() by array. V1.1

Fix compile error due to data type.
Attachment #748710 - Attachment is obsolete: true
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5ac7e59eba8a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9eabb003cc17
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f483c327f7f4
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2febfa48264e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d589291950c3
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → fixed
Backed out for Android build failures.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d06cfe7d67c2

https://tbpl.mozilla.org/php/getParsedLog.php?id=22980741&tree=Mozilla-B2g18
status-b2g18: fixed → affected
Keywords: branch-patch-needed
Created attachment 750225 [details] [diff] [review]
(b2g18) 0001. Accept array as argument for delete() API. V1.2

add a=leo+
Attachment #749824 - Attachment is obsolete: true
Created attachment 750226 [details] [diff] [review]
(b2g18) 0002. Change IPDL to support delete() by array. V1.2

add a=leo+
Attachment #749828 - Attachment is obsolete: true
Created attachment 750227 [details] [diff] [review]
(b2g18) 0003. delete message in DB by array.

add a=leo+
Attachment #748711 - Attachment is obsolete: true
Created attachment 750228 [details] [diff] [review]
(b2g18) 0004. Test case for deleting multiple SMS. V1.1

add a=leo+
Attachment #748712 - Attachment is obsolete: true
Created attachment 750229 [details] [diff] [review]
0005. Support API change on other platform. V1.2

Add a=leo+, and fix build error on try

The follow up for m-c is on bug 871905
Attachment #748713 - Attachment is obsolete: true
Try for b2g18 : https://tbpl.mozilla.org/?tree=Try&rev=11ddc1700b66
https://hg.mozilla.org/releases/mozilla-b2g18/rev/981b651165cd
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6fe91f16878a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/10dce1be7e29
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7bdeff545500
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9671c293d028
status-b2g18: affected → fixed
Keywords: branch-patch-needed

Updated

5 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.