Bug 78390 - Web Inspector: Switch Debugger agent to TypeBuilder
Summary: Web Inspector: Switch Debugger agent to TypeBuilder
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-10 14:24 PST by Peter Rybin
Modified: 2012-02-21 04:04 PST (History)
14 users (show)

See Also:


Attachments
Patch (14.18 KB, patch)
2012-02-10 14:54 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (14.94 KB, patch)
2012-02-14 13:24 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (15.51 KB, patch)
2012-02-14 14:30 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (15.08 KB, patch)
2012-02-15 13:08 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (14.41 KB, patch)
2012-02-15 14:01 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (14.39 KB, patch)
2012-02-17 12:41 PST, Peter Rybin
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Rybin 2012-02-10 14:24:16 PST
TypeBuilder provides type-safe API for output messages.
Switch one agent of domain "Debugger" to the new API.
Comment 1 Peter Rybin 2012-02-10 14:54:01 PST
Created attachment 126587 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-10 14:56:18 PST
Attachment 126587 [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/InjectedScript.cpp:79:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.cpp:93:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.h:80:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.h:82:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Vsevolod Vlasov 2012-02-11 01:41:21 PST
Comment on attachment 126587 [details]
Patch

I suggest adding "using" statements to implementation (e.g. like in InspectorIndexedDBAgent.cpp) to avoid flooding the code.
Comment 4 Peter Rybin 2012-02-14 13:24:18 PST
Created attachment 127029 [details]
Patch
Comment 5 Early Warning System Bot 2012-02-14 14:03:30 PST
Comment on attachment 127029 [details]
Patch

Attachment 127029 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11518411
Comment 6 Gustavo Noronha (kov) 2012-02-14 14:05:41 PST
Comment on attachment 127029 [details]
Patch

Attachment 127029 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11521386
Comment 7 Gyuyoung Kim 2012-02-14 14:28:42 PST
Comment on attachment 127029 [details]
Patch

Attachment 127029 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11523375
Comment 8 Peter Rybin 2012-02-14 14:30:56 PST
Created attachment 127048 [details]
Patch
Comment 9 Vsevolod Vlasov 2012-02-14 15:09:01 PST
Comment on attachment 127048 [details]
Patch

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

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324
> +    using WebCore::TypeBuilder::Debugger::Location;

Why isn't this statement at the beginning of the file?
This seems to be forbidden by style guide and you need to use a long name for return value type because of that.
Comment 10 Peter Rybin 2012-02-14 15:41:31 PST
(In reply to comment #9)
> (From update of attachment 127048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127048&action=review
> 
> > Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324
> > +    using WebCore::TypeBuilder::Debugger::Location;
> 
> Why isn't this statement at the beginning of the file?
> This seems to be forbidden by style guide and you need to use a long name for return value type because of that.

Are you sure it IS forbidden?

Anyway Location clashes with WebCore::Location and it's why the previous patch didn't compile. I think this form might be optimal. Return value is OK, we have to do it in .h anyway.
Comment 11 WebKit Review Bot 2012-02-15 02:11:10 PST
Attachment 127029 [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/InjectedScript.cpp:85:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.cpp:99:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.h:80:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.h:82:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 WebKit Review Bot 2012-02-15 02:26:57 PST
Attachment 127048 [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/InjectedScript.cpp:85:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.cpp:99:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.h:80:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.h:82:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Vsevolod Vlasov 2012-02-15 06:50:34 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 127048 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127048&action=review
> > 
> > > Source/WebCore/inspector/InspectorDebuggerAgent.cpp:324
> > > +    using WebCore::TypeBuilder::Debugger::Location;
> > 
> > Why isn't this statement at the beginning of the file?
> > This seems to be forbidden by style guide and you need to use a long name for return value type because of that.
> 
> Are you sure it IS forbidden?
> 
> Anyway Location clashes with WebCore::Location and it's why the previous patch didn't compile. I think this form might be optimal. Return value is OK, we have to do it in .h anyway.

5. In implementation files, put all other "using" statements at the beginning of the file, before any namespace definitions and after any "include" statements.

This sounds like it IS forbidden to me.
I am fine with using fully qualified name for Location.
Comment 14 Peter Rybin 2012-02-15 13:08:07 PST
Created attachment 127222 [details]
Patch
Comment 15 Vsevolod Vlasov 2012-02-15 13:17:59 PST
Comment on attachment 127222 [details]
Patch

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

> Source/WebCore/inspector/InspectorDebuggerAgent.h:133
> +    PassRefPtr<TypeBuilder::Array<TypeBuilder::Debugger::CallFrame > > currentCallFrames();

Extra space after CallFrame?
Comment 16 Peter Rybin 2012-02-15 14:01:21 PST
Created attachment 127231 [details]
Patch
Comment 17 WebKit Review Bot 2012-02-15 23:24:37 PST
Attachment 127222 [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/InjectedScript.cpp:85:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.cpp:99:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.h:80:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InjectedScript.h:82:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 WebKit Review Bot 2012-02-15 23:35:13 PST
Attachment 127231 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList
Using index info to reconstruct a base tree...
<stdin>:84: trailing whitespace.
        
<stdin>:333: trailing whitespace.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
<stdin>:334: trailing whitespace.
 width="300" height="300" onload="runRepaintTest()">
<stdin>:335: trailing whitespace.
<script xlink:href="../../fast/repaint/resources/repaint.js" />
<stdin>:336: trailing whitespace.
<script><![CDATA[
warning: squelched 16 whitespace errors
warning: 21 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Vsevolod Vlasov 2012-02-17 08:25:35 PST
Committed r108077: <http://trac.webkit.org/changeset/108077>
Comment 20 Simon Fraser (smfr) 2012-02-17 08:46:03 PST
This broke the build, and no-one is around on #webkit to deal with it.
Comment 21 Ilya Tikhonovsky 2012-02-17 08:55:27 PST
Reverted r108077 for reason:

it broke compilation.

Committed r108083: <http://trac.webkit.org/changeset/108083>
Comment 22 Peter Rybin 2012-02-17 12:41:56 PST
Created attachment 127630 [details]
Patch
Comment 23 Vsevolod Vlasov 2012-02-21 04:04:44 PST
Committed r108337: <http://trac.webkit.org/changeset/108337>