699121 - Style Editor should save file:// URLs immediately

Status: VERIFIED FIXED

(DevTools :: Style Editor, defect)

Description

[Kevin Dangoor]

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]]

Attachment: v1

Comment 2

[Cedric Vivier [:cedricv]]

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]]

Attachment: v2

Tested and works on Windows.
Try is green: https://tbpl.mozilla.org/?tree=Try&rev=2e6993b25b67

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v2.1

Gah, updated a wrong patch. v2.1 is the good one.

Comment 5

[Cedric Vivier [:cedricv]]

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]]

Got it.

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Comment on attachment 595346 [details] [diff] [review]
v2.2

Green: https://tbpl.mozilla.org/?tree=Try&rev=96c2febbee78

Comment 9

[Cedric Vivier [:cedricv]]

Comment on attachment 595346 [details] [diff] [review]
v2.2

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

Yay! Green!

Comment 10

[Paul Rouget [:paul]]

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)]

https://hg.mozilla.org/integration/fx-team/rev/017fe66d143b

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/017fe66d143b

Comment 13

[Paul Rouget [:paul]]

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]]

(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 15

[Dave Camp (:dcamp)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f076711d9973

Comment 16

[Ioana (away)]

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