Bug 39487

Created attachment 56717 [details] Patch

Correction: I have removed the speech input status indicator code from this patch (point (e) in the bug description), will send it as a separate patch later as the rest of this patch did not depend on it.

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

Btw, you might want to see who has been involved in a lot of the input code via svn/git blame and cc them on this.

What spec is this from? The ChangeLog should mention.

(In reply to comment #5) > What spec is this from? The ChangeLog should mention. Ok. Can you please clarify if I should add the spec/draft URL to every ChangeLog or only the topmost level ChangeLog?

Just somewhere. :) even to the bug would be a start.

(In reply to comment #0) An informal spec of the new API proposal, along with some sample apps and use cases can be found at http://docs.google.com/Doc?docid=0AaYxrITemjbxZGNmZzc5cHpfM2Ryajc5Zmhx&hl=en. I have also written in the parent bug's description (https://bugs.webkit.org/show_bug.cgi?id=39485#c0) the steps I plan to take in adding the API.

Comment on attachment 56717 [details] Patch Which standards organization is overseeing the development of this spec? Please provide a link to the W3C or WHATWG draft or similar. We don't have a very good experimental features policy and don't as a community know how to support or develop features in trunk that aren't discussed in at least some standards organization draft. You could create a branch for this work, but better would be to start some discussion with a standards body and link to that from this bug before we discuss implementation of the feature in webkit.

(In reply to comment #9) > (From update of attachment 56717 [details]) > Which standards organization is overseeing the development of this spec? Please provide a link to the W3C or WHATWG draft or similar. Thanks, there was a discussion about the API proposal in the WHATWG mailing list recently (http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-May/026338.html) which saw some positive interest. We think as a conditionally enabled feature in WebKit, web developers can try it out on many forms of devices and give useful feedback before it can be turned into a formal draft proposal.

(In reply to comment #9) > (From update of attachment 56717 [details]) > Which standards organization is overseeing the development of this spec? Please provide a link to the W3C or WHATWG draft or similar. Eric, this has been brought up on WhatWG, privately with several browser vendors (including someone at Apple IIRC), and we're currently working to figure out what W3 WG (possibly a new one) would be right to develop this technology. There certainly isn't consensus yet on how to satisfy the use cases, but there are strong arguments for the input tag method. > We don't have a very good experimental features policy and don't as a community know how to support or develop features in trunk that aren't discussed in at least some standards organization draft. This is being discussed. It's behind a compile time flag. It's extremely self contained. Both the parent bug and the document describe the repercussions of someone shipping this feature and then taking it away and web developers not handling it gracefully (worst case: runtime errors in an event handler). Since you're having these questions, I guess Satish didn't make this stuff as clear in the bug as he should have. I am working with him to try and make sure that we follow the spirit of the WebKit process, even though we've never formalized any policy. I'm going to put this back in the review queue so someone can look at the meat. Let me know if you have any further concerns, though.

Created attachment 57006 [details] Patch

(In reply to comment #3) Made the suggested changes and uploaded new patch, with some replies below: > 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() Files under WebKit/chromium don't seem to be including all the common webkit headers, so the ENABLE() macro is not available here. Hence I used this syntax. > 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. Yes > LayoutTests/ChangeLog:10 > + * fast/speechinput/input-speech-create-expected.txt: Added. > Is there a better (less top level) directory for these? "fast/forms" seems like the closest match, if that sounds ok I can move the files over there. > 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 I saw some earlier patches (including one for indexedDb) which had all the 4 Mac skip lists updated in this manner. I can change it if someone can confirm? > WebCore/ChangeLog:14 > + * DerivedSources.make: > What about visual studio, the .pro/.pri, DerivedSources.cpp, android, and CMake? Added to visual studio, Qt (.pri/.pro) and android project files. DerivedSources.cpp does not require an update since the new file inputSpeech.css gets included into UserAgentStyleSheetData.cpp and hence indirectly into DerivedSources.cpp. I see the CMake files were recently checked in and it looks like work in progress (can't find any file referencing the cpp files directly), so perhaps I can wait for a while before including in them?

(In reply to comment #13) > (In reply to comment #3) > > Made the suggested changes and uploaded new patch, with some replies below: > > > 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() > > Files under WebKit/chromium don't seem to be including all the common webkit headers, so the ENABLE() macro is not available here. Hence I used this syntax. Ohhhhhhhhh. Hm. Are there other places in the pulbic API that test enable macros like this? If so, fine. If not, we should run this past Darin Fisher. > > 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. > > Yes > > > LayoutTests/ChangeLog:10 > > + * fast/speechinput/input-speech-create-expected.txt: Added. > > Is there a better (less top level) directory for these? > > "fast/forms" seems like the closest match, if that sounds ok I can move the files over there. SGTM > > 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 > > I saw some earlier patches (including one for indexedDb) which had all the 4 Mac skip lists updated in this manner. I can change it if someone can confirm? Ugh. I bet I edited LayoutTests/platform/*/Skipped and ended up doing this. Would you mind fixing that for me at the same time? :-) > > WebCore/ChangeLog:14 > > + * DerivedSources.make: > > What about visual studio, the .pro/.pri, DerivedSources.cpp, android, and CMake? > > Added to visual studio, Qt (.pri/.pro) and android project files. DerivedSources.cpp does not require an update since the new file inputSpeech.css gets included into UserAgentStyleSheetData.cpp and hence indirectly into DerivedSources.cpp. I see the CMake files were recently checked in and it looks like work in progress (can't find any file referencing the cpp files directly), so perhaps I can wait for a while before including in them? If it's obvious how to edit the CMake file to include this change, please do it. If not, then don't worry about it. They don't have a builder yet, so it's not required, but it is considered polite to at least try.

Comment on attachment 57006 [details] Patch WebCore/rendering/RenderInputSpeechControl.cpp:1 + /* For new files, please use this license: http://webkit.org/coding/bsd-license.html (I noticed this one says "apple computer" which is no longer current.) WebCore/rendering/RenderInputSpeechControl.h:1 + /* ditto

(In reply to comment #15) Thanks, addressed the comments with some replies below: > > > 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() > > > > Files under WebKit/chromium don't seem to be including all the common webkit headers, so the ENABLE() macro is not available here. Hence I used this syntax. > > Ohhhhhhhhh. Hm. Are there other places in the pulbic API that test enable macros like this? If so, fine. If not, we should run this past Darin Fisher. No this is the first instance of an ifdef in the public API. Using ifdefs in an API is generally not recommended to keep the interfaces stable, but in this case it was required because I conditionally defined the input type SPEECH in HTMLInputElement.h (since not doing that caused a whole bunch of unused enum warnings/errors when compiling with speech input option disabled, which is the default). > If it's obvious how to edit the CMake file to include this change, please do it. If not, then don't worry about it. They don't have a builder yet, so it's not required, but it is considered polite to at least try. I figured it would be better to add it to cmake later once they have the source files included in the cmake build scripts as well, so that I don't just add the new CSS file now and miss out on the new source files. I'll try to get it included as soon as there is a valid .cmake file for the relevant new sources.

Created attachment 57015 [details] Patch

Comment on attachment 57015 [details] Patch WebKit/chromium/public/WebInputElement.h:79 + #if (defined ENABLE_INPUT_SPEECH && ENABLE_INPUT_SPEECH) This #if should be removed. Any conditional code needs to be in the .cpp file. Since this is an enum that needs to match the one declared in WebCore, I recommend not wrapping the WebCore one with an #if either. The consumers of the Chromium WebKit API do not necessarily include config.h, so they may not have access to the set of enabled features. Therefore, we do not support feature defines in the public interface.

(In reply to comment #18) > (From update of attachment 57015 [details]) > WebKit/chromium/public/WebInputElement.h:79 > + #if (defined ENABLE_INPUT_SPEECH && ENABLE_INPUT_SPEECH) > This #if should be removed. Any conditional code needs to be in > the .cpp file. Since this is an enum that needs to match the one > declared in WebCore, I recommend not wrapping the WebCore one with > an #if either. > > The consumers of the Chromium WebKit API do not necessarily include > config.h, so they may not have access to the set of enabled features. > Therefore, we do not support feature defines in the public interface. Thanks, I've made the suggested changes. One more clarification about this patch: The bulk of this patch is about setting up the compile time defines and other relevant stuff in place to develop the API. As Jeremy mentioned we are actively discussing at WHATWG and trying to address the comments received so far, including some good tweaks to the proposal. As more feedback flows in and the draft API gets updated, I am fully committed to keep this and subsequent patches in sync. We will also continue to keep all the changes under the conditional compile flag and we won't ship it as part of Chromium until it is sufficiently mature to be part of the web platform. I'm also cc-ing Darin and Kent Tamura as they have recently looked at the input element related code. Eric, Darin, Kent, would you have time to look at this patch?

Created attachment 57089 [details] Patch

(In reply to comment #20) > Created an attachment (id=57089) [details] > Patch Actually, we just had a brainstorming session today - we looked at all the feedback received so far on the speech input API proposal and as a result we'll be making some revisions to the proposal shortly. With that in mind I'd like to withdraw this patch and submit a new one after the API proposal gets updated. Thank you for all the feedback given so far.