Bug 5266 – Support parenthesized comments in Date.parse()

• Component: JavaScriptCore
• Status: VERIFIED FIXED

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.

Actually, "following RFC 2822" would not be "anywhere white space is allowed", but I think Firefox allows 
is more permissive than the RFC.

Created attachment 4209 [details]
allow comments where spaces are allowed

This is more permissive than the RFC, to match Firefox

Created attachment 4211 [details]
testcase for this

[Geoffrey Garen]

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;
    }

[Geoffrey Garen]

PS I think you should add some "Wed (comment 1) (comment 2) Sep 28 2005" cases to the test.

[Geoffrey Garen]

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++;
    }

(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.

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.

Created attachment 4215 [details]
revised patch

Created attachment 4216 [details]
testcase

Added Wed(comment 1)... and We(comment 1)... cases

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).

[Darin Adler]

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

[Geoffrey Garen]

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.

Created attachment 4217 [details]
revised patch

Corrected style and also made skipSpacesAndComments more readable, as suggested
by ggaren.

[Darin Adler]

I landed the fix, with one last tweak I couldn't resist (check isspace first before checking for '(' and ')').