Bug 40230 - run-bindings-tests results broken by Changeset 60776
Summary: run-bindings-tests results broken by Changeset 60776
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-07 05:17 PDT by Andrei Popescu
Modified: 2010-06-07 12:25 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.86 KB, patch)
2010-06-07 05:22 PDT, Andrei Popescu
no flags Details | Formatted Diff | Diff
Patch (4.53 KB, patch)
2010-06-07 10:35 PDT, Andrei Popescu
jorlow: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Popescu 2010-06-07 05:17:59 PDT
run-bindings-tests results broken by Changeset 60776
Comment 1 Andrei Popescu 2010-06-07 05:22:54 PDT
Created attachment 58017 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-06-07 10:07:28 PDT
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?
Comment 3 Jeremy Orlow 2010-06-07 10:16:55 PDT
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?
Comment 4 Jeremy Orlow 2010-06-07 10:17:24 PDT
Comment on attachment 58017 [details]
Patch

Nm...I see...the change log really should be more clear.
Comment 5 Andrei Popescu 2010-06-07 10:21:50 PDT
(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.
Comment 6 Jeremy Orlow 2010-06-07 10:24:31 PDT
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.
Comment 7 Andrei Popescu 2010-06-07 10:35:15 PDT
Created attachment 58045 [details]
Patch
Comment 8 Jeremy Orlow 2010-06-07 10:41:33 PDT
Comment on attachment 58045 [details]
Patch

r=me
Comment 9 Andrei Popescu 2010-06-07 10:53:55 PDT
Thanks Jeremy. Committed r60789.
Comment 10 Eric Seidel (no email) 2010-06-07 11:40:05 PDT
Wasn't this a real bug which was testable via a layout test?  I'm confused why no testing was required.  Jeremy?
Comment 11 Andrei Popescu 2010-06-07 11:45:30 PDT
(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
Comment 12 Jeremy Orlow 2010-06-07 12:25:24 PDT
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.