Bug 52294 – Web Inspector: Extract BrowserDebuggerAgent from InspectorController, InspectorDOMAgent and InspectorDebugger agent.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 78687 [details]
]patch[ initial version

[WebKit Review Bot]

Attachment 78687 [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/InspectorDOMAgent.h:149:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:150:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:151:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:152:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:153:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:154:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h:67:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h:68:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h:70:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h:71:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h:80:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h:81:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 12 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Tikhonovsky]

Created attachment 78689 [details]
[patch] initial version. style errors were fixed.

[WebKit Review Bot]

Attachment 78689 [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/InspectorDOMAgent.h:149:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:150:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:151:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:152:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:153:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorDOMAgent.h:154:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Tikhonovsky]

Created attachment 78690 [details]
[patch] next iteration.

[Yury Semikhatsky]

Comment on attachment 78690 [details]
[patch] next iteration.

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

> Source/WebCore/inspector/InspectorBrowserDebuggerAgent.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h:49
> +class InspectorBrowserDebuggerAgent : public Noncopyable {

InspectorDOMDebuggerAgent would better reflect intent of this class. Also it could be merged into InspectorDebuggerAgent, otherwise we need to rename InspectorDebuggerAgent to InspectorScriptDebuggerAgent.

[Yury Semikhatsky]

Comment on attachment 78690 [details]
[patch] next iteration.

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

> Source/WebCore/inspector/InspectorController.cpp:735
> +        restoreStickyBreakpoints();

Why did this call move? Should it be done only in case there is at least one of the agents?

> Source/WebCore/inspector/InspectorController.cpp:1270
> +    if (type == eventListenerBreakpointType && m_browserDebuggerAgent) {

This method should become a part of a debugger agent.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:92
> +    if (browserDebuggerAgent->shouldBreakOnNodeInsertion(node, parent, eventData)) {

This logic should move into the debugger agent, InspectorInstrumentation should do nothing but pure dispatching.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:118
>          eventData->setString("breakpointType", domNativeBreakpointType);

ditto

> Source/WebCore/inspector/InspectorInstrumentation.cpp:145
>          eventData->setString("breakpointType", domNativeBreakpointType);

ditto

[Ilya Tikhonovsky]

Created attachment 78800 [details]
[patch] another version

(In reply to comment #7)
> (From update of attachment 78690 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78690&action=review
> 
> > Source/WebCore/inspector/InspectorController.cpp:735
> > +        restoreStickyBreakpoints();
> 
> Why did this call move? Should it be done only in case there is at least one of the agents?

done.

> 
> > Source/WebCore/inspector/InspectorController.cpp:1270
> > +    if (type == eventListenerBreakpointType && m_browserDebuggerAgent) {
> 
> This method should become a part of a debugger agent.

The idea was to extract DOM depended debugging things into separate class.

> 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:92
> > +    if (browserDebuggerAgent->shouldBreakOnNodeInsertion(node, parent, eventData)) {
> 
> This logic should move into the debugger agent, InspectorInstrumentation should do nothing but pure dispatching.

done.

> 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:118
> >          eventData->setString("breakpointType", domNativeBreakpointType);
> 
> ditto

done.
> 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:145
> >          eventData->setString("breakpointType", domNativeBreakpointType);
> 
> ditto


done.

[Yury Semikhatsky]

Comment on attachment 78800 [details]
[patch] another version

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

I still don't like InspectorBrowserDebuggerAgent name because there is no such thing as browser in terms of WebCore, otherwise the change looks good to me.

> Source/WebCore/inspector/InspectorDOMAgent.h:147
> +        // We represent embedded doms as a part of the same hierarchy. Hence we treat children of frame owners differently.

Consider moving these methods into a separate utility class.

> Tools/ChangeLog:5
> +        clean

Please provide a meaningful description.

[Ilya Tikhonovsky]

Committed r75774
	M	Source/WebCore/WebCore.pro
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/WebCore.vcproj/WebCore.vcproj
	M	Source/WebCore/WebCore.gypi
	M	Source/WebCore/inspector/CodeGeneratorInspector.pm
	M	Source/WebCore/inspector/InspectorController.cpp
	M	Source/WebCore/inspector/Inspector.idl
	A	Source/WebCore/inspector/InspectorBrowserDebuggerAgent.cpp
	M	Source/WebCore/inspector/InspectorController.h
	M	Source/WebCore/inspector/InspectorInstrumentation.cpp
	A	Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h
	M	Source/WebCore/inspector/InspectorDOMAgent.h
	M	Source/WebCore/inspector/InspectorDOMAgent.cpp
	M	Source/WebCore/CMakeLists.txt
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
	M	Tools/ChangeLog
	M	Tools/Scripts/webkitpy/common/checkout/scm.py
r75774 = 741bf235dca296d278c65aca3864d15566998800 (refs/remotes/trunk)

[WebKit Review Bot]

http://trac.webkit.org/changeset/75774 might have broken GTK Linux 32-bit Release

[Eric Seidel (no email)]

That's not the right fix for scm.py, and it's not unit tested. :)  Thre is a separate bug on it.

I appreciate you looking at it though.

[WebKit Review Bot]

http://trac.webkit.org/changeset/75783 might have broken SnowLeopard Intel Release (Build)

[Ilya Tikhonovsky]

Committed r75788
	M	Source/WebCore/WebCore.pro
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/WebCore.vcproj/WebCore.vcproj
	M	Source/WebCore/GNUmakefile.am
	M	Source/WebCore/WebCore.gypi
	M	Source/WebCore/inspector/CodeGeneratorInspector.pm
	M	Source/WebCore/inspector/InspectorController.cpp
	M	Source/WebCore/inspector/Inspector.idl
	A	Source/WebCore/inspector/InspectorBrowserDebuggerAgent.cpp
	M	Source/WebCore/inspector/InspectorController.h
	M	Source/WebCore/inspector/InspectorInstrumentation.cpp
	A	Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h
	M	Source/WebCore/inspector/InspectorDOMAgent.h
	M	Source/WebCore/inspector/InspectorDOMAgent.cpp
	M	Source/WebCore/CMakeLists.txt
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
r75788 = bd9c8cbc897d00f8cf96184dc5f330be0a6b4cfc (refs/remotes/trunk)