Bug 5266 - Support parenthesized comments in Date.parse()
Summary: Support parenthesized comments in Date.parse()
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Maciej Stachowiak
URL: http://www.excite.com/
Keywords:
Depends on:
Blocks:
 
Reported: 2005-10-04 15:17 PDT by mitz
Modified: 2005-10-09 10:12 PDT (History)
1 user (show)

See Also:


Attachments
allow comments where spaces are allowed (4.11 KB, patch)
2005-10-05 03:13 PDT, mitz
ggaren: review-
Details | Formatted Diff | Diff
testcase for this (5.12 KB, patch)
2005-10-05 03:52 PDT, mitz
no flags Details | Formatted Diff | Diff
revised patch (4.31 KB, patch)
2005-10-05 09:08 PDT, mitz
ggaren: review-
Details | Formatted Diff | Diff
testcase (5.38 KB, patch)
2005-10-05 09:09 PDT, mitz
no flags Details | Formatted Diff | Diff
revised patch (4.36 KB, patch)
2005-10-05 10:19 PDT, mitz
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2005-10-04 15:17:49 PDT
Date.parse() should ignore comments in parentheses anywhere white space is allowed, following RFC 
2822. For example, Date.parse("Oct(ober)4 (today)1995 (10 years ago)") should be the same as Date.parse
("Oct 4 1995"), and not NaN as it currently is.
Comment 1 mitz 2005-10-04 16:13:43 PDT
Actually, "following RFC 2822" would not be "anywhere white space is allowed", but I think Firefox allows 
is more permissive than the RFC.
Comment 2 mitz 2005-10-05 03:13:07 PDT
Created attachment 4209 [details]
allow comments where spaces are allowed

This is more permissive than the RFC, to match Firefox
Comment 3 mitz 2005-10-05 03:52:01 PDT
Created attachment 4211 [details]
testcase for this
Comment 4 Geoffrey Garen 2005-10-05 08:10:47 PDT
Comment on attachment 4209 [details]
allow comments where spaces are allowed

Nice catch on this bug.

I have two concerns:

1) I think your logic is wrong -- it doesn't seem to support dates of the form
"Wed (comment 1) (comment 2) Sep 28 2005", which FF does support. (Correct me
if I'm wrong - I haven't tested.)

2) The nesting of the &&'s and ||'s at the end is pretty confusing at first
glance. I think this might work better:
    while ((ch = *s++)) {
	if (ch == '(')
	    nesting++;
	else if (ch == ')')
	    nesting--;
	else if (!isspace(ch) && nesting == 0)
	    break;
    }
Comment 5 Geoffrey Garen 2005-10-05 08:11:20 PDT
PS I think you should add some "Wed (comment 1) (comment 2) Sep 28 2005" cases to the test.
Comment 6 Geoffrey Garen 2005-10-05 08:14:36 PDT
Actually, while (ch = *s++) won't work if there are no spaces or comments. Instead:

    while ((ch = *s)) {
	if (ch == '(')
	    nesting++;
	else if (ch == ')')
	    nesting--;
	else if (!isspace(ch) && nesting == 0)
	    break;
        
        s++;
    }
Comment 7 mitz 2005-10-05 08:22:46 PDT
(In reply to comment #4)
> Nice catch on this bug.

Can't take credit for that, actually.

> 1) I think your logic is wrong -- it doesn't seem to support dates of the form
> "Wed (comment 1) (comment 2) Sep 28 2005", which FF does support. (Correct me
> if I'm wrong - I haven't tested.)

Tested. It is supported. I'll add it to the test.

> 	if (ch == '(')
> 	    nesting++;
> 	else if (ch == ')')
> 	    nesting--;
> 	else if (!isspace(ch) && nesting == 0)
> 	    break;

Is not good because it doesn't break if we encounter a ')' when nesting is 0.
Comment 8 mitz 2005-10-05 08:32:47 PDT
Actually, while the patch allows
"Wed (comment 1) (comment 2) Dec 25 1995 1:30 GMT"
it doesn't allow
"Wed(comment 1) (comment 2) Dec 25 1995 1:30 GMT"
which Firefox does.

On the other hand, WebKit allows and Firefox rejects
"Junk Dec 25 1995 1:30 GMT"
while WebKit rejects and Firefox allows
"We Dec 25 1995 1:30 GMT"

The latter couple of cases seem to be outside the scope of this bug, though. I'll see what I can do about 
the Wed(comment) case.
Comment 9 mitz 2005-10-05 09:08:12 PDT
Created attachment 4215 [details]
revised patch
Comment 10 mitz 2005-10-05 09:09:46 PDT
Created attachment 4216 [details]
testcase

Added Wed(comment 1)... and We(comment 1)... cases
Comment 11 mitz 2005-10-05 09:14:35 PDT
Comment on attachment 4215 [details]
revised patch

Updated to allow comments between "day of week" and month. I didn't change
skipSpacesAndComments (see my previous comments).
Comment 12 Darin Adler 2005-10-05 10:07:21 PDT
Comment on attachment 4215 [details]
revised patch

I'm surprised to learn that dates can include parenthesized comments, but this
is a very nice way to fix the bug.

Brace should be on the line after the function, but otherwise, looks good.

r=me
Comment 13 Geoffrey Garen 2005-10-05 10:17:40 PDT
Comment on attachment 4215 [details]
revised patch

I'm marking this R- so that nobody lands it yet -- Mitz and I discussed some
changes on IRC, and he'll submit a new patch.
Comment 14 mitz 2005-10-05 10:19:11 PDT
Created attachment 4217 [details]
revised patch

Corrected style and also made skipSpacesAndComments more readable, as suggested
by ggaren.
Comment 15 Darin Adler 2005-10-08 21:41:48 PDT
I landed the fix, with one last tweak I couldn't resist (check isspace first before checking for '(' and ')').