Masatoshi Kimura [:emk] Reporter
	Description • 11 years ago

      No description provided.

	Jean-Yves Perrier [:teoli]
	Comment 1 • 11 years ago

Will this have a user-visible effect? (user = web/front-end dev)

	Masatoshi Kimura [:emk] Reporter
	Comment 2 • 11 years ago

The object "MozSmsSegmentInfo" will disappear from the global object.

	Jean-Yves Perrier [:teoli]
	Comment 3 • 11 years ago

Kimura-san, thanks very much.

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

Remove nsIDOMSmsSegmentInfo.idl and add WebIDL dictionary SmsSegmentInfo.

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

Hi Gene, I just rewrite the two test scripts with Promise.  No meaningful change related to dictionary SmsSegmentInfo adoption here.

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

Depends on bug 958782 because dom/webidl/MobileMessageManager is moved renamed in that bug.

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

Comment on attachment 8435627 [details] [diff] [review]
part 2/4: adopt dictionary SmsSegmentInfo

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

Yeap! It looks good to me.

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

Comment on attachment 8435627 [details] [diff] [review]
part 2/4: adopt dictionary SmsSegmentInfo

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

::: dom/mobilemessage/src/MobileMessageCallback.cpp
@@ +251,5 @@
> +  info.mCharsPerSegment.Construct(aCharsPerSegment);
> +  info.mCharsAvailableInLastSegment.Construct(aCharsAvailableInLastSegment);
> +
> +  JS::Rooted<JS::Value> val(cx);
> +  NS_ENSURE_TRUE(info.ToObject(cx, &val), NS_ERROR_FAILURE);

See bug 878533 comment 36.

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

I wonders whether should we convert MozMobileMessageThread into a dictionary as well?

MozMobileMessageThread is only returned from MobileMessageManager::getThreads() as |event.thread| of a DOMCursor result.  While MozSmsSegmentInfo is also only returned from MobileMessageManager::getSegmentInfoFroTxt() as |request.result| of a DOMRequest, I'd say they play similar roles in the API.  So, what's the rationale behind this task?  Does it apply to MozMobileMessageThread as well?

	Masatoshi Kimura [:emk] Reporter
	Comment 12 • 10 years ago

At least windows.MozMobileMessageThread should be removed from the global object. This is even visible on the desktop Firefox.

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

(In reply to Masatoshi Kimura [:emk] from comment #12)
> At least windows.MozMobileMessageThread should be removed from the global
> object. This is even visible on the desktop Firefox.

Hi, that's a different thing to me. It has to go for sure.

1) The reason it has to be compiled globally is there is no way to selectively disable some IPDL protocols.  The IPDL files are not preprocessed.  No easy solution so far.
2) The reason SMS DOM interfaces appear in all applications is it's not yet WebIDL-ized.  I've had a working branch for this in https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/859764/webidl-MobileMessage .

So all I'm curious about is, based on your point of view, should an object of constant, known fields be a WebIDL dictionary, especially when it only serves as a return value of some APIs and is never ever used as input again?  I wanna know if there is such a basic rule.  Thank you.

	Masatoshi Kimura [:emk] Reporter
	Comment 14 • 10 years ago

Are there any specs for (Moz)MobileMessageThread? I filed this bug just because the spec saying that SmsSegmentInfo is a dictionary.

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

(In reply to Masatoshi Kimura [:emk] from comment #14)
> Are there any specs for (Moz)MobileMessageThread? I filed this bug just
> because the spec saying that SmsSegmentInfo is a dictionary.

http://messaging.sysapps.org/ , but Mozilla has no promise about implementing it in the near future. And, message threading is not included in the draft. It's supposed to be removed in bug 1020198.

	Masatoshi Kimura [:emk] Reporter
	Comment 16 • 10 years ago

I have no strong opinion about that, especially when it will be removed anyway.

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

Rebase only.

Hi, Boris, may I have your review?

This patch is only about interface files. Implementation is in the next patch. I removed XPIDL interface nsIDOMMozSmsSegmentInfo. As a replacement, a WebIDL dictionary SmsSegmentInfo was created.  We don't need IPDL structure SmsSegmentInfoData, either.  It used to be underlying data structure for class SmsSegmentInfo, but that will be removed in the second patch.

Hi, Kyle, this patch touches moz.build to remove XPIDL definition file "nsIDOMSmsSegmentInfo.idl".

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

Rebase and exception handling after |ToJSValue()|.

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

Rebase only.

Hi Boris, this patch touches "test_interfaces.html" to remove tests for removed MozSmsSegmentInfo, so it has to be explicitly reviewed by a DOM peer.

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

Full try: https://tbpl.mozilla.org/?tree=Try&rev=ba4c9d953a55

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

Comment on attachment 8445821 [details] [diff] [review]
part 2/4: adopt dictionary SmsSegmentInfo : v2

This seems to be missing the changes to the NotifySuccess signature....

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

Comment on attachment 8445822 [details] [diff] [review]
part 4/4: fix test cases : v2

>+    ok(true, "preferences pushed: " + JSON.stringify(aPrefs));

Assuming you want this, why is this not info() or log() or whatever the right thing is in this harness that is not a test assertion?

r=me on the test_interfaces.html bit.

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

Comment on attachment 8445819 [details] [diff] [review]
part 1/4: WebIDL, XPIDL and IPDL changes : v2

This looks ok, though depending on how this is used in practice you could also give the members default values to make consumers simpler.

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

(In reply to Boris Zbarsky [:bz] from comment #22)
> Comment on attachment 8445822 [details] [diff] [review]
> part 4/4: fix test cases : v2
> 
> >+    ok(true, "preferences pushed: " + JSON.stringify(aPrefs));
> 
> Assuming you want this, why is this not info() or log() or whatever the
> right thing is in this harness that is not a test assertion?

info() is not an API exported in marionette-simpletest.js.

|ok(true, ...)| is used to print debug messages in logcat (on B2G) but not flooding his console. I always use |ok(true, ...)| for messages you'll only be interested when debugging a test case, |log(...)| for summaries, big titles, etc.

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

(In reply to Boris Zbarsky [:bz] from comment #23)
> Comment on attachment 8445819 [details] [diff] [review]
> part 1/4: WebIDL, XPIDL and IPDL changes : v2
> 
> This looks ok, though depending on how this is used in practice you could
> also give the members default values to make consumers simpler.

The three parameters are always available in both Gonk and Android backends.

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

Comment on attachment 8445821 [details] [diff] [review]
part 2/4: adopt dictionary SmsSegmentInfo : v2

No, NotifySuccess has already two overloads: one accepts a jsval, another accepts an nsISupports.  Both don't show up in the patch.

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

Comment on attachment 8445821 [details] [diff] [review]
part 2/4: adopt dictionary SmsSegmentInfo : v2

> The three parameters are always available in both Gonk and Android backends.

Sure, but the point is that if you have default values then instead of 

>+  info.mSegments.Construct(aSegments);

you can write:

  info.mSegments = aSegments;

Either way, though.

> NotifySuccess has already two overloads

Ah, I see.

Shouldn't you use the same compartment in your new callsite that the non-jsval overload was using before?

r=me with that sorted out, though this use of nsIScriptContext and attendant complexity looks very fishy to me; you may want to check with bholley how that part should really work in today's world.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 28 • 10 years ago

Comment on attachment 8445819 [details] [diff] [review]
part 1/4: WebIDL, XPIDL and IPDL changes : v2

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

Just adding/removing files like that doesn't really require review from a build peer.

Please update the commit message with the correct r= before landing.

	Anshul
	Comment 29 • 10 years ago

Bhavna, is this bug intended for 2.0 as the target milestone says 2.0.

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

I'm to address review comments and land this in a couple of days.

	bhavana bajaj [:bajaj]
	Comment 31 • 10 years ago

(In reply to Anshul from comment #29)
> Bhavna, is this bug intended for 2.0 as the target milestone says 2.0.

I doubt its going to be uplifted to 2.0. None of the blocking bugs here are blocked/known feature work for 2.0. Many teams use target milestones to for tracking purposes to denote central/master landing timestamp as the official 2.1 dev/sprints are not kicked-off until july 21.

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

(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #31)
> I doubt its going to be uplifted to 2.0. None of the blocking bugs here are
> blocked/known feature work for 2.0.

Yes, so let's make it more clear that it won't be uplifted to v2.0.

	Anshul
	Comment 33 • 10 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #32)
> (In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please]
> from comment #31)
> > I doubt its going to be uplifted to 2.0. None of the blocking bugs here are
> > blocked/known feature work for 2.0.
> 
> Yes, so let's make it more clear that it won't be uplifted to v2.0.
How about 2.1?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 34 • 10 years ago

This should land in 2.1.  Can we land it today?

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

Working on it.

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

Got some random crashes even without my patches when running marionette test cases.

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

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 44.134]
RelocationIterator::read (trc=0x4558f898, code=0x469278f8, reader=...) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jit/arm/Assembler-arm.cpp:628
628             offset_ = reader_.readUnsigned();
(gdb) bt
#0  RelocationIterator::read (trc=0x4558f898, code=0x469278f8, reader=...) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jit/arm/Assembler-arm.cpp:628
#1  js::jit::Assembler::TraceJumpRelocations (trc=0x4558f898, code=0x469278f8, reader=...) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jit/arm/Assembler-arm.cpp:791
#2  0x41c8a9ba in js::jit::JitCode::trace (this=0x469278f8, trc=0x4558f898) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jit/Ion.cpp:747
#3  0x41c47414 in MarkChildren (this=0x4558f898, tag=<value optimized out>, addr=1184004344) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/gc/Marking.cpp:1421
#4  js::GCMarker::processMarkStackOther (this=0x4558f898, tag=<value optimized out>, addr=1184004344) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/gc/Marking.cpp:1595
#5  0x41c48a7e in js::GCMarker::processMarkStackTop (this=0x4558f898, budget=...) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/gc/Marking.cpp:1633
#6  js::GCMarker::drainMarkStack (this=0x4558f898, budget=...) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/gc/Marking.cpp:1747
#7  0x41d902f0 in js::gc::GCRuntime::drainMarkStack (this=0x4558f1a0, budget=<value optimized out>, reason=JS::gcreason::DOM_WORKER, gckind=js::GC_SHRINK)
    at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jsgc.cpp:4328
#8  js::gc::GCRuntime::incrementalCollectSlice (this=0x4558f1a0, budget=<value optimized out>, reason=JS::gcreason::DOM_WORKER, gckind=js::GC_SHRINK) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jsgc.cpp:4834
#9  0x41d90b3a in js::gc::GCRuntime::gcCycle (this=0x4558f1a0, incremental=116, budget=0, gckind=js::GC_SHRINK, reason=JS::gcreason::DOM_WORKER) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jsgc.cpp:5042
#10 0x41d90cf8 in js::gc::GCRuntime::collect (this=0x4558f1a0, incremental=false, budget=<value optimized out>, gckind=js::GC_SHRINK, reason=JS::gcreason::DOM_WORKER)
    at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jsgc.cpp:5169
#11 0x41d9133c in GC (rt=<value optimized out>, reason=<value optimized out>) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jsgc.cpp:5197
#12 JS::ShrinkingGC (rt=<value optimized out>, reason=<value optimized out>) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/js/src/jsfriendapi.cpp:198
#13 0x41490e16 in mozilla::dom::workers::WorkerPrivate::GarbageCollectInternal (this=0x4488a400, aCx=0x44ad1660, aShrinking=true, aCollectChildren=false)
    at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/dom/workers/WorkerPrivate.cpp:5599
#14 0x41490e5e in WorkerRun (this=<value optimized out>, aCx=0x0, aWorkerPrivate=0x4558f898) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/dom/workers/WorkerPrivate.cpp:1672
#15 0x41495028 in mozilla::dom::workers::WorkerRunnable::Run (this=0x45577900) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/dom/workers/WorkerRunnable.cpp:312
#16 0x41494122 in mozilla::dom::workers::WorkerPrivate::ProcessAllControlRunnablesLocked (this=0x4488a400) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/dom/workers/WorkerPrivate.cpp:4491
#17 0x414944b4 in mozilla::dom::workers::WorkerPrivate::DoRunLoop (this=0x4488a400, aCx=0x44ad1660) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/dom/workers/WorkerPrivate.cpp:3980
#18 0x414863da in Run (this=0x44844d00) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/dom/workers/RuntimeService.cpp:2733
#19 0x40ca90e6 in nsThread::ProcessNextEvent (this=0x4487be40, aMayWait=false, aResult=0x44bffe67) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/xpcom/threads/nsThread.cpp:766
#20 0x40cb3f58 in NS_ProcessNextEvent (aThread=0x4488a400, aMayWait=false) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/xpcom/glue/nsThreadUtils.cpp:265
#21 0x40dfbf9c in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0x44862c40, aDelegate=0x43efbcc0) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/ipc/glue/MessagePump.cpp:326
#22 0x40df011c in MessageLoop::RunInternal (this=0x1000000) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/ipc/chromium/src/base/message_loop.cc:229
#23 0x40df019a in MessageLoop::RunHandler (this=0x43efbcc0) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/ipc/chromium/src/base/message_loop.cc:222
#24 MessageLoop::Run (this=0x43efbcc0) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/ipc/chromium/src/base/message_loop.cc:196
#25 0x40ca6e48 in nsThread::ThreadFunc (aArg=<value optimized out>) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/xpcom/threads/nsThread.cpp:346
#26 0x4074e4e0 in _pt_root (arg=<value optimized out>) at /home/vicamo/WorkSpace/mozilla/b2g/qemu/ics/arm/master/1/gecko/nsprpub/pr/src/pthreads/ptthread.c:212
#27 0x4006ee4c in __thread_entry (func=0x4074e43d <_pt_root>, arg=0x44890f80, tls=<value optimized out>) at bionic/libc/bionic/pthread.c:217
#28 0x4006e99c in pthread_create (thread_out=<value optimized out>, attr=0xbee0e324, start_routine=0x4074e43d <_pt_root>, arg=0x44890f80) at bionic/libc/bionic/pthread.c:357
#29 0x00000000 in ?? ()

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

m-c Gecko: c60b44a7b137ed1ebb3444efebb089d755424d54
Gaia: 87ffc8423953bf052ed2b953c7f5e0133cbf1384

$ ./mach marionette-webapi `pwd`/gecko/dom/mobilemessage/tests/marionette/manifest.ini

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #38)
> m-c Gecko: c60b44a7b137ed1ebb3444efebb089d755424d54
> Gaia: 87ffc8423953bf052ed2b953c7f5e0133cbf1384
> 
> $ ./mach marionette-webapi
> `pwd`/gecko/dom/mobilemessage/tests/marionette/manifest.ini

mobilemessage marionette test cases pass on emulator-kk with Gecko built from the same revision. Anyway, I'm going to verify rebased patches first.

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

1. Rebase,
2. Correct commit summary,
3. Address review comment 27.

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

Update commit summary only.

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

1. Rebase,
2. Update commit summary,
3. Use the same way with NotifySuccess(nsISupports, ...) in NotifySegmentInfoForTextGot() to convert dictionary to JSValue.

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

https://tbpl.mozilla.org/?tree=Try&rev=9423478824dd , emulator-kk is also trapped in the same segfault but with a lower rate.

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #39)
> mobilemessage marionette test cases pass on emulator-kk with Gecko built
> from the same revision. Anyway, I'm going to verify rebased patches first.

The Gecko crash has gone with today's HEAD, but Marionette still fails occasionally due to bug 1048098.

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

In cleanUp(), "test_incoming_delete.js" and "test_massive_incoming_delete.js" calls

  SpecialPowers.setBoolPref("dom.sms.enabled", false);

to reset SMS environment, but it should have been corrected as |SpecialPowers.clearUserPref| a long time ago.

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

Remove the last |SpecialPowers.clearUserPref| call from "test_segment_info.js" because preference settings changes pushed by |SpecialPowers.pushPrefEnv| will be cleared when test ends. See bug 1048098.

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

With attachment 8466930 [details] [diff] [review], we no longer depend on bug 1048098 here.

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

Sorry, there is also one in "test_getsegmentinfofortext.js". Fixed although it doesn't cause a test failure.

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

https://hg.mozilla.org/integration/b2g-inbound/rev/af90d93bf853
https://hg.mozilla.org/integration/b2g-inbound/rev/93cf77271f0f
https://hg.mozilla.org/integration/b2g-inbound/rev/e2d5cf427ed4
https://hg.mozilla.org/integration/b2g-inbound/rev/f70489df95a6

	Ryan VanderMeulen [:RyanVM]
	Comment 50 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/af90d93bf853
https://hg.mozilla.org/mozilla-central/rev/93cf77271f0f
https://hg.mozilla.org/mozilla-central/rev/e2d5cf427ed4
https://hg.mozilla.org/mozilla-central/rev/f70489df95a6