Bug 88973 - Web Inspector: [WebGL] Add minimum transport protocol from backend to frontend
Summary: Web Inspector: [WebGL] Add minimum transport protocol from backend to frontend
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: 89087 89088 89107 89530 89592 94511 94576
Blocks: 87959 94689
  Show dependency treegraph
 
Reported: 2012-06-13 01:03 PDT by Andrey Adaikin
Modified: 2012-08-22 03:59 PDT (History)
18 users (show)

See Also:


Attachments
Patch (86.45 KB, patch)
2012-06-13 01:18 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (15.63 KB, patch)
2012-08-20 03:22 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (15.80 KB, patch)
2012-08-20 07:23 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Rebased patch to land (15.81 KB, patch)
2012-08-22 01:28 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-06-13 01:03:22 PDT
Minimum transport protocol implementation between backend and frontend to allow capturing WebGL function calls for a given frame.
This will add:
- WebGL.captureFrame command from FE to BE
- WebGL.contextCreated & WebGL.reportFunctionCall events from BE to FE
- simple experimental implementation of the InjectedWebGLScriptSource.js
Comment 1 Andrey Adaikin 2012-06-13 01:18:51 PDT
Created attachment 147259 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-13 01:23:02 PDT
Attachment 147259 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/inspector/InjectedScriptBase.cpp:90:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScriptBase.cpp:109:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScriptBase.h:63:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScriptBase.h:64:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andrey Adaikin 2012-06-14 03:01:17 PDT
Extracted part of this change to https://bugs.webkit.org/show_bug.cgi?id=89087
Comment 4 Andrey Adaikin 2012-06-14 03:20:43 PDT
Implementation of the InjectedWebGLScriptSource.js extracted to https://bugs.webkit.org/show_bug.cgi?id=89088
Comment 5 Pavel Feldman 2012-06-14 08:40:39 PDT
Comment on attachment 147259 [details]
Patch

Clearing r? from the obsolete patch.
Comment 6 Andrey Adaikin 2012-08-20 03:22:43 PDT
Created attachment 159385 [details]
Patch
Comment 7 WebKit Review Bot 2012-08-20 03:26:12 PDT
Attachment 159385 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/inspector/InjectedScriptWebGLModule.cpp:95:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScriptWebGLModule.h:54:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Build Bot 2012-08-20 03:53:41 PDT
Comment on attachment 159385 [details]
Patch

Attachment 159385 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13529906
Comment 9 Pavel Feldman 2012-08-20 06:52:36 PDT
Comment on attachment 159385 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptWebGLModule.cpp:75
> +void InjectedScriptWebGLModule::captureFrame(ErrorString* errorString, String* traceLogId)

captureTraceLog ?

>> Source/WebCore/inspector/InjectedScriptWebGLModule.cpp:95
>> +void InjectedScriptWebGLModule::getTraceLog(ErrorString* errorString, const String& traceLogId, RefPtr<TypeBuilder::WebGL::TraceLog>* traceLog)
> 
> The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]

::traceLog (according to the WebKit guidelines). We only use get prefixes in the protocol.

> Source/WebCore/inspector/InjectedScriptWebGLModule.cpp:97
> +    ScriptFunctionCall function(injectedScriptObject(), "getTraceLog");

traceLog

> Source/WebCore/inspector/Inspector.json:3160
> +                    { "name": "traceLogId", "$ref": "TraceLogId" }

Is there a "logCaptured" event? I wonder if you should return traceLogId there instead.

> Source/WebCore/inspector/Inspector.json:3164
> +                "name": "getTraceLog",

traceLog

> Source/WebCore/inspector/InspectorWebGLAgent.cpp:103
> +    InjectedScript injectedScript = m_injectedScriptManager->injectedScriptForObjectId(traceLogId);

It sounds like N-1 lines from here could be extracted and reused below.
Comment 10 Andrey Adaikin 2012-08-20 07:17:21 PDT
Comment on attachment 159385 [details]
Patch

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

>> Source/WebCore/inspector/InjectedScriptWebGLModule.cpp:75
>> +void InjectedScriptWebGLModule::captureFrame(ErrorString* errorString, String* traceLogId)
> 
> captureTraceLog ?

This is to capture only a single frame, i.e. we say webgl module to start capturing now *and* end capturing after the frame ends. Opposite to that, there may be startCapturing & endCapturing separate commands in the future, should we want to support capturing arbitrary number of frames.

>> Source/WebCore/inspector/InjectedScriptWebGLModule.cpp:97
>> +    ScriptFunctionCall function(injectedScriptObject(), "getTraceLog");
> 
> traceLog

done.

>> Source/WebCore/inspector/Inspector.json:3160
>> +                    { "name": "traceLogId", "$ref": "TraceLogId" }
> 
> Is there a "logCaptured" event? I wonder if you should return traceLogId there instead.

No events for now (need a InjectedModuleHost class implementation). For now we will be poking the trace logs with getTraceLog after a time out.

>> Source/WebCore/inspector/Inspector.json:3164
>> +                "name": "getTraceLog",
> 
> traceLog

did you really mean it? this is a protocol command, and as you said, we do use "get" prefixes in the protocol.

>> Source/WebCore/inspector/InspectorWebGLAgent.cpp:103
>> +    InjectedScript injectedScript = m_injectedScriptManager->injectedScriptForObjectId(traceLogId);
> 
> It sounds like N-1 lines from here could be extracted and reused below.

done.
Comment 11 Andrey Adaikin 2012-08-20 07:23:51 PDT
Created attachment 159420 [details]
Patch
Comment 12 WebKit Review Bot 2012-08-20 07:26:51 PDT
Attachment 159420 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/inspector/InjectedScriptWebGLModule.cpp:95:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScriptWebGLModule.h:56:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Review Bot 2012-08-20 07:49:24 PDT
Comment on attachment 159420 [details]
Patch

Rejecting attachment 159420 [details] from commit-queue.

aandrey@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 14 WebKit Review Bot 2012-08-20 08:05:29 PDT
Comment on attachment 159420 [details]
Patch

Clearing flags on attachment: 159420

Committed r126028: <http://trac.webkit.org/changeset/126028>
Comment 15 WebKit Review Bot 2012-08-20 08:05:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2012-08-21 00:21:58 PDT
Re-opened since this is blocked by 94576
Comment 17 Andrey Adaikin 2012-08-22 01:28:58 PDT
Created attachment 159879 [details]
Rebased patch to land
Comment 18 Andrey Kosyakov 2012-08-22 01:50:19 PDT
Committed r126280: <http://trac.webkit.org/changeset/126280>