[Webkit-unassigned] [Bug 47089] Language tag in speech for HTML input elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47089





--- Comment #12 from Satish Sampath <satish at chromium.org>  2010-10-07 02:55:01 PST ---
(From update of attachment 69940)
View in context: https://bugs.webkit.org/attachment.cgi?id=69940&action=review

> LayoutTests/fast/speech/input-text-language-tag.html:48
> +    var input_none = document.createElement('input');

Can you declare these input elements in html instead of having all this JS code to create and initialize them?

> LayoutTests/fast/speech/input-text-language-tag.html:50
> +    input_none.speech = 'speech';

This will no longer work, I renamed this attribute to webkitSpeech recently. So this will become:
  input.webkitSpeech = 'webkitSpeech'
Same for all other places in this file. And if you take the above suggestion and declare these in html, then use
  <input x-webkit-speech>

> LayoutTests/fast/speech/input-text-speechbutton.html:35
> +        layoutTestController.setMockSpeechInputResult('Pictures of the moon', 'en');

Is 'en' required here or can we pass an empty string? On a related note, does the c++ code get 'en' as the language if the web page did not specify any language at the document or node level?

> WebCore/ChangeLog:5
> +        Patch the current speech HTML tag implementation to use and validate

nit: replace 'speech HTML tag' with 'speech input' since speech input has not introduced any new HTML tags, instead it tries to work with existing ones.

>>>> WebCore/page/SpeechInput.cpp:96
>>>> +bool SpeechInput::startRecognition(int listenerId, const CString& languageTag, const IntRect& elementRect)
>>> 
>>> Why are you using a CString for the language identifier?
>> 
>> Because the expected input is a valid BCP47 language tag: http://tools.ietf.org/rfc/bcp/bcp47.txt. The spec includes a grammar for the valid ones but all of them are composed of alphanumeric ASCII characters and the '-' symbol. Any tag not passing this simple test is considered invalid and the languageTag variable becomes an empty string. Note that a full language tag parsing for validation is not intended here.
>> 
>> So, since any valid content can be safely represented with a 8-bit ASCII string (even with a 7-bit one) there's absolutely no need to use any wide-char strings for this. It's just a waste of memory.
> 
> For simplicity we generally stick with strings (UTF16) unless there's a good reason not to.  The memory saving his are negligible, so we should probably not be using a cstring here.

I agree, keep it simple and use a String

> WebCore/platform/mock/SpeechInputClientMock.cpp:53
> +bool SpeechInputClientMock::startRecognition(int requestId, const CString& languageTag, const IntRect&)

I suggest renaming 'languageTag' to 'language', because though BCP47 calls them as language tags it is confusing in the context of WebCore whether these are html tags or something else. If you agree, could also rename all other occurances of this name

> WebCore/rendering/TextControlInnerElements.cpp:464
> +CString InputFieldSpeechButtonElement::validatedInheritedLanguage(HTMLInputElement *input)

I think a better place for validating this would be inside the Element::computeInheritedLanguage() method you are calling below. That will allow all callers of that method to benefit from this validation instead of just speech input.

>> WebKit/chromium/public/WebSpeechInputController.h:47
>> +    virtual bool startRecognition(int requestId, const WebCString&, const WebRect&)
> 
> this WebCString parameter needs a name.  you should give parameters a name
> unless the name you would pick is redundant with the type name.

Can you also name the WebRect to 'element_rect' since that is not obvious as well?

>> WebKit/chromium/public/WebSpeechInputControllerMock.h:47
>> +    virtual void setMockRecognitionResult(const WebString& result, const WebString& languageTag) = 0;
> 
> We don't do pure virtual methods in the WebKit API anymore.  Please make this (and others) have a WEBKIT_ASSERT_NOT_REACHED() default method body.

I think pure virtual is used for all public APIs called by the embedder, and WEBKIT_ASSERT_NOT_REACHED() is used for all public APIs implemented by the embedder (and called by webkit). In that context, this interface is called by the embedder hence pure virtual is matching the style.

> WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:59
>  void WebSpeechInputControllerMockImpl::setMockRecognitionResult(const WebString& result)

Is this old method still required?

>> WebKit/chromium/src/WebViewImpl.cpp:279
>> +    , m_speechInputClient(new SpeechInputClientImpl(client))
> 
> SpeechInputClientImpl should have a create factory method that returns a passownptr

For the reason behind this suggestion, please see https://lists.webkit.org/pipermail/webkit-dev/2010-August/014006.html

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.