PageScriptDebugServer is singleton now. That's right for V8, since PageScriptDebugServer is a dispatcher for V8, it doesn't record any debug status for a special page or JSGlobalObject (frame). But for JSC things are different, the derived chain of PageScriptDebugServer is JSC::Debugger -> WebCore::ScriptDebugServer -> WebCore::PageScriptDebugServer, the ScriptDebugServer records status for one JSGlobalObject (refer to member variables "m_pauseOnNextStatement" and "m_paused"). It causes a problem that if there more than on pages within one process only one can be inspected. In high level, the InspectorController and PageDebugAgent are one-one mapped to page. So there is a possibility to let JSC's PageScriptDebugServer support multi instance.
Created attachment 154602 [details] Patch
Comment on attachment 154602 [details] Patch Attachment 154602 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13353647
Created attachment 154606 [details] Patch
Comment on attachment 154606 [details] Patch Attachment 154606 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13349679
Created attachment 154609 [details] Patch
Comment on attachment 154609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154609&action=review > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:68 > + if (!page) How can it happen? > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:86 > + m_pageDebugServerMap.add(page, serverForPage); Pleas move this line into PageScriptDebugServer constructor. > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:198 > + deleteInstance(page); I'd replace this with delete this; and get rid of PageScriptDebugServer::deleteInstance > Source/WebCore/bindings/v8/PageScriptDebugServer.cpp:67 > +PageScriptDebugServer& PageScriptDebugServer::shared(Page*) Please rename this method to something like forPage as now there is no shared instance. > Source/WebCore/inspector/PageDebuggerAgent.cpp:72 > + if (PageScriptDebugServer::isMultiInstance()) No need in this check just ignore the Page* argument in case of v8 and delete isMultiInstance.
(In reply to comment #6) > (From update of attachment 154609 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154609&action=review > > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:68 > > + if (!page) > > How can it happen? For InspectorProfilerAgent, I planned just changed the behavior of InspectorDebugAgent firstly to be on safe side. But it seems confused, I'll correct it in new patch. > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:86 > > + m_pageDebugServerMap.add(page, serverForPage); > > Pleas move this line into PageScriptDebugServer constructor. ok. Thx, it's better. > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:198 > > + deleteInstance(page); > > I'd replace this with delete this; and get rid of PageScriptDebugServer::deleteInstance ok. Thx, it's better. > > Source/WebCore/bindings/v8/PageScriptDebugServer.cpp:67 > > +PageScriptDebugServer& PageScriptDebugServer::shared(Page*) > > Please rename this method to something like forPage as now there is no shared instance. > ok. > > Source/WebCore/inspector/PageDebuggerAgent.cpp:72 > > + if (PageScriptDebugServer::isMultiInstance()) > > No need in this check just ignore the Page* argument in case of v8 and delete isMultiInstance. ok.
Created attachment 154892 [details] Patch
Comment on attachment 154892 [details] Patch Attachment 154892 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13373341
Comment on attachment 154892 [details] Patch Attachment 154892 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13368549
Created attachment 155193 [details] Patch
Comment on attachment 155193 [details] Patch Attachment 155193 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13395155
Created attachment 155196 [details] Patch
Comment on attachment 155196 [details] Patch Attachment 155196 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13392202
Created attachment 155200 [details] Patch
Comment on attachment 155200 [details] Patch Attachment 155200 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13380968
Created attachment 155214 [details] Patch
Comment on attachment 155200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155200&action=review > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:64 > +PageScriptDebugServer& PageScriptDebugServer::server(Page* page) Remove this method as PageScriptDebugServer::instance does the same things. > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:161 > + ASSERT(page && page == m_page); What about the comment above about page being null in some cases? > Source/WebCore/bindings/js/PageScriptDebugServer.h:53 > + static void addListener(ScriptDebugListener*, Page*); add/removeListener can be instance methods and we could keep the list of listeners on a corresponding PageScriptDebugServer, no need to have additional static map.
(In reply to comment #18) > (From update of attachment 155200 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155200&action=review > > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:64 > > +PageScriptDebugServer& PageScriptDebugServer::server(Page* page) > > Remove this method as PageScriptDebugServer::instance does the same things. Ok. Maybe I can remove "instance()", "server()" is left for invoked by high level invoking like PageDebugAgent::scriptDebugServer since V8 has the same interface. > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:161 > > + ASSERT(page && page == m_page); > > What about the comment above about page being null in some cases? In ideal situation, if PageScriptDebugServer exists, the "m_page" shouldn't be null and the "page" read by ScriptDebugServer from callFrame should be same. But if there is some accident takes place in released version, I want PageScriptDebugServer to do nothing just return. Do you think it's appropriate? > > Source/WebCore/bindings/js/PageScriptDebugServer.h:53 > > + static void addListener(ScriptDebugListener*, Page*); > > add/removeListener can be instance methods and we could keep the list of listeners on a corresponding PageScriptDebugServer, no need to have additional static map. Yes, thanks. Now for JSC::PageScriptDebugServer the argument "Page" can also be ignored.
Created attachment 155233 [details] Patch
Comment on attachment 155233 [details] Patch Attachment 155233 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13392285
Comment on attachment 155233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155233&action=review > Source/WebCore/bindings/js/PageScriptDebugServer.h:59 > + PageScriptDebugServer(Page* = 0); The constructor should be marked "explicit" also why is the Page parameter 0 by default while it should never be null? > Source/WebCore/bindings/js/PageScriptDebugServer.h:76 > + static PageDebugServerMap m_pageDebugServerMap; m_pageDebugServerMap -> s_pageDebugServerMap as it is a static field. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:106 > + return PageScriptDebugServer::server(0); Worker*Agent must not depend on Page*Server. There should be a separate map WorkerContext -> WorkerScriptDebugServer. > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:-105 > - PageScriptDebugServer::shared().continueProgram(); You don't need to rename this getter for V8 bindings as the patch doesn't affect their functionality.
(In reply to comment #22) > (From update of attachment 155233 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155233&action=review > > > Source/WebCore/bindings/js/PageScriptDebugServer.h:59 > > + PageScriptDebugServer(Page* = 0); > > The constructor should be marked "explicit" Sure. Thx. > also why is the Page parameter 0 by default while it should never be null? Sorry. It comes from old patch. > > Source/WebCore/bindings/js/PageScriptDebugServer.h:76 > > + static PageDebugServerMap m_pageDebugServerMap; > > m_pageDebugServerMap -> s_pageDebugServerMap as it is a static field. ok. > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:106 > > + return PageScriptDebugServer::server(0); > > Worker*Agent must not depend on Page*Server. There should be a separate map WorkerContext -> WorkerScriptDebugServer. I see, thank you. > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:-105 > > - PageScriptDebugServer::shared().continueProgram(); > > You don't need to rename this getter for V8 bindings as the patch doesn't affect their functionality. Sorry, I didn't understand this comment very well. I renamed "JSC::PageScriptDebugServer::shared" to "server", and I had to rename "V8::PageScriptDebugServer::shared" to "server", because if not, high level function like PageDebuggerAgent::scriptDebugServer have to recognize now is V8 or JSC. So I also modified WebDevToolsAgentImpl.cpp to make it pass compiling.
Created attachment 155252 [details] Patch
Comment on attachment 155252 [details] Patch Attachment 155252 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13384544
Created attachment 155267 [details] Patch
(In reply to comment #26) > Created an attachment (id=155267) [details] > Patch I put the "s_pageDebugServerMap" into function "pageServerMap" to avoid break the rules of mac porting. As the comments #14 #16 #21 #25 show, mac porting seems doesn't permit to define global variables and takes static member variable as global variable also.
Comment on attachment 155267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155267&action=review > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:105 > void PageScriptDebugServer::removeListener(ScriptDebugListener* listener, Page* page) We can make v8's implementation also keep a pointer to the Page and remove the second argument from the method. Can be done in another patch. > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:143 > + if (page && page == m_page) I don't think we need page == m_page check as PageScriptDebugServer listens to only one page. > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:152 > + if (page && page == m_page) Ditto. > Source/WebCore/bindings/js/PageScriptDebugServer.h:52 > + static PageDebugServerMap& pageServerMap(); This one should be private. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:107 > + ScriptDebugServer* server = new WorkerScriptDebugServer(m_workerContext); It will create WorkerScriptDebugServer that is never deleted. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:235 > + scriptDebugServer().recompileAllJSFunctionsSoon(); Can we avoid creating ScriptDebugServer just to recompile js functions?
(In reply to comment #28) > (From update of attachment 155267 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155267&action=review > > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:105 > > void PageScriptDebugServer::removeListener(ScriptDebugListener* listener, Page* page) > > We can make v8's implementation also keep a pointer to the Page and remove the second argument from the method. Can be done in another patch. ok. > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:143 > > + if (page && page == m_page) > > I don't think we need page == m_page check as PageScriptDebugServer listens to only one page. > > > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:152 > > + if (page && page == m_page) > > ok. > > > Source/WebCore/bindings/js/PageScriptDebugServer.h:52 > > + static PageDebugServerMap& pageServerMap(); > > This one should be private. > > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:107 > > + ScriptDebugServer* server = new WorkerScriptDebugServer(m_workerContext); > > It will create WorkerScriptDebugServer that is never deleted. It's here just for pass compiling. In my new patch I removed "scriptDebugServer()" from "InspectorProfilerAgent", so we don't need it anymore. > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:235 > > + scriptDebugServer().recompileAllJSFunctionsSoon(); > > Can we avoid creating ScriptDebugServer just to recompile js functions? Thx. It's a great idea. I will have a try.
Created attachment 156665 [details] Patch
(In reply to comment #30) > Created an attachment (id=156665) [details] > Patch According to comments #28, in this patch I removed "m_recompileTimer" from "JSC::ScriptDebugServer", so that we don't need to create a "ScriptDebugServer" instance for "InspectorProfileAgent".
@yurys, are you looking at this?
The related code of this patch has changed a lot recently :(, I'll check if this bug is worth to exist. If yes, I'll upload a new patch that can work with new code.
(In reply to comment #33) > The related code of this patch has changed a lot recently :(, I'll check if this bug is worth to exist. If yes, I'll upload a new patch that can work with new code. Clearing r? until the new version.
Created attachment 168389 [details] Patch
(In reply to comment #29) > (In reply to comment #28) > > (From update of attachment 155267 [details] [details]) ...... > > Can we avoid creating ScriptDebugServer just to recompile js functions? > Thx. It's a great idea. I will have a try. I investigate it again recently, we cannot do that, because all JS code (of different pages) share the Global Data space, so when we are recompiling we have to know which part needs to be recompiled.
Comment on attachment 168389 [details] Patch Clearing r? - it has been there for ages, probably obsolete.