Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
932523003:
DevTools: [Blink-side] Fix debugger command order (Closed)
|
Created:
5 years, 10 months ago by sergeyv Modified:
5 years, 10 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDevTools: [Blink-side] Fix debugger command order
Chromium-side patch: https://codereview.chromium.org/930113002/
BUG=458035
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190317
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address comments #
Total comments: 3
Patch Set 3 : Add m_attached check #
Messages
Total messages: 22 (7 generated)
sergeyv@chromium.org changed reviewers: + yurys@chromium.org
https://codereview.chromium.org/932523003/diff/1/Source/web/WebDevToolsAgentI... File Source/web/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/932523003/diff/1/Source/web/WebDevToolsAgentI... Source/web/WebDevToolsAgentImpl.cpp:447: if (ignoreInterrupt || !WebDevToolsAgent::shouldInterruptForMessage(message)) You don't need this parameter as before the chromium-side patch is landed WebDevToolsAgent::shouldInterruptForMessage must always return false (corresponding command should have been routed into v8 queue) and after it is landed you will start receiving those commands here. Or am I missing something?
https://codereview.chromium.org/932523003/diff/1/Source/web/WebDevToolsAgentI... File Source/web/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/932523003/diff/1/Source/web/WebDevToolsAgentI... Source/web/WebDevToolsAgentImpl.cpp:447: if (ignoreInterrupt || !WebDevToolsAgent::shouldInterruptForMessage(message)) On 2015/02/16 16:33:07, yurys wrote: > You don't need this parameter as before the chromium-side patch is landed > WebDevToolsAgent::shouldInterruptForMessage must always return false > (corresponding command should have been routed into v8 queue) and after it is > landed you will start receiving those commands here. Or am I missing something? To clarify this after offline discussion. We can make DebuggerTask a friend of WebDevToolsAgentImpl and inspectorController()->dispatchMessageFromFrontend(message); from it (it will require static_cast<WebDevToolsAgentImpl*> but that's fine there).
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/932523003/diff/1/Source/web/WebDevToolsAgentI... File Source/web/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/932523003/diff/1/Source/web/WebDevToolsAgentI... Source/web/WebDevToolsAgentImpl.cpp:447: if (ignoreInterrupt || !WebDevToolsAgent::shouldInterruptForMessage(message)) On 2015/02/16 16:56:15, yurys wrote: > On 2015/02/16 16:33:07, yurys wrote: > > You don't need this parameter as before the chromium-side patch is landed > > WebDevToolsAgent::shouldInterruptForMessage must always return false > > (corresponding command should have been routed into v8 queue) and after it is > > landed you will start receiving those commands here. Or am I missing > something? > > To clarify this after offline discussion. We can make DebuggerTask a friend of > WebDevToolsAgentImpl and > inspectorController()->dispatchMessageFromFrontend(message); from it (it will > require static_cast<WebDevToolsAgentImpl*> but that's fine there). Done.
The CQ bit was checked by yurys@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/932523003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)
yurys@chromium.org changed reviewers: + pfeldman@chromium.org
+pfeldman for OWNERS review
lgtm https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... File Source/web/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... Source/web/WebDevToolsAgentImpl.cpp:198: agentImpl->inspectorController()->dispatchMessageFromFrontend(m_descriptor->message()); check for m_attached is missing.
https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... File Source/web/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... Source/web/WebDevToolsAgentImpl.cpp:198: agentImpl->inspectorController()->dispatchMessageFromFrontend(m_descriptor->message()); On 2015/02/17 09:23:16, pfeldman wrote: > check for m_attached is missing. It might make sense to extract if (m_attached) inspectorController()->dispatchMessageFromFrontend(message) in a separate method.
New patchsets have been uploaded after l-g-t-m from yurys@chromium.org,pfeldman@chromium.org
On 2015/02/17 09:44:01, yurys wrote: > https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... > File Source/web/WebDevToolsAgentImpl.cpp (right): > > https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... > Source/web/WebDevToolsAgentImpl.cpp:198: > agentImpl->inspectorController()->dispatchMessageFromFrontend(m_descriptor->message()); > On 2015/02/17 09:23:16, pfeldman wrote: > > check for m_attached is missing. > > It might make sense to extract > > if (m_attached) > inspectorController()->dispatchMessageFromFrontend(message) > > in a separate method. I can't see a profit from such extraction, because we still will need a check of m_attached in dispatchOnInspectorBackend to prevent calls of PageScriptDebugServer::shared().runPendingTasks();
https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... File Source/web/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... Source/web/WebDevToolsAgentImpl.cpp:198: agentImpl->inspectorController()->dispatchMessageFromFrontend(m_descriptor->message()); On 2015/02/17 09:23:16, pfeldman wrote: > check for m_attached is missing. Done.
On 2015/02/17 11:32:36, sergeyv wrote: > On 2015/02/17 09:44:01, yurys wrote: > > > https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... > > File Source/web/WebDevToolsAgentImpl.cpp (right): > > > > > https://codereview.chromium.org/932523003/diff/40001/Source/web/WebDevToolsAg... > > Source/web/WebDevToolsAgentImpl.cpp:198: > > > agentImpl->inspectorController()->dispatchMessageFromFrontend(m_descriptor->message()); > > On 2015/02/17 09:23:16, pfeldman wrote: > > > check for m_attached is missing. > > > > It might make sense to extract > > > > if (m_attached) > > inspectorController()->dispatchMessageFromFrontend(message) > > > > in a separate method. > > I can't see a profit from such extraction, because we still will need a check of > m_attached in dispatchOnInspectorBackend to prevent calls of > PageScriptDebugServer::shared().runPendingTasks(); No, you wouldn't as runPendingTasks would call the extracted method doing the check. But I don't mind either way.
The CQ bit was checked by sergeyv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/932523003/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190317 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
