Bug 87976 – Speech JavaScript API: mock WebSpeechRecognizer for DumpRenderTree

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

Speech JavaScript API: mock WebSpeechRecognizer for DumpRenderTree

[Hans Wennborg]

Created attachment 145094 [details]
Patch

[Kent Tamura]

Can you implement the mock in Source/WebCore/testing/ ?
Implementing a Chromium-specific mock is not great for code sharing.

[Hans Wennborg]

In https://bugs.webkit.org/show_bug.cgi?id=76443#c22 it was questioned why the mock for speech input was living in WebCore, and it was suggested that the mock should just be part of DumpRenderTree, which is why I moved it there. That also means we didn't have to have a bunch of wrappers around it.

This new mock is modeled on that previous MockWebSpeechInputController.

[Kent Tamura]

(In reply to comment #3)
> In https://bugs.webkit.org/show_bug.cgi?id=76443#c22 it was questioned why the mock for speech input was living in WebCore, and it was suggested that the mock should just be part of DumpRenderTree, which is why I moved it there. That also means we didn't have to have a bunch of wrappers around it.
> 
> This new mock is modeled on that previous MockWebSpeechInputController.

It seems fishd wasn't negative about putting a mock to WebCore/testing/. 
It's ok to have mocks in DRT/chromium/, but I think putting mocks to WebCore/testing/ is better.

[Hans Wennborg]

(In reply to comment #4)
> (In reply to comment #3)
> > In https://bugs.webkit.org/show_bug.cgi?id=76443#c22 it was questioned why the mock for speech input was living in WebCore, and it was suggested that the mock should just be part of DumpRenderTree, which is why I moved it there. That also means we didn't have to have a bunch of wrappers around it.
> > 
> > This new mock is modeled on that previous MockWebSpeechInputController.
> 
> It seems fishd wasn't negative about putting a mock to WebCore/testing/. 
> It's ok to have mocks in DRT/chromium/, but I think putting mocks to WebCore/testing/ is better.

I don't see any full-blown mocks in WebCore/testing/, it looks more like hooks into different WebCore objects.

There are some client mocks in WebCore/platform/mock/. For example, that's where the SpeechInputController mock lived before it was moved into DRT. The problem with these mocks is that the embedder is supposed to pass them in when creating the Page object. In Chromium's case, this means we have to wrap it. In this case, I think the overhead of the wrapping stuff would become more code than the mock itself.

If it's OK to have the mock in DRT/chromium, I would prefer that. It's much simpler, and it matches what we do for MockWebSpeechInputController.

[Hans Wennborg]

About the offline request to use the copyright header from Tools/DumpRenderTree/chromium/DumpRenderTree.cpp instead: I believe that's actually the old copyright header, that we're not using for new code anymore. I believe the one I used in my patch is the correct one. (See e.g. jamesr's comment in 
https://bugs.webkit.org/show_bug.cgi?id=66008#c12)

[Kent Tamura]

Comment on attachment 145094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145094&action=review

> Tools/DumpRenderTree/chromium/MockWebSpeechRecognizer.cpp:94
> +    WebVector<WebString> transcripts(static_cast<size_t>(1));
> +    WebVector<float> confidences(static_cast<size_t>(1));

Do we need static_cast<>? Doesn't 1 or 1u work?

> Tools/DumpRenderTree/chromium/MockWebSpeechRecognizer.h:94
> +    // Task class for calling a client function that does not take any parameters.
> +    typedef void (WebKit::WebSpeechRecognizerClient::*ClientFunctionPointer)(const WebKit::WebSpeechRecognitionHandle&);
> +    class ClientCallTask : public MethodTask<MockWebSpeechRecognizer> {
> +    public:
> +        ClientCallTask(MockWebSpeechRecognizer* mock, ClientFunctionPointer function) : MethodTask<MockWebSpeechRecognizer>(mock), m_function(function) { }
> +        virtual void runIfValid() OVERRIDE { (m_object->m_client->*m_function)(m_object->m_handle); }
> +
> +    private:
> +        ClientFunctionPointer m_function;
> +    };
> +
> +    // Task for delivering a result event.
> +    class ResultTask : public MethodTask<MockWebSpeechRecognizer> {
> +    public:
> +        ResultTask(MockWebSpeechRecognizer* mock, const WebKit::WebString transcript, float confidence) : MethodTask<MockWebSpeechRecognizer>(mock), m_transcript(transcript), m_confidence(confidence) { }
> +        virtual void runIfValid() OVERRIDE;
> +
> +    private:
> +        WebKit::WebString m_transcript;
> +        float m_confidence;
> +    };
> +
> +    // Task for delivering a nomatch event.
> +    class NoMatchTask : public MethodTask<MockWebSpeechRecognizer> {
> +    public:
> +        NoMatchTask(MockWebSpeechRecognizer* mock) : MethodTask<MockWebSpeechRecognizer>(mock) { }
> +        virtual void runIfValid() OVERRIDE { m_object->m_client->didReceiveNoMatch(m_object->m_handle, WebKit::WebSpeechRecognitionResult()); }
> +    };

Can you move them to MockWebSpeechRecognizer.cpp?

[Hans Wennborg]

(In reply to comment #7)
> (From update of attachment 145094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145094&action=review
> 
> > Tools/DumpRenderTree/chromium/MockWebSpeechRecognizer.cpp:94
> > +    WebVector<WebString> transcripts(static_cast<size_t>(1));
> > +    WebVector<float> confidences(static_cast<size_t>(1));
> 
> Do we need static_cast<>? Doesn't 1 or 1u work?
Yes, they're needed. If I try 1U or 1, the compiler will try to use the "template <typename C> WebVector(const C& other)" constructor:

WebVector.h:84:29: error: member reference base type 'const unsigned int' is not a structure or union
        initializeFrom(other.size() ? &other[0] : 0, other.size());
                       ~~~~~^~~~~
Tools/DumpRenderTree/chromium/MockWebSpeechRecognizer.cpp:93:26: note: in instantiation of function template specialization 'WebKit::WebVector<WebKit::WebString>::WebVector<unsigned int>' requested here
    WebVector<WebString> transcripts(1U);
                         ^

> > Tools/DumpRenderTree/chromium/MockWebSpeechRecognizer.h:94
> > +    // Task class for calling a client function that does not take any parameters.
> > +    typedef void (WebKit::WebSpeechRecognizerClient::*ClientFunctionPointer)(const WebKit::WebSpeechRecognitionHandle&);
> > +    class ClientCallTask : public MethodTask<MockWebSpeechRecognizer> {
> > +    public:
> > +        ClientCallTask(MockWebSpeechRecognizer* mock, ClientFunctionPointer function) : MethodTask<MockWebSpeechRecognizer>(mock), m_function(function) { }
> > +        virtual void runIfValid() OVERRIDE { (m_object->m_client->*m_function)(m_object->m_handle); }
> > +
> > +    private:
> > +        ClientFunctionPointer m_function;
> > +    };
> > +
> > +    // Task for delivering a result event.
> > +    class ResultTask : public MethodTask<MockWebSpeechRecognizer> {
> > +    public:
> > +        ResultTask(MockWebSpeechRecognizer* mock, const WebKit::WebString transcript, float confidence) : MethodTask<MockWebSpeechRecognizer>(mock), m_transcript(transcript), m_confidence(confidence) { }
> > +        virtual void runIfValid() OVERRIDE;
> > +
> > +    private:
> > +        WebKit::WebString m_transcript;
> > +        float m_confidence;
> > +    };
> > +
> > +    // Task for delivering a nomatch event.
> > +    class NoMatchTask : public MethodTask<MockWebSpeechRecognizer> {
> > +    public:
> > +        NoMatchTask(MockWebSpeechRecognizer* mock) : MethodTask<MockWebSpeechRecognizer>(mock) { }
> > +        virtual void runIfValid() OVERRIDE { m_object->m_client->didReceiveNoMatch(m_object->m_handle, WebKit::WebSpeechRecognitionResult()); }
> > +    };
> 
> Can you move them to MockWebSpeechRecognizer.cpp?

Done.

[Hans Wennborg]

Created attachment 146523 [details]
Patch

[Kent Tamura]

Comment on attachment 146523 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 146523 [details]
Patch

Clearing flags on attachment: 146523

Committed r119818: <http://trac.webkit.org/changeset/119818>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.