Last Comment Bug 699121 - Style Editor should save file:// URLs immediately
: Style Editor should save file:// URLs immediately
Product: Firefox
Classification: Client Software
Component: Developer Tools: Style Editor (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 13
Assigned To: Victor Porof [:vporof][:vp]
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Depends on:
Blocks: 725246
  Show dependency treegraph
Reported: 2011-11-02 09:52 PDT by Kevin Dangoor
Modified: 2014-03-16 18:02 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (2.84 KB, patch)
2011-12-20 10:11 PST, Victor Porof [:vporof][:vp]
cedricv: review-
Details | Diff | Splinter Review
v2 (5.42 KB, patch)
2012-02-02 08:06 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.1 (3.84 KB, patch)
2012-02-02 08:12 PST, Victor Porof [:vporof][:vp]
cedricv: review+
Details | Diff | Splinter Review
v2.2 (5.65 KB, patch)
2012-02-08 01:30 PST, Victor Porof [:vporof][:vp]
cedricv: review+
paul: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description User image Kevin Dangoor 2011-11-02 09:52:09 PDT
If you're editing a local file (file:// URL), it would be nice to be able to save to the original location just by hitting the save button (without having to go through the whole Save As... style dialog).
Comment 1 User image Victor Porof [:vporof][:vp] 2011-12-20 10:11:48 PST
Created attachment 583210 [details] [diff] [review]
Comment 2 User image Cedric Vivier [:cedricv] 2011-12-22 00:44:42 PST
Comment on attachment 583210 [details] [diff] [review]

Review of attachment 583210 [details] [diff] [review]:

I was quite surprised to see such a small patch :p
Unfortunately I think it might end up not that simple :
- it doesn't seem possible for this to work on Windows (disclaimer: I haven't tried)
- as it somehow rely on the 'relative current path', when you save you lose the full path info (eg. media/default.css becomes default.css)

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +94,4 @@
>    this._styleSheet = aStyleSheet;
>    this._styleSheetIndex = -1; // unknown for now, will be set after load
> +  this._styleSheetFilePaths = []; // original file paths for the style sheets

I don't see a need for an array here?
Every StyleEditor instance can maintain its own styleSheetFilePath (singular).
Comment 3 User image Victor Porof [:vporof][:vp] 2012-02-02 08:06:35 PST
Created attachment 593844 [details] [diff] [review]

Tested and works on Windows.
Try is green:
Comment 4 User image Victor Porof [:vporof][:vp] 2012-02-02 08:12:17 PST
Created attachment 593850 [details] [diff] [review]

Gah, updated a wrong patch. v2.1 is the good one.
Comment 5 User image Cedric Vivier [:cedricv] 2012-02-05 22:13:48 PST
Comment on attachment 593850 [details] [diff] [review]

Review of attachment 593850 [details] [diff] [review]:


r+ with few nits and a required testcase change.

::: browser/devtools/styleeditor/StyleEditor.jsm
@@ +581,2 @@
>     */
>    saveToFile: function SE_saveToFile(aFile, aCallback)

Prepend "Optional." to the aFile documentation, if not specified the original style sheet URI is used.

::: browser/devtools/styleeditor/test/browser_styleeditor_filesave.js
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + */
> +
> +// http rather than chrome to improve coverage
> +const TESTCASE_URI = TEST_BASE_HTTP + "simple.html";

This testcase does test a 'normal' save to file scenario (which is already tested elsewhere).

To test this bug specifically, we need to copy simple.html and simple.css to, say, TmpD - before setting content.location to that TmpD+"simple.html" URI.
You can probably use NetUtils.asyncFetch+asyncCopy for that.

@@ +43,5 @@
> +function run(aEditor)
> +{
> +  gFilename = FileUtils.getFile("ProfD", ["styleeditor-file-test.css"]);
> +
> +  aEditor.saveToFile(""), function (aFile) {

Then we can remove gFilename here and just do aEditor.saveToFile(null, function (aFile) {)

Which should save directly to TmpD+"simple.css".
Comment 6 User image Victor Porof [:vporof][:vp] 2012-02-06 00:28:22 PST
Got it.
Comment 7 User image Victor Porof [:vporof][:vp] 2012-02-08 01:30:54 PST
Created attachment 595346 [details] [diff] [review]

I had to use nsIScriptableInputStream instead of the easier NetUtil.asyncFetch because we're dealing with chrome urls here.

Let's see if this survives try.
Comment 8 User image Victor Porof [:vporof][:vp] 2012-02-08 04:17:10 PST
Comment on attachment 595346 [details] [diff] [review]

Comment 9 User image Cedric Vivier [:cedricv] 2012-02-08 04:30:00 PST
Comment on attachment 595346 [details] [diff] [review]

Review of attachment 595346 [details] [diff] [review]:

Yay! Green!
Comment 10 User image Paul Rouget [:paul] 2012-02-08 04:45:28 PST
Not sure we can land that without a r+ from a peer:

(I can review this if you want)
Comment 11 User image Rob Campbell [:rc] (:robcee) 2012-02-08 06:50:08 PST
Comment 12 User image Tim Taubert [:ttaubert] 2012-02-09 00:33:55 PST
Comment 13 User image Paul Rouget [:paul] 2012-02-09 10:46:20 PST
Comment on attachment 595346 [details] [diff] [review]

This little patch will seriously improve the "usefulness" of the Style Editor (new feature in firefox 11). We would love to get it in Firefox 11.

[Approval Request Comment]
Regression caused by (bug #): New feature
User impact if declined: User Experience would be a little deteriorated.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low. ~15 JS line, mostly trapped in a try{}catch{}
String changes made by this patch: none
Comment 14 User image Alex Keybl [:akeybl] 2012-02-09 13:16:10 PST
(In reply to Paul Rouget [:paul] from comment #13)
> Comment on attachment 595346 [details] [diff] [review]
> v2.2
> This little patch will seriously improve the "usefulness" of the Style
> Editor (new feature in firefox 11). We would love to get it in Firefox 11.

I sympathize with the sentiment, but this doesn't meet the criteria of Beta. Given how early we are in Aurora 12, however, I'm OK with approving for that branch.
Comment 16 User image Ioana (away) 2012-04-02 06:35:18 PDT
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0

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