Add Copy URI and View File to XBL Bindings Viewer

RESOLVED FIXED

Status

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

People

(Reporter: crussell, Assigned: crussell)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

This adds a shared menupopup containing Copy URI and View File to the bindings menulist, its menupopup, and the tree in the Resources tab.
Created attachment 492044 [details] [diff] [review]
Used a shared menupopup for these items

Attachment creation failed there for some reason.
Assignee: nobody → Sevenspade
Status: NEW → ASSIGNED
Attachment #492044 - Flags: review?(neil)

Comment 2

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

Comment 3

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

Comment 4

7 years ago
(for View File at least; Copy URI is probably OK.)
Created attachment 492115 [details] [diff] [review]
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.
Attachment #492044 - Attachment is obsolete: true
Attachment #492115 - Flags: review?(neil)
Attachment #492044 - Flags: review?(neil)

Comment 6

7 years ago
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...]
Attachment #492115 - Flags: review?(neil) → review+
Created 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 #6)
> >+  this.wrappedJSObject = this;
> [I wish there was a better way...]
Attachment #492207 - Flags: review?(neil)

Comment 8

7 years ago
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

7 years ago
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?
Created attachment 492222 [details] [diff] [review]
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.
Attachment #492222 - Flags: feedback?(neil)
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?
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.
Sorry, I meant viewers, not panels. No wonder you were confused.
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.
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 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...]
Attachment #492222 - Flags: feedback?(neil) → feedback+
Attachment #492207 - Attachment is obsolete: true
Attachment #492207 - Flags: review?(neil)
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?
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.
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+.
(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.
Huh, I can't get this to apply any more. What am I doing wrong?
(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.
Attachment #492222 - Attachment description: Expose accessors for cached views → followup to 492115: eliminating wrappedJSObject variant 2 - Expose accessors for cached views
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".
Attachment #492207 - Attachment description: don't expose ResourceTreeView implementation through wrappedJSObject → followup to 492115: eliminating wrappedJSObject variant 2 - overriding setTree to initialize a reference to the controller
Attachment #492207 - Attachment description: followup to 492115: eliminating wrappedJSObject variant 2 - overriding setTree to initialize a reference to the controller → followup to 492115: eliminating wrappedJSObject variant 1 - overriding setTree to initialize a reference to the controller
Created attachment 499700 [details] [diff] [review]
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.
Attachment #492222 - Attachment is obsolete: true
Attachment #499700 - Flags: review?(neil)
Attachment #499700 - Flags: feedback+

Updated

7 years ago
Attachment #499700 - Flags: review?(neil) → review+
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.
Blocks: 592530
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.