Bug 57770 - Factoring the creation of 'FunctionOnly' callbacks in JavaScriptCore
Summary: Factoring the creation of 'FunctionOnly' callbacks in JavaScriptCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Leandro Graciá Gil
URL:
Keywords:
Depends on:
Blocks: 40012 56586
  Show dependency treegraph
 
Reported: 2011-04-04 11:27 PDT by Leandro Graciá Gil
Modified: 2012-09-19 07:17 PDT (History)
12 users (show)

See Also:


Attachments
Patch (14.20 KB, patch)
2011-04-05 09:04 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (13.52 KB, patch)
2011-04-05 09:51 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (13.66 KB, patch)
2011-04-05 10:29 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (19.69 KB, patch)
2011-04-06 07:34 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (20.31 KB, patch)
2011-04-06 08:59 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (20.29 KB, patch)
2011-04-06 10:11 PDT, Leandro Graciá Gil
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2011-04-04 11:27:53 PDT
Make a template of the existing functions to create callbacks from arguments in JSGeolocationCustom.cpp and place it in a common place somewhere else. This method is going to be used in common by Geolocation, the Media Stream API and possibly more.
Comment 1 Leandro Graciá Gil 2011-04-04 11:32:24 PDT
This is the JavaScriptCore equivalent of https://bugs.webkit.org/show_bug.cgi?id=57760 for V8.
Comment 2 Leandro Graciá Gil 2011-04-05 09:04:47 PDT
Created attachment 88244 [details]
Patch
Comment 3 Alexey Proskuryakov 2011-04-05 09:20:02 PDT
>     // FIXME: createFunctionOnlyCallback disallows callable objects created via JSC API. It's not clear what exactly the specification intends to allow.

My understanding is that it's clear now, see bug 40012 comment 3.

Gavin or Oliver would probably be the best reviewers for this patch.
Comment 4 Darin Adler 2011-04-05 09:23:36 PDT
Comment on attachment 88244 [details]
Patch

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

Since this patch is entirely about refactoring and not a behavior change, stylistic and clarity issues are paramount, more important even than with a bug fix. So please read my comments with that in mind.

I’m going to say review+ but I strongly suggest making at least some changes in response to my comments even though with my review+ that is not formally required.

> Source/WebCore/bindings/js/CallbackUtilities.h:25
> +#ifndef CallbackUtilities_h

We frown on terms like “utilities” and “manager” in our filenames and class names. These  excess words don’t add much clarity and in the case of classes, are a bit of an object oriented programming anti-pattern.

It looks like some have crept over time, which is unfortunate: IDBBindingUtilities, DOMUtility, V8Utilities, c_utility.cpp, JNIUtility, objc_utility, AudioUtilities, ClipboardUtilitiesChromium, ClipboardUtilitiesWin, SVGParserUtilities, InjectedScriptManager, BreakpointManager, TextureManager, ResourceHandleManager. You’ll notice that most of these are on recent additions to WebKit, so probably just folks who didn’t participate in the consensus view that we want to avoid those vague words.

For this header file, I would suggest the name Callback.h or CallbackFunction.h or FunctionOnlyCallback.h.

> Source/WebCore/bindings/js/CallbackUtilities.h:42
> +// 'FunctionOnly' is assumed for the created callback. Callable objects created via JSC API are disallowed.

Comments need to answer the question “why?” and this no longer makes it clear that the “callable objects are disallowed” behavior is possibly a bug. I know we still have FIXMEs at the call site, but it seems wrong to make a function that has behavior that’s probably not helpful anywhere, but comment on it in a way that makes it seem deliberate.

So I’d like to see a comment that’s more clear on this point. And I think that having a FIXME at each call site is not really all that helpful. There’s some slight chance that some day we might change one call site and not others, but it’s more likely we’ll fix things in this central location.

Also, I’m something of an expert on this, but I don’t understand what FunctionOnly means. Is this a reference to something in a specification somewhere?

> Source/WebCore/bindings/js/CallbackUtilities.h:53
> +    if ((value.isUndefined() && (acceptedValues & CallbackAllowUndefined))
> +        || (value.isNull() && (acceptedValues & CallbackAllowNull)))
> +        return 0;
> +
> +    if (!value.inherits(&JSC::JSFunction::s_info)) {
> +        setDOMException(exec, TYPE_MISMATCH_ERR);
> +        return 0;
> +    }

If performance is an issue, then the checks for undefined and null should go inside the !inherits block. Otherwise we’ll be doing extra checks for them.

The undefined and null checks would be easier to read in separate if statements, with more straightforward formatting without that line break.

> Source/WebCore/bindings/js/CallbackUtilities.h:56
> +    JSC::JSObject* object = asObject(value);
> +    return JSCallbackType::create(object, globalObject);

The local variable here doesn’t add any clarity; I suggest using asObject directly inside the function call.
Comment 5 Steve Block 2011-04-05 09:40:43 PDT
Comment on attachment 88244 [details]
Patch

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

>> Source/WebCore/bindings/js/CallbackUtilities.h:42
>> +// 'FunctionOnly' is assumed for the created callback. Callable objects created via JSC API are disallowed.
> 
> Comments need to answer the question “why?” and this no longer makes it clear that the “callable objects are disallowed” behavior is possibly a bug. I know we still have FIXMEs at the call site, but it seems wrong to make a function that has behavior that’s probably not helpful anywhere, but comment on it in a way that makes it seem deliberate.
> 
> So I’d like to see a comment that’s more clear on this point. And I think that having a FIXME at each call site is not really all that helpful. There’s some slight chance that some day we might change one call site and not others, but it’s more likely we’ll fix things in this central location.
> 
> Also, I’m something of an expert on this, but I don’t understand what FunctionOnly means. Is this a reference to something in a specification somewhere?

'FunctionOnly' is a WebIDL tag used by Geolocation and MediaStream. There's ongoing discussion as to how we should handle this in https://bugs.webkit.org/show_bug.cgi?id=40012.

I agree that the FIXME should be in this new helper, not at each call site. The helper creates a FunctionOnly callback, and that won't change, the question is how to do this.

> Source/WebCore/bindings/js/JSGeolocationCustom.cpp:118
> +        static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), exec->argument(0), CallbackAllowFunction);

There's no maximum line length, so there's no need to break this line.
Comment 6 Steve Block 2011-04-05 09:47:12 PDT
> My understanding is that it's clear now, see bug 40012 comment 3.
> 
> Gavin or Oliver would probably be the best reviewers for this patch.
OK, if there's consensus we'll make fix. Are you OK with us doing it after this refactoring is in?
Comment 7 Leandro Graciá Gil 2011-04-05 09:51:37 PDT
Created attachment 88260 [details]
Patch
Comment 8 Leandro Graciá Gil 2011-04-05 09:54:35 PDT
Comment on attachment 88244 [details]
Patch

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

>> Source/WebCore/bindings/js/CallbackUtilities.h:25
>> +#ifndef CallbackUtilities_h
> 
> We frown on terms like “utilities” and “manager” in our filenames and class names. These  excess words don’t add much clarity and in the case of classes, are a bit of an object oriented programming anti-pattern.
> 
> It looks like some have crept over time, which is unfortunate: IDBBindingUtilities, DOMUtility, V8Utilities, c_utility.cpp, JNIUtility, objc_utility, AudioUtilities, ClipboardUtilitiesChromium, ClipboardUtilitiesWin, SVGParserUtilities, InjectedScriptManager, BreakpointManager, TextureManager, ResourceHandleManager. You’ll notice that most of these are on recent additions to WebKit, so probably just folks who didn’t participate in the consensus view that we want to avoid those vague words.
> 
> For this header file, I would suggest the name Callback.h or CallbackFunction.h or FunctionOnlyCallback.h.

Changed to CallbackFunction.h as this option should keep the previous ordering in the project files.

>>> Source/WebCore/bindings/js/CallbackUtilities.h:42
>>> +// 'FunctionOnly' is assumed for the created callback. Callable objects created via JSC API are disallowed.
>> 
>> Comments need to answer the question “why?” and this no longer makes it clear that the “callable objects are disallowed” behavior is possibly a bug. I know we still have FIXMEs at the call site, but it seems wrong to make a function that has behavior that’s probably not helpful anywhere, but comment on it in a way that makes it seem deliberate.
>> 
>> So I’d like to see a comment that’s more clear on this point. And I think that having a FIXME at each call site is not really all that helpful. There’s some slight chance that some day we might change one call site and not others, but it’s more likely we’ll fix things in this central location.
>> 
>> Also, I’m something of an expert on this, but I don’t understand what FunctionOnly means. Is this a reference to something in a specification somewhere?
> 
> 'FunctionOnly' is a WebIDL tag used by Geolocation and MediaStream. There's ongoing discussion as to how we should handle this in https://bugs.webkit.org/show_bug.cgi?id=40012.
> 
> I agree that the FIXME should be in this new helper, not at each call site. The helper creates a FunctionOnly callback, and that won't change, the question is how to do this.

This 'callable objects are disallowed' comes from the spec and has been discussed and solved here: https://bugs.webkit.org/show_bug.cgi?id=40012.
I'm applying the suggested solution and removing that part from the comment.

FunctionOnly is an extended attributed of [Callback] according to the WebIDL specification: http://dev.w3.org/2006/webapi/WebIDL/#Callback

>> Source/WebCore/bindings/js/CallbackUtilities.h:53
>> +    }
> 
> If performance is an issue, then the checks for undefined and null should go inside the !inherits block. Otherwise we’ll be doing extra checks for them.
> 
> The undefined and null checks would be easier to read in separate if statements, with more straightforward formatting without that line break.

Fixed. Undefined and null checks separated. Not moved into the later check since they are not expected to set an exception, but to allow uses like creating callbacks from optional (undefined) arguments.

>> Source/WebCore/bindings/js/CallbackUtilities.h:56
>> +    return JSCallbackType::create(object, globalObject);
> 
> The local variable here doesn’t add any clarity; I suggest using asObject directly inside the function call.

Fixed.
Comment 9 Leandro Graciá Gil 2011-04-05 09:55:30 PDT
(In reply to comment #3)
> >     // FIXME: createFunctionOnlyCallback disallows callable objects created via JSC API. It's not clear what exactly the specification intends to allow.
> 
> My understanding is that it's clear now, see bug 40012 comment 3.
> 
> Gavin or Oliver would probably be the best reviewers for this patch.

Applied the solution proposed there.
Comment 10 Steve Block 2011-04-05 09:59:37 PDT
Comment on attachment 88260 [details]
Patch

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

> Source/WebCore/bindings/js/CallbackFunction.h:42
> +// 'FunctionOnly' is assumed for the created callback.

Let's make this explicit - 'Creates callback objects for callbacks marked as FunctionOnly in WebIDL'.

> Source/WebCore/bindings/js/CallbackFunction.h:52
> +    if (!value.getCallData() != CallTypeNone) {

I think it best to keep the bug fix in a separate patch from this refactoring. You can do the fix in Bug 40012 and do so for V8 and JSC together. In any case, you have an extra '!' here.

> Source/WebCore/bindings/js/JSGeolocationCustom.cpp:117
> +        static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), exec->argument(0), CallbackAllowFunction);

No need for these line breaks.
Comment 11 Leandro Graciá Gil 2011-04-05 10:29:45 PDT
Created attachment 88272 [details]
Patch
Comment 12 Leandro Graciá Gil 2011-04-05 10:30:12 PDT
Comment on attachment 88260 [details]
Patch

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

>> Source/WebCore/bindings/js/CallbackFunction.h:42
>> +// 'FunctionOnly' is assumed for the created callback.
> 
> Let's make this explicit - 'Creates callback objects for callbacks marked as FunctionOnly in WebIDL'.

Fixed.

>> Source/WebCore/bindings/js/CallbackFunction.h:52
>> +    if (!value.getCallData() != CallTypeNone) {
> 
> I think it best to keep the bug fix in a separate patch from this refactoring. You can do the fix in Bug 40012 and do so for V8 and JSC together. In any case, you have an extra '!' here.

Done. Restored original contents to separate the refactoring from fixing the problem. This latter task is left to bug 40012.

>> Source/WebCore/bindings/js/JSGeolocationCustom.cpp:117
>> +        static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), exec->argument(0), CallbackAllowFunction);
> 
> No need for these line breaks.

Fixed.
Comment 13 Steve Block 2011-04-05 10:34:57 PDT
Comment on attachment 88272 [details]
Patch

r=me
Comment 14 Leandro Graciá Gil 2011-04-05 12:21:02 PDT
Darin, Alexey, any comments?
Comment 15 Darin Adler 2011-04-05 18:10:24 PDT
Comment on attachment 88272 [details]
Patch

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

Generally seems OK. A little awkward the way this handles undefined and null, but fine.

> Source/WebCore/bindings/js/CallbackFunction.h:30
> +#include <wtf/Forward.h>

What is wtf/Forward.h needed for? I don’t see anything here that would benefit from including that.

> Source/WebCore/bindings/js/CallbackFunction.h:35
> +    CallbackAllowFunction = 0,

Should probably be named CallbackAllowFunctionOnly. It’s misleading to have it be named AllowFunction when a function will be allowed even if you don’t pass this value!

> Source/WebCore/bindings/js/CallbackFunction.h:57
> +    if (value.isUndefined() && (acceptedValues & CallbackAllowUndefined))
> +        return 0;
> +
> +    if (value.isNull() && (acceptedValues & CallbackAllowNull))
> +        return 0;
> +
> +    // FIXME: disallows callable objects created via JSC API. It's not clear what exactly the specification intends to allow.
> +    // https://bugs.webkit.org/show_bug.cgi?id=40012
> +    if (!value.inherits(&JSC::JSFunction::s_info)) {
> +        setDOMException(exec, TYPE_MISMATCH_ERR);
> +        return 0;
> +    }

There’s no real reason this has to be in the header. If this logic was in a non-template function called by the template function we could make this header include fewer header files.
Comment 16 Leandro Graciá Gil 2011-04-06 03:44:52 PDT
Comment on attachment 88272 [details]
Patch

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

>> Source/WebCore/bindings/js/CallbackFunction.h:30
>> +#include <wtf/Forward.h>
> 
> What is wtf/Forward.h needed for? I don’t see anything here that would benefit from including that.

The return value of the template is a PassRefPtr object.

>> Source/WebCore/bindings/js/CallbackFunction.h:35
>> +    CallbackAllowFunction = 0,
> 
> Should probably be named CallbackAllowFunctionOnly. It’s misleading to have it be named AllowFunction when a function will be allowed even if you don’t pass this value!

I already thought that, but then it makes no sense to write something valid like CallbackAllowFunctionOnly | CallbackAllowNull. Do you think it should be better to have CallbackAllowFunctionOnly since that value is only used in the exclusive cases and ignore it in other situations?
Comment 17 Steve Block 2011-04-06 04:01:32 PDT
Comment on attachment 88272 [details]
Patch

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

>>> Source/WebCore/bindings/js/CallbackFunction.h:35
>>> +    CallbackAllowFunction = 0,
>> 
>> Should probably be named CallbackAllowFunctionOnly. It’s misleading to have it be named AllowFunction when a function will be allowed even if you don’t pass this value!
> 
> I already thought that, but then it makes no sense to write something valid like CallbackAllowFunctionOnly | CallbackAllowNull. Do you think it should be better to have CallbackAllowFunctionOnly since that value is only used in the exclusive cases and ignore it in other situations?

Yes, I'd thought about this too. I see problems with both names, so don't feel strongly either way.
Comment 18 Darin Adler 2011-04-06 07:22:14 PDT
Comment on attachment 88272 [details]
Patch

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

>>> Source/WebCore/bindings/js/CallbackFunction.h:30
>>> +#include <wtf/Forward.h>
>> 
>> What is wtf/Forward.h needed for? I don’t see anything here that would benefit from including that.
> 
> The return value of the template is a PassRefPtr object.

Yes, but including Forward.h is insufficient to compile the definition of a function that returns a PassRefPtr. It’s enough to compile a declaration, but not a definition.

The code compiles because JSFunction.h ends up including PassRefPtr.h and your include of Forward.h has no effect.

>>>> Source/WebCore/bindings/js/CallbackFunction.h:35
>>>> +    CallbackAllowFunction = 0,
>>> 
>>> Should probably be named CallbackAllowFunctionOnly. It’s misleading to have it be named AllowFunction when a function will be allowed even if you don’t pass this value!
>> 
>> I already thought that, but then it makes no sense to write something valid like CallbackAllowFunctionOnly | CallbackAllowNull. Do you think it should be better to have CallbackAllowFunctionOnly since that value is only used in the exclusive cases and ignore it in other situations?
> 
> Yes, I'd thought about this too. I see problems with both names, so don't feel strongly either way.

Yes, I think the real problem is more fundamental. Since these constants are designed to or together it makes no sense to have one with the value 0; you can’t or that with something else in a meaningful way. We’d be better off with no name for the value 0, and with the flags defaulting to 0. Then no caller has to pass 0 explicitly.
Comment 19 Leandro Graciá Gil 2011-04-06 07:34:06 PDT
Created attachment 88414 [details]
Patch
Comment 20 Leandro Graciá Gil 2011-04-06 07:35:58 PDT
Comment on attachment 88272 [details]
Patch

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

>>>> Source/WebCore/bindings/js/CallbackFunction.h:30
>>>> +#include <wtf/Forward.h>
>>> 
>>> What is wtf/Forward.h needed for? I don’t see anything here that would benefit from including that.
>> 
>> The return value of the template is a PassRefPtr object.
> 
> Yes, but including Forward.h is insufficient to compile the definition of a function that returns a PassRefPtr. It’s enough to compile a declaration, but not a definition.
> 
> The code compiles because JSFunction.h ends up including PassRefPtr.h and your include of Forward.h has no effect.

Fixed.

>>>>> Source/WebCore/bindings/js/CallbackFunction.h:35
>>>>> +    CallbackAllowFunction = 0,
>>>> 
>>>> Should probably be named CallbackAllowFunctionOnly. It’s misleading to have it be named AllowFunction when a function will be allowed even if you don’t pass this value!
>>> 
>>> I already thought that, but then it makes no sense to write something valid like CallbackAllowFunctionOnly | CallbackAllowNull. Do you think it should be better to have CallbackAllowFunctionOnly since that value is only used in the exclusive cases and ignore it in other situations?
>> 
>> Yes, I'd thought about this too. I see problems with both names, so don't feel strongly either way.
> 
> Yes, I think the real problem is more fundamental. Since these constants are designed to or together it makes no sense to have one with the value 0; you can’t or that with something else in a meaningful way. We’d be better off with no name for the value 0, and with the flags defaulting to 0. Then no caller has to pass 0 explicitly.

Fixed. Will prepare a bug to make the V8 version consistent to this.

>> Source/WebCore/bindings/js/CallbackFunction.h:57
>> +    }
> 
> There’s no real reason this has to be in the header. If this logic was in a non-template function called by the template function we could make this header include fewer header files.

Fixed.
Comment 21 Build Bot 2011-04-06 08:37:04 PDT
Attachment 88414 [details] did not build on win:
Build output: http://queues.webkit.org/results/8342213
Comment 22 Leandro Graciá Gil 2011-04-06 08:59:42 PDT
Created attachment 88436 [details]
Patch
Comment 23 Leandro Graciá Gil 2011-04-06 09:00:43 PDT
(In reply to comment #22)
> Created an attachment (id=88436) [details]
> Patch

Adding missing CallbackFunction.cpp to JSBindingsAllInOne.cpp. The style error is caused by this.
Comment 24 WebKit Review Bot 2011-04-06 09:02:16 PDT
Attachment 88436 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/bindings/js/JSBindingsAllInOne.cpp:28:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Darin Adler 2011-04-06 09:28:47 PDT
Comment on attachment 88436 [details]
Patch

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

> Source/WebCore/bindings/js/CallbackFunction.h:52
> +        return JSCallbackType::create(asObject(value), globalObject);

Sorry, the refactoring I asked you for broke the function. This line will assert or crash when asObject is called on a null or undefined value.
Comment 26 Darin Adler 2011-04-06 09:29:36 PDT
Comment on attachment 88436 [details]
Patch

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

>> Source/WebCore/bindings/js/CallbackFunction.h:52
>> +        return JSCallbackType::create(asObject(value), globalObject);
> 
> Sorry, the refactoring I asked you for broke the function. This line will assert or crash when asObject is called on a null or undefined value.

No, my mistake.
Comment 27 WebKit Commit Bot 2011-04-06 09:37:58 PDT
Comment on attachment 88436 [details]
Patch

Rejecting attachment 88436 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
 file Source/WebCore/WebCore.vcproj/WebCore.vcproj.rej
patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj
patching file Source/WebCore/bindings/js/CallbackFunction.cpp
patching file Source/WebCore/bindings/js/CallbackFunction.h
patching file Source/WebCore/bindings/js/JSBindingsAllInOne.cpp
patching file Source/WebCore/bindings/js/JSGeolocationCustom.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8343223
Comment 28 Leandro Graciá Gil 2011-04-06 10:11:51 PDT
Created attachment 88461 [details]
Patch
Comment 29 Leandro Graciá Gil 2011-04-06 10:12:55 PDT
(In reply to comment #28)
> Created an attachment (id=88461) [details]
> Patch

Rebased to fix the problems with WebCore.vcproj (updated by 83065 before the commit queue).
Comment 30 Steve Block 2011-04-06 10:14:00 PDT
Comment on attachment 88461 [details]
Patch

r=me
Comment 31 WebKit Review Bot 2011-04-06 10:15:04 PDT
Attachment 88461 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/bindings/js/JSBindingsAllInOne.cpp:28:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 WebKit Commit Bot 2011-04-06 10:21:22 PDT
Comment on attachment 88461 [details]
Patch

Rejecting attachment 88461 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2

Last 500 characters of output:
ects to file Source/WebCore/WebCore.vcproj/WebCore.vcproj.rej
patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj
patching file Source/WebCore/bindings/js/CallbackFunction.cpp
patching file Source/WebCore/bindings/js/CallbackFunction.h
patching file Source/WebCore/bindings/js/JSBindingsAllInOne.cpp
patching file Source/WebCore/bindings/js/JSGeolocationCustom.cpp

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Steve Block', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8347258
Comment 33 Alexey Proskuryakov 2011-04-06 10:27:38 PDT
It probably has problems with WebCore.vcproj, and needs a manual commit.
Comment 34 Eric Seidel (no email) 2011-04-06 10:29:35 PDT
Dan Bates has some theories on the vcproj apply problems.  Hopefully we'll have a fix soon.
Comment 35 Steve Block 2011-04-06 11:47:13 PDT
Committed r83079: <http://trac.webkit.org/changeset/83079>
Comment 36 Steve Block 2011-04-06 11:47:55 PDT
Comment on attachment 88461 [details]
Patch

Clearing r+ now committed
Comment 37 Csaba Osztrogonác 2011-04-06 12:57:35 PDT
(In reply to comment #35)
> Committed r83079: <http://trac.webkit.org/changeset/83079>

And Qt buildfix landed in http://trac.webkit.org/changeset/83086