Bug 42340 - Web Inspector: extract debugger implementation into DebuggerAgent
Summary: Web Inspector: extract debugger implementation into DebuggerAgent
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: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-14 23:23 PDT by Yury Semikhatsky
Modified: 2010-08-06 10:06 PDT (History)
14 users (show)

See Also:


Attachments
Patch (49.56 KB, patch)
2010-08-06 02:50 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (50.70 KB, patch)
2010-08-06 04:51 PDT, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff
Patch (129.97 KB, patch)
2010-08-06 09:45 PDT, Andrei Popescu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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