[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 #10 from Jeremy Orlow <jorlow at chromium.org>  2010-10-06 10:56:29 PST ---
(From update of attachment 69940)
View in context: https://bugs.webkit.org/attachment.cgi?id=69940&action=review

A few initial thoughts...

> LayoutTests/fast/speech/input-text-language-tag.html:15
> +function onChange_none() {

this is not webkit style..fix here and elsewhere

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

> WebCore/rendering/TextControlInnerElements.cpp:103
> +    

get rid of white space

> WebCore/rendering/TextControlInnerElements.cpp:286
> + 

get rid of white space

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

> WebKit/chromium/src/WebSpeechInputControllerMockImpl.h:72
> +    // TODO(leandrogracia): delete this temporary fix. Required to launch a two-sided patch.

This is not Webkit style.  Use FIXME.

> WebKit/chromium/src/WebViewImpl.cpp:279
> +    , m_speechInputClient(new SpeechInputClientImpl(client))

SpeechInputClientImpl should have a create factory method that returns a passownptr

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