613718 - Add Copy URI and View File to XBL Bindings Viewer

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

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

This adds a shared menupopup containing Copy URI and View File to the bindings menulist, its menupopup, and the tree in the Resources tab.

Comment 1

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

Attachment: Used a shared menupopup for these items

Attachment creation failed there for some reason.

Comment 2

[[e-mail]]

What's the difference between aController.contextNode and document.popupNode?

Comment 3

[[e-mail]]

Also, should the #anchor (if any) get trimmed from the binding URI?

Comment 4

[[e-mail]]

(for View File at least; Copy URI is probably OK.)

Comment 5

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

Attachment: use document.popupNode in the BindingsListController transactions

(In reply to comment #2)
> What's the difference between aController.contextNode and document.popupNode?

That the latter was causing me trouble at one point, to which I chalked it up to a race condition (that I no longer happen to be able to reproduce anymore).

> Also, should the #anchor (if any) get trimmed from the binding URI?
> (for View File at least; Copy URI is probably OK.)

For Copy URI, I'd say the hash string is desirable, even.  I left it alone for View File because it didn't seem to make sense to write something extra to get rid of it ff it already works.

Comment 6

[[e-mail]]

Comment on attachment 492115 [details] [diff] [review]
use document.popupNode in the BindingsListController transactions

>+  this.mTree = aTree;
[You only ever use the view, so might be worth caching that here instead?]

>+  this.wrappedJSObject = this;
[I wish there was a better way...]

Comment 7

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

Attachment: followup to 492115: eliminating wrappedJSObject variant 1 - overriding setTree to initialize a reference to the controller

(In reply to comment #6)
> >+  this.wrappedJSObject = this;
> [I wish there was a better way...]

Comment 8

[[e-mail]]

Comment on attachment 492207 [details] [diff] [review]
followup to 492115: eliminating wrappedJSObject variant 1 - overriding setTree to initialize a reference to the controller

>+ResourceTreeView.prototype.setTree = function RTV_SetTree(aTreeBoxObject)
I guess you're lucky that inBaseTreeView doesn't use its mTree.

Comment 9

[[e-mail]]

Comment on attachment 492207 [details] [diff] [review]
followup to 492115: eliminating wrappedJSObject variant 1 - overriding setTree to initialize a reference to the controller

Perhaps you could set it in displayResources instead?

Comment 10

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

Attachment: followup to 492115: eliminating wrappedJSObject variant 2 - Expose accessors for cached views

Here's another variant that I was going to go with before.

(In reply to comment #8)
> I guess you're lucky that inBaseTreeView doesn't use its mTree.

(In reply to comment #9)
> Perhaps you could set it in displayResources instead?

Or I could just add the line

this.mTree = aTreeBoxObject;

to RTV_SetTree.

Comment 11

[[e-mail]]

Comment on attachment 492222 [details] [diff] [review]
followup to 492115: eliminating wrappedJSObject variant 2 - Expose accessors for cached views

So, do the other panels cache their views?

Comment 12

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

What do you mean?  I don't know which one you're excluding (with "other"), but all tree views get cached in attachment 492222 [details] [diff] [review].  The content view is created and cached in the XBLBindingsViewer constructor, the rest are created and cached in displayMethods, displayProperties, displayHandlers, and displayResources.

Comment 13

[[e-mail]]

Sorry, I meant viewers, not panels. No wonder you were confused.

Comment 14

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

Yes—at least the dom, domNode, stylesheets, styleRules, and computedStyle viewers do (I don't know and didn't check the accessibility viewers, and the jsObject viewer uses a content tree).  They keep a field, but they don't expose them with a public-looking accessor.

Comment 15

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

The difference here is that none of the other viewers really need public accessors, because they don't use controllers to handle commands/transactions.  The styleRules viewer wants to; it uses a focus event listener and the mFocusedTree field to discriminate in isCommandEnabled, for example.

Comment 16

[[e-mail]]

Comment on attachment 492222 [details] [diff] [review]
followup to 492115: eliminating wrappedJSObject variant 2 - Expose accessors for cached views

>+  contentView.whatToShow &= ~(NodeFilter.SHOW_TEXT);
[The ()s hardly seem necessary...]

Comment 17

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

Comment on attachment 492222 [details] [diff] [review]

This was supposed to be an alternative approach used in lieu of the one in attachment 492207 [details] [diff] [review].  Can I count feedback+ as review+ here?

Comment 18

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

I don't know if you're receiving bugmail for this, Neil.  I didn't think to check last night when I modified the bug.

Comment 19

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

Neil, can I get a clarification on this?  I added attachment 492207 [details] [diff] [review] with r? and then added attachment 492222 [details] [diff] [review] with feedback?.  You gave 492222 feedback+.  It was supposed to be an alternate approach to 492207, so, given feedback+, I took that to mean it was preferable.  I removed the r? flag on 492207, but I need to know if I can count that feedback+ on 492222 as r+.

Comment 20

[[e-mail]]

(In reply to comment #19)
> Neil, can I get a clarification on this?  I added attachment 492207 [details] [diff] [review] with r? and
> then added attachment 492222 [details] [diff] [review] with feedback?.  You gave 492222 feedback+.  It
> was supposed to be an alternate approach to 492207, so, given feedback+, I took
> that to mean it was preferable.  I removed the r? flag on 492207, but I need to
> know if I can count that feedback+ on 492222 as r+.
492222 is preferable to 492207. I'm undecided as to whether it's preferable to 492115. I can't remember whether I reviewed it properly or not.

Comment 21

[[e-mail]]

Huh, I can't get this to apply any more. What am I doing wrong?

Comment 22

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

(In reply to comment #20)
> 492222 is preferable to 492207. I'm undecided as to whether it's preferable to
> 492115. I can't remember whether I reviewed it properly or not.

(In reply to comment #21)
> Huh, I can't get this to apply any more. What am I doing wrong?

492222/492207 aren't supposed to replace 492115.  492222/492207 are two different approaches to eliminate the use of wrappedJSObject in 492115.  492115 should precede each when applying.  For whichever one gets r+, I'll merge it with 492115 into the same patch before I push.

Comment 23

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

Comment on attachment 492207 [details] [diff] [review]
followup to 492115: eliminating wrappedJSObject variant 1 - overriding setTree to initialize a reference to the controller

(In reply to comment #10)
> (In reply to comment #8)
> > I guess you're lucky that inBaseTreeView doesn't use its mTree.
...
> Or I could just add the line
> 
> this.mTree = aTreeBoxObject;
> 
> to RTV_SetTree.

Or I could've done inBaseTreeView.prototype.setTree.apply(this, arguments), which would be a little more "proper".

Comment 24

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

Attachment: followup to 492115: eliminating wrappedJSObject variant 2 - Expose accessors for cached views; no aController passed to transaction constructors

I just noticed that with 492222, I didn't remove the aController arg in the the Resource tree controller command constructors.

Comment 25

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

Pushed:
http://hg.mozilla.org/dom-inspector/rev/9e42bcbfc4fe
http://hg.mozilla.org/dom-inspector/rev/dcb31acffaef

I didn't merge them into the same patch, just because.