Summary: | Web Inspector: Remove unused callId parameter from evaluateInWebInspector | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||
Component: | Web Inspector | Assignee: | BJ Burg <bburg> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | graouts, joepeck, ossy, timothy, webkit-bug-importer | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 129217 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
BJ Burg
2014-03-05 10:08:04 PST
Created attachment 226559 [details]
the patch
Comment on attachment 226559 [details]
the patch
Crazy number of files!
Yeah, I groaned when that many change logs got touched :| Comment on attachment 226559 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=226559&action=review > Source/JavaScriptCore/inspector/agents/InspectorAgent.cpp:78 > + for (auto& testCommand : m_pendingEvaluateTestCommands) { > + if (!m_frontendDispatcher) > + break; > + You could just not loop at all if !m_frontendDispatcher. if (m_frontendDispatcher) { for (...) ... } However, having said that I don't believe it is possible for m_frontendDispatcher to be null at this time. Enable can only be called (looking at the code right now) via the protocol. And, if it was null, we wouldn't have wanted to clear m_pendingEvaluateTestCommands. So, just drop the m_frontendDispatcher check, or turn it into an ASSERT. Comment on attachment 226559 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=226559&action=review >> Source/JavaScriptCore/inspector/agents/InspectorAgent.cpp:78 >> + > > You could just not loop at all if !m_frontendDispatcher. > > if (m_frontendDispatcher) { > for (...) > ... > } > > However, having said that I don't believe it is possible for m_frontendDispatcher to be null at this time. Enable can only be called (looking at the code right now) via the protocol. And, if it was null, we wouldn't have wanted to clear m_pendingEvaluateTestCommands. So, just drop the m_frontendDispatcher check, or turn it into an ASSERT. On further discussion: If a frontend dispatched messages synchronously, and one of the test messages called InspectorAgent.disable() further down the call stack (thus nulling out the frontendDispatcher) then we'd have a problem. So it makes sense to be cautious and re-check here, even if we won't ever hit this in our frontend. (In reply to comment #5) > (From update of attachment 226559 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226559&action=review > > >> Source/JavaScriptCore/inspector/agents/InspectorAgent.cpp:78 > >> + > > > > You could just not loop at all if !m_frontendDispatcher. > > > > if (m_frontendDispatcher) { > > for (...) > > ... > > } > > > > However, having said that I don't believe it is possible for m_frontendDispatcher to be null at this time. Enable can only be called (looking at the code right now) via the protocol. And, if it was null, we wouldn't have wanted to clear m_pendingEvaluateTestCommands. So, just drop the m_frontendDispatcher check, or turn it into an ASSERT. > > On further discussion: If a frontend dispatched messages synchronously, and one of the test messages called InspectorAgent.disable() further down the call stack (thus nulling out the frontendDispatcher) then we'd have a problem. So it makes sense to be cautious and re-check here, even if we won't ever hit this in our frontend. Thinking about this even further, any incoming messages wouldn't be handled immediately. They would be queued, right? So maybe we don't need this. Committed r165545: <http://trac.webkit.org/changeset/165545> (In reply to comment #7) > Committed r165545: <http://trac.webkit.org/changeset/165545> It broke the Apple Windows build: 1>..\..\win\WebInspector.cpp(45): error C2259: 'WebInspector' : cannot instantiate abstract class due to following members: 'HRESULT IWebInspectorPrivate::evaluateInFrontend(ULONG,BSTR)' : is abstract (In reply to comment #8) > (In reply to comment #7) > > Committed r165545: <http://trac.webkit.org/changeset/165545> > > It broke the Apple Windows build: > > 1>..\..\win\WebInspector.cpp(45): error C2259: 'WebInspector' : cannot instantiate abstract class > due to following members: > 'HRESULT IWebInspectorPrivate::evaluateInFrontend(ULONG,BSTR)' : is abstract Sounds like we should remove the ULONG requirement somewhere. I will look into it. Committed r165585: <http://trac.webkit.org/changeset/165585> |