Bug 40984

Created attachment 59648 [details] Patch

Attachment 59648 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.cpp" WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.h" WebCore/platform/graphics/qt/ImageQt.cpp:67: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 59649 [details] Patch

Attachment 59649 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3326686

Comment on attachment 59649 [details] Patch Drive by comments. WebCore/rendering/RenderInputSpeech.cpp:34 + #include "RenderInputSpeech.h" You only put this within guards when the file is generated only when enabled. Since this file is being checked in, you know it exists and thus it can go right under the config.h part.

Created attachment 59716 [details] Patch

Thanks Jeremy. Latest patch has your suggestion implemented. The cr-linux build fails for all these patches because of a known issue in the cr-linux build bot (it does not run 'gclient runhooks' for GYP/GYPI changes so keeps using the earlier makefiles). I have verified again on my local machine that cr-linux builds fine.

Created attachment 59717 [details] Address Jeremy's comments

Attachment 59717 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3267674

Comment on attachment 59717 [details] Address Jeremy's comments I don't think we need to introduce new files; RenderInputSpeech.{cpp,h}.

Future patches will be adding more speech input related rendering code hence I created a new file as it would help in better code organization, looking at how the media controls were implemented in RenderMediaControls.cpp. If you strongly feel about not creating new files I can change it.

(In reply to comment #11) > Future patches will be adding more speech input related rendering code hence I created a new file as it would help in better code organization, looking at how the media controls were implemented in RenderMediaControls.cpp. If you strongly feel about not creating new files I can change it. Ok, I understand. I don't have strong objection. The code looks ok. We should have layout tests for type=number + speech and type=search + speech.

Created attachment 59776 [details] Add new tests per tkent and some fixes

Attachment 59776 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3307709

(adding Adele) Kent, thanks for the additional layout tests suggestion. I found some issues in my patch due to that and fixed now (in RenderThemeMac.mm and RenderTextControlSingleLine.cpp). Please take another look at them in particular. Adele - can you please let me know if the changes to RenderThemeMac.mm and switching from setDisplay(BLOCK) to setDisplay(INLINE_BLOCK) in RenderTextControlSingleLine.cpp look ok? (Background info: in this patch I am enabling render of a speech input button in <input> elements, the feature need to be explicitly enabled with an @speech attribute and all the code is behind a define). - The setDisplay() change to INLINE_BLOCK is required because RenderTextControlSingleLine can now contain both innerBlock and other child elements (i.e. spin button in the 'type=number' case), so to render them both in the same line I'm using INLINE_BLOCK - The RenderThemeMac.mm change is required because the search field's clear (x) button may not be the right-most child element if the speech button is enabled. For both the above changes, all layout tests passed in my local machine.

Comment on attachment 59776 [details] Add new tests per tkent and some fixes LayoutTests/ChangeLog:15 + * platform/mac/fast/forms/input-appearance-numberandspeech.html: Added. These tests should be put to fast/forms/, not platform/mac/fast/forms/. We should check appearance on other platforms.

(In reply to comment #15) > Adele - can you please let me know if the changes to RenderThemeMac.mm and switching from setDisplay(BLOCK) to setDisplay(INLINE_BLOCK) in RenderTextControlSingleLine.cpp look ok? (Background info: in this patch I am enabling render of a speech input button in <input> elements, the feature need to be explicitly enabled with an @speech attribute and all the code is behind a define). I think these changes are reasonable.

Created attachment 60000 [details] Moved layout tests and updated Skipped lists.

Comment on attachment 60000 [details] Moved layout tests and updated Skipped lists. LayoutTests/ChangeLog:24 + * platform/chromium/Skipped: Skipped all these new tests as the speech input feature is disabled by default. chromium/Skpped -> chromium/test_expectations.txt LayoutTests/platform/chromium/test_expectations.txt:2896 + BUG44844: fast/forms/input-appearance-numberandspeech.html = TEXT I think "TEXT" should be "IMAGE+TEXT" or "FAIL"

Created attachment 60001 [details] Updated changelog and test_expectations.txt

Created attachment 60004 [details] Merged with latest revision

Comment on attachment 60004 [details] Merged with latest revision Rejecting patch 60004 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: dertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19232 test cases. fast/css/input-search-padding.html -> failed Exiting early after 1 failures. 5961 tests run. 108.46s total testing time 5960 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3352216

Created attachment 60216 [details] Changed RenderTextControlSingleLine to use INLINE_BLOCK only if a spin button is also present, so all existing layout test and the new ones pass now.

Comment on attachment 60216 [details] Changed RenderTextControlSingleLine to use INLINE_BLOCK only if a spin button is also present, so all existing layout test and the new ones pass now. Clearing flags on attachment: 60216 Committed r62249: <http://trac.webkit.org/changeset/62249>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/62249 might have broken Chromium Linux Release