Closed Bug 609950 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(blocking2.0 beta8+)

RESOLVED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: ddahl, Assigned: Gavin)

References

Details

(Whiteboard: [followup])

Attachments

(1 file, 1 obsolete file)

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.
>
Assignee: nobody → ddahl
Whiteboard: [followup]
Blocks: devtools4
Attached patch patch (obsolete) — Splinter Review
This is what I had in mind. It includes some unrelated cleanup to HCO_observe.
Attachment #491072 - Flags: review?(ddahl)
Assignee: ddahl → gavin.sharp
Status: NEW → ASSIGNED
Comment on attachment 491072 [details] [diff] [review]
patch

Cool. Nice and clean.
Attachment #491072 - Flags: review?(ddahl) → review+
blocking2.0: --- → beta8+
Summary: Make sure CAO_observe use getHudIdByWindow(getWindowByWindowId(ID)) → Make sure CAO_observe use getHudIdByWindow(getWindowByWindowId(ID)) (fix console API for subframes)
Attached patch updated patchSplinter Review
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.
Attachment #491072 - Attachment is obsolete: true
Attachment #491569 - Flags: feedback?
Attachment #491569 - Flags: feedback? → feedback?(mihai.sucan)
Duplicate of this bug: 613013
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).
Attachment #491569 - Flags: feedback?(mihai.sucan) → feedback+
https://hg.mozilla.org/mozilla-central/rev/44bbb8e7929c
Flags: in-testsuite+
Hardware: x86 → All
Target Milestone: --- → Firefox 4.0b8
(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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.