419433 - Remove use of ISO date helpers from Microformats

Status: RESOLVED FIXED

(Toolkit Graveyard :: Microformats, defect)

Description

[Mike Kaply [:mkaply]]

When this change was originally made to my code (without any review or approval by me, I might add), I thought it would work, but I have since discovered it broke my microformats code.

1. It removed an API that is used by implementors (Microformats.dataFromISO8601), 
2. ISO dates can have a date but no time. When they are converted to JS objects, those objects have a time of midnight and you can never find out that they didn't have a time associated with them. My implementation uses an internal boolean (time) to indicate if the ISO date that was passed in has a time or not. This is important when converting back to an ISO date.

So this bug puts my code back (with a test to make sure this doesn't happen again)

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Put my code back and add a test

Comment 2

[Manish Singh]

Wouldn't it be better to change the ISO8601DateUtils code to cover case #2 instead, so other users get the functionality as well?

Comment 3

[Mike Kaply [:mkaply]]

(In reply to comment #2)
> Wouldn't it be better to change the ISO8601DateUtils code to cover case #2
> instead, so other users get the functionality as well?

That would be difficult simply because JS Date objects don't support Date only.

So for instance, how would doing a toLocaleString on a ISO date that was converted to JS know to only display the date?

Comment 4

[Manish Singh]

Don't these same questions apply to the code you're adding back?

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

a=beltzner for 1.9

Comment 6

[Mike Kaply [:mkaply]]

(In reply to comment #4)
> Don't these same questions apply to the code you're adding back?
> 

Since I'm parsing the date, I can know if it it has a time or not, so I set a boolean on the JS object to indicate that it is a date only.

Then when I convert the JS object back to an ISO date, I don't add the time.

ISO code could in theory do something similar.

Comment 7

[Manish Singh]

(In reply to comment #6)
> ISO code could in theory do something similar.

Exactly, so it seems to me that changing ISO8601DateUtils to do what you want would be better, since it helps more than just microsummaries users.

Comment 8

[Mike Kaply [:mkaply]]

Fix checked in.

ISO dates with just the date are not used by many other things, but if I have time I'll fix the core ISO date code.

This needs to go in to make FF 3.

Comment 9

[Reed Loden [:reed]]

This completely landed without approval. http://tinderbox.mozilla.org/Firefox/ clearly says "The tree is frozen for beta4. Only checkins with approval1.9b4+ are permitted." You either need to get post-approval, or you should back out now.

Comment 10

[Mike Kaply [:mkaply]]

Sorry, I thought approval 1.9 was enough.

Comment 11

[Mike Kaply [:mkaply]]

Reopened since I backed out.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

a1.9b4=beltzner

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

Actually, can we hold off on landing this? We need to hold the tree for a bit to try out some build flag changes. I'll remark as approved when you're free to land.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

OK, good to go again. Please land for beta4.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

Sadly, this missed the beta 4 checkin cutoff. Please land after we open for post-b4 checkins.

Comment 16

[Mike Kaply [:mkaply]]

Actually I got it in. Forgot to close the bug.

Comment 17

[Daniel Holbert [:dholbert]]

FWIW, here's the bonsai link for the checkin that stuck:
  http://tinyurl.com/2ooa74

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

Apparently this did land before the cut off. Putting the flag to rights for some reason which honestly eludes me.