99224 - document.lastModified should be localized

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Koike Kazuhiko]

document.lastModified returns GMT, but it should return local time.

IE and N4 return local time.

Comment 1

[Koike Kazuhiko]

Attachment: Testcase

Comment 2

[Johnny Stenback (:jst)]

This is true for http documents, for file documents we get it right.

Targetting for mozilla1.0.

Comment 3

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 4

[Prashant Desale]

Severity = LOW [No Crash, Functional failure, No Cosmetic failure]
Visibility = HIGH [Lot of real world website usage, Gets one point of 
compatibility with other browsers since it works on other browsers. gets one 
more point on compliance with adopted techonology, that is JS] 

Priority = Visibility * Severity

Priority = p3

adding word "qawanted" because I'm setting this priority on available data & if 
someone feels otherwise then please investigate this more & feel free to change 
this priority.

Comment 5

[Håkan Waara]

Comment on attachment 48971 [details]
Testcase

This testcase is useless since it doesn't actually show a document's real
lastModified date; only the hardcoded 'unknown' one.

Comment 6

[Daniel Wang]

if this bug is ASSIGNED, should bug 107445 be WONTFIX ?

Comment 7

[Daniel Wang]

missed moz 1.01 deadline
move target milestone -> future

Comment 8

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 9

[Qiang]

no one is working on this ??  

firefox uses server timezone while shows document.lastModified.

it should follow the client timezone regardless of the server side. IE works 
fine and this bug should be fixed.

visit http://www.easyya.com and see the bottom of the page. It shows GMT 
always.

Comment 10

[timeless]

[e-mail]: thank you for volunteering, can you tell us when you'll
have this implemented?

Comment 11

[Qiang]

I have no idea why i was assigned the bug. sorry, guys. 
now it's back to the public.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

This patch has a problem. But the problem already exists current Trunk.

If document.lastModified value is localized (currently, that is local file and
HTTP header's last-modified header doesn't exist only), the value cannot parsed
by Javascript's Date object.

E.g., if the patch is checked-in, we can see "Not specified" in Modified field
on PageInfo dialog's General tab.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently:
When                        | Localized?
----------------------------+-------------
Local file                  | YES
Existing last-modified      | NO
Not existing last-modified  | YES

After the patch is checked-in:
When                           | Localized?
-------------------------------+-------------
Local file                     | YES
Existing last-modified         | YES
Existing invalid last-modified | NO
Not existing last-modified     | YES

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 190579 [details] [diff] [review]
Patch rv1.0

r+sr=jst

Do we need a followup bug filed to fix the page info dialog, or is that already
in bugzilla?

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #14)
> (From update of attachment 190579 [details] [diff] [review] [edit])
> r+sr=jst
> 
> Do we need a followup bug filed to fix the page info dialog, or is that already
> in bugzilla?
> 

I cannot fine the bugs.
I will file the bugs after check-in (after 1.9 opened).

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Maybe we need to change following code.

http://lxr.mozilla.org/mozilla/source/browser/base/content/pageInfo.js#390
/browser/base/content/pageInfo.js, line 390 -- var modifiedText =
formatDate(theDocument.lastModified, gStrings.notSet);

http://lxr.mozilla.org/mozilla/source/editor/ui/dialogs/content/EdPageProps.js#81
/editor/ui/dialogs/content/EdPageProps.js, line 81 -- lastmod =
editor.document.lastModified; // get string of last modified date

http://lxr.mozilla.org/mozilla/source/js/rhino/docs/faq.html#19
http://lxr.mozilla.org/mozilla/source/js/rhino/docs/scopes.html#23
http://lxr.mozilla.org/mozilla/source/js/rhino/docs/scriptjava.html#19
http://lxr.mozilla.org/mozilla/source/js/rhino/docs/serialization.html#23
/js/rhino/docs/faq.html, line 19 -- var d = new Date(document.lastModified);
/js/rhino/docs/scopes.html, line 23 -- var d = new Date(document.lastModified);
/js/rhino/docs/scriptjava.html, line 19 -- var d = new Date(document.lastModified);
/js/rhino/docs/serialization.html, line 23 -- var d = new
Date(document.lastModified);

http://lxr.mozilla.org/mozilla/source/xpfe/browser/resources/content/pageInfo.js#388
/xpfe/browser/resources/content/pageInfo.js, line 388 -- var modifiedText =
formatDate(theDocument.lastModified, gStrings.notSet);

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

I propose this approach.
This patch's result is same as IE6 and the result can convert Date object in JS
code.

lastModified always returns "MM/DD/YYYY hh:mm:ss" format. It can parsed by
current JS implementation. If page author needs previous patch's result, they
can write as following.

var date = new Date(document.lastModified);
document.write(date.toLocaleString());

If you grant for this patch, we can fix this bug in 1.8.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase

In this case, if we use Patch rv2.0, we get more compatibility for IE.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Screenshot of the testcase

Comment 20

[timeless]

please say there's going to be an annoucement about this api change somewhere
fairly visible.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

r=jst, but I'd like brendan to have a say on this one too.

Comment 22

[Brendan Eich [:brendan]]

So if we're really just emulating IE and unbreaking sites in major locales, then
I'm all for this.  Do we feel the need to do more testing in all locales? 
Cc'ing some folks in case they have thoughts.  I'll sr shortly.

/be

Comment 23

[Bob Clary [:bc] (inactive)]

The change seemed very reasonable to me. Did timeless have any concrete objections?

Comment 24

[Jungshik Shin]

(In reply to comment #23)
> The change seemed very reasonable to me. Did timeless have any concrete
objections?

See bug 224703 comment #24. 

Anyway, I don't think MSIE 'hard-coded' 'MM/DD/YYYY HH:MM:SS' (have you tried MS
IE under a different locale (say, French)?) . If you want to hard-code anything,
it had better be ISO 8601-compliant (YYYY-MM-DD ......), but what really need to
be done is to format it according to the locale (corresponding to the language
of a doc.)  of a document (if it's not trivial which is indeed the case, the
current locale). (see bug 224703). 

BTW, it's 'ironic' to see IE use 'UTC' now given that we kinda refrained from
using it partly because of IE 'compatibility' (bug 224744 comment #3, bug 107445)

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Jungshik:

Can you attach better patch? I think that if you cannot create better patch
soon, we should use my patch. Because my patch makes better behavior than
current behavior.

> I don't think MSIE 'hard-coded' 'MM/DD/YYYY HH:MM:SS'

I don't think so. See
http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/lastmodified.asp

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

Unless jshin speaks quickly and has a better patch, I think this should go in
for 1.8b4/Firefox1.5.

/be

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

Requesting explicit second-review+ from jshin.

/be

Comment 28

[Johnny Stenback (:jst)]

Agreed.

Comment 29

[Mike Schroepfer]

Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

Approved for 1.8b5 per bug meeting

Comment 30

[Scott MacGregor]

Please get this checked in on the branch and trunk ASAP or this bug will be
removed from the blocking list. Thanks!

Comment 31

[Scott MacGregor]

just another reminder to get this landed ASAP or it may get taken off the
blocker list.

Comment 32

[Scott MacGregor]

Is there anything holding this up from getting checked in? 

Comment 33

[Asa Dotzler [:asa]]

Time is running out. If there's an advocate for this fix with cvs access, now's
the time to step up.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Brendan and jst:

Do we need jshin's review? We don't have response from him in a half month.

Comment 35

[Jo Hermans]

This code-pattern with the formated string in the buffer, while very interesting
and self-documenting, isn't thread-safe. Variable formatedTime doesn't point to
the stack, but to anonyous global memory.

+    char formatedTime[] = "MM/DD/YYYY hh:mm:ss";
+    if (sprintf(formatedTime, "%02d/%02d/%04d %02d:%02d:%02d",
+                prtime.tm_month + 1, prtime.tm_mday, prtime.tm_year,
+                prtime.tm_hour     ,  prtime.tm_min,  prtime.tm_sec)) {
+      CopyASCIItoUTF16(nsDependentCString(formatedTime), mLastModified);
+    }

Maybe this is a bit better :

+    // "MM/DD/YYYY hh:mm:ss";
+    char formatedTime[30];
+    if (sprintf(formatedTime, "%02d/%02d/%04d %02d:%02d:%02d",
+                prtime.tm_month + 1, prtime.tm_mday, prtime.tm_year,
+                prtime.tm_hour     ,  prtime.tm_min,  prtime.tm_sec)) {
+      CopyASCIItoUTF16(nsDependentCString(formatedTime), mLastModified);
+    }

Comment 36

[Jungshik Shin]

Comment on attachment 190611 [details] [diff] [review]
Patch rv2.0

Sorry for the delay. I was mistaken that this already had been checked in /

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to trunk and 1.8branch.
Thank you everyone!

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for check-in