609950 - Make sure CAO_observe use getHudIdByWindow(getWindowByWindowId(ID)) (fix console API for subframes)

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

Followup bug from bug 587734:

> >+let ConsoleAPIObserver = {
> 
> >+  observe: function CAO_observe(aMessage, aTopic, aData)
> >+  {
> >+    if (aTopic == "console-api-log-event") {

> (gavin says:)
> As mentioned earlier, this method should use
> getHudIdByWindow(getWindowByWindowId(ID)) once bug 597136 is landed. If that
> doesn't happen before this lands, we need to make sure it gets a followup.
>

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This is what I had in mind. It includes some unrelated cleanup to HCO_observe.

Comment 2

[David Dahl :ddahl]

Comment on attachment 491072 [details] [diff] [review]
patch

Cool. Nice and clean.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

We can also remove the registerDisplay code that stores the hudID mapping, since it's no longer used. This includes Mihai's test from bug 613013.

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 491569 [details] [diff] [review]
updated patch

The patch looks fine.

Comments:

-    // get the window Id
-    var windowId = this.getWindowId(aContentWindow);
-    this._headsUpDisplays[aHUDId] = { id: aHUDId, windowId: windowId };
+    this._headsUpDisplays[aHUDId] = { id: aHUDId };

Oh oh, looks weird. This should be removed entirely, but the removal of _headsUpDisplays would require more work.


+    if (!(aSubject instanceof Ci.nsIScriptError))
+      return;
+
+    let hudIds = ConsoleUtils.getHUDIdsForScriptError(aSubject);
+
+    switch (aSubject.category) {

Why not move the call to getHUDIdsForScriptError() in the default case? The other cases are ignored anyway, so no need to waste execution resources to find the hudIds for messages we ignore.

Also, if you see fit, I think the test from bug 598438 should be added as well, since it's related (good to catch future regressions).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/44bbb8e7929c

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> Oh oh, looks weird. This should be removed entirely, but the removal of
> _headsUpDisplays would require more work.

Yeah, it is weird. We'll revisit it for future cleanup.

> Why not move the call to getHUDIdsForScriptError() in the default case?

Good catch, made that change.

> Also, if you see fit, I think the test from bug 598438 should be added as well,
> since it's related (good to catch future regressions).

I have a couple of issue with that test, I'll followup there.