Closed
Bug 597103
Opened 14 years ago
Closed 14 years ago
HUDService.deactivateHUDForContext(tab) fails when the given tab is not from the focused window
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(blocking2.0 betaN+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: msucan, Assigned: msucan)
References
Details
(Whiteboard: [patchclean:1012])
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
If one invokes HUDService.deactivateHUDForContext(tab) when tab is not from the currently focused window, the method throws an error. The problem is caused because the code always checks the currentContext() - the currently focused window. The fix is really trivial - writing the mochitest takes more lines. :)
Assignee | ||
Comment 1•14 years ago
|
||
Proposed fix and test code.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0917]
Updated•14 years ago
|
Blocks: devtools4b8
Assignee | ||
Comment 2•14 years ago
|
||
Rebased and updated the patch with some test code fixes.
Attachment #476339 -
Attachment is obsolete: true
Attachment #477168 -
Flags: feedback?(ddahl)
Attachment #476339 -
Flags: feedback?(ddahl)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0917] → [patchclean:0921]
Assignee | ||
Comment 3•14 years ago
|
||
Fixed a bug which caused failures in the upcoming patch for bug 594523.
Please note that I also commented HUDService.shutdown() in the big tests file, since it causes intermittent failures on my system. Sometimes the executeSoon() function executes before finish() which makes the Web Console unusable - subsequent tests fail.
Attachment #477168 -
Attachment is obsolete: true
Attachment #477231 -
Flags: feedback?(ddahl)
Attachment #477168 -
Flags: feedback?(ddahl)
Comment 4•14 years ago
|
||
Comment on attachment 477231 [details] [diff] [review]
updated patch
looks good
Attachment #477231 -
Flags: feedback?(ddahl) → feedback+
Assignee | ||
Comment 5•14 years ago
|
||
Comment on attachment 477231 [details] [diff] [review]
updated patch
Thanks David for the feedback+!
Asking for review and approval2.0. This patch fixes an important bug in the way we deactivate the Web Console from each tab. This bug is also a dependency for bug 594523. Thanks!
Attachment #477231 -
Flags: review?(gavin.sharp)
Attachment #477231 -
Flags: approval2.0?
Comment 6•14 years ago
|
||
Comment on attachment 477231 [details] [diff] [review]
updated patch
>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)
>+ let window = aContext.linkedBrowser.contentWindow;
>+ let hudId = "hud_" + aContext.linkedPanel;
>+ let displayNode = aContext.ownerDocument.getElementById(hudId);
This spreads an existing problem with HUD code that I noticed in bug 596371 (see bug 596371 comment 10 and bug 596371 comment 15). You shouldn't rely on getElementById() returning the anonymous notificationbox element. I'll file a bug on the general problem, but in the mean time, we shouldn't make it worse. This should be able to use aContext.ownerDocument.defaultView.getNotificationBox(aContext.linkedBrowser.contentWindow) instead.
>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js b/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
> function testEnd() {
> // testUnregister();
> executeSoon(function () {
> HUDService.deactivateHUDForContext(tab);
>- HUDService.shutdown();
>+ //HUDService.shutdown();
> });
This bothers me a bit - why is the executeSoon needed at all? Assuming it really is needed, can't you just also put the finish() inside the executeSoon? And why is testUnregister() duplicating the executeSoon()ed code?
Attachment #477231 -
Flags: review?(gavin.sharp)
Attachment #477231 -
Flags: review-
Attachment #477231 -
Flags: approval2.0?
Assignee | ||
Comment 7•14 years ago
|
||
(In reply to comment #6)
> Comment on attachment 477231 [details] [diff] [review]
> updated patch
>
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>
> > deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)
>
> >+ let window = aContext.linkedBrowser.contentWindow;
> >+ let hudId = "hud_" + aContext.linkedPanel;
> >+ let displayNode = aContext.ownerDocument.getElementById(hudId);
>
> This spreads an existing problem with HUD code that I noticed in bug 596371
> (see bug 596371 comment 10 and bug 596371 comment 15). You shouldn't rely on
> getElementById() returning the anonymous notificationbox element. I'll file a
> bug on the general problem, but in the mean time, we shouldn't make it worse.
> This should be able to use
> aContext.ownerDocument.defaultView.getNotificationBox(aContext.linkedBrowser.contentWindow)
> instead.
I wasn't aware of that situation. Thanks for pointing it out to me.
Will fix the patch.
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js b/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
>
> > function testEnd() {
> > // testUnregister();
> > executeSoon(function () {
> > HUDService.deactivateHUDForContext(tab);
> >- HUDService.shutdown();
> >+ //HUDService.shutdown();
> > });
>
> This bothers me a bit - why is the executeSoon needed at all? Assuming it
> really is needed, can't you just also put the finish() inside the executeSoon?
> And why is testUnregister() duplicating the executeSoon()ed code?
Same thoughts here... I just made a minimal change to make the code run fine (see comment 3).
I'll update the patch to remove the commented lines, and will call finish() inside executeSoon(), after deactivateHUDForContext().
Thanks for your review!
Assignee | ||
Comment 8•14 years ago
|
||
Patch updated based on review.
I should note that the code wasn't trying to retrieve the notificationbox. It is trying to reach the HUD node. Anyhow, I made the change to first get to the notificationbox using gBrowser.getNotificationBox(aContext.linkedBrowser), then with a querySelector() I retrieve the HUD node.
Hopefully the updates properly address your review comments. Thanks!
Attachment #477231 -
Attachment is obsolete: true
Attachment #477582 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0921] → [patchclean:0922]
Assignee | ||
Comment 9•14 years ago
|
||
As per IRC discussion: dropping the use of gBrowser for getNotificationBox().
Attachment #477582 -
Attachment is obsolete: true
Attachment #477607 -
Flags: review?(gavin.sharp)
Attachment #477582 -
Flags: review?(gavin.sharp)
Comment 10•14 years ago
|
||
Comment on attachment 477607 [details] [diff] [review]
updated patch
>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> /**
>- * Activate a HeadsUpDisplay for the current window
>+ * Activate a HeadsUpDisplay for the given tab context.
> *
>- * @param nsIDOMWindow aContext
>+ * @param Element aContext the tab element.
> * @returns void
Can we just change the parameter name to "aTab" and stop using "context" in all of the methods for which "context" is a tab? Or better yet, stop passing around tabs entirely and just have these methods take content windows (from which you can get tabs/browsers/displayNodes etc.). Followup bug, I suppose.
> deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)
>- var displayNode = this.getHeadsUpDisplay(hudId);
We need to get rid of this method. Followup bug?
>+ let hudId = "hud_" + nBox.getAttribute("id");
nBox.id
>+ if (hudId in this.displayRegistry && displayNode) {
>+ this.unregisterActiveContext(hudId);
>+ this.unregisterDisplay(displayNode);
>+ if (window) {
>+ window.focus();
I don't think it's possible for tab.linkedBrowser.contentWindow to be null, so the check should be unnecessary.
>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js
>+Cu.import("resource://gre/modules/HUDService.jsm");
Browser chrome tests shouldn't import things into the global scope. Another followup bug?
Attachment #477607 -
Flags: review?(gavin.sharp) → review+
Updated•14 years ago
|
blocking2.0: --- → betaN+
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10)
> Comment on attachment 477607 [details] [diff] [review]
> updated patch
>
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>
> > /**
> >- * Activate a HeadsUpDisplay for the current window
> >+ * Activate a HeadsUpDisplay for the given tab context.
> > *
> >- * @param nsIDOMWindow aContext
> >+ * @param Element aContext the tab element.
> > * @returns void
>
> Can we just change the parameter name to "aTab" and stop using "context" in all
> of the methods for which "context" is a tab? Or better yet, stop passing around
> tabs entirely and just have these methods take content windows (from which you
> can get tabs/browsers/displayNodes etc.). Followup bug, I suppose.
>
Good point. As far as I know there's a bug report about something like this, already. It's about storing a reference to the browser element and inferring stuff from that. Would help with the case you are suggesting as well. (can't find the bug number, argh)
> > deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)
>
> >- var displayNode = this.getHeadsUpDisplay(hudId);
>
> We need to get rid of this method. Followup bug?
Not sure about this. The getHeadsUpDisplay() method is still *very* much user all over the place. Removing this method would perhaps mean a refactor of the HUDService.
> >+ let hudId = "hud_" + nBox.getAttribute("id");
>
> nBox.id
Hehe, will do.
> >+ if (hudId in this.displayRegistry && displayNode) {
> >+ this.unregisterActiveContext(hudId);
> >+ this.unregisterDisplay(displayNode);
> >+ if (window) {
> >+ window.focus();
>
> I don't think it's possible for tab.linkedBrowser.contentWindow to be null, so
> the check should be unnecessary.
Will update.
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js
>
> >+Cu.import("resource://gre/modules/HUDService.jsm");
>
> Browser chrome tests shouldn't import things into the global scope. Another
> followup bug?
Reported bug 602970.
Thanks for the review+!
Assignee | ||
Comment 12•14 years ago
|
||
Patch update 5, based on review.
Attachment #477607 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [patchclean:0922] → [patchclean:1008]
Assignee | ||
Comment 13•14 years ago
|
||
Rebased patch. Only test file changes.
Attachment #481917 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:1008] → [patchclean:1012]
Updated•14 years ago
|
Keywords: checkin-needed
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•