212754 - Should be able to copy stylesheet URIs from DOM Inspector

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

[Hixie (not reading bugmail)]

If you are looking at the list of style rules for a node in the DOM inspector,
you should be able to right click on the rule row and copy the URI to the clipboard.

(It would also be cool if you could right click and select "go to stylesheet" or
some such to jump to the relevant place in the Stylesheets list.)

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass re-assigning bugs to [e-mail]

Comment 2

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

Attachment: rewrite code intended to open editor; add clipboard functionality; don't use anonymous functions

There was already a hack that attempted to open notepad.exe on the URI for the selected style rule, so I changed it to call the toolkit source viewer. This has the added benefit that if the app is configured to use an external editor, it will use that (unless you're on 1.9.0). I also added a menuitem to copy the URI to the clipboard, as requested.

There was also a comment in styleRules.js about not using anonymous functions, so I changed them all to named functions. I hope this isn't too spammy in the patch.

Comment 3

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

Attachment: same as before, but fixes function names and locale string

It just occurred to me that the string "Open File in Editor" didn't make sense when external editors aren't configured, and that the corresponding function name was a misnomer. Also, some of the function names didn't match the property names they were attached to, so I fixed that.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 380722 [details] [diff] [review]
same as before, but fixes function names and locale string

>+++ b/resources/content/viewers/styleRules/styleRules.js
>-  addObserver: function(aEvent, aObserver) { this.mObsMan.addObserver(aEvent, aObserver); },
>-  removeObserver: function(aEvent, aObserver) { this.mObsMan.removeObserver(aEvent, aObserver); },
>+  addObserver: function addObserver(aEvent, aObserver) { this.mObsMan.addObserver(aEvent, aObserver); },
>+  removeObserver: function removeObserver(aEvent, aObserver) { this.mObsMan.removeObserver(aEvent, aObserver); },
nit: since you are touching these lines, please update the style so bracing is on a new line like the rest of the file.

>+    if (!selectedURI) {
>+      return;
>+    }
nit: no braces for a one line if please

>+    try {
>+      // 1.9.0 toolkit doesn't have this method
>+      gViewSourceUtils.viewSource(selectedURI, null, null, lineNumber);
>+    }
>+    catch (ex) {
>+      openDialog("chrome://global/content/viewSource.xul",
>+                 "_blank",
>+                 "all,dialog=no",
>+                 selectedURI, null, null, lineNumber, null);
couple things here.  First, where does gViewSourceUtils come from?  I don't see us importing viewSourceUtils.js, but I might be missing something.  Additionally, it'd be better if we tested for "viewSource" in gViewSourceUtils than blindly wrapping this in a try-catch block.

>+++ b/resources/content/viewers/styleRules/styleRules.xul
>+  <commandset id="cmdsRules">
>+    <!-- These oncommand handlers were stubbed out and never written
>+    <command id="cmdNewRule" oncommand="viewer.cmdNewRule();"/>
>+    <command id="cmdDeleteSelectedRule" oncommand="viewer.cmdDeleteSelectedRule();"/>
>+    <command id="cmdToggleSelectedRule" oncommand="viewer.cmdToggleSelectedRule();"/>
>+    -->
I'm not so sure we want to bother adding this in at this point, but maybe we should file a bug on them.

>+      <!-- See above note about cmdsRules
>+      <menuitem label="&newRules.label;" id="mnNewRule" command="cmdNewRule"/>
>+      <menuitem label="&deleteSelectedRules.label;" id="mnDeleteSelectedRule" command="cmdDeleteSelectedRule"/>
>+      <menuitem label="&disableSelectedRules.label;" id="mnToggleSelectedRule" command="cmdToggleSelectedRule"/>
>       <menuseparator/>
>-      <menuitem label="&openSelectedFileInEditor.label;" oncommand="viewer.cmdOpenSelectedFileInEditor()"/>
>+      -->
likewise

>+      <menuitem label="&copySelectedFileURI.label;" id="mnCopySelectedFileURI" command="cmdCopySelectedFileURI"/>
>+      <menuitem label="&viewSelectedFileURI.label;" id="mnViewSelectedFileURI" command="cmdViewSelectedFileURI"/>
also, make sure you line wrap at 80 characters.  These lines are close

>+++ b/resources/locale/en-US/viewers/styleRules.dtd
You'll need to update the makefile too because locales will break with this change.

Comment 5

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

Regarding line wrapping:
I work within 80 columns, but many (most?) of the files in the inspector use a mixture of manual linebreaks and allowing lines to span 100+ columns. You'll see lines in the sections immediately preceding and following the altered lines in styleRules.xul breaking the 80 column barrier, for example. Should a code style bug be filed on the whole of the inspector codebase to fix this?

Comment 6

[Shawn Wilsher :sdwilsh]

I only care about 80 characters if you are touching the line, or for new code.  No need to fix the old code unless we are already there.  Now, if you want to fix the rest in a different bug, that's OK too.

Comment 7

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

Attachment: third revision; fix problems identified by sdwilsh

Should take care of issues identified in last review.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 382082 [details] [diff] [review]
third revision; fix problems identified by sdwilsh

>+  onPopupShowing: function onPopupShowing(commandsetId)
nit: aCommandSetId please

> StyleRuleView.prototype.__defineGetter__("rowCount",
>-function() 
>+function getRowCount() 
this can actually stay anonymous

>+++ b/resources/content/viewers/styleRules/styleRules.xul	Sun Jun 07 23:05:18 2009 >+  <script type="application/x-javascript" 
>+          src="chrome://global/content/viewSourceUtils.js"/>
nit: just use application/javascript here please

In general, you want to name the functions with a prefix to avoid conflicts too.  For example, everything that is part of StyleRuleView would pre prefixed with SRV_ and then the function name.

r=sdwilsh with these changes.  You'll want to request sr from neil@httl with your new patch.

Comment 9

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

What do you mean by conflicts? Functions created with function expressions won't clash with identically named functions.

Comment 10

[Shawn Wilsher :sdwilsh]

conflicts with the searching ability of developers

Comment 11

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

Attachment: fourth revisions; fix other concerns by sdwilsh

Comment 12

[[e-mail]]

Comment on attachment 384237 [details] [diff] [review]
fourth revisions; fix other concerns by sdwilsh

>-  cmdNewRule: function()
>+  cmdNewRule: function SRVr_CmdNewRule()
>   {
>   },
>   
>-  cmdToggleSelectedRule: function()
>+  cmdToggleSelectedRule: function SRVr_CmdToggleSelectedRule()
>   {
>   },
> 
>-  cmdDeleteSelectedRule: function()
>+  cmdDeleteSelectedRule: function SRVr_CmdDeleteSelectedRule()
>   {
>   },
You removed the last "caller" of these...

>+    var row = this.mRuleTree.currentIndex;
>+    var fileCol = document.getElementById("olcFileURL");
>+    var selectedURI = this.mRuleView.getCellText(row, fileCol);
This should use getSelectedRule which you helpfully added without a caller...
(Same applies to cmdViewSelectedFileURI)

>+    var clipboard = XPCU.getService("@mozilla.org/widget/clipboardhelper;1",
>+                                    "nsIClipboardHelper");
Nit: Other files (e.g. dom.js) define kClipboardHelperCID globally.
(And they also call it helper, for some reason... better be consistent.)

>+  getSelectedRule: function SRVr_GetSelectedRule() {
Nit: inconsistent bracing, other functions put { on its own line.

>   <script type="application/x-javascript" src="chrome://inspector/content/jsutil/events/ObserverManager.js"/>
>+  <script type="application/javascript" 
>+          src="chrome://global/content/viewSourceUtils.js"/>
[I'm surprised sdwilsh didn't ask you to fix the other script tags ;-) ]

Comment 13

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

Attachment: more changes following neil's review

(In reply to comment #12)
> >   <script type="application/x-javascript" src="chrome://inspector/content/jsutil/events/ObserverManager.js"/>
> >+  <script type="application/javascript" 
> >+          src="chrome://global/content/viewSourceUtils.js"/>
> [I'm surprised sdwilsh didn't ask you to fix the other script tags ;-) ]

See comments 5 and 6 regarding inconsistent code style across DOM Inspector code. Done.

Comment 14

[[e-mail]]

Comment on attachment 384655 [details] [diff] [review]
more changes following neil's review

>-  return this.mSheetRules[aRow].style;
>+  // for CSSStyleRule, CSSFontFaceRule, CSSPageRule, and ElementCSSInlineStyle
>+  if ("style" in this.mSheetRules[aRow])
>+    return this.mSheetRules[aRow].style;
>+  return null;
The rest of the patch looks fine, but where did this change come from?

Comment 15

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

Attachment: Remove lines that should stay with bug 192841 [Checkin: Comment 17]

The style rules viewer behaves differently if the left pane is set to the stylesheets viewer. I made this change for another bug, but testing the patch with the latest changes made for your last review gave warnings in the error console. I see it's not because of something that I introduced, so I suppose it really should stay in the fix for the other bug.

Comment 16

[[e-mail]]

Comment on attachment 384811 [details] [diff] [review]
Remove lines that should stay with bug 192841
[Checkin: Comment 17]

Thanks for that, it's best to keep things separate so as to avoid another round of reviews ;-)

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 384811 [details] [diff] [review]
Remove lines that should stay with bug 192841
[Checkin: Comment 17]


http://hg.mozilla.org/dom-inspector/rev/25a73baaede4

after fixing
{
patching file resources/Makefile.in
Hunk #1 FAILED at 44
1 out of 1 hunks FAILED
}

Comment 18

[Michal 'hramrach' Suchanek]

How is this fixed?

I have no ability to copy CSS URIs in DOM Inspector 2.0.3.

There is no menu at all in the Object - CSS Style Rules view.

Comment 19

[Boris Zbarsky [:bzbarsky]]

This is fixed in the development version of DOM inspector.  2.0.3 is from February 2009; this bug was fixed in July 2009.