Style Editor default filename for saving

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Developer Tools: Style Editor
P2
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: sys.sgx, Assigned: Willian Gustavo Veiga)

Tracking

11 Branch
Firefox 29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120309131123

Steps to reproduce:

Open Style Editor.
Click the save button.


Actual results:

There is no default filename.


Expected results:

A default filename could have been suggested, the one of the original css file.
(Reporter)

Updated

6 years ago
Component: Untriaged → Developer Tools: Style Editor
OS: Windows Vista → All
Priority: -- → P3
Hardware: x86 → All
(Reporter)

Updated

6 years ago
Priority: P3 → P2

Updated

6 years ago
QA Contact: untriaged → developer.tools.style.editor
(Assignee)

Comment 1

4 years ago
Created attachment 8344419 [details] [diff] [review]
734666-on-save-suggests-the-filename.patch

My first attempt.

There's a code smell and I'm not comfortable with it. "suggestedFilename" is used only when "toSave" is true.

I'll wait your review.

Thank you very much.
Attachment #8344419 - Flags: review?
Comment on attachment 8344419 [details] [diff] [review]
734666-on-save-suggests-the-filename.patch

Rob, would you mind reviewing or finding a reviewer?
Attachment #8344419 - Flags: review? → review?(rcampbell)
Assignee: nobody → contact
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8344419 [details] [diff] [review]
734666-on-save-suggests-the-filename.patch

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

code smell. :)

This looks good William. It should probably have a unit test though I'll leave that for the ultimate reviewer here.

Thanks for the patch!
Attachment #8344419 - Flags: review?(rcampbell) → review?(fayearthur)
Comment on attachment 8344419 [details] [diff] [review]
734666-on-save-suggests-the-filename.patch

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

This is a great change. Looks good except for this one line here:

::: browser/devtools/styleeditor/StyleSheetEditor.jsm
@@ +377,5 @@
>        }.bind(this));
>      };
>  
> +    let osFile = Cu.import("resource://gre/modules/osfile.jsm");
> +    showFilePicker(file || this._styleSheetFilePath, true, this._window, onFile, osFile.OS.Path.basename(this._friendlyName));

This is a long line here, break off like `let defaultName = osFile.OS.Path.basename(this._friendlyName)` to alleviate it.
Attachment #8344419 - Flags: review?(fayearthur) → review+
Comment on attachment 8344419 [details] [diff] [review]
734666-on-save-suggests-the-filename.patch

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

One more thing...

::: browser/devtools/styleeditor/StyleSheetEditor.jsm
@@ +376,5 @@
>          this.sourceEditor.setClean();
>        }.bind(this));
>      };
>  
> +    let osFile = Cu.import("resource://gre/modules/osfile.jsm");

Put this import at the top of the file.
Comment on attachment 8344419 [details] [diff] [review]
734666-on-save-suggests-the-filename.patch

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

Just tried this out. An error occurs when there you try to save an inline style sheet, (try http://google.com, which has a lot of inline sheets). The error is:

System JS : ERROR resource://gre/modules/osfile/ospath_unix.jsm:44 - TypeError: path is undefined

We'll need to fix this, as the error stops the file picker dialog from even showing up.
(Assignee)

Comment 7

4 years ago
Created attachment 8348394 [details] [diff] [review]
734666-on-save-suggests-the-filename.patch

This patch seems to fix the issues.
Thank you guys!
Attachment #8348394 - Flags: review?(fayearthur)
(In reply to Willian Gustavo Veiga from comment #7)
> Created attachment 8348394 [details] [diff] [review]
> 734666-on-save-suggests-the-filename.patch
> 
> This patch seems to fix the issues.
> Thank you guys!

This patch seems to be based on your previous patch. Can you combine it into one patch?
Created attachment 8355746 [details] [diff] [review]
Combined patch

I went ahead and combined them, and it works great. Try build ongoing: https://tbpl.mozilla.org/?tree=Try&rev=9e6f230e6411
Created attachment 8355801 [details] [diff] [review]
Combined patch, for check in

Passed try. Thanks a bunch for the patches, Willian.
Attachment #8355746 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8344419 - Attachment is obsolete: true
Attachment #8348394 - Attachment is obsolete: true
Attachment #8348394 - Flags: review?(fayearthur)
https://hg.mozilla.org/integration/fx-team/rev/974401cb4866
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/974401cb4866
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.