485265 - JSON backup should force .json extension

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now, we don't check if the backup file has the .json extension set and end-up in a file name without an extension if the user uses "backup" as the file name. The same thing can happen on Windows when the "dont show me extensions for known file types" setting is enabled (which is default for any new installation). Users will even not see this file extension in the save dialog textbox.

Finally when those users will import one of the backups again, they will not be listed because we filter on .json in the open dialog. They could think that the backup wasn't saved correctly.

Steps:
1. Open Library and click on "Import and Backup" -> Backup
2. Replace "Bookmarks 2009-03-25.json" with "test"
3. Click on Save
3. Try to restore the same backup now

As you can see the backup isn't listed because it doesn't have a .json file extension. I think it would be great if we could force the addition of the .json extension.

Comment 1

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 2

[michael.harte]

Attachment: Patch resolving issue.

first patch

Comment 3

[michael.harte]

This is the first time I've ever submitted a patch and I'm still a little hazy on the process. Feedback and constructive criticism welcome.

Comment 4

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to michael.harte from comment #3)
In Future feel free to seek Feedback by related Devs using the "needinfo" Flag :-).

Comment 5

[Raymond Lee [:raymondlee]]

Comment on attachment 785286 [details] [diff] [review]
Patch resolving issue.

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

::: toolkit/components/places/PlacesBackups.jsm
@@ +139,5 @@
>      return Task.spawn(function() {
>        if (!aFile.exists())
> +		if ( !aFile.leafName.endsWith( ".json" ) ) {
> +			aFile.leafName += ".json";
> +		}

Please use spaces instead of tabs for indentation.

Also, please add a unit test in toolkit/components/places/tests/unit to ensure your code work.
https://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests

Comment 6

[Marco Bonardo [:mak]]

Hi, first of all, thank you for you contribution

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 785286 [details] [diff] [review]
Patch resolving issue.

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

::: toolkit/components/places/PlacesBackups.jsm
@@ +139,5 @@
>      return Task.spawn(function() {
>        if (!aFile.exists())
> +		if ( !aFile.leafName.endsWith( ".json" ) ) {
> +			aFile.leafName += ".json";
> +		}

This is a backend method that should be able to store into any required file, the duty of passing in a valid file is up to the caller, that is the browser code.

More specifically, nsIFilePicker (that is a file picker implementation) has already an attribute that allows to define the behavior in the proper way for each platform. That attribute is .defaultExtension: see http://mxr.mozilla.org/mozilla-central/source/widget/nsIFilePicker.idl#105

So, my take on this is that you should rather fix the calling points by using the above attribute, in the same points where currently we just define .defaultString (that is another attribute of nsIFilePicker that sets a default file name). See:
mxr.mozilla.org/mozilla-central/search?string=defaultString&find=places.js

Comment 8

[Marco Bonardo [:mak]]

trimURL hits again, the right url:
http://mxr.mozilla.org/mozilla-central/search?string=defaultString&find=places.js

Comment 9

[Marco Bonardo [:mak]]

Also, based on a filepicker implementation, writing a test would be more time consuming than useful, so I'd not request it here.  It would have been good for the original patch, as Raymond pointed out, but that's because it was much easier to make one for such implementation.

Comment 10

[Samridh Kudesia]

Hi.
I'm new to BugZilla, and to Open Source in general.
I have a basic understanding of JS, and am looking to learn more about Open Source contributions by working on bugs.
This is the first bug I'm trying my hands on.
Could you please guide me as to where to start?
Thank you!

Comment 11

[Marco Bonardo [:mak]]

(In reply to Samridh Kudesia from comment #10)
> Hi.
> I'm new to BugZilla, and to Open Source in general.
> I have a basic understanding of JS, and am looking to learn more about Open
> Source contributions by working on bugs.
> This is the first bug I'm trying my hands on.
> Could you please guide me as to where to start?
> Thank you!

Please hae a look at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
Start by having a working build of Firefox, then you can continue with what is suggested in comment 7
Feel free to ask question in #introduction channel, or set a needinfo flag on the mentor of the bug in bugzilla.

Comment 12

[Prakhar Gupta]

Attachment: Patch for Bug 485265

Comment 13

[Prakhar Gupta]

Attachment: Patch for Bug 485265

Please review and let me know if anything needs to be changed.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8764024 [details] [diff] [review]
Patch for Bug 485265

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

::: browser/components/places/content/places.js
@@ +553,5 @@
> +        var fileSavePath=fp.file.path;
> +        if(!fileSavePath.endsWith(".json"))
> +        {
> +            fileSavePath+=".json";
> +        }

defaultExtension should already do the required appending, according to nsIFilePicker.idl:

/**
  * The extension that should be associated with files of the type we
  * want to work with.  On some platforms, this extension will be
  * automatically appended to filenames the user enters, if needed.  
  */
  attribute AString defaultExtension;

So, I don't think this change is needed at all, we should just set defaultExtension.

Comment 15

[Prakhar Gupta]

In attachment 8764024 [details] [diff] [review], I did include the line:

>+    fp.defaultExtension = "json";

But it did not produce the desired effect. I looked up for prior usage of defaultExtension here:

https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/pippki.js#98

where it is used when exporting certificates. I tried exporting certificates without using any extension and it successfully got exported. Thus defaultExtension did not force ".crt" extension here as well. In fact a comment just before the use of defaultExtension reads:

// Note: defaultExtension is more of a suggestion to some file picker
//       implementations

I am stuck here and don't know how to use defaultExtension alone to force the .json extension. Please help  me and assign me this bug if you feel I am on the right track.

Comment 16

[Rohit Kumar Jena]

Is someone working on it? Or can I work on it? Thanks. :)

Comment 17

[Marco Bonardo [:mak]]

(In reply to Rohit Kumar Jena from comment #16)
> Is someone working on it? Or can I work on it? Thanks. :)

Sorry, as you can see from previous comments, Prakhar Gupta is already working on this

(In reply to Prakhar Gupta from comment #15)
> In attachment 8764024 [details] [diff] [review], I did include the line:
> 
> >+    fp.defaultExtension = "json";
> 
> But it did not produce the desired effect.

ok, let me check and I'll let you know.

Comment 18

[Marco Bonardo [:mak]]

(In reply to Prakhar Gupta from comment #15)
> But it did not produce the desired effect. I looked up for prior usage of
> defaultExtension here:
> 
> https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/
> resources/content/pippki.js#98
> 
> where it is used when exporting certificates. I tried exporting certificates
> without using any extension and it successfully got exported. Thus
> defaultExtension did not force ".crt" extension here as well.

I tried doing the same, on Windows, I exported a certificate, removed the default suggested name and typed a random string. It saved the file with .crt extension.
I think it depends on the platform, Windows is more likely to force an extension, I think that Linux and OSX don't do that, also cause they are less dependent on files having an extension.
Thus by just using defaultExtension we are more coherent with the OS style, and if that's wrong for today's OS versions, we should fix the filepicker, rather than trying to workaround that after the fact.

Moreover a user could want, for whatever reason, to store the file with a different extension, thus your if(!fileSavePath.endsWith(".json")) check would add a further extension.

I still think we should only set defaultExtension, on Windows it should do the right thing, Mac and Linux users are more likely to be used to fight missing extension problems.

Comment 19

[Prakhar Gupta]

(In reply to Marco Bonardo [::mak] from comment #18)
> I think it depends on the platform, Windows is more likely to force an
> extension, I think that Linux and OSX don't do that, also cause they are
> less dependent on files having an extension.

The functionality of defaultExtension clearly seems to be platform dependent and it is mentioned that it that it automatically appends the extension only on some platforms.

https://dxr.mozilla.org/mozilla-central/source/widget/nsIFilePicker.idl#100

/**
  * The extension that should be associated with files of the type we
  * want to work with.  On some platforms, this extension will be
  * automatically appended to filenames the user enters, if needed.  
  */
  attribute AString defaultExtension;

I am currently using Ubuntu 15.10 but I checked this issue on a Windows 8 machine running Firefox 47.0 and it already forces the .json format automatically in case of missing extension because it is the only available file format filter to the filepicker.

> Moreover a user could want, for whatever reason, to store the file with a
> different extension, thus your if(!fileSavePath.endsWith(".json")) check
> would add a further extension.
> 
> I still think we should only set defaultExtension, on Windows it should do
> the right thing, Mac and Linux users are more likely to be used to fight
> missing extension problems.

Your argument against using the 'if(!fileSavePath.endsWith(".json")) check' seems valid but since things are working fine currently on Windows and defaultExtension not forcing file format on Linux and OSX we would not be able to see any change even after setting the value of defaultExtension.

Please correct me if went wrong anywhere above and suggest what should be done.

Comment 20

[Marco Bonardo [:mak]]

(In reply to Prakhar Gupta from comment #19)
> The functionality of defaultExtension clearly seems to be platform dependent
> and it is mentioned that it that it automatically appends the extension only
> on some platforms.

Right, and that's what we should do. It's done like that cause on those platforms it's not expected for an extension to be forced.

> I am currently using Ubuntu 15.10 but I checked this issue on a Windows 8
> machine running Firefox 47.0 and it already forces the .json format
> automatically in case of missing extension because it is the only available
> file format filter to the filepicker.

right.

> Your argument against using the 'if(!fileSavePath.endsWith(".json")) check'
> seems valid but since things are working fine currently on Windows and
> defaultExtension not forcing file format on Linux and OSX we would not be
> able to see any change even after setting the value of defaultExtension.

That is doing what is expected by platform convention on Windows, Mac and Linux.
We should only set defaultExtension, and if that's not coherent with the system apps on Mac/Linux, the filepicker code should be fixed instead of adding our own workaround.

Comment 21

[Prakhar Gupta]

Attachment: Patch for Bug 485265. Set the value of defaultExtension to json.

This is my first bug and I don't know if there is another step after getting a bug reviewed. Please let me know if there is any.

Comment 22

[Marco Bonardo [:mak]]

Ideally at this point you should push to the Try Server, geta green tests run and then set checkin-needed. But since the likelyhood of a failure with this patch is extremely low, we'll skip the Try run to save on resources.

Comment 23

[Marco Bonardo [:mak]]

Also, it would be great if you could add author and commit title in the patch, see

https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/bb2fbb0f9984
"JSON backup should force .json extension". r=mak77

Comment 25

[Phil Ringnalda (:philor)]

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