Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

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

People

(Reporter: crussell, Assigned: crussell)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Created attachment 485848 [details] [diff] [review]
cleanup styleRules.js (phase 1)
Attachment #485848 - Flags: review?(neil)
Created attachment 485849 [details] [diff] [review]
Add View File and Copy URI to editingOverlay.xul
Attachment #485849 - Flags: review?(neil)
Created attachment 485850 [details] [diff] [review]
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?
Attachment #485850 - Flags: review?(neil)
Created attachment 485855 [details] [diff] [review]
Add View File and Copy URI to editingOverlay.xul, mkII (phase2)

I didn't realize the ru localization had already been updated.
Attachment #485849 - Attachment is obsolete: true
Attachment #485855 - Flags: review?(neil)
Attachment #485849 - Flags: review?(neil)
Attachment #485848 - Attachment description: cleanup styleRules.js → cleanup styleRules.js (phase 1)
Attachment #485855 - Attachment description: Add View File and Copy URI to editingOverlay.xul, mkII → Add View File and Copy URI to editingOverlay.xul, mkII (phase2)
Attachment #485850 - Attachment description: Hook up cmdEditCopyFileURI and cmdEditViewFileURI → Hook up cmdEditCopyFileURI and cmdEditViewFileURI (phase 3)
Created attachment 485861 [details] [diff] [review]
Move non-English entities around (phase 4)
Attachment #485861 - Flags: review?(neil)
Attachment #485861 - Attachment description: Move non-English entities around → Move non-English entities around (phase 4)

Comment 5

7 years ago
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.]
(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 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.
Attachment #485850 - Flags: feedback?(sdwilsh)
(In reply to comment #6)
> Aside: maybe the style sheets viewer shouldn't be available when there are no
> style sheets?
probably
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 7 is supposed to be the rationale, by the way.
(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 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.
Attachment #485850 - Flags: feedback?(sdwilsh) → feedback+
(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.
Blocks: 609789
(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 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.
Created attachment 488384 [details] [diff] [review]
Hook up cmdEditCopyFileURI and cmdEditViewFileURI mkII (phase 3)
Attachment #485850 - Attachment is obsolete: true
Attachment #488384 - Flags: review?(neil)
Attachment #485850 - Flags: review?(neil)
Attachment #485855 - Flags: review?(neil)
Attachment #488384 - Flags: review?(neil)
(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 ...?
(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.

Updated

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

Updated

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