Style Editor should save file:// URLs immediately

VERIFIED FIXED in Firefox 12

Status

()

Firefox
Developer Tools: Style Editor
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Kevin Dangoor, Assigned: vporof)

Tracking

unspecified
Firefox 13
Points:
---

Firefox Tracking Flags

(firefox12 verified)

Details

(Whiteboard: [styleeditor][qa!])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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).
(Assignee)

Updated

6 years ago
Assignee: nobody → vporof
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

6 years ago
Component: Developer Tools → Developer Tools: Style Editor
(Assignee)

Comment 1

6 years ago
Created attachment 583210 [details] [diff] [review]
v1
Attachment #583210 - Flags: review?(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).
Attachment #583210 - Flags: review?(cedricv) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 593844 [details] [diff] [review]
v2

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

Comment 4

6 years ago
Created attachment 593850 [details] [diff] [review]
v2.1

Gah, updated a wrong patch. v2.1 is the good one.
Attachment #583210 - Attachment is obsolete: true
Attachment #593844 - Attachment is obsolete: true
Attachment #593850 - Flags: review?(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".
Attachment #593850 - Flags: review?(cedricv) → review+
Status: NEW → ASSIGNED
(Assignee)

Comment 6

6 years ago
Got it.
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

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

Green: https://tbpl.mozilla.org/?tree=Try&rev=96c2febbee78
Attachment #595346 - Flags: review?(cedricv)
Comment on attachment 595346 [details] [diff] [review]
v2.2

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

Yay! Green!
Attachment #595346 - Flags: review?(cedricv) → review+
Whiteboard: [styleeditor][land-in-fx-team]

Comment 10

6 years ago
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)
Whiteboard: [styleeditor][land-in-fx-team] → [styleeditor]
Attachment #595346 - Flags: review?(paul)

Updated

6 years ago
Attachment #595346 - Flags: review?(paul) → review+

Updated

6 years ago
Whiteboard: [styleeditor] → [styleeditor][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/017fe66d143b
Whiteboard: [styleeditor][land-in-fx-team] → [styleeditor][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/017fe66d143b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [styleeditor][fixed-in-fx-team] → [styleeditor]
Target Milestone: --- → Firefox 13

Comment 13

6 years ago
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
Attachment #595346 - Flags: approval-mozilla-beta?
Attachment #595346 - Flags: approval-mozilla-aurora?
(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.

Updated

6 years ago
Attachment #595346 - Flags: approval-mozilla-beta?
Attachment #595346 - Flags: approval-mozilla-beta-
Attachment #595346 - Flags: approval-mozilla-aurora?
Attachment #595346 - Flags: approval-mozilla-aurora+

Updated

6 years ago
Whiteboard: [styleeditor] → [styleeditor][land-in-aurora]

Comment 15

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/f076711d9973
status-firefox12: --- → fixed
Whiteboard: [styleeditor][land-in-aurora] → [styleeditor]
Whiteboard: [styleeditor] → [styleeditor][qa+]

Comment 16

6 years ago
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
Status: RESOLVED → VERIFIED
status-firefox12: fixed → verified
Whiteboard: [styleeditor][qa+] → [styleeditor][qa!]
Blocks: 725246
You need to log in before you can comment on or make changes to this bug.