1. Navigate to https://www.mozilla.org in the browser. 2. Open the DOM Inspector on that page. 3. Open the DOM Inspector's View menu Expected results: The Browser menuitem is disabled, because we attached to a document from elsewhere; the inspectee isn't loaded in the browser pane. Actual results: The Browser menuitem is enabled. Toggling it opens the browser pane with an unrelated blank document visible there.

Related-but-not-dupe with bug 159027?

Yes.

As, once loaded, a document in a Browser pane cannot being close, the menu item should be disabled until the button "Inspect" is pressed for first time in a session. The Browser pane itself can be closed, but when the menu item is selected, it will be opened again. Even trying to inspect an empty url, the current document in the pane doesn't unloads itself. Is this right, Colby?

I'm not sure about comment 3, but there are two ways to inspect a document: 1. Enter the address in the DOM Inspector's address bar and click "Inspect". 2. Use the "Inspect Content Document" or "Inspect Chrome Document" menus to attach to a document that's already loaded. The browser pane exists for case 1. In case 2, the browser pane is entirely irrelevant; the inspected document is loaded through some other browser and is not displayed in the browser pane. When we attach to a document as in case 2, a few things should happen: - If the browser pane is already open, it should be closed - The menuitem to toggle the browser in the View menu should be disabled When we inspect a document as in case 1, if the menuitem has been previously disabled as above, it should be re-enabled.

Ok, from comment 4 now it seems clearer to me what needs to be done. Sorry to misunderstand the description of the bug.

Done with the easy part of disabling/enabling the menu depending on the first action (inspecting a URL or inspecting a externally loaded URL). I will try to improve it for the following actions from the user.

Could you review this, Neil? Thank you in advance. Patch uses a variable to be able to differentiate when to disable the menu item and close the unrelated Browser panel.

Comment on attachment 586554 [details] [diff] [review] patch >+ var cmd = document.getElementById("cmdToggleDocument"); >+ cmd.setAttribute("disabled", true); I think you would be better off setting the command to disabled in the XUL. > gotoURL: function IA_GotoURL(aURL, aNoSaveHistory) > { > this.mPendingURL = aURL; > this.mPendingNoSave = aNoSaveHistory; > this.browseToURL(aURL); > this.setBrowser(true, true); >+ var cmd = document.getElementById("cmdToggleDocument"); >+ cmd.setAttribute("disabled", false); You don't need to do this because it will get done when the document loads. >+ this.mIsTargetWindow = true; > this.setTargetWindow(content); >+ this.mIsTargetWindow = false; I don't think global state is the right way to do this. Two possibilities come to mind: 1. Change setTargetDocument to automatically detect whether it's being set to the internal browser content document or an external document. 2. Pass a second parameter to setTargetWindow/setTargetDocument to indicate whether the internal browser is loading or an external target was selected.

Implements option 2 from comment 8. Passes "true" to the "setTargetDocument()" function when the document is from an external source. Passing "false" to it will open the Browser splitter and enable the "Show Document in Browser" check-box menu item. This behaves as expected when switching between the "Inspect" button, the "Inspect Content Document" menu item and the "Inspect Chrome Document" menu item.

Comment on attachment 587060 [details] [diff] [review] patch 1.1 >+ setTargetDocument: function IA_SetTargetDocument(aDoc, aIsExternal) I'm still undecided as to whether the default should be to assume that this is an internal or external document. At the moment I still have a slight preference that the internal browser should have to pass true, but if you don't like it feel free to try to convince me that your way is better. >+ this.openSplitter("Browser", !aIsExternal); I just noticed that you probably should call setBrowser instead, otherwise you won't set cmdToggleBrowser menuitem's checked state correctly.

Ok. I changed to setBrowser() and also from aIsExternal to aIsInternal (so setTargetWindow function could pass true when using the internal browser). Initially I thought the especial case was the externally loaded document, although it was the case that DOM Inspector uses when it opens. Maybe a little bit strange, yeah. So I am going to use your recommendation. I inserted code to make a decision in the case setTargetDocument didn't get a value for isInternal. In my last patch it uses false as a default value. This way the patch will only modify inspector.js mainly and popupOverlay.xul only in order to disable the menu item at start.

Also, when introducing this new parameter, setTargetWindow could be removed.

Comment on attachment 587060 [details] [diff] [review] patch 1.1 Cancelling review presuming that a better patch is on the way.

Could you review this patch, Neil? I have used "false" as the default value in case it is not passed as a parameter when calling this function. My decision was based in making the few changes as possible, and if using this value it is possible to not to change XUL files (menu items declaration). In code I modified or created the parameter is always passed. In exception of said XUL file.

Comment on attachment 591453 [details] [diff] [review] patch 1.2 OK this looks good, thanks.

Comment on attachment 591453 [details] [diff] [review] patch 1.2 Review of attachment 591453 [details] [diff] [review]: ----------------------------------------------------------------- ::: resources/content/inspector.js @@ +590,5 @@ > { > + var cmd = document.getElementById("cmdToggleDocument"); > + > + if (typeof aIsInternal == "undefined") > + aIsInternal = false; Two things: Local style has braces around even single-line bodies of control statements like if/else/while/et cetera. Also, in other places when checking whether an argument is undefined, we simply do a strict equality check against undefined, i.e.: if (aIsInternal === undefined) { It's true that (in previous versions of JS) the global "undefined" can be assigned some other value, but this is not something we guard against, because this is chrome, not the web; arbitrary third-party code messing up our globals is not a real risk here.

(In reply to Colby Russell :crussell from comment #16) > Two things: > Local style has braces around even single-line bodies of control statements > like if/else/while/et cetera. > if (aIsInternal === undefined) { Are you asking for solve this 2 problems and re-attach it again? Just to be sure of it :-)

(In reply to Javi Rueda from comment #17) > Are you asking for solve this 2 problems and re-attach it again? Just to be > sure of it :-) With regard to the braces, yes, please. Whether you want to change the |typeof aIsInternal == "undefined"| part is your choice.

Actually, you don't need to submit the changes for another review. Just making sure it's changed before you commit it to the repository is okay. And thanks for the patch.

(In reply to Colby Russell :crussell from comment #19) > Actually, you don't need to submit the changes for another review. Just > making sure it's changed before you commit it to the repository is okay. > > And thanks for the patch. I don't have check-in privilege on any Mozilla repository, so I flag it as needed, Colby. :-)

(In reply to Colby Russell :crussell from comment #19) > Actually, you don't need to submit the changes for another review. Just > making sure it's changed before you commit it to the repository is okay. > > And thanks for the patch. I don't have the check-in privilege over any Mozilla repository, so I flag it as needed, Colby. :-)

Pushed: http://hg.mozilla.org/dom-inspector/rev/353582009a92