Bug 40230 – run-bindings-tests results broken by Changeset 60776

• Component: New Bugs
• Status: RESOLVED FIXED

[Andrei Popescu]

run-bindings-tests results broken by Changeset 60776

[Andrei Popescu]

Created attachment 58017 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 58017 [details]
Patch

I'm confused.  There is a functional change here.  Why are we changing CodeGeneratorJS.pm if this is just supposed to update test reuslts?

[Jeremy Orlow]

Comment on attachment 58017 [details]
Patch

This was arguably a build fix.  I think Andrei was just being cautious because he just became a committer.  I guess we need to get some bot running these tests?

[Jeremy Orlow]

Comment on attachment 58017 [details]
Patch

Nm...I see...the change log really should be more clear.

[Andrei Popescu]

(In reply to comment #2)
> (From update of attachment 58017 [details])
> I'm confused.  There is a functional change here.  Why are we changing CodeGeneratorJS.pm if this is just supposed to update test reuslts?

That code in CodeGeneratorJS.pm was added by me in 

http://trac.webkit.org/changeset/60776/trunk/WebCore/bindings/scripts/CodeGeneratorJS.pm

Unfortunately, when I tried to submit the change, I noticed a conflict due to this change that was submitted just seconds before my attempt:

http://trac.webkit.org/changeset/60775

I rebased and solved the conflict but got confused and solved the conflict the wrong way. The problem was that, after I got the r+ for my patch but before I submitted, the CodeGeneratorJS.pm was modified to make native functions return EncodedJSValue instead of JSValues.

http://trac.webkit.org/changeset/60631/trunk/WebCore/bindings/scripts/CodeGeneratorJS.pm

So when I solved my conflict, instead of what I did, I should have updated my change to the Code Generator so that it would also return a EncodedJSValue and then then re-invoke "run-bindings-tests". This patch does just that.

[Jeremy Orlow]

What you just put (maybe slightly condensed) is a much better ChangeLog message.

Btw, as I hinted towards: this could have been an unreviewed build fix.

[Andrei Popescu]

Created attachment 58045 [details]
Patch

[Jeremy Orlow]

Comment on attachment 58045 [details]
Patch

r=me

[Andrei Popescu]

Thanks Jeremy. Committed r60789.

[Eric Seidel (no email)]

Wasn't this a real bug which was testable via a layout test?  I'm confused why no testing was required.  Jeremy?

[Andrei Popescu]

(In reply to comment #10)
> Wasn't this a real bug which was testable via a layout test?  I'm confused why no testing was required.  Jeremy?

This was adding the possibility to specify [CallWith=ScriptExecutionContext] in idl files and was needed by the indexed database code. I am adding layout tests for that in a separate patch.

Thanks,
Andrei

[Jeremy Orlow]

We're a bit behind on landing layout tests at the moment because so much is in flight at once.  It should all be tested again by EOW though.

Note that none of the build.webkit.org bots will be running these tests for some time, though.