Last Comment Bug 699121 - Style Editor should save file:// URLs immediately
: Style Editor should save file:// URLs immediately
Status: VERIFIED FIXED
[styleeditor][qa!]
:
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]
:
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
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 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 Victor Porof [:vporof][:vp] 2011-12-20 10:11:48 PST
Created attachment 583210 [details] [diff] [review]
v1
Comment 2 Cedric Vivier [:cedricv] 2011-12-22 00:44:42 PST
Comment on attachment 583210 [details] [diff] [review]
v1

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 Victor Porof [:vporof][:vp] 2012-02-02 08:06:35 PST
Created attachment 593844 [details] [diff] [review]
v2

Tested and works on Windows.
Try is green: https://tbpl.mozilla.org/?tree=Try&rev=2e6993b25b67
Comment 4 Victor Porof [:vporof][:vp] 2012-02-02 08:12:17 PST
Created attachment 593850 [details] [diff] [review]
v2.1

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

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

Cool.

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://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// 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(Services.io.newFileURI(gFilename).resolve(""), 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 Victor Porof [:vporof][:vp] 2012-02-06 00:28:22 PST
Got it.
Comment 7 Victor Porof [:vporof][:vp] 2012-02-08 01:30:54 PST
Created attachment 595346 [details] [diff] [review]
v2.2

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 Victor Porof [:vporof][:vp] 2012-02-08 04:17:10 PST
Comment on attachment 595346 [details] [diff] [review]
v2.2

Green: https://tbpl.mozilla.org/?tree=Try&rev=96c2febbee78
Comment 9 Cedric Vivier [:cedricv] 2012-02-08 04:30:00 PST
Comment on attachment 595346 [details] [diff] [review]
v2.2

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

Yay! Green!
Comment 10 Paul Rouget [:paul] 2012-02-08 04:45:28 PST
Not sure we can land that without a r+ from a peer: https://wiki.mozilla.org/Modules/Other#Devtools

(I can review this if you want)
Comment 11 Rob Campbell [:rc] (:robcee) 2012-02-08 06:50:08 PST
https://hg.mozilla.org/integration/fx-team/rev/017fe66d143b
Comment 12 Tim Taubert [:ttaubert] 2012-02-09 00:33:55 PST
https://hg.mozilla.org/mozilla-central/rev/017fe66d143b
Comment 13 Paul Rouget [:paul] 2012-02-09 10:46:20 PST
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.

[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 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 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
20120328051619

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