Closed Bug 835148 Opened 7 years ago Closed 7 years ago

WebSMS: Implement SmsEvent using event generator

Categories

(Core :: DOM: Events, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: vicamo, Assigned: vicamo)

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Assignee: nobody → vyang
Comment on attachment 710124 [details] [diff] [review]
Implement SmsEvent using event generator

>diff --git a/dom/sms/tests/marionette/test_outgoing.js b/dom/sms/tests/marionette/test_outgoing.js
>--- a/dom/sms/tests/marionette/test_outgoing.js
>+++ b/dom/sms/tests/marionette/test_outgoing.js
>@@ -99,18 +99,17 @@ function doSendMessageAndCheckSuccess(re
>-    ok(event instanceof MozSmsEvent,
>-       "event is instanceof " + event.constructor);
>+    ok(event, "event is valid");

Test whether the event is an instance of MozSmsEvent throws an
exception and fails the test case.  The command

  for e in `cat js/xpconnect/src/event_impl_gen.conf.in | \
            grep Event | \
            cut -d\' -f 2`;
  do \
    grep -nR --exclude-dir=.git "instanceof $e"; \
  done

gives a list of generated event types which is then tested with
instanceof operator in some test script. I got only one --
ProgressEvent.  However, ProgressEvent is actually defined in
dom/workers/Events.cpp:799 and is not a typical generated event as
I've ever seen.  Is there any way I can keep the original test case?
Attachment #710124 - Flags: feedback?(bugs)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> Is there any way I can keep the original test case?

The same thing happened in bug 813596 comment #5, test cases for CellBroadcastEvent.
ProgressEvent in workers is a different thing. For main thread progressevent is implemented using codegen.
Comment on attachment 710124 [details] [diff] [review]
Implement SmsEvent using event generator

Why do you change the uuid of nsIDOMMozSmsMessage?

I don't understand why the changes to test_outgoing.js are needed.
Attachment #710124 - Flags: feedback?(bugs)
I mean bug 813596 comment #5 talks about using ok(event, ...) because instanceof crashes.
Does it really crash? If so, we need to fix that crash, not just test something else.
If JS can crash the browser, it means webpages can crash it. Not good.

As an example
(new ProgressEvent("foo")) instanceof ProgressEvent.constructor certainly doesn't crash on
desktop.
(In reply to Olli Pettay [:smaug] from comment #5)
> Why do you change the uuid of nsIDOMMozSmsMessage?

I renamed nsIDOMSmsMessage.idl to nsIDOMMozSmsMessage.idl.  Do I have to change uuid in this case?
Attached patch v2 (obsolete) — Splinter Review
fix https://tbpl.mozilla.org/?tree=Try&rev=2b61804a81bd
Attachment #710124 - Attachment is obsolete: true
We did received onsending notification in line 713, but nothing happens then. It's because testing whether the received event is an instance of MozSmsEvent triggers an exception "'prototype' property of MozSmsEvent is not an object" and skips all following handling code. Soon in line 745, the test script fails.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> I renamed nsIDOMSmsMessage.idl to nsIDOMMozSmsMessage.idl.  Do I have to
> change uuid in this case?
Well, the interface itself stays the same, not uuid change isn't needed.


(In reply to Vicamo Yang [:vicamo][:vyang] from comment #9)
> We did received onsending notification in line 713, but nothing happens
> then. It's because testing whether the received event is an instance of
> MozSmsEvent triggers an exception "'prototype' property of MozSmsEvent is
> not an object" and skips all following handling code. Soon in line 745, the
> test script fails.
Do you know why we get that exception. On desktop I can certainly do the instanceof check for
generated events.
(In reply to Olli Pettay [:smaug] from comment #10)
> Do you know why we get that exception. On desktop I can
> certainly do the instanceof check for generated events.

I noticed you test following pattern in comment #6,

  (new ProgressEvent("foo")) instanceof ProgressEvent.constructor

but that doesn't work for me, either. I got:

  invalid 'instanceof' operand ProgressEvent.constructor

And test

  (new ProgressEvent("foo")) instanceof ProgressEvent

results in the same exception:

  'prototype' property of ProgressEvent is not an object
bug 838542 filed, will address this in test scripts comments.
Attached patch v3Splinter Review
mention bug 838542 in comments.
Attachment #710550 - Attachment is obsolete: true
Attachment #710628 - Flags: review?(bugs)
Comment on attachment 710628 [details] [diff] [review]
v3

Throwing is odd, but not about this bug.
Attachment #710628 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/3b05660cbdd2
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.