Convert MozSmsSegmentInfo to WebIDL dictionary

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
6 years ago
3 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
Reporter

Description

6 years ago
No description provided.
Will this have a user-visible effect? (user = web/front-end dev)
Flags: needinfo?(VYV03354)
Reporter

Comment 2

6 years ago
The object "MozSmsSegmentInfo" will disappear from the global object.
Flags: needinfo?(VYV03354)
Kimura-san, thanks very much.
Assignee

Updated

5 years ago
Blocks: 859764
Assignee

Updated

5 years ago
Assignee: nobody → vyang
Whiteboard: [ft:ril][p=3]
Target Milestone: --- → 2.0 S4 (20june)
Assignee

Comment 4

5 years ago
Remove nsIDOMSmsSegmentInfo.idl and add WebIDL dictionary SmsSegmentInfo.
Attachment #8435626 - Flags: review?(Ms2ger)
Assignee

Comment 5

5 years ago
Attachment #8435627 - Flags: review?(gene.lian)
Attachment #8435627 - Flags: review?(Ms2ger)
Assignee

Comment 6

5 years ago
Attachment #8435629 - Flags: review?(blassey.bugs)
Assignee

Comment 7

5 years ago
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)
Assignee

Comment 8

5 years ago
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+
Assignee

Comment 10

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

Updated

5 years ago
Depends on: 1023280, 1023295
Assignee

Comment 11

5 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?
Flags: needinfo?(VYV03354)
Reporter

Comment 12

5 years ago
At least windows.MozMobileMessageThread should be removed from the global object. This is even visible on the desktop Firefox.
Flags: needinfo?(VYV03354)
Assignee

Comment 13

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

Comment 14

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

Comment 15

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

Updated

5 years ago
Flags: needinfo?(VYV03354)
Reporter

Comment 16

5 years ago
I have no strong opinion about that, especially when it will be removed anyway.
Flags: needinfo?(VYV03354)
Assignee

Comment 17

5 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".
Attachment #8435626 - Attachment is obsolete: true
Attachment #8435626 - Flags: review?(Ms2ger)
Attachment #8445819 - Flags: review?(khuey)
Attachment #8445819 - Flags: review?(bzbarsky)
Assignee

Comment 18

5 years ago
Rebase and exception handling after |ToJSValue()|.
Attachment #8435627 - Attachment is obsolete: true
Attachment #8435627 - Flags: review?(Ms2ger)
Attachment #8445821 - Flags: review?(bzbarsky)
Assignee

Comment 19

5 years ago
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+
Assignee

Comment 24

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

Comment 25

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

Comment 26

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

Comment 29

5 years ago
Bhavna, is this bug intended for 2.0 as the target milestone says 2.0.
Flags: needinfo?(bbajaj)
Assignee

Comment 30

5 years ago
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)
Assignee

Comment 32

5 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.
Target Milestone: 2.0 S6 (18july) → ---

Comment 33

5 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?
This should land in 2.1.  Can we land it today?
Flags: needinfo?(vyang)
Assignee

Comment 35

5 years ago
Working on it.
Assignee

Comment 36

5 years ago
Got some random crashes even without my patches when running marionette test cases.
Assignee

Comment 37

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

Comment 38

5 years ago
m-c Gecko: c60b44a7b137ed1ebb3444efebb089d755424d54
Gaia: 87ffc8423953bf052ed2b953c7f5e0133cbf1384

$ ./mach marionette-webapi `pwd`/gecko/dom/mobilemessage/tests/marionette/manifest.ini
Flags: needinfo?(vyang)
Assignee

Comment 39

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

Comment 40

5 years ago
1. Rebase,
2. Correct commit summary,
3. Address review comment 27.
Attachment #8445819 - Attachment is obsolete: true
Attachment #8466125 - Flags: review+
Assignee

Comment 41

5 years ago
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+
Assignee

Comment 42

5 years ago
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+
Assignee

Comment 43

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

Comment 44

5 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.
Depends on: 1048098
Assignee

Comment 45

5 years ago
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+
Assignee

Comment 46

5 years ago
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+
Assignee

Comment 47

5 years ago
With attachment 8466930 [details] [diff] [review], we no longer depend on bug 1048098 here.
No longer depends on: 1048098
Assignee

Comment 48

5 years ago
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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.