Remove use of .toLocaleFormat() from Places

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: contato, Mentored)

Tracking

unspecified
mozilla40
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 7 obsolete attachments)

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)
(Assignee)

Comment 1

4 years ago
Hello Marco,

What files need this correction?
Flags: needinfo?(mak77)
(Assignee)

Comment 3

4 years ago
Sorry,

I dont see mxr link.

I can do it.

What the best form of to do this?
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.
(Assignee)

Comment 5

4 years ago
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?)
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.
(Assignee)

Comment 7

4 years ago
Posted patch RemoveToLocaleFormat.diff (obsolete) — Splinter Review
I change the .toLocaleFormat() for .toISOString() in all points of mxr show me.

Please, this is my first patch, it is correct?
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
Assignee: nobody → contato
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
(Assignee)

Comment 10

4 years ago
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?
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
(Assignee)

Comment 12

4 years ago
Posted patch RemoveToLocaleFormat.diff (obsolete) — Splinter Review
It's correct?
Attachment #8553018 - Flags: review?(mak77)
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.
Attachment #8552314 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
OK, 

I see this option but I did not know if it is corret, and I dont mark this option :-(
Comment on attachment 8553018 [details] [diff] [review]
RemoveToLocaleFormat.diff

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

yes, it's good now!
Attachment #8553018 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/fx-team/rev/638a3da0cf2f
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/638a3da0cf2f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → mozilla38
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 :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla38 → ---
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 :/
I have reapplied the patch on top of fx-team locally, and I cannot reproduce any of the failures :(
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.
Flags: needinfo?(contato)
(Assignee)

Comment 22

4 years ago
Posted patch RemoveToLocale.diff (obsolete) — Splinter Review
Hello mark,

Sorry for delay.

My new Patch is attached now.
Attachment #8553018 - Attachment is obsolete: true
Flags: needinfo?(contato) → needinfo?(mak77)
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("-");
Attachment #8561015 - Flags: review-
(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.
Flags: needinfo?(mak77)
(Assignee)

Comment 25

4 years ago
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("-");
}
...}
Flags: needinfo?(mak77)
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...
  },

...
}
Flags: needinfo?(mak77)
(Assignee)

Comment 27

4 years ago
Posted patch RemoveToLocale.diff (obsolete) — Splinter Review
Hello Marco,

I think this correct now.

Thanks
Attachment #8561015 - Attachment is obsolete: true
Flags: needinfo?(mak77)
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
Attachment #8562197 - Flags: feedback+
Flags: needinfo?(mak77)
Ping? Would be a nice utility to have to let us get rid of toLocaleFormat in the future.
(Assignee)

Comment 30

4 years ago
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

4 years ago
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?
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(mak77)

Comment 33

4 years ago
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]
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...
(Assignee)

Comment 35

4 years ago
Posted patch RemoveToLocale.diff (obsolete) — Splinter Review
Here my new patch, I go remove this and retest the build.
Attachment #8562197 - Attachment is obsolete: true
Flags: needinfo?(mak77)
(Assignee)

Comment 36

4 years ago
Posted patch RemoveToLocale.diff (obsolete) — Splinter Review
Attachment #8597545 - Attachment is obsolete: true
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
Attachment #8597546 - Flags: feedback+
Flags: needinfo?(mak77)
(Assignee)

Comment 38

4 years ago
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?
(Assignee)

Updated

4 years ago
Flags: needinfo?(mak77)
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
Flags: needinfo?(mak77)
(Assignee)

Comment 41

4 years ago
Posted patch RemoveToLocale.diff (obsolete) — Splinter Review
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
Attachment #8597546 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Posted patch patch v2Splinter Review
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
Attachment #8597634 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Attachment #8598264 - Flags: review+
you can ignore the JP failure that I inherited from the parent
https://hg.mozilla.org/mozilla-central/rev/94818e6ab15b
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.