607080 - Add Copy URI and View File to stylesheets viewer's context menu

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

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

Attachment: cleanup styleRules.js (phase 1)

Comment 1

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

Attachment: Add View File and Copy URI to editingOverlay.xul

Comment 2

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

Attachment: Hook up cmdEditCopyFileURI and cmdEditViewFileURI (phase 3)

Comment on attachment 485850 [details] [diff] [review]
> +function cmdEditInspectInNewWindowBase()

It would've been nice if this was done before bug 588071, huh?

Comment 3

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

Attachment: Add View File and Copy URI to editingOverlay.xul, mkII (phase2)

I didn't realize the ru localization had already been updated.

Comment 4

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

Attachment: Move non-English entities around (phase 4)

Comment 5

[[e-mail]]

Comment on attachment 485850 [details] [diff] [review]
Hook up cmdEditCopyFileURI and cmdEditViewFileURI (phase 3)

>+  content/inspector/jsutil/commands/baseCommands.js                     (resources/content/jsutil/commands/baseCommands.js)
Can you get sign-off from sdwilsh on creating this new file?

>+cmdEditInspectInNewWindowBase.isInspectable =
>+  function InspectInNewWindowBase_IsInspectable(aValue)
>+{
>+  var type = typeof aValue;
>+  return (type == "object" && aValue !== null) || type == "function";
jsObjectViewer.js uses typeof(aValue) == "object" && aValue !== null
Or maybe this should be a utility method so that jsObjectViewer.js can share.
[Another possibility would be to write this as
  for (let i in aValue)
    return true;
  return false;
Thus excluding such objects as [] and {}.]

>+  this.mLineNumber = null;
Would 0 be better?

>-    this.mRules = this.mDOMUtils.getCSSStyleRules(aObject);
>+    this.mRules = viewer.DOMUtils.getCSSStyleRules(aObject);
This change makes no sense to me...

>       case "cmdEditInspectInNewWindow":
>-        return !!this.getSelectedSheet();
>+        return cmdEditInspectInNewWindowBase.isInspectable(
>+          this.getSelectedSheet()
[Although you know that getSelectedSheet never returns a primitive type.]

Comment 6

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

(In reply to comment #5)
> Comment on attachment 485850 [details] [diff] [review]
> Hook up cmdEditCopyFileURI and cmdEditViewFileURI (phase 3)
...
> >+  this.mLineNumber = null;
> Would 0 be better?

Hmm, yeah.  I was thinking that the source viewer indexed lines beginning at 0.

> >+cmdEditInspectInNewWindowBase.isInspectable =
> >+  function InspectInNewWindowBase_IsInspectable(aValue)
> >+{
> >+  var type = typeof aValue;
> >+  return (type == "object" && aValue !== null) || type == "function";
> jsObjectViewer.js uses typeof(aValue) == "object" && aValue !== null

That's set to change with my patch to bug 545565, though.  See below.

> Or maybe this should be a utility method so that jsObjectViewer.js can share.

That's part of the intent, which is basically to go back and touch everything from bug 588071. :/  (Did you see that it's a method of the constructor instead of the prototype?)

> [Another possibility would be to write this as
>   for (let i in aValue)
>     return true;
>   return false;
> Thus excluding such objects as [] and {}.]

That's actually what attachment 449543 [details] [diff] [review] for bug 545565 does to determine whether a row should be a container.

My original response was going to be, "I'm hesitant to do the same for Inspect in New Window, though, because without some other indicator that this is the reason why, it might look like it's disabled in error."

Thinking a little bit more about it, though, I'm not sure where anyone would have the chance to use the command on those kinds of objects outside the JS object viewer, where you'd have the twisty as an indicator.  The JS object viewer *does* have an Evaluate JavaScript menuitem, so it might be desirable to open a new viewer on an empty object and evaluate a snippet that sets one or more properties, at which point you could open the twisty if you wanted.  (There are currently issues with caching inherent to the way the JS object viewer uses content trees that prevents this kind of thing right now, however.)

> >-    this.mRules = this.mDOMUtils.getCSSStyleRules(aObject);
> >+    this.mRules = viewer.DOMUtils.getCSSStyleRules(aObject);
> This change makes no sense to me...

cmdEditViewFileURI needs to use inIDOMUtils to open on the correct line.  Rather than obtain the service in the command since the rule view uses it as well, I removed the rule view's field and put it on the viewer so they could share it.

> >       case "cmdEditInspectInNewWindow":
> >-        return !!this.getSelectedSheet();
> >+        return cmdEditInspectInNewWindowBase.isInspectable(
> >+          this.getSelectedSheet()
> [Although you know that getSelectedSheet never returns a primitive type.]

It can return null when there are no style sheets.  There's actually a bug right now as a result of bug 606821 where, if you're inspecting a document with no style sheets then begin inspecting one with style sheets, the menuitem remains disabled until you focus the object pane and then refocus the document pane (at which point updateAllCommands will do its thing).  Oops.

Aside: maybe the style sheets viewer shouldn't be available when there are no style sheets?

Comment 7

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

Comment on attachment 485850 [details] [diff] [review]
Hook up cmdEditCopyFileURI and cmdEditViewFileURI (phase 3)

(In reply to comment #5)
> >+  content/inspector/jsutil/commands/baseCommands.js                     (resources/content/jsutil/commands/baseCommands.js)
> Can you get sign-off from sdwilsh on creating this new file?

grepping for txnMerge shows 26 consumers (1 occurrence for the definition), with all of them roughly reproducing this boilerplate:

./resources/content/viewers/dom/dom.js-  // required for nsITransaction
./resources/content/viewers/dom/dom.js-  QueryInterface: txnQueryInterface,
./resources/content/viewers/dom/dom.js-  merge: txnMerge,
./resources/content/viewers/dom/dom.js:  isTransient: true,
./resources/content/viewers/dom/dom.js-  redoTransaction: txnRedoTransaction,

This patch introduces inBaseCommand so that the above can be achieved by those 26 consumers inheriting from inBaseCommand.

Additionally, from bug 588071 and bug 606821, there are 6 providers for cmdEditInspectInNewWindow.  All of them essentially look like the implementation found in dom.js:

> function cmdEditInspectInNewWindow()
> {
>   this.mNode = viewer.selectedNode;
> }
> 
> cmdEditInspectInNewWindow.prototype = {
>   isTransient: true,
>   merge: txnMerge,
>   QueryInterface: txnQueryInterface,
> 
>   doTransaction: function InspectInNewWindow_DoTransaction()
>   {
>     if (this.mNode) {
>       inspectObject(this.mNode);
>     }
>   }
> };

With baseCommands.js, these can all be reduced to their ~three- to four-line constructors (and the line to set the prototype).

With this patch, there will be two cmdEditCopyFileURI/cmdEditViewFileURI providers.  (Cases can be made to add two more: one to the XBL Bindings viewer and an additional one to the rule pane in the CSS Rules viewer.)  They can all benefit from a common implementation, with the consumer-specific pieces contained to their constructors (as in attachment 485850 [details] [diff] [review]).

utils.js currently contains the definitions for txnMerge, txnQueryInterface, and txnRedoTransaction, and a copy transaction.  With the addition of the three new base classes in this patch, the code mentioned has enough in common and is sizeable enough to split it off into a pseudo-module concerned with command implementations, leaving utils.js to contain the sort of miscellaneous things that remain.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Aside: maybe the style sheets viewer shouldn't be available when there are no
> style sheets?
probably

Comment 9

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

Shawn, sorry, I CCed you accidentally, before deciding to use the feedback flag with comment 7.  In comment 5, Neil says to get clearance about creating baseCommands.js.

Comment 10

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

Comment 7 is supposed to be the rationale, by the way.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> Shawn, sorry, I CCed you accidentally, before deciding to use the feedback flag
> with comment 7.  In comment 5, Neil says to get clearance about creating
> baseCommands.js.
I'm still watching the qa component so I still see all the bugmail anyway.  I'll get to this next early next week.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 485850 [details] [diff] [review]
Hook up cmdEditCopyFileURI and cmdEditViewFileURI (phase 3)

>+* TODO: Switch over transaction boilerplate throughout the codebase to use
>+* inBaseCommand and move the relevant pieces from utils.js into this file.
nit: file bugs, and reference them here please?

>+function inBaseCommand() {
>+}
nit: inconsistent bracing compared to the rest of the file.

Comment 13

[[e-mail]]

(In reply to comment #6)
>(In reply to comment #5)
>>(From update of attachment 485850 [details] [diff] [review])
>>>       case "cmdEditInspectInNewWindow":
>>>-        return !!this.getSelectedSheet();
>>>+        return cmdEditInspectInNewWindowBase.isInspectable(
>>>+          this.getSelectedSheet()
>>[Although you know that getSelectedSheet never returns a primitive type.]
>It can return null when there are no style sheets.
Sorry, I meant isInspectable(this.getSelectedSheet()) always returns the same result as !!this.getSelectedSheet() does, since it has a limited return type.

Comment 14

[[e-mail]]

(In reply to comment #6)
>(In reply to comment #5)
>>(From update of attachment 485850 [details] [diff] [review])
>>>-    this.mRules = this.mDOMUtils.getCSSStyleRules(aObject);
>>>+    this.mRules = viewer.DOMUtils.getCSSStyleRules(aObject);
>>This change makes no sense to me...
>cmdEditViewFileURI needs to use inIDOMUtils to open on the correct line. 
>Rather than obtain the service in the command since the rule view uses it as
>well, I removed the rule view's field and put it on the viewer so they could
>share it.
Except I don't see where you do that.

Comment 15

[[e-mail]]

Comment on attachment 485855 [details] [diff] [review]
Add View File and Copy URI to editingOverlay.xul, mkII (phase2)

> <!ENTITY mnEditInspectInNewWindow.label "Inspect in New Window">
> <!ENTITY mnEditInspectInNewWindow.accesskey "N">
> <!ENTITY mnEditInspectInNewWindow.key "I">
>+<!ENTITY mnEditCopyFileURI.label "Copy URI">
>+<!ENTITY mnEditCopyFileURI.accesskey "u">
>+<!ENTITY mnEditViewFileURI.label "View File">
>+<!ENTITY mnEditViewFileURI.accesskey "v">
Nit: accesskeys should be in the case that the letter is in the label, since layout does a case-sensitive match first for speed.

Comment 16

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

Attachment: Hook up cmdEditCopyFileURI and cmdEditViewFileURI mkII (phase 3)

Comment 17

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

(In reply to comment #14)
> Except I don't see where you do that.

Do what?  Put it on the viewer or use it in cmdEditViewFileURI or ...?

Comment 18

[[e-mail]]

(In reply to comment #17)
> (In reply to comment #14)
> > Except I don't see where you do that.
> Do what?  Put it on the viewer or use it in cmdEditViewFileURI or ...?
PEBKAC. I was searching for the wrong string. Oops.

Comment 19

[[e-mail]]

Comment on attachment 485855 [details] [diff] [review]
Add View File and Copy URI to editingOverlay.xul, mkII (phase2)

r=me with those access keys capitalised.

Comment 20

[[e-mail]]

Comment on attachment 488384 [details] [diff] [review]
Hook up cmdEditCopyFileURI and cmdEditViewFileURI mkII (phase 3)

r=me as my remaining issues will apparently be taken care of in other bugs.

Comment 21

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

Pushed with changes:
http://hg.mozilla.org/dom-inspector/rev/f8f07c67847b
http://hg.mozilla.org/dom-inspector/rev/1c24635b17dd
http://hg.mozilla.org/dom-inspector/rev/04619b0c014d
http://hg.mozilla.org/dom-inspector/rev/458b0fb5c00a