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"

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

Nice to have, but doesn't block the release.

Depends on SmsFilter refactoring code in bug 838467.

Julien, do you want this to accept a SmsFilter as well?

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.

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.

asking leo+ here because bug 860607 is leo+.

Blocks a blocker, so leo+ for now.

Handle array object in delete() API. delete() accepts jsval as argument, so no API change is required.

Change argument type of delete message in IPDL to array, and make corresponding modification.

Delete every message contains in array.

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.

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.

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.

Address comment 15

Address comment 16 including rebase.

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

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?

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.

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.

Address comment 19.

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.

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.

Add Calculation of execution time of message delete API.

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

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

Address comment 27 to 29

Use mozMobileMessage instead of mozSms as gaia updated.

Rewrite test case into task-based form.

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 '='

Fix nit per comment 34.

Remove debug message

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.

Address comment 37.

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

Removing NO_UPLIFT, as no change required in commercial RIL.

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

It's already r+ but platform build on try server doesn't progress except B2G, and I have no idea why.

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

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

Comment on attachment 747337 [details] [diff] [review] 0005. Support API change on other platform Review of attachment 747337 [details] [diff] [review]: ----------------------------------------------------------------- Thank you :)

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.

Address comment 48.

rebase needed?

Rebase

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

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

Rebase

Rebase

Rebase

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.

Rebase

Fix compile error due to data type.

Fix compile error due to data type.

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

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

add a=leo+

add a=leo+

add a=leo+

add a=leo+

Add a=leo+, and fix build error on try The follow up for m-c is on bug 871905

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