Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18729002:
Enable message delivering in inspector-protocol tests when on pause (Closed)
|
Created:
7 years, 5 months ago by Peter.Rybin Modified:
7 years, 5 months ago CC:
blink-reviews, Nils Barth (inactive), marja+watch_chromium.org, caseq+blink_chromium.org, Nate Chapin, loislo+blink_chromium.org, jsbell+bindings_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, eae+blinkwatch, yurys+blink_chromium.org, lushnikov+blink_chromium.org, abarth-chromium, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, adamk+blink_chromium.org, haraken, aandrey+blink_chromium.org, do-not-use Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionEnable message delivering in inspector-protocol tests when on pause
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154298
Patch Set 1 #
Total comments: 8
Patch Set 2 : fix tests #
Total comments: 3
Patch Set 3 : follow codereview #
Total comments: 11
Patch Set 4 : follow code review #Patch Set 5 : rebase #
Messages
Total messages: 23 (0 generated)
FYI https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... Source/core/testing/InspectorFrontendClientLocal.cpp:74: owner->onTimer(); can this execute after owner is destructed?
https://codereview.chromium.org/18729002/diff/1/Source/WebKit/chromium/src/We... File Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/18729002/diff/1/Source/WebKit/chromium/src/We... Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:366: ClientMessageLoopAdapter::ensureClientMessageLoopCreated(m_client); Why do you need it to be initialized when there is no devtools client? This functionality is not supposed to be triggered without devtools front-end. https://codereview.chromium.org/18729002/diff/1/Source/bindings/v8/PageScript... File Source/bindings/v8/PageScriptDebugServer.h (right): https://codereview.chromium.org/18729002/diff/1/Source/bindings/v8/PageScript... Source/bindings/v8/PageScriptDebugServer.h:56: static void setClientMessageLoop(PassOwnPtr<ClientMessageLoop>); Why are you changing this? https://codereview.chromium.org/18729002/diff/1/Source/bindings/v8/PageScript... Source/bindings/v8/PageScriptDebugServer.h:72: static OwnPtr<ClientMessageLoop> m_clientMessageLoop; We regularly don't use classes as types of static members because of possible problems with their destruction at shutdown time. DEFINE_STATIC_LOCAL should be used instead. In this particular case you already have PageScriptDebugServer::shared and I don't see why to make this particular field static. https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... Source/core/testing/InspectorFrontendClientLocal.cpp:79: WebKit::Platform::current()->currentThread()->postDelayedTask(taskImpl, 0); Does it guarantee the sequence of execution for the tasks posted with the same delay?
The problem I am trying to solve with the static initialization is that ClientMessageLoopCreated must be set in case of tests, but Internals.cpp is quite abstract class, that knows nothing about V8 and has no callbacks for doing that. So I chose to set ClientMessageLoopCreated eagerly, since no actual data or initialization work is done there (instance is very light and only keeps a pointer to thread message loop). I'm fine with an explicit initialization from Internals (create dummy channel method), but I don't know how to do it. https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... Source/core/testing/InspectorFrontendClientLocal.cpp:79: WebKit::Platform::current()->currentThread()->postDelayedTask(taskImpl, 0); On 2013/07/08 11:13:04, Yury Semikhatsky wrote: > Does it guarantee the sequence of execution for the tasks posted with the same > delay? You mean order of execution? I am not sure. But anyway, I don't think the order matters, since all the actual tasks are identical.
On 2013/07/08 12:23:38, peter.rybin wrote: > The problem I am trying to solve with the static initialization is that > ClientMessageLoopCreated must be set in case of tests, but Internals.cpp is > quite abstract class, that knows nothing about V8 and has no callbacks for doing > that. > So I chose to set ClientMessageLoopCreated eagerly, since no actual data or > initialization work is done there (instance is very light and only keeps a > pointer to thread message loop). > > I'm fine with an explicit initialization from Internals (create dummy channel > method), but I don't know how to do it. > You could plumb it though InspectorClient but I'm not sure it's worth it. If you move client loop initialization into WebDevToolsAgentImpl constructor you should remove it from WebDevToolsAgentImpl::attach.
https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... Source/core/testing/InspectorFrontendClientLocal.cpp:79: WebKit::Platform::current()->currentThread()->postDelayedTask(taskImpl, 0); On 2013/07/08 12:23:38, peter.rybin wrote: > On 2013/07/08 11:13:04, Yury Semikhatsky wrote: > > Does it guarantee the sequence of execution for the tasks posted with the same > > delay? > > You mean order of execution? I am not sure. > But anyway, I don't think the order matters, since all the actual tasks are > identical. You are right. But if the order of execution is guaranteed then we could get rid of our own message queue and simply post all dispatch tasks to the thread's loop.
https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... Source/core/testing/InspectorFrontendClientLocal.cpp:74: owner->onTimer(); On 2013/07/05 09:31:56, aandrey wrote: > can this execute after owner is destructed? I guess the old implementation didn't care about it. I also took a look and saw, that the main class is being destructed pretty late, so that problem must be unlikely.
> You could plumb it though InspectorClient but I'm not sure it's worth it. If you tell me, I'll plumb it. But I doubt worthiness too. > If you move client loop initialization into WebDevToolsAgentImpl constructor you > should remove it from WebDevToolsAgentImpl::attach. Done. And from reattach.
On 2013/07/08 13:16:11, Yury Semikhatsky wrote: > https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... > File Source/core/testing/InspectorFrontendClientLocal.cpp (right): > > https://codereview.chromium.org/18729002/diff/1/Source/core/testing/Inspector... > Source/core/testing/InspectorFrontendClientLocal.cpp:79: > WebKit::Platform::current()->currentThread()->postDelayedTask(taskImpl, 0); > On 2013/07/08 12:23:38, peter.rybin wrote: > > On 2013/07/08 11:13:04, Yury Semikhatsky wrote: > > > Does it guarantee the sequence of execution for the tasks posted with the > same > > > delay? > > > > You mean order of execution? I am not sure. > > But anyway, I don't think the order matters, since all the actual tasks are > > identical. > You are right. But if the order of execution is guaranteed then we could get rid > of our own message queue and simply post all dispatch tasks to the thread's > loop. That's right. But currently we have it much simpler, including simplicity for debugging. I don't think it is a goal to hand over the data to queue tasks.
https://codereview.chromium.org/18729002/diff/12001/LayoutTests/inspector-pro... File LayoutTests/inspector-protocol/layers/layers-for-node.html (left): https://codereview.chromium.org/18729002/diff/12001/LayoutTests/inspector-pro... LayoutTests/inspector-protocol/layers/layers-for-node.html:81: var layerCount = 0; Unused variable. https://codereview.chromium.org/18729002/diff/12001/LayoutTests/inspector-pro... File LayoutTests/inspector-protocol/layers/layers-for-node.html (right): https://codereview.chromium.org/18729002/diff/12001/LayoutTests/inspector-pro... LayoutTests/inspector-protocol/layers/layers-for-node.html:176: // Makes next step to wait half a second to let backend do its asynchronous processing. Backend updates layers by timer, that provokes some race conditions in this test. With a new message dispatcher, debugger messages start to win the race. https://codereview.chromium.org/18729002/diff/12001/LayoutTests/inspector-pro... File LayoutTests/inspector-protocol/page/javascriptDialogEvents.html (right): https://codereview.chromium.org/18729002/diff/12001/LayoutTests/inspector-pro... LayoutTests/inspector-protocol/page/javascriptDialogEvents.html:32: function onDoneAlert() With a new message loop runner, dialogs starts to nest inside each other. Strictly speaking, we are not ready for this (to the fact that a dialog window starts nested loop with us involved), but this test can afford to become more realistic.
Pointer work is redone with RefPtr. 500ms delay in test is fixed.
https://codereview.chromium.org/18729002/diff/19001/LayoutTests/inspector-pro... File LayoutTests/inspector-protocol/layers/layers-for-node.html (right): https://codereview.chromium.org/18729002/diff/19001/LayoutTests/inspector-pro... LayoutTests/inspector-protocol/layers/layers-for-node.html:177: // Scheduling on timer with no delay is enough deferring. Good point, now it should work same way it was before. https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.cpp:50: InspectorBackendDispatchTask(InspectorController* inspectorController) While you're here, pleas mark this constructor as explicit. https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.cpp:64: m_messages.clear(); m_stopped = true is missing also I'd rather set m_inspectorController to 0, that way we would also avoid keeping pointer on a potentially dead object. https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.cpp:68: void schedule(PassRefPtr<InspectorBackendDispatchTask> selfRef) Why not use this and get rid of selfRef parameter? https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... File Source/core/testing/InspectorFrontendClientLocal.h (right): https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.h:83: RefPtr<InspectorBackendDispatchTask> m_dispatchQueue; Please rename InspectorBackendDispatchTask into InspectorBackendMessageQueue
I'm going to address other things. I'm sorry I forget 2 ideas we discussed yesterday. https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.cpp:68: void schedule(PassRefPtr<InspectorBackendDispatchTask> selfRef) On 2013/07/16 06:56:18, Yury Semikhatsky wrote: > Why not use this and get rid of selfRef parameter? I'm not sure if it's fine to convert raw this to RefPtr (without any adopt or similar things). I thought this way it's more elegant.
https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.cpp:68: void schedule(PassRefPtr<InspectorBackendDispatchTask> selfRef) On 2013/07/16 09:59:02, peter.rybin wrote: > On 2013/07/16 06:56:18, Yury Semikhatsky wrote: > > Why not use this and get rid of selfRef parameter? > > I'm not sure if it's fine to convert raw this to RefPtr (without any adopt or > similar things). > I thought this way it's more elegant. That's absolutely fine to assign raw pointer to RefPtr. See for example https://code.google.com/p/chromium/codesearch#search/&q=RefPtr.*protect&sq=pa... The idea of adoptRef/adoptPtr is to have each object created with new immediately wrapped into a smart pointer. Also we could enforce with presubmit check use of adopt wherever 'new' is called.
https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... File Source/core/testing/InspectorFrontendClientLocal.cpp (right): https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.cpp:50: InspectorBackendDispatchTask(InspectorController* inspectorController) On 2013/07/16 06:56:18, Yury Semikhatsky wrote: > While you're here, pleas mark this constructor as explicit. Done. https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.cpp:64: m_messages.clear(); On 2013/07/16 06:56:18, Yury Semikhatsky wrote: > m_stopped = true is missing also I'd rather set m_inspectorController to 0, that > way we would also avoid keeping pointer on a potentially dead object. Done. https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.cpp:68: void schedule(PassRefPtr<InspectorBackendDispatchTask> selfRef) On 2013/07/16 10:15:55, Yury Semikhatsky wrote: > On 2013/07/16 09:59:02, peter.rybin wrote: > > On 2013/07/16 06:56:18, Yury Semikhatsky wrote: > > > Why not use this and get rid of selfRef parameter? > > > > I'm not sure if it's fine to convert raw this to RefPtr (without any adopt or > > similar things). > > I thought this way it's more elegant. > That's absolutely fine to assign raw pointer to RefPtr. See for example > https://code.google.com/p/chromium/codesearch#search/&q=RefPtr.*protect&sq=pa... > > > The idea of adoptRef/adoptPtr is to have each object created with new > immediately wrapped into a smart pointer. Also we could enforce with presubmit > check use of adopt wherever 'new' is called. Done. https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... File Source/core/testing/InspectorFrontendClientLocal.h (right): https://codereview.chromium.org/18729002/diff/19001/Source/core/testing/Inspe... Source/core/testing/InspectorFrontendClientLocal.h:83: RefPtr<InspectorBackendDispatchTask> m_dispatchQueue; On 2013/07/16 06:56:18, Yury Semikhatsky wrote: > Please rename InspectorBackendDispatchTask into InspectorBackendMessageQueue Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prybin@chromium.org/18729002/29001
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...
Source/web lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prybin@chromium.org/18729002/38001
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prybin@chromium.org/18729002/38001
Message was sent while issue was closed.
Change committed as 154298 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
