Remove parameter aFileExt from methods in PlacesBackup.jsm

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ahameez, Assigned: ahameez)

Tracking

30 Branch
Firefox 32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 years ago
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
Assignee

Updated

5 years ago
Blocks: 818587
Assignee

Comment 1

5 years ago
I've removed the html option as well in this patch itself if it's alright.
Attachment #8423993 - Flags: review?(mak77)
Component: Untriaged → Bookmarks & History
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → althaf.mozilla
Status: NEW → ASSIGNED
I'll look at these patches asap, in the meanwhile I'm giving you canconfirm privilege on bugzilla, good job :)
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)
Attachment #8423993 - Flags: review?(mak77) → review+
Assignee

Comment 4

5 years ago
fixes done
Attachment #8423993 - Attachment is obsolete: true
Attachment #8426297 - Flags: review?(mak77)
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
Attachment #8426297 - Flags: review?(mak77) → review+
Keywords: checkin-needed
OS: Windows 8.1 → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/b472fae17716
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
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.
You need to log in before you can comment on or make changes to this bug.