[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 #71 from Leandro Graciá Gil <leandrogracia at chromium.org>  2010-10-28 10:55:47 PST ---
(In reply to comment #67)
> (From update of attachment 72049 [details])
> Sorry, I only looked at the latest patch now. I'm not happy with it.
> 
> +        http://codereview.chromium.org/3595018/show. The last of the 4 patches
> +        depends also on the language tag validation provided by this patch:
> +        https://bugs.webkit.org/show_bug.cgi?id=48225.
> ...
> +description('Tests for language tag inheritance and validation in speech buttons.');
> ...
> +        Patch the current speech input implementation to use and validate the
> +        nearest language tag. The language is now passed to the startRecognition
> ...
> +        (WebCore::Element::computeInheritedLanguage): includes a brief character
> +          validation for the BCP 47 language tag.
> 
> We don't validate, correct?
> 
> +namespace {
> 
> As a matter of coding style, we use static keyword, not anonymous namespaces.
> 
> +    // HashMap doesn't support empty strings as keys, so this value (an invalid BCP47 tag) is used for those cases.
> +    const String emptyLanguage = "_";
> 
> This will have an observable effect, now that we don't validate - the computed language can be "_". Perhaps not a big deal, but somewhat unclean nonetheless. This creates an untested and poorly understood code path - something that security bugs need to breed.
> 
> Also, we can not have global static objects with constructors and destructors. The rationale is that no WebKit code at all should run when WebKit library is loaded into an application, and no WebKit code should run when an application quits.
> 
> We have an automated test that fails the build for constructors and destructors on Mac, so this should have broken Mac build. I'm not sure if the check failed, or if you didn't try building on Mac.
> 
> Please consider using AtomicString for language codes - they are quite likely to all be the same in a particular engine instance, so atomizing should be highly beneficial.
> 
> I didn't read the whole patch, just the parts around instances of the word "valid".

Changes done and AtomicString adopted for the language codes.

About solving the problem with the empty language case in the hash map, I think that handling the case separately is probably the simplest and cleanest way. Other possible solutions like STL maps required operator overloading, or using STL vectors required creating (language, result) tuples and checking for existing (language,*) entries. Could be done but this seems to be quite simpler.

Any suggestions are welcome, and thank you for the style information on the static objects.

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