1011581 - Remove parameter aFileExt from methods in PlacesBackup.jsm

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Althaf Hameez [:ahameez]]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140512231802

Steps to reproduce:

we should remove the aFileExt argument, it is only used in a couple places with "json", that is the default btw (http://mxr.mozilla.org/mozilla-central/search?string=getMostRecentBackup)

see bug 818587 comment 11

Comment 1

[Althaf Hameez [:ahameez]]

Attachment: bug1011581_remove_aFileExt.diff

I've removed the html option as well in this patch itself if it's alright.

Comment 2

[Marco Bonardo [:mak]]

I'll look at these patches asap, in the meanwhile I'm giving you canconfirm privilege on bugzilla, good job :)

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8423993 [details] [diff] [review]
bug1011581_remove_aFileExt.diff

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

it looks good.

please attach the final patch with these comments and r=mak in the commit message, and I will take care of pushing it to Try Server for you.

You should also ask for commit level 1 if you don't have it yet, so you can push to the try server.

::: toolkit/components/places/PlacesBackups.jsm
@@ +267,2 @@
>      for (let i = 0; i < this._entries.length; i++) {
>        let rx = new RegExp("\." + fileExt + "$");

let's just hardcode "json" in the Regexp, doesn't make sense to have a var for just a single use.

@@ +282,2 @@
>       return Task.spawn(function* () {
> +       let fileExt = "json";

ditto (as-is the same as above)

Comment 4

[Althaf Hameez [:ahameez]]

Attachment: Remove aFileExt v2

fixes done

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8426297 [details] [diff] [review]
Remove aFileExt v2

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

Thanks!

I pushed to tryserver just now https://tbpl.mozilla.org/?tree=Try&rev=d5913c9c8515
once green, tree sheriffs will take care of pushing the patch

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/b472fae17716

Comment 8

[[e-mail]]

Comment on attachment 8426297 [details] [diff] [review]
Remove aFileExt v2

>-      let rx = new RegExp("\." + fileExt + "$");
>+      let rx = new RegExp("\.json$");
When you changed this from a variable to a fixed regexp you missed the chance to change it to a literal, which would have fixed the bug introduced by bug 852032.