Last Comment Bug 212754 - Should be able to copy stylesheet URIs from DOM Inspector
: Should be able to copy stylesheet URIs from DOM Inspector
Status: RESOLVED FIXED
:
Product: Other Applications
Classification: Client Software
Component: DOM Inspector (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Colby Russell :crussell (no longer Mozilla-ing)
:
Mentors:
Depends on: 536012
Blocks: 477844
  Show dependency treegraph
 
Reported: 2003-07-15 06:02 PDT by Hixie (not reading bugmail)
Modified: 2010-01-04 22:05 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
rewrite code intended to open editor; add clipboard functionality; don't use anonymous functions (15.55 KB, patch)
2009-05-31 09:44 PDT, Colby Russell :crussell (no longer Mozilla-ing)
no flags Details | Diff | Splinter Review
same as before, but fixes function names and locale string (15.55 KB, patch)
2009-05-31 10:21 PDT, Colby Russell :crussell (no longer Mozilla-ing)
no flags Details | Diff | Splinter Review
third revision; fix problems identified by sdwilsh (16.38 KB, patch)
2009-06-07 21:13 PDT, Colby Russell :crussell (no longer Mozilla-ing)
sdwilsh: review+
Details | Diff | Splinter Review
fourth revisions; fix other concerns by sdwilsh (17.53 KB, patch)
2009-06-20 10:21 PDT, Colby Russell :crussell (no longer Mozilla-ing)
bugzilla: review+
neil: superreview-
Details | Diff | Splinter Review
more changes following neil's review (15.37 KB, patch)
2009-06-23 10:45 PDT, Colby Russell :crussell (no longer Mozilla-ing)
bugzilla: review+
Details | Diff | Splinter Review
Remove lines that should stay with bug 192841 [Checkin: Comment 17] (15.17 KB, patch)
2009-06-23 21:40 PDT, Colby Russell :crussell (no longer Mozilla-ing)
bugzilla: review+
neil: superreview+
Details | Diff | Splinter Review

Description Hixie (not reading bugmail) 2003-07-15 06:02:27 PDT
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) 2003-09-05 03:20:21 PDT
Mass re-assigning bugs to dom.inspector@extensions.bugs
Comment 2 Colby Russell :crussell (no longer Mozilla-ing) 2009-05-31 09:44:30 PDT
Created attachment 380721 [details] [diff] [review]
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) 2009-05-31 10:21:37 PDT
Created attachment 380722 [details] [diff] [review]
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 2009-06-02 09:53:53 PDT
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) 2009-06-02 11:42:02 PDT
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 2009-06-02 12:02:26 PDT
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) 2009-06-07 21:13:06 PDT
Created attachment 382082 [details] [diff] [review]
third revision; fix problems identified by sdwilsh

Should take care of issues identified in last review.
Comment 8 Shawn Wilsher :sdwilsh 2009-06-19 17:15:49 PDT
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) 2009-06-19 22:26:14 PDT
What do you mean by conflicts? Functions created with function expressions won't clash with identically named functions.
Comment 10 Shawn Wilsher :sdwilsh 2009-06-20 07:18:12 PDT
conflicts with the searching ability of developers
Comment 11 Colby Russell :crussell (no longer Mozilla-ing) 2009-06-20 10:21:05 PDT
Created attachment 384237 [details] [diff] [review]
fourth revisions; fix other concerns by sdwilsh
Comment 12 neil@parkwaycc.co.uk 2009-06-22 05:46:02 PDT
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) 2009-06-23 10:45:45 PDT
Created attachment 384655 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-06-23 16:10:26 PDT
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) 2009-06-23 21:40:54 PDT
Created attachment 384811 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-06-24 04:45:36 PDT
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) 2009-07-01 06:59:26 PDT
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 2009-07-22 07:30:38 PDT
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 [:bz] 2009-07-22 09:08:13 PDT
This is fixed in the development version of DOM inspector.  2.0.3 is from February 2009; this bug was fixed in July 2009.

Note You need to log in before you can comment on or make changes to this bug.