734666 - Style Editor default filename for saving

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P2)

Description

[sys.sgx]

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.

Comment 1

[Willian Gustavo Veiga]

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

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 8344419 [details] [diff] [review]
734666-on-save-suggests-the-filename.patch

Rob, would you mind reviewing or finding a reviewer?

Comment 3

[Rob Campbell [:rc] (:robcee)]

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!

Comment 4

[Heather Arthur [:harth]]

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.

Comment 5

[Heather Arthur [:harth]]

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 6

[Heather Arthur [:harth]]

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.

Comment 7

[Willian Gustavo Veiga]

Attachment: 734666-on-save-suggests-the-filename.patch

This patch seems to fix the issues.
Thank you guys!

Comment 8

[Heather Arthur [:harth]]

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

Comment 9

[Heather Arthur [:harth]]

Attachment: Combined patch

I went ahead and combined them, and it works great. Try build ongoing: https://tbpl.mozilla.org/?tree=Try&rev=9e6f230e6411

Comment 10

[Heather Arthur [:harth]]

Attachment: Combined patch, for check in

Passed try. Thanks a bunch for the patches, Willian.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/974401cb4866

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/974401cb4866