Bug 129744 - Web Inspector: Remove unused callId parameter from evaluateInWebInspector
Summary: Web Inspector: Remove unused callId parameter from evaluateInWebInspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on: 129217
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-05 10:08 PST by BJ Burg
Modified: 2014-03-13 18:14 PDT (History)
5 users (show)

See Also:


Attachments
the patch (34.97 KB, patch)
2014-03-12 16:46 PDT, BJ Burg
timothy: review+
bburg: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2014-03-05 10:08:04 PST
The old test harness used to pass magic call identifiers to try and intercept the InspectorTest.completeTest() method when it is sent back to the test page. The new inspector test harness doesn't need this functionality, so we can probably remove it from TestRunner and the WK/WK2 entry points (WKBundleInspectorEvaluateScriptForTest and WebInspector:evaluateInFrontend:callId:script) that also have this extra parameter.
Comment 1 BJ Burg 2014-03-12 16:46:08 PDT
Created attachment 226559 [details]
the patch
Comment 2 Timothy Hatcher 2014-03-12 17:13:01 PDT
Comment on attachment 226559 [details]
the patch

Crazy number of files!
Comment 3 BJ Burg 2014-03-12 17:14:04 PDT
Yeah, I groaned when that many change logs got touched :|
Comment 4 Joseph Pecoraro 2014-03-12 17:21:56 PDT
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 5 BJ Burg 2014-03-12 17:35:23 PDT
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.
Comment 6 Joseph Pecoraro 2014-03-12 18:11:38 PDT
(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.
Comment 7 BJ Burg 2014-03-13 10:48:19 PDT
Committed r165545: <http://trac.webkit.org/changeset/165545>
Comment 8 Csaba Osztrogonác 2014-03-13 15:12:53 PDT
(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
Comment 9 Joseph Pecoraro 2014-03-13 15:21:18 PDT
(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.
Comment 10 BJ Burg 2014-03-13 15:36:24 PDT
I will look into it.
Comment 11 BJ Burg 2014-03-13 18:14:10 PDT
Committed r165585: <http://trac.webkit.org/changeset/165585>