Bug 42340

Summary: Web Inspector: extract debugger implementation into DebuggerAgent
Product: WebKit Reporter: Yury Semikhatsky <yurys>
Component: Web Inspector (Deprecated)Assignee: Yury Semikhatsky <yurys>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andreip, bweinstein, dglazkov, eric, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
pfeldman: review+
Patch none

Description Yury Semikhatsky 2010-07-14 23:23:51 PDT 
There is a bunch of methods on the InspectorBackend providing debugger facilities. All of them can be easily extracted and placed into there own class DebuggerAgent. That would limit the InspectorController responsibilities.
Comment 1 Yury Semikhatsky 2010-08-06 02:50:07 PDT 
Created attachment 63707 [details]
Patch
Comment 2 Ilya Tikhonovsky 2010-08-06 03:07:45 PDT 
Comment on attachment 63707 [details]
Patch

WebCore/GNUmakefile.am:1600
 +  	WebCore/inspector/InspectorDebuggerAgent.h \
Incorrect order.

WebCore/WebCore.xcodeproj/project.pbxproj: 
 +  			developmentRegion = English;
is it necessary?

WebCore/inspector/InspectorDebuggerAgent.cpp:44
 +  static String md5Base16(const String& string)
Why do you need your own implementation of md5Base16?

WebCore/inspector/InspectorDebuggerAgent.cpp:70
 +      ScriptDebugServer::shared().addListener(agent.get(), inspectorController->inspectedPage());
I think it would be better to move this code to ctor.
Comment 3 WebKit Review Bot 2010-08-06 03:12:36 PDT 
Attachment 63707 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3624570
Comment 4 Yury Semikhatsky 2010-08-06 04:35:33 PDT 
(In reply to comment #2)
> (From update of attachment 63707 [details])
> WebCore/GNUmakefile.am:1600
>  +      WebCore/inspector/InspectorDebuggerAgent.h \
> Incorrect order.
> 
Fixed.

> WebCore/WebCore.xcodeproj/project.pbxproj: 
>  +              developmentRegion = English;
> is it necessary?
> 
No. Thanks.

> WebCore/inspector/InspectorDebuggerAgent.cpp:44
>  +  static String md5Base16(const String& string)
> Why do you need your own implementation of md5Base16?
> 
It was there before. Just forgot to remove it from InspectorController.cpp

> WebCore/inspector/InspectorDebuggerAgent.cpp:70
>  +      ScriptDebugServer::shared().addListener(agent.get(), inspectorController->inspectedPage());
> I think it would be better to move this code to ctor.
I'd rather keep it in the create method to make sure it's called only when the object is fully constructed. It would work either way I think since the constructor is private.
Comment 5 Yury Semikhatsky 2010-08-06 04:51:19 PDT 
Created attachment 63714 [details]
Patch
Comment 6 Yury Semikhatsky 2010-08-06 07:07:01 PDT 
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/CMakeLists.txt
	M	WebCore/ChangeLog
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.pro
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/inspector/CodeGeneratorInspector.pm
	M	WebCore/inspector/Inspector.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	A	WebCore/inspector/InspectorDebuggerAgent.cpp
	A	WebCore/inspector/InspectorDebuggerAgent.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/front-end/ScriptsPanel.js
Committed r64846
Comment 7 Andrei Popescu 2010-08-06 09:45:41 PDT 
Created attachment 63733 [details]
Patch
Comment 8 Andrei Popescu 2010-08-06 09:46:59 PDT 
(In reply to comment #7)
> Created an attachment (id=63733) [details]
> Patch

Sorry, passed wrong bug no to webkit-patch :)
Comment 9 WebKit Review Bot 2010-08-06 10:06:57 PDT 
http://trac.webkit.org/changeset/64846 might have broken Qt Windows 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/64845
http://trac.webkit.org/changeset/64846