Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: philikon, Assigned: vicamo)

Tracking

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

Firefox Tracking Flags

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

Details

Attachments

(3 attachments, 11 obsolete attachments)

5.63 KB, patch
vicamo
: review+
vicamo
: superreview+
Details | Diff | Splinter Review
16.51 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
40.52 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
As part of integrating WebSMS and WebMMS, it would be good to harmonize the request implementations that we use in the new DOM device APIs and refactor WebSMS to use DOMRequest.

Functionally this should not change the WebSMS at all. Only the spelling will change. `request.result` will be the canonical way to access a request's result (instead of the various `event.message`, `event.deleted`, etc.)
Assignee: nobody → htsai
WebSMS is already using DOMRequest, more precisely SmsDOMRequest IIRC. So, you can do that but there will be no change at all except code cleanup. event.message will not be removed, this is completely orthogonal with DOMRequest objects.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> WebSMS is already using DOMRequest, more precisely SmsDOMRequest IIRC.

Well, it's not an nsIDOMDOMRequest.

> So, you can do that but there will be no change at all except code cleanup.

That, and interface harmonization.

> event.message will not be removed, this is completely orthogonal with
> DOMRequest objects.

Good point! We don't have to change the event object.
I wouldn't waste too much time on that but yes, we could have those request objects be nsIDOMDOMRequest...
Version: unspecified → Trunk
Posted patch Patch: work in progress (obsolete) — Splinter Review
Work in progress: 
I've re-factored WebSMS by using DOMRequest. The sms application can launch a request and receive the DOM request result. However, the current code isn't stable that "segmentation fault" occurs when we repeatedly read messages, leave application running for a longer time, etc. 

Below is the gdb backtrace. I wonder the fault is caused by "WrapNative" in SmsManager.cpp. I am still working on investigating this and debugging. Your feedbacks and comments are very welcome as always. Thanks :)

--------------------------------------------------------------
SmsDatabaseService: Retrieving object store sms
SmsDatabaseService: Fetching message 2
SmsDatabaseService: Transaction [object IDBTransaction] completed.
SmsDatabaseService: gSmsDomReqManager.notifyCreateMessageList No.: 5 requestId: 0
SmsDatabaseService: Getting next message in list 5
SmsDatabaseService: Reached the end of the list!
SmsDatabaseService: gSmsDomReqManager.notifyNoMessageInList: 0

Program received signal SIGSEGV, Segmentation fault.
0x4567c3d0 in ?? ()
(gdb) bt
#0  0x4567c3d0 in ?? ()
Cannot access memory at address 0x1
#1  0x827fbc4c in CallQueryInterface<nsISupports, nsWrapperCache> (aSource=0x40ae7900, aDestination=0xbed0d684) at ../../../dist/include/nsISupportsUtils.h:149
#2  0x827fe5ee in XPCWrappedNative::FlatJSObjectFinalized (this=0x41546280) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/xpconnect/src/XPCWrappedNative.cpp:1352
#3  0x82802d66 in WrappedNativeFinalize (fop=0xbed0d758, obj=0x41444c80, helperType=WN_NOHELPER) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:617
#4  0x82802d86 in XPC_WN_NoHelper_Finalize (fop=0x40ae7900, obj=0x83162994) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:623
#5  0x82d0a582 in finalize (this=<optimized out>, fop=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsobjinlines.h:233
#6  finalize<JSObject> (fop=<optimized out>, this=<optimized out>, thingSize=<optimized out>, thingKind=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:296
#7  FinalizeTypedArenas<JSObject> (thingKind=<optimized out>, al=<optimized out>, fop=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:343
#8  js::gc::FinalizeArenas (fop=0xbed0d758, al=<optimized out>, thingKind=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:383
#9  0x82d0cac0 in finalizeNow (thingKind=<optimized out>, fop=<optimized out>, this=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:1457
#10 finalizeObjects (fop=<optimized out>, this=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:1559
#11 SweepPhase (startBackgroundSweep=<optimized out>, gckind=<optimized out>, rt=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:3274
#12 GCCycle (rt=0x40a01000, incremental=<optimized out>, budget=<optimized out>, gckind=js::GC_NORMAL) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:3706
#13 0x82d0d3d2 in Collect (rt=0x40a01000, incremental=true, budget=<optimized out>, gckind=js::GC_NORMAL, reason=js::gcreason::PAGE_HIDE) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:3802
#14 0x82d0d54e in js::GCSlice (rt=0x40ae7900, gckind=<optimized out>, reason=js::gcreason::PAGE_HIDE) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsgc.cpp:3832
#15 0x82d02058 in js::IncrementalGC (rt=0x40ae7900, reason=3201357444) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/js/src/jsfriendapi.cpp:149
#16 0x82600992 in nsJSContext::GarbageCollectNow (aReason=js::gcreason::PAGE_HIDE, aGckind=0, aGlobal=false) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/dom/base/nsJSEnvironment.cpp:2968
#17 0x82600a2a in GCTimerFired (aTimer=<optimized out>, aClosure=0x11) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/dom/base/nsJSEnvironment.cpp:3140
#18 0x82abb0c2 in nsTimerImpl::Fire (this=0x415b4fd0) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/xpcom/threads/nsTimerImpl.cpp:473
#19 0x82abb1a4 in nsTimerEvent::Run (this=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/xpcom/threads/nsTimerImpl.cpp:556
#20 0x82ab8ebe in nsThread::ProcessNextEvent (this=0x40212460, mayWait=<optimized out>, result=0xbed0d89f) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/xpcom/threads/nsThread.cpp:624
#21 0x82a953f2 in NS_ProcessNextEvent_P (thread=0x40ae7900, mayWait=true) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/objdir-prof-gonk/xpcom/build/nsThreadUtils.cpp:213
#22 0x82a13938 in mozilla::ipc::MessagePump::Run (this=0x4020f1f0, aDelegate=0x4021a0e0) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/ipc/glue/MessagePump.cpp:113
#23 0x82ade68a in MessageLoop::RunInternal (this=0x83162994) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/ipc/chromium/src/base/message_loop.cc:208
#24 0x82ade6fa in RunHandler (this=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/ipc/chromium/src/base/message_loop.cc:201
#25 MessageLoop::Run (this=0x40ae7900) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/ipc/chromium/src/base/message_loop.cc:175
#26 0x82992c20 in nsBaseAppShell::Run (this=0x40aa0400) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/widget/xpwidgets/nsBaseAppShell.cpp:163
#27 0x828b0fe2 in nsAppStartup::Run (this=0x40a8c5e0) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/toolkit/components/startup/nsAppStartup.cpp:256
#28 0x8225e944 in XREMain::XRE_mainRun (this=0xbed0da84) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/toolkit/xre/nsAppRunner.cpp:3786
#29 0x82261474 in XREMain::XRE_main (this=0xbed0da84, argc=<optimized out>, argv=0xbed0fc74, aAppData=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/toolkit/xre/nsAppRunner.cpp:3863
#30 0x822615fa in XRE_main (argc=1, argv=0xbed0fc74, aAppData=0xa16c, aFlags=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/toolkit/xre/nsAppRunner.cpp:3939
#31 0x000088d4 in do_main (argv=<optimized out>, argc=<optimized out>) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/b2g/app/nsBrowserApp.cpp:153
#32 main (argc=1, argv=0xbed0fc74) at /home/hsinyi/WorkSpace/mozilla/B2G/gecko/b2g/app/nsBrowserApp.cpp:236
Posted patch Patch: work in progress (obsolete) — Splinter Review
WIP: variable renamed
Attachment #628606 - Attachment is obsolete: true
Comment on attachment 628607 [details] [diff] [review]
Patch: work in progress

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

This change seems really huge and *far* from being important. Wasting time on that in that right now doesn't look like a good idea.

A very simple change instead would be to have nsIMozSmsRequest inheriting from nsIDOMDOMRequest.
Depends on: 760011
Is there any reason to be urged to have that bug fixed if bug 760011 and bug 708546 are fixed? Both have very simple patches ready to be reviewed.

IMO, if we only want to remove the code of SmsRequest, we could probably wait for a less timely stressed time.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7)
> Is there any reason to be urged to have that bug fixed if bug 760011 and bug
> 708546 are fixed? Both have very simple patches ready to be reviewed.

Yes, I figured from comment 3 that this was the plan. Perhaps we should have been clearer. Sorry Hsinyi!

Thanks for filing + fixing bug 760011, Mounir. I still think there's still value in doing the consolidation that Hsinyi was trying to do, especially if we're aiming for a unified messaging API (SMS/MMS) where the cost of keeping this code around may be greater than converting to DOMRequest. But not if it's too hard / invasive. Let's revisit this when we get to the point of unifying APIs.
Philipp and Mounir, thanks for the feedback and sorry that I did not understand you clearly enough. Agree that we revisit this issue later with the unified API. :)
Assignee

Comment 10

6 years ago
Depends on bug 838467 to remove DOMRequestCursor related stuff from SmsRequest first.
Depends on: 838467
Assignee

Comment 11

6 years ago
Depends on bug 849739 for it removes thread list related stuff from SmsRequest and simplifies the task.
Depends on: 849739
Assignee

Updated

6 years ago
QA Contact: vyang
Assignee

Updated

6 years ago
Assignee: htsai → vyang
QA Contact: vyang
Assignee

Updated

6 years ago
Blocks: 847744
Assignee

Comment 13

6 years ago
Posted patch Part 1/3: IDL changes (obsolete) — Splinter Review
Attachment #628607 - Attachment is obsolete: true
Attachment #728173 - Flags: superreview?(jonas)
Attachment #728173 - Flags: review?(bent.mozilla)
Assignee

Comment 14

6 years ago
Posted patch Part 2/3: DOM & IPC (obsolete) — Splinter Review
Attachment #728176 - Flags: superreview?(bent.mozilla)
Attachment #728176 - Flags: review?(jonas)
Assignee

Comment 15

6 years ago
Posted patch Part 3/3: fix tests (obsolete) — Splinter Review
Attachment #728177 - Flags: review?(anygregor)
Assignee

Updated

6 years ago
Attachment #728176 - Flags: superreview?(bent.mozilla) → review?(bent.mozilla)
Assignee

Comment 16

6 years ago
Blocking bug 847744.

In current SMS IPC implementation, SmsRequest is actually a DOM class and it relies on SmsRequestForwarder to communicate with javascript based database service. There is no such thing in MobileMessageCallback, which is first introduced in bug 844431 to replace SmsRequest. SmsRequest is for SMS and MobileMessageCallback is for MMS, at least for now. The MobileMessageCallback is a must at the time because we want to return DOMRequest to content directly. No more SmsRequest in newly added MMS code. However, MMS and SMS now shares the same MobileMessageManager. The MobileMessageManager::GetMessages(id) may return either type of messages to user. So we need an unified method to handle both MMS and SMS, or we'll have big trouble in bug 847744, MMS IPC.

In this bug we retire SmsRequest forever. MobileMessageCallback now takes the responsibility for both SMS and MMS. In OOP mode, we only create MobileMessageCallback in child process. SmsRequestParent inherits nsIMobileMessageCallback as well, so it takes the same position with MobileMessageCallback but in parent process. This is also the design of MobileMessageCursorCallback by the way.
blocking-b2g: --- → leo?
marking leo+ as this blocks a blocking MMS bug
blocking-b2g: leo? → leo+
Comment on attachment 728176 [details] [diff] [review]
Part 2/3: DOM & IPC

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +133,3 @@
>  
> +  // nsISupports is ambiguous in DOMRequest. Cast to nsIDOMDOMRequest first.
> +  nsCOMPtr<nsIDOMDOMRequest> domRequest = (DOMRequest*)request;

Make that request.get() instead of using a cast.

@@ +137,2 @@
>    if (NS_FAILED(rv)) {
>      NS_ERROR("Failed to create the js value!");

This could also be

JSObject* wrappedRequest = request->WrapObject(aCx, aGlobal);
if (!wrappedRequest) {
  return NS_ERROR_FAILURE;
}
aRequest->setObject(*wrappedRequest);
return NS_OK;

::: dom/mobilemessage/src/SmsManager.cpp
@@ +159,3 @@
>  
> +  // nsISupports is ambiguous in DOMRequest. Cast to nsIDOMDOMRequest first.
> +  nsCOMPtr<nsIDOMDOMRequest> domRequest = (DOMRequest*)request;

Same here
how are we with reviews here? Thanks
Assignee

Comment 20

6 years ago
Posted patch Part 1/3: IDL changes : v2 (obsolete) — Splinter Review
Rebase only.
Attachment #728173 - Attachment is obsolete: true
Attachment #728173 - Flags: superreview?(jonas)
Attachment #728173 - Flags: review?(bent.mozilla)
Attachment #730834 - Flags: superreview?(jonas)
Attachment #730834 - Flags: review?(bent.mozilla)
Assignee

Comment 21

6 years ago
Posted patch Part 2/3: DOM & IPC : v2 (obsolete) — Splinter Review
Rebase only.
Attachment #728176 - Attachment is obsolete: true
Attachment #728176 - Flags: review?(jonas)
Attachment #728176 - Flags: review?(bent.mozilla)
Attachment #730836 - Flags: review?(jonas)
Attachment #730836 - Flags: review?(bent.mozilla)
Comment on attachment 730834 [details] [diff] [review]
Part 1/3: IDL changes : v2

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

r=me with this fixed.

::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
@@ +18,5 @@
>    DOMString? smil;
>    jsval      attachments; // MmsAttachment[]
>  };
>  
> +[scriptable, builtinclass, uuid(a7984cb3-27c8-4e3d-82a4-01553e93c078)]

Changing the comment doesn't require a UUID bump here.
Attachment #730834 - Flags: review?(bent.mozilla) → review+
Assignee

Comment 23

6 years ago
Posted patch Part 2/3: DOM & IPC : v3 (obsolete) — Splinter Review
Use nsRefPtr::get() instead of explicit type cast.

Hi Ms2ger, I keep the nsContentUtils::WrapNative() because we use this function also in MobileMessageCallback, MobileMessageCursorCallback. Please let me know if you highly suggest we should use the other way.
Attachment #730836 - Attachment is obsolete: true
Attachment #730836 - Flags: review?(jonas)
Attachment #730836 - Flags: review?(bent.mozilla)
Attachment #730852 - Flags: review?(jonas)
Attachment #730852 - Flags: review?(bent.mozilla)
Assignee

Comment 24

6 years ago
(In reply to ben turner [:bent] from comment #22)
> Part 1/3: IDL changes : v2
> Review of attachment 730834 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
> Changing the comment doesn't require a UUID bump here.

I change it because the return type of send() is actually changed from SmsRequest to DOMRequest. However we can't tell from the idl because the return type is jsval so that we can support sending SMS to multiple recipients. Is this a good reason to change interface uuid?
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #24)
> I change it because the return type of send() is actually changed from
> SmsRequest to DOMRequest. However we can't tell from the idl because the
> return type is jsval so that we can support sending SMS to multiple
> recipients. Is this a good reason to change interface uuid?

It makes sense conceptually, yes, but on a technical level it is a meaningless distinction.
Comment on attachment 730836 [details] [diff] [review]
Part 2/3: DOM & IPC : v2

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +133,3 @@
>  
> +  // nsISupports is ambiguous in DOMRequest. Cast to nsIDOMDOMRequest first.
> +  nsCOMPtr<nsIDOMDOMRequest> domRequest = (DOMRequest*)request;

There's no real need for this, it just causes an extra AddRef/Release pair. Just do this instead:

   rv = nsContentUtils::WrapNative(aCx, aGlobal,
                                   static_cast<nsIDOMDOMRequest*>(request.get()),
                                   aRequest);

::: dom/mobilemessage/src/SmsManager.cpp
@@ +159,3 @@
>  
> +  // nsISupports is ambiguous in DOMRequest. Cast to nsIDOMDOMRequest first.
> +  nsCOMPtr<nsIDOMDOMRequest> domRequest = (DOMRequest*)request;

Same here.

::: dom/mobilemessage/src/ipc/SmsChild.cpp
@@ +95,5 @@
>  SmsChild::DeallocPSmsRequest(PSmsRequestChild* aActor)
>  {
> +  // SmsRequestChild is refcounted, must not be freed manually. Originally
> +  // AddRefed in SendRequest() in SmsIPCService.cpp.
> +  static_cast<MobileMessageCursorChild*>(aActor)->Release();

Wait... This isn't a cursor, right? It's just a SmsRequestChild I think.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +311,3 @@
>  
> +  if (NS_FAILED(rv)) {
> +    rv = NS_SUCCEEDED(NotifyDeleteMessageFailed(nsIMobileMessageCallback::INTERNAL_ERROR));

This should return, right?

@@ +337,5 @@
>  
> +nsresult
> +SmsRequestParent::SendReply(const MessageReply& aReply)
> +{
> +  return Send__delete__(this, aReply) ? NS_OK : NS_ERROR_FAILURE;

Rather than duplicate the whole "actor may be dead" comment and check in each method below why not just move it here? That's much cleaner.

@@ +351,5 @@
> +  // an error here to avoid sending a message to the dead process.
> +  NS_ENSURE_TRUE(!mActorDestroyed, NS_ERROR_FAILURE);
> +
> +  SmsMessage* message = static_cast<SmsMessage*>(aMessage);
> +  return SendReply(MessageReply(ReplyMessageSend(message->GetData())));

For all of these you shouldn't need to explicitly construct a 'MessageReply' here, it should happen automatically. It looks a lot cleaner too:

  return SendReply(ReplyMessageSend(message->GetData()));
Attachment #730836 - Attachment is obsolete: false
Comment on attachment 730852 [details] [diff] [review]
Part 2/3: DOM & IPC : v3

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

Er... Something funny happened with bugzilla. I think I reviewed this, but it says I didn't...
Attachment #730852 - Flags: review?(jonas)
Attachment #730852 - Flags: review?(bent.mozilla)
Attachment #730852 - Flags: review-
Attachment #728177 - Flags: review?(anygregor) → review+
Assignee

Updated

6 years ago
Attachment #730836 - Attachment is obsolete: true
Assignee

Comment 28

6 years ago
Posted patch Part 2/3: DOM & IPC : v4 (obsolete) — Splinter Review
Address comment 26. About SmsRequestChild reference counting stuff, it's simply not a refcount-enabled class at all, so we don't really need AddRef()/Release(). SmsRequestParent does, because it implements nsIMobileMessageCallback.

Rebuild all and verified with marionette test cases and message app with patches[1].

[1]: https://github.com/vicamo/b2g_gaia/tree/bugzilla/838467/domcursor-for-sms , https://github.com/vicamo/b2g_gaia/tree/bugzilla/849739/cursor-based-get-threads
Attachment #730852 - Attachment is obsolete: true
Attachment #731487 - Flags: review?(bent.mozilla)
Comment on attachment 731487 [details] [diff] [review]
Part 2/3: DOM & IPC : v4

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

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +348,5 @@
> +{
> +  // The child process could die before this asynchronous notification, in which
> +  // case ActorDestroy() was called and mActorDestroyed is set to true. Return
> +  // an error here to avoid sending a message to the dead process.
> +  NS_ENSURE_TRUE(!mActorDestroyed, NS_ERROR_FAILURE);

Let's please move all of this duplicated stuff to SendReply.
Attachment #731487 - Flags: review?(bent.mozilla) → review-
Comment on attachment 731487 [details] [diff] [review]
Part 2/3: DOM & IPC : v4

Ok, we looked deeper and the next patch in this MMS series causes us to have some special cases where we can't always wait until SendReply to check the actor state. This is fine then.
Attachment #731487 - Flags: review- → review+
Assignee

Comment 31

6 years ago
(In reply to ben turner [:bent] from comment #29)
> Let's please move all of this duplicated stuff to SendReply.

In bug 847744, MMS IPC, we'll have something similar to following[1]:

  SmsRequestParent::NotifyMessageGot(nsISupports *aMessage)
  {
    NS_ENSURE_TRUE(!mActorDestroyed, NS_ERROR_FAILURE);

    MobileMessageData data; // An union of SmsMessageData and MmsMessageData.

    // Prepare that data here. However, for MMS we have to call 

    return SendReply(ReplyGetMessage(data));
  }

That's why I suggested that we should bail out early if we just find the actor is already destroyed. I should have written all these done in previous comment. And, I'm totally fine with it. Let's fix this in bug 847744.

https://github.com/vicamo/b2g_releases-mozilla-central/blob/bugzilla/847744/mms-ipc/dom/mobilemessage/src/ipc/SmsParent.cpp#L539
Assignee

Comment 32

6 years ago
Posted patch Part 2/3: DOM & IPC : v5 (obsolete) — Splinter Review
Address comment 29.
Attachment #731487 - Attachment is obsolete: true
Attachment #731497 - Flags: review+
leo+ as this is a part of MMS. No_UPLIFT for now before the whole MMS is completed
Whiteboard: NO_UPLIFT
Attachment #730834 - Flags: superreview?(jonas) → superreview+
Whiteboard: NO_UPLIFT → [NO_UPLIFT]
Assignee

Comment 34

6 years ago
Rebase and add sr=sicking, r=bent
Attachment #730834 - Attachment is obsolete: true
Attachment #734594 - Flags: superreview+
Attachment #734594 - Flags: review+
Assignee

Comment 35

6 years ago
Posted patch Part 2/3: DOM & IPC : v6 (obsolete) — Splinter Review
Rebase and add r=bent
Attachment #731497 - Attachment is obsolete: true
Attachment #734596 - Flags: review+
Assignee

Comment 36

6 years ago
Add r=gwagner
Attachment #728177 - Attachment is obsolete: true
Attachment #734597 - Flags: review+
Assignee

Comment 38

6 years ago
Waiting for Windows xpcshell tests: https://tbpl.mozilla.org/?tree=Try&rev=c836f88da3a4
Assignee

Comment 40

6 years ago
Update landed patch. Fix android 2.2 debug build.
Attachment #734596 - Attachment is obsolete: true
Attachment #734961 - Flags: review+
Unfortunately, I had to back this push out because the new test for bug 849739 was failing frequently. Not knowing the how these depended on each other, I backed out the entire push. Sorry for the hassle :(.
https://hg.mozilla.org/integration/mozilla-inbound/rev/81dc5a203ac2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla23 → ---
Assignee

Comment 44

6 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #42)
> Sorry for the hassle :(.

No, the apology belongs to me. I saw the same thing this afternoon but didn't sure the root cause. I've re-submitted all the patches except the error prone test case in bug 849739.
https://hg.mozilla.org/mozilla-central/rev/8318ed5d76af
https://hg.mozilla.org/mozilla-central/rev/bc3fdf70603a
https://hg.mozilla.org/mozilla-central/rev/f9f0cd9ffe78
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.