Bug 129744 – Web Inspector: Remove unused callId parameter from evaluateInWebInspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

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.

[Blaze Burg]

Created attachment 226559 [details]
the patch

[Timothy Hatcher]

Comment on attachment 226559 [details]
the patch

Crazy number of files!

[Blaze Burg]

Yeah, I groaned when that many change logs got touched :|

[Joseph Pecoraro]

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.

[Blaze Burg]

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.

[Joseph Pecoraro]

(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.

[Blaze Burg]

Committed r165545: <http://trac.webkit.org/changeset/165545>

[Csaba Osztrogonác]

(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

[Joseph Pecoraro]

(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.

[Blaze Burg]

I will look into it.

[Blaze Burg]

Committed r165585: <http://trac.webkit.org/changeset/165585>