Should be able to copy stylesheet URIs from DOM Inspector

RESOLVED FIXED

Status

Other Applications
DOM Inspector
--
enhancement
RESOLVED FIXED
14 years ago
8 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: crussell)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

14 years ago
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.)

Updated

14 years ago
OS: Windows 2000 → All
Hardware: PC → All
Mass re-assigning bugs to dom.inspector@extensions.bugs
Assignee: caillon → dom.inspector
Product: Core → Other Applications
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
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.
Attachment #380721 - Flags: review?(sdwilsh)
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.
Attachment #380721 - Attachment is obsolete: true
Attachment #380722 - Flags: review?(sdwilsh)
Attachment #380721 - Flags: review?(sdwilsh)

Updated

8 years ago
Blocks: 477844
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.
Attachment #380722 - Flags: review?(sdwilsh)
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?
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.
Created attachment 382082 [details] [diff] [review]
third revision; fix problems identified by sdwilsh

Should take care of issues identified in last review.
Attachment #380722 - Attachment is obsolete: true
Attachment #382082 - Flags: review?(sdwilsh)

Updated

8 years ago
Assignee: nobody → Sevenspade
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.
Attachment #382082 - Flags: review?(sdwilsh) → review+
What do you mean by conflicts? Functions created with function expressions won't clash with identically named functions.
Status: NEW → ASSIGNED
conflicts with the searching ability of developers
Created attachment 384237 [details] [diff] [review]
fourth revisions; fix other concerns by sdwilsh
Attachment #382082 - Attachment is obsolete: true
Attachment #384237 - Flags: superreview?(neil)
Attachment #384237 - Flags: review+
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 ;-) ]
Attachment #384237 - Flags: superreview?(neil) → superreview-
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.
Attachment #384237 - Attachment is obsolete: true
Attachment #384655 - Flags: superreview?(neil)
Attachment #384655 - Flags: review+
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?
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.
Attachment #384655 - Attachment is obsolete: true
Attachment #384811 - Flags: superreview?(neil)
Attachment #384811 - Flags: review+
Attachment #384655 - Flags: superreview?(neil)
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 ;-)
Attachment #384811 - Flags: superreview?(neil) → superreview+
Keywords: checkin-needed
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
}
Attachment #384811 - Attachment description: Remove lines that should stay with bug 192841 → Remove lines that should stay with bug 192841 [Checkin: Comment 17]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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

8 years ago
This is fixed in the development version of DOM inspector.  2.0.3 is from February 2009; this bug was fixed in July 2009.
Depends on: 536012
You need to log in before you can comment on or make changes to this bug.