Bug 152574 – [ES6] Date.prototype should be a plain object

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andy VanWagoner]

[ES6] Date.prototype should be a plain object

[Andy VanWagoner]

Created attachment 267952 [details]
Patch

[Geoffrey Garen]

Comment on attachment 267952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267952&action=review

You should also test Date instanceof Date.

> Source/JavaScriptCore/runtime/DatePrototype.h:32
> +// ES6 20.3.4
> +// http://www.ecma-international.org/ecma-262/6.0/index.html#sec-properties-of-the-date-prototype-object
> +// The Date prototype object is itself an ordinary object. It is not a Date instance and does not have a [[DateValue]] internal slot.

You can leave this comment out. svn and bugzilla and test cases should suffice. We used to have a spec link for every function in the project. It was unwieldy.

[Andy VanWagoner]

Created attachment 268257 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 268257 [details]
Patch

Clearing flags on attachment: 268257

Committed r194603: <http://trac.webkit.org/changeset/194603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #4)
> Comment on attachment 268257 [details]
> Patch
> 
> Clearing flags on attachment: 268257
> 
> Committed r194603: <http://trac.webkit.org/changeset/194603>

It broke the cloop build, please fix it ASAP.

[Csaba Osztrogonác]

(In reply to comment #6)
> (In reply to comment #4)
> > Comment on attachment 268257 [details]
> > Patch
> > 
> > Clearing flags on attachment: 268257
> > 
> > Committed r194603: <http://trac.webkit.org/changeset/194603>
> 
> It broke the cloop build, please fix it ASAP.

Ah, not build failure, but test faiure.

[Ryan Haddad]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > Comment on attachment 268257 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 268257
> > > 
> > > Committed r194603: <http://trac.webkit.org/changeset/194603>
> > 
> > It broke the cloop build, please fix it ASAP.
> 
> Ah, not build failure, but test faiure.

** The following JSC stress test failures have been introduced:
	mozilla-tests.yaml/ecma/Date/15.9.5.js.mozilla
	mozilla-tests.yaml/ecma/Date/15.9.5.js.mozilla-baseline
	mozilla-tests.yaml/ecma/Date/15.9.5.js.mozilla-dfg-eager-no-cjit-validate-phases
	mozilla-tests.yaml/ecma/Date/15.9.5.js.mozilla-ftl
	mozilla-tests.yaml/ecma/Date/15.9.5.js.mozilla-ftl-eager-no-cjit-validate-phases
	mozilla-tests.yaml/ecma/Date/15.9.5.js.mozilla-llint

[WebKit Commit Bot]

Re-opened since this is blocked by bug 152762

[Ryan Haddad]

Example of failing CLoop and JSC test runs:
<https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/2254>
<https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/1233>

[Andy VanWagoner]

The failed test is explicitly testing for the old behavior, where Date.prototype is a Date instance. Either the expectation should change, or the tests should be skipped.

[Keith Miller]

I believe if you put "//@ skip" at the top of the test file it should be skipped. I'm not positive for the mozilla tests, however. If you do so you should put a comment linking to this bugzilla so others know why it's skipped. If adding "//@ skip" doesn't work let me know.

[Andy VanWagoner]

(In reply to comment #12)
> I believe if you put "//@ skip" at the top of the test file it should be
> skipped. I'm not positive for the mozilla tests, however. If you do so you
> should put a comment linking to this bugzilla so others know why it's
> skipped. If adding "//@ skip" doesn't work let me know.

"//@ skip" doesn't seem to work in this case. It looks like mozilla-tests.yaml can specify that it should be skipped, though. :negative, :fail, and :skip all look like valid options. Which is the most appropriate for this case?

[Keith Miller]

(In reply to comment #13)
> (In reply to comment #12)
> > I believe if you put "//@ skip" at the top of the test file it should be
> > skipped. I'm not positive for the mozilla tests, however. If you do so you
> > should put a comment linking to this bugzilla so others know why it's
> > skipped. If adding "//@ skip" doesn't work let me know.
> 
> "//@ skip" doesn't seem to work in this case. It looks like
> mozilla-tests.yaml can specify that it should be skipped, though. :negative,
> :fail, and :skip all look like valid options. Which is the most appropriate
> for this case?

Ah, I see. Looking at the run-jsc-stress-tests script, negative appears to mean there was an exit code 3 (uncaught exception) and failed means the test return the string "failed!". Based on the test logs from the bots I think you want negative.

[Andy VanWagoner]

Created attachment 268347 [details]
Patch

[Benjamin Poulain]

Comment on attachment 268347 [details]
Patch

Let's try again!

[Benjamin Poulain]

Thanks for the help everyone.

[WebKit Commit Bot]

Comment on attachment 268347 [details]
Patch

Clearing flags on attachment: 268347

Committed r194636: <http://trac.webkit.org/changeset/194636>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.