916607 - Convert MozSmsSegmentInfo to WebIDL dictionary

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Jean-Yves Perrier [:teoli]]

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

Comment 2

[Masatoshi Kimura [:emk]]

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

Comment 3

[Jean-Yves Perrier [:teoli]]

Kimura-san, thanks very much.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: WebIDL, XPIDL and IPDL changes

Remove nsIDOMSmsSegmentInfo.idl and add WebIDL dictionary SmsSegmentInfo.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/4: adopt dictionary SmsSegmentInfo

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/4: android backend changes

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix test cases

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

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 9

[Gene Lian (I already quit Mozilla)]

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.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

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?

Comment 12

[Masatoshi Kimura [:emk]]

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

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 14

[Masatoshi Kimura [:emk]]

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

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 16

[Masatoshi Kimura [:emk]]

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

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: WebIDL, XPIDL and IPDL changes : v2

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

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/4: adopt dictionary SmsSegmentInfo : v2

Rebase and exception handling after |ToJSValue()|.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix test cases : v2

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.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 29

[Anshul]

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

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 31

[bhavana bajaj [:bajaj]]

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

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 33

[Anshul]

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

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

Working on it.

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

m-c Gecko: c60b44a7b137ed1ebb3444efebb089d755424d54
Gaia: 87ffc8423953bf052ed2b953c7f5e0133cbf1384

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

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: WebIDL, XPIDL and IPDL changes : v3

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

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix test cases : v3

Update commit summary only.

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/4: adopt dictionary SmsSegmentInfo : v3

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

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix test cases : v4

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.

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix test cases : v5

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.

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix test cases : v6

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

Comment 49

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 50

[Ryan VanderMeulen [:RyanVM]]

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