Convert MozSmsSegmentInfo to WebIDL dictionary

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: emk, Assigned: vicamo)

Tracking

(Depends on 1 bug, Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:ril][p=3], )

Attachments

(4 attachments, 9 obsolete attachments)

3.04 KB, patch
blassey
: review+
Details | Diff | Splinter Review
9.46 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
18.78 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
21.28 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
No description provided.
Will this have a user-visible effect? (user = web/front-end dev)
Flags: needinfo?(VYV03354)
The object "MozSmsSegmentInfo" will disappear from the global object.
Flags: needinfo?(VYV03354)
Kimura-san, thanks very much.
Blocks: 859764
Assignee: nobody → vyang
Whiteboard: [ft:ril][p=3]
Target Milestone: --- → 2.0 S4 (20june)
Remove nsIDOMSmsSegmentInfo.idl and add WebIDL dictionary SmsSegmentInfo.
Attachment #8435626 - Flags: review?(Ms2ger)
Attachment #8435627 - Flags: review?(gene.lian)
Attachment #8435627 - Flags: review?(Ms2ger)
Attachment #8435629 - Flags: review?(blassey.bugs)
Posted patch part 4/4: fix test cases (obsolete) — Splinter Review
Hi Gene, I just rewrite the two test scripts with Promise.  No meaningful change related to dictionary SmsSegmentInfo adoption here.
Attachment #8435633 - Flags: review?(gene.lian)
Attachment #8435633 - Flags: review?(Ms2ger)
Depends on bug 958782 because dom/webidl/MobileMessageManager is moved renamed in that bug.
Depends on: 958782
Attachment #8435629 - Flags: review?(blassey.bugs) → review+
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.
Attachment #8435627 - Flags: review?(gene.lian) → review+
Attachment #8435633 - Flags: review?(gene.lian) → review+
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.
Depends on: 1023280, 1023295
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?
Flags: needinfo?(VYV03354)
At least windows.MozMobileMessageThread should be removed from the global object. This is even visible on the desktop Firefox.
Flags: needinfo?(VYV03354)
(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.
Are there any specs for (Moz)MobileMessageThread? I filed this bug just because the spec saying that SmsSegmentInfo is a dictionary.
(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.
Flags: needinfo?(VYV03354)
I have no strong opinion about that, especially when it will be removed anyway.
Flags: needinfo?(VYV03354)
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".
Attachment #8435626 - Attachment is obsolete: true
Attachment #8435626 - Flags: review?(Ms2ger)
Attachment #8445819 - Flags: review?(khuey)
Attachment #8445819 - Flags: review?(bzbarsky)
Rebase and exception handling after |ToJSValue()|.
Attachment #8435627 - Attachment is obsolete: true
Attachment #8435627 - Flags: review?(Ms2ger)
Attachment #8445821 - Flags: review?(bzbarsky)
Posted patch part 4/4: fix test cases : v2 (obsolete) — Splinter Review
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.
Attachment #8435633 - Attachment is obsolete: true
Attachment #8435633 - Flags: review?(Ms2ger)
Attachment #8445822 - Flags: review?(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....
Attachment #8445821 - Flags: review?(bzbarsky) → review-
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.
Attachment #8445822 - Flags: review?(bzbarsky) → review+
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.
Attachment #8445819 - Flags: review?(bzbarsky) → review+
(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.
(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 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.
Attachment #8445821 - Flags: review- → review?(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.
Attachment #8445821 - Flags: review?(bzbarsky) → review+
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.
Attachment #8445819 - Flags: review?(khuey) → review+
Bhavna, is this bug intended for 2.0 as the target milestone says 2.0.
Flags: needinfo?(bbajaj)
I'm to address review comments and land this in a couple of days.
Target Milestone: 2.0 S4 (20june) → 2.0 S6 (18july)
(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.
Flags: needinfo?(bbajaj)
(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.
Target Milestone: 2.0 S6 (18july) → ---
(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?
This should land in 2.1.  Can we land it today?
Flags: needinfo?(vyang)
Working on it.
Got some random crashes even without my patches when running marionette test cases.
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 ?? ()
m-c Gecko: c60b44a7b137ed1ebb3444efebb089d755424d54
Gaia: 87ffc8423953bf052ed2b953c7f5e0133cbf1384

$ ./mach marionette-webapi `pwd`/gecko/dom/mobilemessage/tests/marionette/manifest.ini
Flags: needinfo?(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.
1. Rebase,
2. Correct commit summary,
3. Address review comment 27.
Attachment #8445819 - Attachment is obsolete: true
Attachment #8466125 - Flags: review+
Posted patch part 4/4: fix test cases : v3 (obsolete) — Splinter Review
Update commit summary only.
Attachment #8445822 - Attachment is obsolete: true
Attachment #8466129 - Flags: review+
1. Rebase,
2. Update commit summary,
3. Use the same way with NotifySuccess(nsISupports, ...) in NotifySegmentInfoForTextGot() to convert dictionary to JSValue.
Attachment #8445821 - Attachment is obsolete: true
Attachment #8466136 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=9423478824dd , emulator-kk is also trapped in the same segfault but with a lower rate.
(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.
Depends on: 1048098
Posted patch part 4/4: fix test cases : v4 (obsolete) — Splinter Review
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.
Attachment #8466129 - Attachment is obsolete: true
Attachment #8466894 - Flags: review+
Posted patch part 4/4: fix test cases : v5 (obsolete) — Splinter Review
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.
Attachment #8466894 - Attachment is obsolete: true
Attachment #8466930 - Flags: review+
With attachment 8466930 [details] [diff] [review], we no longer depend on bug 1048098 here.
No longer depends on: 1048098
Sorry, there is also one in "test_getsegmentinfofortext.js". Fixed although it doesn't cause a test failure.
Attachment #8466930 - Attachment is obsolete: true
Attachment #8466934 - Flags: review+
Depends on: 1274518
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.