Bug 47089

Created attachment 69914 [details] Patch

Created attachment 69925 [details] Patch

Attachment 69925 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4219107

Created attachment 69934 [details] Patch

Attachment 69934 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4202096

Created attachment 69940 [details] Patch

Comment on attachment 69940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69940&action=review > 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?

(In reply to comment #7) > (From update of attachment 69940 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69940&action=review > > > 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.

Comment on attachment 69940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69940&action=review > 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.

Comment on attachment 69940 [details] Patch 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

Satish, can you please do an informal review of this?

Comment on attachment 69940 [details] Patch 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

(In reply to comment #12) > > WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:59 > > void WebSpeechInputControllerMockImpl::setMockRecognitionResult(const WebString& result) > > Is this old method still required? I'm afraid that for the moment it is. It breaks chromium's test_shell otherwise. I'll remove this in a new patch once the chromium-side one has been applied.

(In reply to comment #12) > (From update of attachment 69940 [details]) > > 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? In this case we should pass 'en' there since the empty string is left for the invalid and unspecified language tag cases, which have their own tests. The c++ code in this patch won't get any language if none is specified. This will be done in the chromium side of the patch, where the application locale will be used when receiving an empty language tag string.

Created attachment 70128 [details] Patch

Comment on attachment 69940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69940&action=review >> LayoutTests/fast/speech/input-text-language-tag.html:15 >> +function onChange_none() { > > this is not webkit style..fix here and elsewhere Done. >> 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? Done. >> 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> Done. >>> 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? > > In this case we should pass 'en' there since the empty string is left for the invalid and unspecified language tag cases, which have their own tests. The c++ code in this patch won't get any language if none is specified. This will be done in the chromium side of the patch, where the application locale will be used when receiving an empty language tag string. Sorry, my mistake. For some reason I though you where talking about the language tag layout test. You're right, in this case the most logical choice would be to pass an empty language string as no language is set in the test. However I found today some segmentation fault bugs caused by the HashMap handling these results when the language was an empty string. Because of this, now any call to setMockSpeechInputResult with an empty language string is ignored. In the new patch I have set the appropriate language tags in this and other tests using 'en' as language (could be any valid one). >> 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. Done. >>>>> 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 Done. >> 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 Done. >> WebCore/rendering/TextControlInnerElements.cpp:103 >> + > > get rid of white space Done. >> WebCore/rendering/TextControlInnerElements.cpp:286 >> + > > get rid of white space Done. >> 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. Done. >>> 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? Done. >>> 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. No changes here for the moment. >> WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:59 >> void WebSpeechInputControllerMockImpl::setMockRecognitionResult(const WebString& result) > > Is this old method still required? Yes, for the moment it is. It breaks chromium's test_shell otherwise. I'll delete these in a future patch once the chromium side has landed. >> 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. Done. >>> 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 Done. Thanks for the link.

Attachment 70128 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4222120

Created attachment 70243 [details] Patch

Created attachment 70248 [details] Patch

Comment on attachment 70248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70248&action=review > LayoutTests/fast/speech/input-text-language-tag.html:16 > + shouldBeEqualToString('document.getElementById("speechInputNone").value', 'error: no result found for language \'\''); Instead of treating this as an error case, can you set a mock result for the empty language tag and check that here? > WebCore/platform/mock/SpeechInputClientMock.cpp:33 > +#include <wtf/text/CString.h> Do you need this include in the latest code? > WebCore/platform/mock/SpeechInputClientMock.cpp:88 > + m_recognitionResult.set("_", result); Define "_" as a constant at the top of the file and use it both here and in timerFired(). Also add a comment with this constant declaration explaining what it is used for. > WebCore/platform/mock/SpeechInputClientMock.cpp:103 > + String error("error: no result found for language '"); Since this is test code and from the test result we know exactly which portion of the code called this, perhaps we don't need this human-readable error string. Instead you can just skip calling setRecognitionResult(). That also matches what the real recognizer would do, if it was not able to recognize the user voice in the given language. > WebCore/platform/mock/SpeechInputClientMock.h:55 > + bool startRecognition(int, const WTF::String&, const IntRect&); Remove the WTF:: specifier > WebKit/chromium/src/SpeechInputClientImpl.h:39 > +#include <wtf/text/WTFString.h> Remove this since you have a forward declaration for String below. > WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:60 > + m_webcoreMock->setRecognitionResult(result, WebString::fromUTF8("en")); Should you pass an empty string here instead of 'en' ? > WebKit/chromium/src/WebSpeechInputControllerMockImpl.h:39 > +#include <wtf/text/WTFString.h> Remove this since you have a forward declaration for String below.

Comment on attachment 70248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70248&action=review >> LayoutTests/fast/speech/input-text-language-tag.html:16 >> + shouldBeEqualToString('document.getElementById("speechInputNone").value', 'error: no result found for language \'\''); > > Instead of treating this as an error case, can you set a mock result for the empty language tag and check that here? Discussed and rejected. We need a result in all cases to start the change event, and this is as good as any other. >> WebCore/platform/mock/SpeechInputClientMock.cpp:33 >> +#include <wtf/text/CString.h> > > Do you need this include in the latest code? Sorry, my mistake during rebase merges. Done. >> WebCore/platform/mock/SpeechInputClientMock.cpp:88 >> + m_recognitionResult.set("_", result); > > Define "_" as a constant at the top of the file and use it both here and in timerFired(). Also add a comment with this constant declaration explaining what it is used for. Done. >> WebCore/platform/mock/SpeechInputClientMock.cpp:103 >> + String error("error: no result found for language '"); > > Since this is test code and from the test result we know exactly which portion of the code called this, perhaps we don't need this human-readable error string. Instead you can just skip calling setRecognitionResult(). That also matches what the real recognizer would do, if it was not able to recognize the user voice in the given language. Wee need some output even for the empty language case. Otherwise no change event will be generated and the test will timeout without any checking. >> WebCore/platform/mock/SpeechInputClientMock.h:55 >> + bool startRecognition(int, const WTF::String&, const IntRect&); > > Remove the WTF:: specifier Done. >> WebKit/chromium/src/SpeechInputClientImpl.h:39 >> +#include <wtf/text/WTFString.h> > > Remove this since you have a forward declaration for String below. Done. >> WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:60 >> + m_webcoreMock->setRecognitionResult(result, WebString::fromUTF8("en")); > > Should you pass an empty string here instead of 'en' ? Done. Now that mock results for empty language strings are again supported it makes sense. >> WebKit/chromium/src/WebSpeechInputControllerMockImpl.h:39 >> +#include <wtf/text/WTFString.h> > > Remove this since you have a forward declaration for String below. Done.

Created attachment 70252 [details] Patch

Comment on attachment 70252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70252&action=review r=me Looks good, small nit below. > WebCore/platform/mock/SpeechInputClientMock.cpp:39 > + // Empty language strings crash the HashMap. This value, an invalid BCP47 tag, is used for those cases. I'd suggest rewording this as 'HashMap doesn't support empty strings as keys, so ...'

Comment on attachment 70252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70252&action=review > WebCore/dom/Element.cpp:1474 > + CString languageTag = value.string().ascii(); The creation of a CString here is unnecessary. Please don’t do this. > WebCore/page/SpeechInputClient.h:39 > +namespace WTF { > +class String; > +} > + Why is this needed?

Created attachment 70258 [details] Patch

Created attachment 70602 [details] Patch

Here are the links to all the other parts of this patch (4-sided): - 1st part: http://codereview.chromium.org/3615005/show - 2nd part: https://bugs.webkit.org/show_bug.cgi?id=47089 (this one) - 3rd part: http://codereview.chromium.org/3595018/show - 4th part: https://bugs.webkit.org/show_bug.cgi?id=47420

Created attachment 70608 [details] Patch

(In reply to comment #24) > (From update of attachment 70252 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70252&action=review > > > WebCore/page/SpeechInputClient.h:39 > > +namespace WTF { > > +class String; > > +} > > + > > Why is this needed? That's for the language string in the startRecognition method. I already tried to remove it but the build breaks if I do.

(In reply to comment #29) > (In reply to comment #24) > > (From update of attachment 70252 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=70252&action=review > > > > > WebCore/page/SpeechInputClient.h:39 > > > +namespace WTF { > > > +class String; > > > +} > > > + > > > > Why is this needed? > > That's for the language string in the startRecognition method. I already tried to remove it but the build breaks if I do. Please just include <wtf/Forward.h> and use unqualified String.

Created attachment 70611 [details] Patch

(In reply to comment #30) > (In reply to comment #29) > > (In reply to comment #24) > > > (From update of attachment 70252 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=70252&action=review > > > > > > > WebCore/page/SpeechInputClient.h:39 > > > > +namespace WTF { > > > > +class String; > > > > +} > > > > + > > > > > > Why is this needed? > > > > That's for the language string in the startRecognition method. I already tried to remove it but the build breaks if I do. > > Please just include <wtf/Forward.h> and use unqualified String. Done there and in WebKit/chromium/src/SpeechInputClientImpl.h/cpp

Comment on attachment 70611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70611&action=review Looks fine to me. Mitz, please speak up if you have any additional concerns? > WebKit/chromium/src/SpeechInputClientImpl.cpp:38 > #include "page/SpeechInputListener.h" no page/ > WebKit/chromium/src/SpeechInputClientImpl.h:37 > #include "page/SpeechInputClient.h" Remove page/

Created attachment 70867 [details] Patch

(In reply to comment #33) > (From update of attachment 70611 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70611&action=review > > Looks fine to me. Mitz, please speak up if you have any additional concerns? > > > WebKit/chromium/src/SpeechInputClientImpl.cpp:38 > > #include "page/SpeechInputListener.h" > > no page/ > > > WebKit/chromium/src/SpeechInputClientImpl.h:37 > > #include "page/SpeechInputClient.h" > > Remove page/ Done.

Created attachment 71163 [details] Patch

(In reply to comment #36) > Created an attachment (id=71163) [details] > Patch Rebased for review and landing.

Comment on attachment 71163 [details] Patch Rebase performed. Please review and commit if looks good.

Comment on attachment 71163 [details] Patch r=me

Comment on attachment 71163 [details] Patch Rejecting patch 71163 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2 Last 500 characters of output: ....ok WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnPropertyValue..........ok WebKitTools/Scripts/webkitperl/VCSUtils_unittest/prepareParsedPatch.............ok WebKitTools/Scripts/webkitperl/VCSUtils_unittest/runPatchCommand................ok WebKitTools/Scripts/webkitperl/VCSUtils_unittest/setChangeLogDateAndReviewer....ok All tests successful. Files=14, Tests=304, 1 wallclock secs ( 0.63 cusr + 0.13 csys = 0.76 CPU) Running build-dumprendertree Compiling DumpRenderTree failed! Full output: http://queues.webkit.org/results/4430115

Comment on attachment 71163 [details] Patch once more

Comment on attachment 71163 [details] Patch Rejecting patch 71163 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2 Last 500 characters of output: ....ok WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnPropertyValue..........ok WebKitTools/Scripts/webkitperl/VCSUtils_unittest/prepareParsedPatch.............ok WebKitTools/Scripts/webkitperl/VCSUtils_unittest/runPatchCommand................ok WebKitTools/Scripts/webkitperl/VCSUtils_unittest/setChangeLogDateAndReviewer....ok All tests successful. Files=14, Tests=304, 0 wallclock secs ( 0.64 cusr + 0.14 csys = 0.78 CPU) Running build-dumprendertree Compiling DumpRenderTree failed! Full output: http://queues.webkit.org/results/4517018

http://trac.webkit.org/changeset/70149 might have broken SnowLeopard Intel Release (Tests) and Qt Linux Release

Created attachment 71299 [details] Patch

Comment on attachment 71299 [details] Patch Added the missing patches to the Win, Mac and Wx versions of the LayoutTestController that were causing the build to break.

Comment on attachment 71299 [details] Patch looks fine...hopefully ASSERT_UNUSEDs aren't necessary, but I guess the commit queue will let us know if it is?

Comment on attachment 71299 [details] Patch Rejecting patch 71299 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71299]" exit_code: 2 Cleaning working directory Updating working directory Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Full output: http://queues.webkit.org/results/4524017

Comment on attachment 71299 [details] Patch I think this was a false rejection from a run-away commit-node.

Comment on attachment 71299 [details] Patch Rejecting patch 71299 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71299]" exit_code: 2 Cleaning working directory Updating working directory Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=71299&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=47089&ctype=xml Processing 1 patch from 1 bug. Processing patch 71299 from bug 47089. Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Jeremy Orlow', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/4616007

Created attachment 71487 [details] Patch

Comment on attachment 71487 [details] Patch Rebase and patches to the Qt port.

Created attachment 71540 [details] Patch

Comment on attachment 71540 [details] Patch Rebase.

Comment on attachment 71540 [details] Patch Clearing flags on attachment: 71540 Committed r70301: <http://trac.webkit.org/changeset/70301>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/70301 might have broken Qt Linux Release The following tests are not passing: fast/css/lang-selector-empty-attribute.xhtml fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml

Created attachment 71569 [details] Patch

Comment on attachment 71569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71569&action=review Fixed problems in Element.cpp that caused some layout tests to fail. > WebCore/dom/Element.cpp:1480 > + CString asciiOnly = value.string().ascii(); The CString object is required here to ensure that the memory pointed by p is valid until the end of the if block. Note that value contains the object returned by string() but ascii creates a new CString with the ASCII-only representation of the UTF16 string value, replacing all non-ASCII characters with question mark symbols.

Reopening, since r70301 has been rolled out.

Comment on attachment 71569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71569&action=review > WebCore/ChangeLog:18 > + Fixed a problem in dom/Element.cpp that caused some layout tests to fail. No need to mention that - this doesn't fix any problems in ToT. > WebCore/ChangeLog:22 > + (WebCore::Element::computeInheritedLanguage): includes a brief character > + validation for the BCP 47 language tag. Please file a separate bug for this, and provide a patch that tests this change with default WebKit build options (i.e. outside of speech recognition context). r- for the need to split DOM changes out.

Created attachment 72049 [details] Patch

Comment on attachment 72049 [details] Patch Language validation has been removed and handled separately in bug https://bugs.webkit.org/show_bug.cgi?id=48225 (currently a WONTFIX). The invalid language tag case has been removed from the layout test.

Comment on attachment 72049 [details] Patch yup...looks pretty much like before minus those bits

(In reply to comment #63) > (From update of attachment 72049 [details]) > yup...looks pretty much like before minus those bits r=me (AP's comments seem to be addressed, but please yell if that's not true)

Comment on attachment 72049 [details] Patch Clearing flags on attachment: 72049 Committed r70665: <http://trac.webkit.org/changeset/70665>

All reviewed patches have been landed. Closing bug.

Comment on attachment 72049 [details] Patch 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".

Reverted r70665 for reason: Need to address Alexey's review comments. Committed r70669: <http://trac.webkit.org/changeset/70669>

http://trac.webkit.org/changeset/70665 might have broken GTK Linux 32-bit Debug

Created attachment 72208 [details] Patch

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

Thanks Leandro! I don't have any complaints about this patch.

STL containers generally require exceptions to be enabled, so we cannot use them in WebKit (we sometimes use them in separate tools).

(In reply to comment #73) > STL containers generally require exceptions to be enabled, so we cannot use them in WebKit (we sometimes use them in separate tools). Good and useful to know. Thank you for your help.

Created attachment 72316 [details] Patch

Comment on attachment 72316 [details] Patch rubber stamp...again

Comment on attachment 72316 [details] Patch Clearing flags on attachment: 72316 Committed r70863: <http://trac.webkit.org/changeset/70863>

All reviewed patches have been landed. Closing bug.