588071 - Add "Inspect in New Window" to the Edit menu and give it a shortcut

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

[Colby Russell :crussell (no longer Mozilla-ing)]

The following viewers have an "Inspect in New Window" command/context item:
dom, jsObject, accessibleProps, accessibleTree

There are use cases where "Inspect in New Window" would be useful in these viewers (i.e., they should have such an item, but don't):
styleRules, stylesheets, xblBindings

There are probably more.

We should just put this item in the global Edit menu. For those viewers which already include this item in their context menus, we should add a keyboard shortcut for it.

Comment 1

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 1 - clean up the xul we're going to touch

Comment 2

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 2 - consolidate Inspect in New Window entities from dom.dtd and inspector.dtd, by moving them into editing.dtd and replacing viewer-specific menuitems with one that uses the new entity in a common overlay

This conflicts with bug 589299.  Review them in whichever order you want, and I'll update the other.

Comment 3

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 3 - wire up dom viewer to acknowledge and react to cmdEditInspectInNewWindow

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 4 - make jsObject viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction

Comment 5

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 5 - make accessibleTree viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction

Comment 6

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 6 - make accessibleProps viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction

Comment 7

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 7 - make accessibleRelations viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 8 - shuffle non-English entities around

Comment 9

[Colby Russell :crussell (no longer Mozilla-ing)]

Phases 3 through 8 are independent and can actually come in any order, I think.

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

I pushed an altered version of attachment 480467 [details] [diff] [review]: http://hg.mozilla.org/dom-inspector/rev/852fb84645a7

Comment 11

[[e-mail]]

Comment on attachment 480468 [details] [diff] [review]
phase 2 - consolidate Inspect in New Window entities from dom.dtd and inspector.dtd, by moving them into editing.dtd and replacing viewer-specific menuitems with one that uses the new entity in a common overlay

No obvious issues with the patch, but cancelling review as per comment #2.

Comment 12

[[e-mail]]

Comment on attachment 480470 [details] [diff] [review]
phase 4 - make jsObject viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction

>+function cmdEditInspectInNewWindow()
>+{
>+  var sel = getSelectedItem();
>+  this.mVal = sel ? sel.__JSValue__ : null;
>+}
>+
>+cmdEditInspectInNewWindow.prototype = {
>+  isTransient: true,
>+  merge: txnMerge,
>+  QueryInterface: txnQueryInterface,
>+
>+  doTransaction: function InspectInNewWindow_DoTransaction()
>+  {
>+    if (this.mVal) {
>+      inspectObject(this.mVal);
>+    }
>+  }
>+};
Why not write this as
this.mVal = getSelectedItem();
...
if (this.mVal) {
  inspectObject(this.mVal.__JSValue__);
}

Comment 13

[[e-mail]]

Comment on attachment 480473 [details] [diff] [review]
phase 7 - make accessibleRelations viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction

>+AccessibleTargetsView.prototype.getSelectedDOMNode =
>+  function getSelectedDOMNode()
>+{
>+  var targetsView = document.getElementById("olAccessibleTargets").view; 
Isn't that the same as "this"?

>+  var targetsView =
>+    document.getElementById("olAccessibleTargets").view.wrappedJSObject; 
Is there no way to get at the mTargetsView property?

Comment 14

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 2 - consolidate Inspect in New Window entities from dom.dtd and inspector.dtd mkII, fix bitrot

Comment 15

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 3 - wire up dom viewer to acknowledge and react to cmdEditInspectInNewWindow mkII, fix bitrot

Comment 16

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 5 - make accessibleTree viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction mkII, fix bitrot

Comment 17

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 7 - make accessibleRelations viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction mkII, fix bitrot and address comment 13

(In reply to comment #13)
> >+  var targetsView =
> >+    document.getElementById("olAccessibleTargets").view.wrappedJSObject; 
> Is there no way to get at the mTargetsView property?

I was trying to respect the "private" status of that field.  I guess it doesn't matter.

Comment 18

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 8 - shuffle non-English entities around mkII, fix bitrot

Comment 19

[[e-mail]]

(In reply to comment #17)
> (In reply to comment #13)
> > >+  var targetsView =
> > >+    document.getElementById("olAccessibleTargets").view.wrappedJSObject; 
> > Is there no way to get at the mTargetsView property?
> I was trying to respect the "private" status of that field.  I guess it doesn't
> matter.
I just noticed that some of the other viewers have a way of getting the selected object (thus encapsulating the access to the "private" view).

Comment 20

[[e-mail]]

Comment on attachment 480470 [details] [diff] [review]
phase 4 - make jsObject viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction

>-    var sel = getSelectedItem();
>-    if (sel)
>-      inspectObject(sel.__JSValue__);
...
>+function cmdEditInspectInNewWindow()
>+{
>+  var sel = getSelectedItem();
>+  this.mVal = sel ? sel.__JSValue__ : null;
>+}
>+
>+cmdEditInspectInNewWindow.prototype = {
>+  isTransient: true,
>+  merge: txnMerge,
>+  QueryInterface: txnQueryInterface,
>+
>+  doTransaction: function InspectInNewWindow_DoTransaction()
>+  {
>+    if (this.mVal) {
>+      inspectObject(this.mVal);
This fails when sel exists but sel.__JSValue__ is one of the primitive types that casts to false.

If it's possible for sel to be null, is it worth checking in isCommandEnabled?

Comment 21

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #19)
> (In reply to comment #17)
...
> > I was trying to respect the "private" status of that field.  I guess it doesn't
> > matter.
> I just noticed that some of the other viewers have a way of getting the
> selected object (thus encapsulating the access to the "private" view).

Is that the same as "I just noticed that's what you were doing.  It's okay to put it back."?  Or is it "I just noticed some of the other viewers were doing some other thing that differs from the original patch did, but still accomplishes what you were going for there.  Maybe you should do that thing too."?

Comment 22

[[e-mail]]

Comment on attachment 480472 [details] [diff] [review]
phase 6 - make accessibleProps viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction

> <page id="winAccessibleProps"
>       xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> 
>+  <commandset id="cmdsEditingExtras"/>
>+  <script type="application/javascript"
>+          src="chrome://inspector/content/utils.js"/>
<commandset> belongs next to the existing <commandset>. r=me with that fixed.

Comment 23

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 4 - jsObject viewer cleanup

Includes standard js cleanup, removing dead onContextCreate method, and moving unwrapObject (it's not part of the inIViewer interface)

Comment 24

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 4 real changes mkII - make jsObject viewer currentIndex-clean with respect to all its commands, and wire up its cmdEditInspectInNewWindow transaction

Also not present in the previous patch is my removal of the now-unused cmdInspectInNewView from the XUL.

Comment 25

[[e-mail]]

(In reply to comment #21)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > I was trying to respect the "private" status of that field.  I guess it doesn't
> > > matter.
> > I just noticed that some of the other viewers have a way of getting the
> > selected object (thus encapsulating the access to the "private" view).
> Is that the same as "I just noticed that's what you were doing.  It's okay to
> put it back."?  Or is it "I just noticed some of the other viewers were doing
> some other thing that differs from the original patch did, but still
> accomplishes what you were going for there.  Maybe you should do that thing
> too."?
The latter. For instance, the DOM viewer has a selectedNode property, while the accessible tree viewer has a getSelectedAccessible method.

Comment 26

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #20)
> If it's possible for sel to be null, is it worth checking in isCommandEnabled?

No, because there are only two ways for sel (now mSelectedItem in attachment 485529 [details] [diff] [review]) to be null:

1) getSelectedItem returns null because selection count == 1.  This is the check that isCommandEnabled performs now.
2) getSelectedItem returns null because tree.contentView.getItemAtIndex returns null.  This isn't possible.  It will always return a DOM node or throw an exception.

Comment 27

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 7 - make accessibleRelations viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction mkIII, address comment 25

Comment 28

[[e-mail]]

(In reply to comment #26)
> (In reply to comment #20)
> > If it's possible for sel to be null, is it worth checking in isCommandEnabled?
> No, because
Actually I was thinking of something else that turned out to be a bad idea.
(Well an inconsistent idea at least.)

Comment 29

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #22)
> Comment on attachment 480472 [details] [diff] [review]
...
> >+  <commandset id="cmdsEditingExtras"/>
> >+  <script type="application/javascript"
> >+          src="chrome://inspector/content/utils.js"/>
> <commandset> belongs next to the existing <commandset>. r=me with that fixed.

It was actually the same commandset.  It has been removed.

Pushed:
http://hg.mozilla.org/dom-inspector/rev/6c6d01554584
http://hg.mozilla.org/dom-inspector/rev/f5cbc92ebfcd
http://hg.mozilla.org/dom-inspector/rev/a0e1d2ff94f2
http://hg.mozilla.org/dom-inspector/rev/5f1abfaea12c
http://hg.mozilla.org/dom-inspector/rev/597b50983bcc
http://hg.mozilla.org/dom-inspector/rev/2f106556e4dd
http://hg.mozilla.org/dom-inspector/rev/6690e0bd7a91
http://hg.mozilla.org/dom-inspector/rev/ec124ac0cf10

Comment 30

[[e-mail]]

Did I remember to moan about splitting a literal string on to two lines?

Comment 31

[Colby Russell :crussell (no longer Mozilla-ing)]

Not in this bug.

Pushed http://hg.mozilla.org/dom-inspector/rev/37b63b396aee.