1123372 - Remove use of .toLocaleFormat() from Places

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Date.toLocaleFormat is deprecated (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleFormat) and will soon or later be removed (see bug 818634).

We use it only to generate ISO date strings:
http://mxr.mozilla.org/mozilla-central/search?string=toLocaleFormat&find=places

For that we can just do:
Date.toISOString().substr(0, 10)

Comment 1

[Antonio Ladeia]

Hello Marco,

What files need this correction?

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Hi, see the link in comment 0:

http://mxr.mozilla.org/mozilla-central/search?string=toLocaleFormat&find=places

Comment 3

[Antonio Ladeia]

Sorry,

I dont see mxr link.

I can do it.

What the best form of to do this?

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'd start from here
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

once you have a build you can take a look here
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

simple sum up:
- have a working build
- ensure Mercurial is properly configured (https://developer.mozilla.org/en-US/docs/Installing_Mercurial)
- Create a patch (either as an mercurial bookmark or more easily as a mercurial queue patch) in the proper format, as explained in the links above
- Attach the patch here and ask for review

For any question feel free to ask, if you wish you can join the #introduction channel on irc.mozilla.org and ask questions there for more immediate help.

Comment 5

[Antonio Ladeia]

Hello Marco, 

I already one development build with the newest version of gecko (but I use the git version of this repo). 

I talk about this task.

Thanks

(I want make this task, I may assign this task for me?)

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Mentored bugs are assigned when the first patch is attached, feel free to work on it.

I think comment 0 has enough information to do the replacements, do you have specific questions? The only thing to do is to replace the deprecated calls with the suggested replacement code in all of the pointed at files.

Note the patch for review should be in mercurial format, per convention. There is some experimental tools to export from git to the mozilla mercurial format, but I'm not up-to-date regarding those, you might try asking on irc about those.

Comment 7

[Antonio Ladeia]

Attachment: RemoveToLocaleFormat.diff

I change the .toLocaleFormat() for .toISOString() in all points of mxr show me.

Please, this is my first patch, it is correct?

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8552314 [details] [diff] [review]
RemoveToLocaleFormat.diff

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

Yes, the patch format looks correct!

The only thing to change, is the commit message (you can modify it through hg qref -m "message" or hg qref -e, if you configured your default editor.
The commit message should be in the form "Bug # - description. r=reviewer", so in this case should be something like:

Bug 1123372 - Stop using Date.toLocaleFormat() in Places code. r=mak

Once done, please set the review? flag on me.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efabcf966328

Comment 10

[Antonio Ladeia]

Hello Marco,

I already put the command in HG, hg qref -m "Bug 1123372 - Stop using Date.toLocaleFormat() in Places code. r=mak". I attach the diff archiev again? I flag you in this bug?

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Antonio Ladeia from comment #10)
> Hello Marco,
> 
> I already put the command in HG, hg qref -m "Bug 1123372 - Stop using
> Date.toLocaleFormat() in Places code. r=mak". I attach the diff archiev
> again? I flag you in this bug?

yes, and add the "review?" flag on me on the patch.

Comment 12

[Antonio Ladeia]

Attachment: RemoveToLocaleFormat.diff

It's correct?

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8552314 [details] [diff] [review]
RemoveToLocaleFormat.diff

when adding a new version of a patch, you can mark the old version "obsolete" in the attachment ui.

Comment 14

[Antonio Ladeia]

OK, 

I see this option but I did not know if it is corret, and I dont mark this option :-(

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8553018 [details] [diff] [review]
RemoveToLocaleFormat.diff

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

yes, it's good now!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/638a3da0cf2f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Frustratingly, this landed on fx-team and was fine. However, when it was merged to mozilla-central, xpcshell started failing across the board. I backed this out to get the trees reopened.
https://hg.mozilla.org/mozilla-central/rev/494632b9afed

These are the failures that started happening:
https://treeherder.mozilla.org/logviewer.html#?job_id=926018&repo=mozilla-central
https://treeherder.mozilla.org/logviewer.html#?job_id=925655&repo=mozilla-central

I wish I had a better explanation for what happened. All I can say for sure is that fx-team was merged to mozilla-central after inbound. Below is the pushlog for what was contained in that inbound merge.
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=568bbf124221

My only guess is that something in that range had a bad conflict with the patch here, but I don't know which one that be. Sorry for the hassle, situations like these are rare and painful when they happen :(

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't see anything interesting in that pushlog, I have no explanations so far.

I see some javascript engine fixes there, thus I wonder if something untested broke down there :/

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I have reapplied the patch on top of fx-team locally, and I cannot reproduce any of the failures :(

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, Tim Taubert was able to reproduce the problem locally.
The fact is that toISOString() always uses UTC timezone, and that confuses the tests code. moreover we want to use the user's time.

Could be this started failing at the same time as the merge for pure coincidence.

It's possible to correct the time before using it, but that method looks unreadable/fragile, so I'd take the old boring path.

So, sorry for restarting from scratch here, I'd say in spite of that, let's add an helper to PlacesUtils.jsm: toISODateString(dateObj) that will internally build the string using getDate, getMonth, getFullYear. And then just .join("-").
Note that date and month must be 0-padded to respect the iso format.

We'll then use that method at the callpoints.

Comment 22

[Antonio Ladeia]

Attachment: RemoveToLocale.diff

Hello mark,

Sorry for delay.

My new Patch is attached now.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8561015 [details] [diff] [review]
RemoveToLocale.diff

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

Thanks for the update, no worries for the delay.

After looking at the patch I think it would be better to move the new method into PlacesBackups instead of PlacesUtils. It is only used by code related to that and thus it makes more sense to put it there.

After doing changes, please run tests with

./mach xpcshell-tests toolkit/components/places/tests

if you did, you'd have noticed the patch as-is doesn't work.

::: toolkit/components/places/PlacesBackups.jsm
@@ +232,5 @@
>    getFilenameForDate: function PB_getFilenameForDate(aDateObj, aCompress) {
>      let dateObj = aDateObj || new Date();
>      // Use YYYY-MM-DD (ISO 8601) as it doesn't contain illegal characters
>      // and makes the alphabetical order of multiple backup files more useful.
> +      return "bookmarks-" + toISODateString(dateObj) + ".json" +

When using an util exposed by a module, you should invoke it as moduleName.methodName()

Once you move this to PlacesBackups it will be PlacesBackups.toISODateString()

The PlacesBackups module should be already imported in all the call points (otherwise you should also Cu.import() it, but not your case)

::: toolkit/components/places/PlacesUtils.jsm
@@ +3172,5 @@
>    }
>  };
> +
> +/**
> +* Generate one ISO Date (YYYY-MM-DD)from one DateObject.

you need to indent one space more so that it looks like
/**
 * Generates a ISO date string (YYYY-MM-DD) from a Date object.
 *
 * @param dateObj
 *        The date object to parse.
 * @return an ISO date string.
 */

@@ +3176,5 @@
> +* Generate one ISO Date (YYYY-MM-DD)from one DateObject.
> +* @param dateObj
> +*        One Date javascript object.
> +*/
> +function toISODateString(dateObj) {

you need to move this method INTO PlacesBackups as

toISODateString(dateObj) {
...

I'd suggest to put if just before the getFilenameForDate() method ( http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesBackups.jsm#220 )

@@ +3194,5 @@
> +    _date = "0"+_date;
> +  
> +  _dateArr.push(_date);
> +  
> +  return _dateArr.join("-");

Please pay attention to trailing spaces, we don't allow them in patches. Many text editors have an option to remove trailing spaces from the lines you touched, so you might look into that.

You can very likely reduce this code by far, for example:

let padDate = val => val < 10 ? ("0" + val).substr(0, 2) : val;
return [ dateObj.getFullYear(),
         dateObj.getMonth() + 1,
         dateObj.getDate()
       ].map(padDate)
        .join("-");

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #23)
> I'd suggest to put if just before the getFilenameForDate() method (

Sorry for the typo, I meant IT, not if.

Comment 25

[Antonio Ladeia]

Hello Marco,

Thanks for all, but I dont understand where I put the new method, I put this inside of this.PlacesBackups?

In this case my method would look like this?

this.PlacesBackups = { ...

/**
 * Generates a ISO date string (YYYY-MM-DD) from a Date object.
 *
 * @param dateObj
 *        The date object to parse.
 * @return an ISO date string.
 */
toISODateString: function toISODateString(dateObj) {
let padDate = val => val < 10 ? ("0" + val).substr(0, 2) : val;
return [ dateObj.getFullYear(),
         dateObj.getMonth() + 1,
         dateObj.getDate()
       ].map(padDate)
        .join("-");
}
...}

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes, apart the indentation that should be on par with the other methods, and the fact you should finish it with a comma, like any other property.

Also, thank you to ES6 properties shorthands you can effectively just write

this.PlacesBackups = {
...

  toISODateString(dateObj) {
    ...your code...
  },

...
}

Comment 27

[Antonio Ladeia]

Attachment: RemoveToLocale.diff

Hello Marco,

I think this correct now.

Thanks

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8562197 [details] [diff] [review]
RemoveToLocale.diff

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

yes, this looks much better and likely working.

Did you have any issues with running the tests?

Btw, please attach a merged patch against the current tree, so I can push it to the Try server to do a full tests run.

::: toolkit/components/places/PlacesBackups.jsm
@@ +217,5 @@
>        return this._backupFiles;
>      }.bind(this));
>    },
> +  
> +  /**

there are some trailing spaces in the line above this

@@ +226,5 @@
> +   * @return an ISO date string.
> +   */
> +  toISODateString: function toISODateString(dateObj) {
> +  let padDate = val => val < 10 ? ("0" + val).substr(0, 2) : val;
> +  return [ dateObj.getFullYear(),

please fix indentation of the function body (2 spaces more)

@@ +230,5 @@
> +  return [ dateObj.getFullYear(),
> +	  dateObj.getMonth() + 1,
> +	  dateObj.getDate()
> +	].map(padDate)
> +	  .join("-");

please align the dots

::: toolkit/components/places/PlacesUtils.jsm
@@ -3176,5 @@
> -* Generate one ISO Date (YYYY-MM-DD)from one DateObject.
> -* @param dateObj
> -*        One Date javascript object.
> -*/
> -function toISODateString(dateObj) {

looks like you have created a patch on top of the previous one, please merge them.

The patch should be a diff against a clean tree to be pushable

::: toolkit/components/places/tests/head_common.js
@@ +401,5 @@
>  }
>  
>  const FILENAME_BOOKMARKS_HTML = "bookmarks.html";
>  const FILENAME_BOOKMARKS_JSON = "bookmarks-" +
> +  (PlacesBackups.toISODateString(new Date())) + ".json";

I think you can remove the additional parenthesis from here

Comment 29

[Tom S [:evilpie]]

Ping? Would be a nice utility to have to let us get rid of toLocaleFormat in the future.

Comment 30

[Antonio Ladeia]

Sorry mark and evilpie.

I lost my repo and modifications, but I work in this bug again, in some day I send a new patch.

Comment 31

[tecnowancer]

Hello Mark, a make this same steps, but when a make ./mach xpcshell-tests toolkit/components/places/tests
I receive this:

Ran 20216 tests (507 parents, 19709 subtests)
Expected results: 19959
Unexpected results: 254 (FAIL: 252, TIMEOUT: 2)
Skipped: 3

You can help me?

Comment 32

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to tecnowancer from comment #31)
> You can help me?

Please attach the updated patch, looks like you might have a syntax error or something similar, since all of the tests are failing.

Comment 33

[tecnowancer]

I inspect my code and the result is

Summary
=======

Ran 264 tests (255 parents, 9 subtests)
Expected results: 259
Unexpected results: 2 (FAIL: 2)
Skipped: 3

Unexpected Results
==================

toolkit/components/places/tests/queries/test_sort-date-site-grouping.js
-----------------------------------------------------------------------
FAIL [Parent]

Comment 34

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'm sorry but it's very hard for me to help you debugging a patch I can't see...
Can you please attach the current diff?

Btw, that failure sounds very strange, since that test doesn't touch any of the code you are touching... I suspect it might be failing due to unexpected timezone issues. Please try removing your patch, compile and run that test alone, if it still fails it would be nice to have a separate bug where you could report your timezone...

Comment 35

[Antonio Ladeia]

Attachment: RemoveToLocale.diff

Here my new patch, I go remove this and retest the build.

Comment 36

[Antonio Ladeia]

Attachment: RemoveToLocale.diff

Comment 37

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8597546 [details] [diff] [review]
RemoveToLocale.diff

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

Please set a proper commit message using:
hg qref -m "Bug 1123372: Remove use of .toLocaleFormat() from Places. r=mak"

looks like you forgot to replace the calls in
toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js

Apart these, the basics of the patch are correct, there are only some style changes to apply:

::: toolkit/components/places/PlacesBackups.jsm
@@ +214,5 @@
>          return aDate < bDate ? 1 : aDate > bDate ? -1 : 0;
>        });
>  
>        return this._backupFiles;
> +    }.bind(this));},

the "}," should stay on its own line, as it was before.

@@ +224,5 @@
> +   *        The date object to parse.
> +   * @return an ISO date string.
> +   */
> +   toISODateString: function toISODateString(dateObj) {
> +    let padDate = val => val < 10 ? ("0" + val).substr(0, 2) : val;

I think we can avoid doing both the if and the substr, either you do:
val < 10 ? ("0" + val) : val;
or:
("0" + val).substr(-2, 2);

@@ +226,5 @@
> +   */
> +   toISODateString: function toISODateString(dateObj) {
> +    let padDate = val => val < 10 ? ("0" + val).substr(0, 2) : val;
> +    return [ dateObj.getFullYear(),
> +      dateObj.getMonth() + 1,

please move getFullYear() to the next line, align it with the next like:
return [
  dateObj.getFullYear(),
  dateObj.getMonth() + 1,
  ...

@@ +229,5 @@
> +    return [ dateObj.getFullYear(),
> +      dateObj.getMonth() + 1,
> +      dateObj.getDate()
> +    ].map(padDate)
> +    .join("-");

and please align .join with .map

Comment 38

[Antonio Ladeia]

How I can remove patch of my repository for generate one new with the new changes in test_1016953-renaming-uncompressed.js and new styles?

Comment 39

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Looks like you are using mq, you can verify this patch is at the top of the stack using
hg qtop
if so, you can just do the changes you want, then update the top patch using
hg qref

Comment 40

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://developer.mozilla.org/en-US/docs/Mercurial_Queues

Comment 41

[Antonio Ladeia]

Attachment: RemoveToLocale.diff

I make new changes and the result is this


Summary
=======

Ran 270 tests (255 parents, 15 subtests)
Expected results: 258
Unexpected results: 9 (FAIL: 9)
Skipped: 3

Comment 42

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2

The last patch was missing part of the previous one, probably you didn't fold the patches using hg qfold.
I merged the 2 patches and fixed the failures you found, one was due to wrongly calling padDate on the year, one due to the fact new Date(ISO-date) was generating Invalid Date.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c9d422cef9

Comment 43

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

you can ignore the JP failure that I inherited from the parent

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94818e6ab15b

Comment 45

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

https://hg.mozilla.org/mozilla-central/rev/94818e6ab15b