[Webkit-unassigned] [Bug 39487] Add a skeleton 'speech' type input element

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39487





--- Comment #3 from Jeremy Orlow <jorlow at chromium.org>  2010-05-21 11:06:10 PST ---
(From update of attachment 56717)
I'm not qualified to review the meat of this, but I can do the rest.  Thus I'm making comments but leaving this in the review queue (for someone who can review the meat to do so).

Here's some initial comments...


WebKitTools/ChangeLog:5
 +          Command line flag to conditionally enable speech input API code
no need to wrap at 80 chars

WebKit/chromium/public/WebInputElement.h:79
 +  #if (defined ENABLE_INPUT_SPEECH && ENABLE_INPUT_SPEECH)
This syntax is only necessary in IDLs.  Everywhere else, use #if ENABLE()

WebKit/chromium/ChangeLog:5
 +          Keep the input type enum in sync with the list in HTMLInputElement.
You're OK this time, but I just want to note that the standard format for change logs is as follows:

"""
<headline/subject...one line only>
<url>

<description of what and (moreso) why>
"""

Note the blank line.

WebCore/rendering/RenderInputSpeechControl.h:59
 +  #endif // RenderInputSpeechControl_h
newline between

WebCore/rendering/RenderInputSpeechControl.h:58
 +  #endif
// ENABLE(INPUT_SPEECH)

WebCore/rendering/RenderInputSpeechControl.cpp:30
 +  #include "RenderInputSpeechControl.h"
custom bindings' .cpp files are the only place where this include needs to be in the guard (since there the .h file isn't even generated if it's disabled).  This should be grouped with the #include "config.h"

WebCore/rendering/RenderInputSpeechControl.cpp:2
 +   * Copyright (C) 2010 Apple Inc. All rights reserved.
I assume this is because you copied code?  If so, it's OK.

LayoutTests/ChangeLog:5
 +          Basic layout test and associated setup for testing speech input
This is probably a place where you should use the changelog style I mentioned earlier.

LayoutTests/ChangeLog:10
 +          * fast/speechinput/input-speech-create-expected.txt: Added.
Is there a better (less top level) directory for these?

LayoutTests/fast/speechinput/script-tests/input-speech-create.js:7
 +  shouldBe('input.type', '"speech"');
You can use shouldBeEqualToString

LayoutTests/platform/chromium/test_expectations.txt:2832
 +  BUGSATISH DEFER : fast/speechinput/input-speech-create.html = TEXT
Create a chromium bug that links to the webkit bug.  Assign the chromium bug to yourself.  Get rid of "defer".  Maybe make this be the whole directory rather than just the one test.

LayoutTests/platform/mac-leopard/Skipped:72
 +  fast/speechinput
I believe you only need to add it to the mac skipped list, not all the sub-platforms

WebCore/ChangeLog:5
 +          Speech input element CSS styles added to UA stylesheet.
Please follow the standard changelog format.

WebCore/ChangeLog:14
 +          * DerivedSources.make:
What about visual studio, the .pro/.pri, DerivedSources.cpp, android, and CMake?

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