Closed
Bug 1098074
Opened 10 years ago
Closed 10 years ago
Remove the ReportPendingException in nsFrameMessageManager::ReceiveMessage
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(3 files)
2.20 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
3.96 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e5d806e25bce
Attachment #8521870 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 1•10 years ago
|
||
And add some missing error handling.
Attachment #8521872 -
Flags: review?(bobbyholley)
Updated•10 years ago
|
Attachment #8521870 -
Flags: review?(bobbyholley) → review+
Comment 2•10 years ago
|
||
Comment on attachment 8521872 [details] [diff] [review] nsframemessagemanager_2-v0.diff Review of attachment 8521872 [details] [diff] [review]: ----------------------------------------------------------------- My last review on this got lost :-( Please switch this to: bool ok = JS_Foo() && JS_Bar(); NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE); So that we're consistent in error handling style between XPCOM and SM calls.
Attachment #8521872 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 3•10 years ago
|
||
And, finally, a test poking the new error code.
Attachment #8522527 -
Flags: review?(bobbyholley)
Comment 4•10 years ago
|
||
Comment on attachment 8522527 [details] [diff] [review] nsframemessagemanager_3-v0.diff Review of attachment 8522527 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a million for adding all these tests! ::: dom/base/test/chrome/test_bug1098074_throw_from_ReceiveMessage.xul @@ +26,5 @@ > + var globalMM = Cc["@mozilla.org/globalmessagemanager;1"] > + .getService(Ci.nsIMessageListenerManager); > + globalMM.addMessageListener("flimfniffle", function onMessage(msg) { > + globalMM.removeMessageListener("flimfniffle", onMessage); > + is(msg.data, "toofeltor", "correct message"); In german I'd read this as a slight misspelling of "Devil Door" - intentional? ;-) @@ +47,5 @@ > + <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1098074" > + target="_blank">Mozilla Bug 1098074</a> > + </body> > + > + <browser messagemanagergroup="test1" type="content-targetable" src="about:mozilla" /> I'm not sure about this part, but it looks cribbed from other tests, so it's probably right?
Attachment #8522527 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #4) > ::: dom/base/test/chrome/test_bug1098074_throw_from_ReceiveMessage.xul > @@ +26,5 @@ > > + var globalMM = Cc["@mozilla.org/globalmessagemanager;1"] > > + .getService(Ci.nsIMessageListenerManager); > > + globalMM.addMessageListener("flimfniffle", function onMessage(msg) { > > + globalMM.removeMessageListener("flimfniffle", onMessage); > > + is(msg.data, "toofeltor", "correct message"); > > In german I'd read this as a slight misspelling of "Devil Door" - > intentional? ;-) Yes, it is a town in a D&D campaign my friend is running. Initially I used foobar here, however, I noticed when cribbing from other tests that everyone else is too. Since we're explicitly testing the error paths, I didn't want to trip over any cases where someone used a placeholder string either in firefox, or maybe racing with another test. > @@ +47,5 @@ > > + <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1098074" > > + target="_blank">Mozilla Bug 1098074</a> > > + </body> > > + > > + <browser messagemanagergroup="test1" type="content-targetable" src="about:mozilla" /> > > I'm not sure about this part, but it looks cribbed from other tests, so it's > probably right? Yes it's cribbed; unfortunately, I was never able to get the environment correct enough for the messageManager property to show up on those browser windows though. Using the main globalMM hits the same code path, however, so I don't think it matters. I will remove the extra browser windows as it seems they are unnecessary.
Assignee | ||
Comment 6•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/977889c9dbb7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b9f43e6375 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/57b4489b0a5e https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e5d806e25bce
https://hg.mozilla.org/mozilla-central/rev/977889c9dbb7 https://hg.mozilla.org/mozilla-central/rev/a2b9f43e6375 https://hg.mozilla.org/mozilla-central/rev/57b4489b0a5e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•