Bug 44427

Created attachment 65125 [details] Patch

Comment on attachment 65125 [details] Patch WebCore/rendering/TextControlInnerElements.cpp:401 + m_listenerId, input->renderer()->absoluteBoundingBoxRect())) { don't wrap...don't need {}'s WebCore/ChangeLog:5 + Pass the element's bounds to embedder during speech recognition. Please explain why here.

Created attachment 65161 [details] Patch

Thanks for the comments, made both the changes.

Comment on attachment 65161 [details] Patch WebCore/rendering/TextControlInnerElements.cpp:400 + if (speechInput()->startRecognition( You didn't put this all on one line r=me

Created attachment 65162 [details] Patch Sorry, my bad. Moved the if statement to a single line now.

Comment on attachment 65162 [details] Patch Let's cq+ it tomorrow when we're in the office.

Comment on attachment 65162 [details] Patch Rejecting patch 65162 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20910 test cases. http/tests/security/xssAuditor/dom-write-location-inline-event.html -> failed Exiting early after 1 failures. 20558 tests run. 553.41s total testing time 20557 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 36 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/3771547

Comment on attachment 65162 [details] Patch try and try again

Comment on attachment 65162 [details] Patch Rejecting patch 65162 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20912 test cases. http/tests/security/xssAuditor/dom-write-location-inline-event.html -> failed Exiting early after 1 failures. 20560 tests run. 557.65s total testing time 20559 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 35 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/3715552

Comment on attachment 65162 [details] Patch Clearing flags on attachment: 65162 Committed r66007: <http://trac.webkit.org/changeset/66007>

All reviewed patches have been landed. Closing bug.

Comment on attachment 65162 [details] Patch WebKit/chromium/public/WebSpeechInputController.h:46 + virtual bool startRecognition(int requestId, const WebRect&) Looking at this public API, it is quite mysterious what this WebRect parameter is supposed to mean. You should give it a meaningful name and perhaps add some documentation. Neither the ChangeLog nor the bug description says anything about why you need this. Are you trying to have the embedder externally position something on top of the input element? You should be sure to document the coordinates of this WebRect. Have you considered what happens if the user zooms or scrolls the page? (Note you can scroll the page without changing focus by using the mouse wheel.)

(In reply to comment #13) > Neither the ChangeLog nor the bug description says anything about why > you need this. Ah, I overlooked comment #0. Some form of that text should be in the ChangeLog.

(In reply to comment #14) > (In reply to comment #13) > > Neither the ChangeLog nor the bug description says anything about why > > you need this. > > Ah, I overlooked comment #0. Some form of that text should be in the ChangeLog. Something very similar to that is in the ChangeLog, I believe.

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > Neither the ChangeLog nor the bug description says anything about why > > > you need this. > > > > Ah, I overlooked comment #0. Some form of that text should be in the ChangeLog. > > Something very similar to that is in the ChangeLog, I believe. I had included it in the WebCore/Changelog, but missed out in the WebKit/chromium/Changelog. Would you prefer I edit the WebKit/chromium/Changelog now to include it? I will make sure to include sufficient information in all changelogs in future patches.