Bug 87960 - Web Inspector: [WebGL] Add WebGL instrumentation support on the backend
Summary: Web Inspector: [WebGL] Add WebGL instrumentation support on the backend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Adaikin
URL:
Keywords:
Depends on:
Blocks: 87959 87975
  Show dependency treegraph
 
Reported: 2012-05-31 05:31 PDT by Andrey Adaikin
Modified: 2012-06-06 01:49 PDT (History)
23 users (show)

See Also:


Attachments
Patch (54.25 KB, patch)
2012-05-31 05:46 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (53.27 KB, patch)
2012-05-31 23:44 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (53.23 KB, patch)
2012-06-01 00:52 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (51.17 KB, patch)
2012-06-01 02:53 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (47.86 KB, patch)
2012-06-01 05:45 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (47.83 KB, patch)
2012-06-01 06:01 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (47.51 KB, patch)
2012-06-01 06:13 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (47.59 KB, patch)
2012-06-01 08:05 PDT, Andrey Adaikin
aandrey: commit-queue-
Details | Formatted Diff | Diff
Patch (49.73 KB, patch)
2012-06-01 09:15 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (49.60 KB, patch)
2012-06-01 09:57 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (50.32 KB, patch)
2012-06-01 10:23 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (49.66 KB, patch)
2012-06-04 00:31 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (52.87 KB, patch)
2012-06-04 03:02 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (51.35 KB, patch)
2012-06-04 03:29 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (53.36 KB, patch)
2012-06-04 03:45 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (52.87 KB, patch)
2012-06-04 23:58 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (54.88 KB, patch)
2012-06-05 00:46 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (55.57 KB, patch)
2012-06-05 02:12 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (56.09 KB, patch)
2012-06-05 05:16 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (56.09 KB, patch)
2012-06-05 05:51 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (71.27 KB, patch)
2012-06-05 07:41 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (69.26 KB, patch)
2012-06-05 08:17 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
=Rebased patch (55.45 KB, patch)
2012-06-06 01:21 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Adaikin 2012-05-31 05:31:19 PDT
Adding WebGL instrumentation support on the backend side.
Comment 1 Andrey Adaikin 2012-05-31 05:46:33 PDT
Created attachment 145064 [details]
Patch
Comment 2 Build Bot 2012-05-31 06:13:49 PDT
Comment on attachment 145064 [details]
Patch

Attachment 145064 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12859347
Comment 3 Tor Arne Vestbø 2012-05-31 06:16:01 PDT
Comment on attachment 145064 [details]
Patch

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

> Source/WebCore/DerivedSources.pri:779
> +InjectedWebGLScriptSource.commands = perl $$PWD/inspector/xxd.pl InjectedWebGLScriptSource_js $$PWD/inspector/InjectedWebGLScriptSource.js ${QMAKE_FILE_OUT}

$$PWD/inspector/InjectedWebGLScriptSource.js -- > ${QMAKE_FILE_IN}
Comment 4 Andrey Kosyakov 2012-05-31 06:48:18 PDT
Comment on attachment 145064 [details]
Patch

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

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:81
> +    if (context->is3d() && InspectorInstrumentation::isWebGLInspectorEnabled(canvas->document())) {
> +        ScriptObject wrapped = InspectorInstrumentation::wrapWebGLRenderingContextForInstrumentation(static_cast<WebGLRenderingContext*>(context));

Can we merge isWebGLInspectorEnabled() and wrapWebGLRenderingContextForInstrumentation() into one call, e.g. wrapWebGLRenderingContextForInstrumentationIfNecessary()?

> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87
> +    // FIXME: Implement this!

Please file a bug to have this implemented.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:62
> +template<typename T> static v8::Local<v8::Object> toV8InLocalContext(v8::Handle<v8::FunctionTemplate> functionTemplate, WrapperTypeInfo* type, T* cptr)

We avoid abbreviations in WebKit, please find a more descriptive name for cptr.

> Source/WebCore/inspector/InjectedScriptManager.cpp:42
> +#if ENABLE(WEBGL)
> +#include "InjectedWebGLScriptSource.h"
> +#endif

Is this really necessary?

> Source/WebCore/inspector/InjectedScriptManager.cpp:182
> +    ScriptObject injectedWebGLScript = injectWebGLContextWrapperScript(injectedWebGLScriptSource(), scriptState);
> +    return executeInjectedWebGLContextWrapperScript(&injectedWebGLScript, scriptState, glContext);

Do we ever call any of these two function independently?

> Source/WebCore/inspector/InjectedWebGLScriptSource.js:2
> + * Copyright (C) 2012 Apple Inc.  All rights reserved.

Google copyright, please :)

> Source/WebCore/inspector/InspectorWebGLAgent.cpp:60
> +    m_instrumentingAgents = 0;

This appears redundant. I know we occasionally do this in other places, but I don't see justification for this there either.

> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:47
> +    if (InstrumentingAgents * instrumentingAgents = instrumentingAgentsForDocument(document)) {

InstrumentingAgents * => InstrumentingAgents*

> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:48
> +        if (InspectorWebGLAgent * webGLAgent = instrumentingAgents->inspectorWebGLAgent())

ditto

> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:59
> +    if (InstrumentingAgents * instrumentingAgents = instrumentingAgentsForDocument(glContext->canvas()->document())) {

ditto

> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:60
> +        if (InspectorWebGLAgent * webGLAgent = instrumentingAgents->inspectorWebGLAgent())

ditto
Comment 5 Yury Semikhatsky 2012-05-31 07:04:44 PDT
Comment on attachment 145064 [details]
Patch

r- per Andrey's comments
Comment 6 Andrey Adaikin 2012-05-31 23:40:25 PDT
Comment on attachment 145064 [details]
Patch

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

>> Source/WebCore/DerivedSources.pri:779
>> +InjectedWebGLScriptSource.commands = perl $$PWD/inspector/xxd.pl InjectedWebGLScriptSource_js $$PWD/inspector/InjectedWebGLScriptSource.js ${QMAKE_FILE_OUT}
> 
> $$PWD/inspector/InjectedWebGLScriptSource.js -- > ${QMAKE_FILE_IN}

Done.

>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:81
>> +        ScriptObject wrapped = InspectorInstrumentation::wrapWebGLRenderingContextForInstrumentation(static_cast<WebGLRenderingContext*>(context));
> 
> Can we merge isWebGLInspectorEnabled() and wrapWebGLRenderingContextForInstrumentation() into one call, e.g. wrapWebGLRenderingContextForInstrumentationIfNecessary()?

Removed isWebGLInspectorEnabled.

>> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87
>> +    // FIXME: Implement this!
> 
> Please file a bug to have this implemented.

Filed https://bugs.webkit.org/show_bug.cgi?id=87975

>> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:62
>> +template<typename T> static v8::Local<v8::Object> toV8InLocalContext(v8::Handle<v8::FunctionTemplate> functionTemplate, WrapperTypeInfo* type, T* cptr)
> 
> We avoid abbreviations in WebKit, please find a more descriptive name for cptr.

Done. cptr -> nativeObject

>> Source/WebCore/inspector/InjectedScriptManager.cpp:42
>> +#endif
> 
> Is this really necessary?

Yes, for the InjectedWebGLScriptSource_js variable

>> Source/WebCore/inspector/InjectedScriptManager.cpp:182
>> +    return executeInjectedWebGLContextWrapperScript(&injectedWebGLScript, scriptState, glContext);
> 
> Do we ever call any of these two function independently?

No. Merged into one injectWebGLScript()

>> Source/WebCore/inspector/InjectedWebGLScriptSource.js:2
>> + * Copyright (C) 2012 Apple Inc.  All rights reserved.
> 
> Google copyright, please :)

Done.

>> Source/WebCore/inspector/InspectorWebGLAgent.cpp:60
>> +    m_instrumentingAgents = 0;
> 
> This appears redundant. I know we occasionally do this in other places, but I don't see justification for this there either.

What exactly is redundant? Only the last line, or both?
As far as I see, it is to avoid a circular dependency, and both lines are "symmetrical" in that sense. If this is the case, I'd suggest preserving the symmetry, although it may be redundant.

>> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:47
>> +    if (InstrumentingAgents * instrumentingAgents = instrumentingAgentsForDocument(document)) {
> 
> InstrumentingAgents * => InstrumentingAgents*

Done.

>> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:48
>> +        if (InspectorWebGLAgent * webGLAgent = instrumentingAgents->inspectorWebGLAgent())
> 
> ditto

Done.

>> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:59
>> +    if (InstrumentingAgents * instrumentingAgents = instrumentingAgentsForDocument(glContext->canvas()->document())) {
> 
> ditto

Done.

>> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:60
>> +        if (InspectorWebGLAgent * webGLAgent = instrumentingAgents->inspectorWebGLAgent())
> 
> ditto

Done.
Comment 7 Andrey Adaikin 2012-05-31 23:44:27 PDT
Created attachment 145219 [details]
Patch
Comment 8 Build Bot 2012-06-01 00:25:31 PDT
Comment on attachment 145219 [details]
Patch

Attachment 145219 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12866431
Comment 9 Andrey Adaikin 2012-06-01 00:52:42 PDT
Created attachment 145233 [details]
Patch
Comment 10 Yury Semikhatsky 2012-06-01 01:11:02 PDT
Comment on attachment 145219 [details]
Patch

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

> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87
> +    // FIXME: Implement this!

Please file a bug against JSC and put its number after the fixme.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:138
> +    windowGlobal->SetHiddenValue(key, v);

I don't think you need to store the function in a hidden property. As I understand it is called only once to wrap glContextObject and then you use the wrapper. If so the hidden property is not needed. Please explain what the expected behavior is here.

> Source/WebCore/inspector/InspectorWebGLAgent.cpp:100
> +    ScriptState* scriptState = mainWorldScriptState(glContext->canvas()->document()->frame());

You should pass ScriptState as an explicit parameter. It is known in the bindings, you will need to covert v8::Context into ScriptState though.
Comment 11 Andrey Adaikin 2012-06-01 02:33:06 PDT
Comment on attachment 145219 [details]
Patch

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

>> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87
>> +    // FIXME: Implement this!
> 
> Please file a bug against JSC and put its number after the fixme.

Done.

>> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:138
>> +    windowGlobal->SetHiddenValue(key, v);
> 
> I don't think you need to store the function in a hidden property. As I understand it is called only once to wrap glContextObject and then you use the wrapper. If so the hidden property is not needed. Please explain what the expected behavior is here.

Removed caching in the hidden property.

>> Source/WebCore/inspector/InspectorWebGLAgent.cpp:100
>> +    ScriptState* scriptState = mainWorldScriptState(glContext->canvas()->document()->frame());
> 
> You should pass ScriptState as an explicit parameter. It is known in the bindings, you will need to covert v8::Context into ScriptState though.

Done.
Comment 12 Andrey Adaikin 2012-06-01 02:53:32 PDT
Created attachment 145251 [details]
Patch
Comment 13 Build Bot 2012-06-01 03:18:44 PDT
Comment on attachment 145251 [details]
Patch

Attachment 145251 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12873223
Comment 14 Andrey Kosyakov 2012-06-01 03:33:29 PDT
Comment on attachment 145251 [details]
Patch

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

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:82
> +        if (wrapped.jsValue().isObject())

Drop the .jsValue(). Just wrapped.isObject(). It's cleaner.

> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:46
> +#include "JSWebGLRenderingContext.h"

This could be replaced with forward declaration for the time being, I guess?

> Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94
> +        if (!wrapped.v8Value().IsEmpty())

ditto.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:144
> +    v8::Local<v8::Value> injectedScriptValue = v8::Function::Cast(*v)->Call(windowGlobal, 1, args);

IIRC, this will crash if !v->IsFunction(). This might be ok given the assert above, but considering this may result from any JS error in a frequently modified injected script, we may want to fail more gracefully.
Comment 15 Yury Semikhatsky 2012-06-01 03:36:12 PDT
Comment on attachment 145251 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:2602
> +ADD_CUSTOM_COMMAND(

Should we check if WebGL is enabled here?

> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87
> +    // FIXME(87975): Implement this!

It shouldn't be hard to implement this for JSC, we already do similar things with the inspector injected script.

> Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94
> +        if (!wrapped.v8Value().IsEmpty())

This code will create ScriptState instance for all contexts even if WebGL instrumentation is disabled. This should be fixed. We should probably check if WebGL inspection is enabled before getting ScriptState.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:-60
> -    v8::Local<v8::Function> function = V8InjectedScriptHost::GetTemplate()->GetFunction();

These changes are not needed anymore, please revert them if so.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:129
> +    v8::Local<v8::Object> glContextObject = toV8InLocalContext(V8WebGLRenderingContext::GetTemplate(), &V8WebGLRenderingContext::info, glContext);

You should be able to use standard toV8 mechanism here, it will create a binding for you which you will wrap with the instrumenting code. Existing code could probably changed as well I need to check.
Comment 16 Andrey Adaikin 2012-06-01 05:39:11 PDT
Comment on attachment 145251 [details]
Patch

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

>> Source/WebCore/CMakeLists.txt:2602
>> +ADD_CUSTOM_COMMAND(
> 
> Should we check if WebGL is enabled here?

Done.

>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:82
>> +        if (wrapped.jsValue().isObject())
> 
> Drop the .jsValue(). Just wrapped.isObject(). It's cleaner.

Done. Using the hasNoValue() method now.

>> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87
>> +    // FIXME(87975): Implement this!
> 
> It shouldn't be hard to implement this for JSC, we already do similar things with the inspector injected script.

Will do it in a separate patch. Let us finish with this first.

>>> Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94
>>> +        if (!wrapped.v8Value().IsEmpty())
>> 
>> ditto.
> 
> This code will create ScriptState instance for all contexts even if WebGL instrumentation is disabled. This should be fixed. We should probably check if WebGL inspection is enabled before getting ScriptState.

Done.

>> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:129
>> +    v8::Local<v8::Object> glContextObject = toV8InLocalContext(V8WebGLRenderingContext::GetTemplate(), &V8WebGLRenderingContext::info, glContext);
> 
> You should be able to use standard toV8 mechanism here, it will create a binding for you which you will wrap with the instrumenting code. Existing code could probably changed as well I need to check.

Done.
Comment 17 Andrey Adaikin 2012-06-01 05:45:08 PDT
Created attachment 145277 [details]
Patch
Comment 18 Andrey Kosyakov 2012-06-01 05:54:30 PDT
Comment on attachment 145277 [details]
Patch

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

> Source/WebCore/inspector/InspectorWebGLAgent.cpp:62
> +    m_instrumentingAgents = 0;

This one still appears redundant. Do we really need to clear this field in desturctor? Also, it belongs to the base class and is initialized by the base class, so it's unclear why it's cleared by derived class.
Comment 19 Andrey Adaikin 2012-06-01 05:59:38 PDT
Comment on attachment 145277 [details]
Patch

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

>> Source/WebCore/inspector/InspectorWebGLAgent.cpp:62
>> +    m_instrumentingAgents = 0;
> 
> This one still appears redundant. Do we really need to clear this field in desturctor? Also, it belongs to the base class and is initialized by the base class, so it's unclear why it's cleared by derived class.

Good point. Removed the line.
Comment 20 Yury Semikhatsky 2012-06-01 05:59:53 PDT
Comment on attachment 145277 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94
> +        if (InspectorInstrumentation::isWebGLInspectorEnabled(imp->document())) {

Alternatively you could check InspectorInstrumentation::hasFrontends() here to avoid adding additional is* method on the inspector instrumentation.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:138
> +    v8::Local<v8::Object> injectedScriptObject(v8::Object::Cast(*injectedScriptValue));

You could do v8::Handle<v8::Object>::Cast(injectedScriptValue) instead to avoid creating additional handle.
Comment 21 Andrey Adaikin 2012-06-01 06:01:05 PDT
Created attachment 145282 [details]
Patch
Comment 22 Andrey Adaikin 2012-06-01 06:13:24 PDT
Created attachment 145287 [details]
Patch
Comment 23 Andrey Adaikin 2012-06-01 06:14:29 PDT
Comment on attachment 145277 [details]
Patch

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

>> Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94
>> +        if (InspectorInstrumentation::isWebGLInspectorEnabled(imp->document())) {
> 
> Alternatively you could check InspectorInstrumentation::hasFrontends() here to avoid adding additional is* method on the inspector instrumentation.

Done.

>> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:138
>> +    v8::Local<v8::Object> injectedScriptObject(v8::Object::Cast(*injectedScriptValue));
> 
> You could do v8::Handle<v8::Object>::Cast(injectedScriptValue) instead to avoid creating additional handle.

Done.
Comment 24 Build Bot 2012-06-01 07:15:08 PDT
Comment on attachment 145287 [details]
Patch

Attachment 145287 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12864557
Comment 25 Andrey Adaikin 2012-06-01 08:05:16 PDT
Created attachment 145310 [details]
Patch
Comment 26 Andrey Adaikin 2012-06-01 09:15:39 PDT
Created attachment 145325 [details]
Patch
Comment 27 Build Bot 2012-06-01 09:51:18 PDT
Comment on attachment 145325 [details]
Patch

Attachment 145325 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12864605
Comment 28 Andrey Adaikin 2012-06-01 09:57:14 PDT
Created attachment 145331 [details]
Patch
Comment 29 Andrey Adaikin 2012-06-01 10:23:49 PDT
Created attachment 145336 [details]
Patch
Comment 30 Build Bot 2012-06-01 12:02:01 PDT
Comment on attachment 145336 [details]
Patch

Attachment 145336 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12866608
Comment 31 Build Bot 2012-06-01 12:02:40 PDT
Comment on attachment 145336 [details]
Patch

Attachment 145336 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12869564
Comment 32 Andrey Adaikin 2012-06-04 00:31:11 PDT
Created attachment 145526 [details]
Patch

Checking build bots
Comment 33 Build Bot 2012-06-04 01:10:24 PDT
Comment on attachment 145526 [details]
Patch

Attachment 145526 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12893213
Comment 34 Andrey Kosyakov 2012-06-04 01:20:32 PDT
Comment on attachment 145526 [details]
Patch

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

> Source/WebCore/WebCore.gypi:2824
> +            'inspector/InspectorWebGLAgent.cpp',
> +            'inspector/InspectorWebGLAgent.h',
> +            'inspector/InspectorWebGLInstrumentation.h',

Please also add this to WebCore.vcproj.
Comment 35 Andrey Adaikin 2012-06-04 03:02:56 PDT
Created attachment 145551 [details]
Patch

Checking build bots: Updated project.pbxproj, reverted WebCode.exp.in
Comment 36 Andrey Adaikin 2012-06-04 03:29:14 PDT
Created attachment 145553 [details]
Patch

Checking build bots
Comment 37 Build Bot 2012-06-04 03:38:30 PDT
Comment on attachment 145551 [details]
Patch

Attachment 145551 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12898201
Comment 38 Andrey Adaikin 2012-06-04 03:45:48 PDT
Created attachment 145555 [details]
Patch

Checking build bots: added WebCode.exp.in back
Comment 39 Build Bot 2012-06-04 03:52:20 PDT
Comment on attachment 145553 [details]
Patch

Attachment 145553 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12903155
Comment 40 Early Warning System Bot 2012-06-04 04:00:34 PDT
Comment on attachment 145555 [details]
Patch

Attachment 145555 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12902169
Comment 41 Build Bot 2012-06-04 04:41:11 PDT
Comment on attachment 145555 [details]
Patch

Attachment 145555 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12894281
Comment 42 Andrey Adaikin 2012-06-04 23:58:19 PDT
Created attachment 145705 [details]
Patch

Checking build bots
Comment 43 Andrey Adaikin 2012-06-05 00:46:25 PDT
Created attachment 145714 [details]
Patch

Checking build bots
Comment 44 Build Bot 2012-06-05 01:22:55 PDT
Comment on attachment 145705 [details]
Patch

Attachment 145705 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12900423
Comment 45 Build Bot 2012-06-05 01:33:15 PDT
Comment on attachment 145705 [details]
Patch

Attachment 145705 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12901418
Comment 46 Build Bot 2012-06-05 01:49:09 PDT
Comment on attachment 145714 [details]
Patch

Attachment 145714 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12898475
Comment 47 Andrey Adaikin 2012-06-05 02:12:05 PDT
Created attachment 145734 [details]
Patch

Checking build bots: added WebCore.order
Comment 48 jochen 2012-06-05 02:37:27 PDT
(In reply to comment #47)
> Created an attachment (id=145734) [details]
> Patch
> 
> Checking build bots: added WebCore.order

You need to add those symbols to WebCore.exp.in.

WebCore.order is only used for Safari builds or something, Apple engineers will take care of updating it
Comment 49 Andrey Adaikin 2012-06-05 02:42:29 PDT
(In reply to comment #48)
> (In reply to comment #47)
> > Created an attachment (id=145734) [details] [details]
> > Patch
> > 
> > Checking build bots: added WebCore.order
> 
> You need to add those symbols to WebCore.exp.in.
> 
> WebCore.order is only used for Safari builds or something, Apple engineers will take care of updating it

I did add those symbols to WebCore.exp.in - no luck so far...
Comment 50 jochen 2012-06-05 02:54:12 PDT
(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #47)
> > > Created an attachment (id=145734) [details] [details] [details]
> > > Patch
> > > 
> > > Checking build bots: added WebCore.order
> > 
> > You need to add those symbols to WebCore.exp.in.
> > 
> > WebCore.order is only used for Safari builds or something, Apple engineers will take care of updating it
> 
> I did add those symbols to WebCore.exp.in - no luck so far...

ah, indeed

on closer inspection, your patch to project.pbxproj looks incomplete. It doesn't list the new files in the PBXBuildFile section
Comment 51 Andrey Adaikin 2012-06-05 03:00:28 PDT
(In reply to comment #50)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > (In reply to comment #47)
> > > > Created an attachment (id=145734) [details] [details] [details] [details]
> > > > Patch
> > > > 
> > > > Checking build bots: added WebCore.order
> > > 
> > > You need to add those symbols to WebCore.exp.in.
> > > 
> > > WebCore.order is only used for Safari builds or something, Apple engineers will take care of updating it
> > 
> > I did add those symbols to WebCore.exp.in - no luck so far...
> 
> ah, indeed
> 
> on closer inspection, your patch to project.pbxproj looks incomplete. It doesn't list the new files in the PBXBuildFile section

yeah I played with it earlier at patch https://bugs.webkit.org/attachment.cgi?id=145551&action=review (my xcode would not add PBXBuildFile section for some reason) and got the following error from the bot:


xcodebuild: error: Unable to read project 'WebCore.xcodeproj'.
	Reason: Project /Volumes/Data/WebKit/Source/WebCore/WebCore.xcodeproj cannot be opened because because an exception occurred.: -[PBXFileReference _setBuildPhase:]: unrecognized selector sent to instance 0x4010c9140
Comment 52 Early Warning System Bot 2012-06-05 03:11:14 PDT
Comment on attachment 145734 [details]
Patch

Attachment 145734 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12905415
Comment 53 Build Bot 2012-06-05 04:02:17 PDT
Comment on attachment 145734 [details]
Patch

Attachment 145734 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12902459
Comment 54 Build Bot 2012-06-05 04:40:08 PDT
Comment on attachment 145734 [details]
Patch

Attachment 145734 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12893550
Comment 55 Andrey Adaikin 2012-06-05 05:16:48 PDT
Created attachment 145765 [details]
Patch

Checking build bots: tweaked project.pbxproj, removed WebCore.order
Comment 56 Andrey Adaikin 2012-06-05 05:51:03 PDT
Created attachment 145770 [details]
Patch

Checking build bots: fixed project.pbxproj
Comment 57 Build Bot 2012-06-05 06:05:57 PDT
Comment on attachment 145765 [details]
Patch

Attachment 145765 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12901478
Comment 58 Build Bot 2012-06-05 07:27:00 PDT
Comment on attachment 145770 [details]
Patch

Attachment 145770 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12902506
Comment 59 Andrey Adaikin 2012-06-05 07:41:27 PDT
Created attachment 145791 [details]
Patch

Checking build bots (hoping for the last time)
Comment 60 Build Bot 2012-06-05 07:42:21 PDT
Comment on attachment 145770 [details]
Patch

Attachment 145770 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12893593
Comment 61 Andrey Adaikin 2012-06-05 08:17:47 PDT
Created attachment 145802 [details]
Patch

Checking build bots: reverted WebCore.exp.in
Comment 62 Andrey Adaikin 2012-06-06 01:21:54 PDT
Created attachment 145959 [details]
=Rebased patch

Rebased patch
Comment 63 Andrey Kosyakov 2012-06-06 01:37:42 PDT
Committed r119572: <http://trac.webkit.org/changeset/119572>