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

RESOLVED FIXED

Status

Other Applications
DOM Inspector
--
enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: crussell, Assigned: crussell)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 7 obsolete attachments)

12.09 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
7.82 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
20.00 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
2.26 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
5.03 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
36.20 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
13.90 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
5.60 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
4.59 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
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.
Created attachment 480467 [details] [diff] [review]
phase 1 - clean up the xul we're going to touch
Assignee: nobody → Sevenspade
Status: NEW → ASSIGNED
Attachment #480467 - Flags: review?(neil)
Attachment #480467 - Attachment description: clean up the xul we're going to touch → phase 1 - clean up the xul we're going to touch
Created 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

This conflicts with bug 589299.  Review them in whichever order you want, and I'll update the other.
Attachment #480468 - Flags: review?(neil)
Created attachment 480469 [details] [diff] [review]
phase 3 - wire up dom viewer to acknowledge and react to cmdEditInspectInNewWindow
Attachment #480469 - Flags: review?(neil)
Attachment #480468 - Attachment description: 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 → 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
Attachment #480469 - Attachment description: wire up dom viewer to acknowledge and react to cmdEditInspectInNewWindow → phase 3 - wire up dom viewer to acknowledge and react to cmdEditInspectInNewWindow
Created attachment 480470 [details] [diff] [review]
phase 4 - make jsObject viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction
Attachment #480470 - Flags: review?(neil)
Created attachment 480471 [details] [diff] [review]
phase 5 - make accessibleTree viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction
Attachment #480471 - Flags: review?(neil)
Created attachment 480472 [details] [diff] [review]
phase 6 - make accessibleProps viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction
Attachment #480472 - Flags: review?(neil)
Created attachment 480473 [details] [diff] [review]
phase 7 - make accessibleRelations viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction
Attachment #480473 - Flags: review?(neil)
Created attachment 480474 [details] [diff] [review]
phase 8 - shuffle non-English entities around
Attachment #480474 - Flags: review?(neil)
Phases 3 through 8 are independent and can actually come in any order, I think.
Blocks: 533124
Blocks: 537994

Updated

7 years ago
Attachment #480467 - Flags: review?(neil) → review+
I pushed an altered version of attachment 480467 [details] [diff] [review]: http://hg.mozilla.org/dom-inspector/rev/852fb84645a7
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.
Attachment #480468 - Flags: review?(neil)
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 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?
Created attachment 485190 [details] [diff] [review]
phase 2 - consolidate Inspect in New Window entities from dom.dtd and inspector.dtd mkII, fix bitrot
Attachment #480468 - Attachment is obsolete: true
Attachment #480469 - Attachment is obsolete: true
Attachment #480471 - Attachment is obsolete: true
Attachment #480473 - Attachment is obsolete: true
Attachment #480474 - Attachment is obsolete: true
Attachment #485190 - Flags: review?(neil)
Attachment #480469 - Flags: review?(neil)
Attachment #480471 - Flags: review?(neil)
Attachment #480473 - Flags: review?(neil)
Attachment #480474 - Flags: review?(neil)
Created attachment 485191 [details] [diff] [review]
phase 3 - wire up dom viewer to acknowledge and react to cmdEditInspectInNewWindow mkII, fix bitrot
Attachment #485191 - Flags: review?(neil)
Created attachment 485193 [details] [diff] [review]
phase 5 - make accessibleTree viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction mkII, fix bitrot
Attachment #485193 - Flags: review?(neil)
Created attachment 485194 [details] [diff] [review]
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.
Attachment #485194 - Flags: review?(neil)
Created attachment 485196 [details] [diff] [review]
phase 8 - shuffle non-English entities around mkII, fix bitrot
Attachment #485196 - Flags: review?(neil)
(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 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?
(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."?

Updated

7 years ago
Attachment #485190 - Flags: review?(neil) → review+

Updated

7 years ago
Attachment #485191 - Flags: review?(neil) → review+

Updated

7 years ago
Attachment #485193 - Flags: review?(neil) → review+
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.
Attachment #480472 - Flags: review?(neil) → review+

Updated

7 years ago
Attachment #485196 - Flags: review?(neil) → review+
Created attachment 485528 [details] [diff] [review]
phase 4 - jsObject viewer cleanup

Includes standard js cleanup, removing dead onContextCreate method, and moving unwrapObject (it's not part of the inIViewer interface)
Attachment #480470 - Attachment is obsolete: true
Attachment #485528 - Flags: review?(neil)
Attachment #480470 - Flags: review?(neil)
Created attachment 485529 [details] [diff] [review]
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.
Attachment #485529 - Flags: review?(neil)
(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.
(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.
Created attachment 485548 [details] [diff] [review]
phase 7 - make accessibleRelations viewer currentIndex-clean and wire up its cmdEditInspectInNewWindow transaction mkIII, address comment 25
Attachment #485194 - Attachment is obsolete: true
Attachment #485548 - Flags: review?(neil)
Attachment #485194 - Flags: review?(neil)

Updated

7 years ago
Attachment #485528 - Flags: review?(neil) → review+

Updated

7 years ago
Attachment #485529 - Flags: review?(neil) → review+

Updated

7 years ago
Attachment #485548 - Flags: review?(neil) → review+
(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.)
(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
Blocks: 592530
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Did I remember to moan about splitting a literal string on to two lines?
Not in this bug.

Pushed http://hg.mozilla.org/dom-inspector/rev/37b63b396aee.
You need to log in before you can comment on or make changes to this bug.