Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6869024:
Add IME UI related extension API. (Closed)
|
Created:
9 years, 8 months ago by Peng Modified:
9 years, 5 months ago Reviewers:
zork, commit-bot: I haz the power, bryeung, Mihai Parparita -not on Chrome, asargent_no_longer_on_chrome, Zachary Kuznia, Yusuke Sato CC:
chromium-reviews, Rick Byers, satorux1 Visibility:
Public. |
DescriptionAdd IME UI related extension API.
Add IME UI related extension API. We will use it to show IME candidates in a chrome extension.
BUG=none
TEST=seaboard
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : Update #
Total comments: 9
Patch Set 4 : Update #Patch Set 5 : Rename ime.ui to input.ui #
Total comments: 7
Patch Set 6 : Update #Patch Set 7 : Update License header #Patch Set 8 : Fix build errors #
Total comments: 3
Patch Set 9 : Rename input.ui to inputUI #Patch Set 10 : Rename input.ui to inputUI #
Total comments: 6
Patch Set 11 : Create html template and update doc in extension_api.json #Patch Set 12 : Rebase #Messages
Total messages: 32 (0 generated)
http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2371: "namespace": "experimental.ime.ui", I use name space experimental.ime.ui temporarily. I am not sure what's a good name space for those APIs. How about experimental.input.imeui?
http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_ime_ui_api.cc (right): http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_ime_ui_api.cc:117: chromeos::NotifyPageUp(ui_status_connection_); CursorUp, CursorDown, PageUp and PageDown are not used by any current ChromeOS IMEs, and won't be supported in the extension API. It might make sense to leave them out. http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2371: "namespace": "experimental.ime.ui", I think experiemental.input is a better place. imeui is a little hard to read, how about input.candidates or candidateList? On 2011/04/15 21:16:11, Peng wrote: > I use name space experimental.ime.ui temporarily. I am not sure what's a good > name space for those APIs. How about experimental.input.imeui?
Peng, could you let me know the typical use case of the extension API? 1) you'll implement a candidate window for Touch in HTML/CSS/JS and all VKs (including 3rd party ones) will use your candidate window? or 2) each VK can show its own candidate window? I'm wondering if we can allow IME/VK extension developers to develop their own candidate window UIs as well. http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_ime_ui_api.cc (right): http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_ime_ui_api.cc:39: // libcros. |input_method_library| is a void pointer to this object. It might be better to rename input_method_library to ui_controller or something? I think we don't have to use the same variable name as src/chrome/browser/chromeos/cros/input_method_library.cc. http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_ime_ui_api.h (right): http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_ime_ui_api.h:68: class ImeUiFunction : public AsyncExtensionFunction { The ImeUiFunction class looks exactly the same as SyncExtensionFunction in src/chrome/browser/extensions/extension_function.h. You might want to use the class instead.
http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_ime_ui_api.cc (right): http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_ime_ui_api.cc:39: // libcros. |input_method_library| is a void pointer to this object. On 2011/04/18 08:21:29, Yusuke Sato wrote: > It might be better to rename input_method_library to ui_controller or something? > I think we don't have to use the same variable name as > src/chrome/browser/chromeos/cros/input_method_library.cc. Done. http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_ime_ui_api.cc:117: chromeos::NotifyPageUp(ui_status_connection_); On 2011/04/18 03:52:29, Zachary Kuznia wrote: > CursorUp, CursorDown, PageUp and PageDown are not used by any current ChromeOS > IMEs, and won't be supported in the extension API. It might make sense to leave > them out. I think it is necessary for touch devices. Because VKs may not have allow and page down up keys, and users can not use them to move cursor in candidate windows. Or we have to find out a new way to do it. http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_ime_ui_api.h (right): http://codereview.chromium.org/6869024/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_ime_ui_api.h:68: class ImeUiFunction : public AsyncExtensionFunction { On 2011/04/18 08:21:29, Yusuke Sato wrote: > The ImeUiFunction class looks exactly the same as SyncExtensionFunction in > src/chrome/browser/extensions/extension_function.h. You might want to use the > class instead. Done. http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2371: "namespace": "experimental.ime.ui", I think we need to find a good way to organize IME APIs. In ibus, methods are bound with IBusBus, IBusPanelService, IBusEngine and etc classes. In IME extension API, I think it is better to separate them to several name spaces too. It will be more developer friendly. Why I use ime.ui is because I may add some APIs for switching IME, and some focus related events in this namespace later. So the name candidates is too limited. How about input.ime.engine for Engine related API, and input.ime.ui for UI related API? On 2011/04/18 03:52:29, Zachary Kuznia wrote: > I think experiemental.input is a better place. imeui is a little hard to read, > how about input.candidates or candidateList? > > > On 2011/04/15 21:16:11, Peng wrote: > > I use name space experimental.ime.ui temporarily. I am not sure what's a good > > name space for those APIs. How about experimental.input.imeui? >
On Mon, Apr 18, 2011 at 10:35 AM, <penghuang@chromium.org> wrote: http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... > chrome/common/extensions/api/extension_api.json:2371: "namespace": > "experimental.ime.ui", > I think we need to find a good way to organize IME APIs. In ibus, > methods are bound with IBusBus, IBusPanelService, IBusEngine and etc > classes. In IME extension API, I think it is better to separate them to > several name spaces too. It will be more developer friendly. > > Why I use ime.ui is because I may add some APIs for switching IME, and > some focus related events in this namespace later. So the name > candidates is too limited. > > How about input.ime.engine for Engine related API, and input.ime.ui for > UI related API? Why do we need the additional "ime" in there? Or, to ask it another way, do you see APIs under "input.<somethingelse>"? What is the <somethingelse>? Bryan
On 2011/04/18 15:46:08, bryeung wrote: > On Mon, Apr 18, 2011 at 10:35 AM, <mailto:penghuang@chromium.org> wrote: > http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... > > chrome/common/extensions/api/extension_api.json:2371: "namespace": > > "experimental.ime.ui", > > I think we need to find a good way to organize IME APIs. In ibus, > > methods are bound with IBusBus, IBusPanelService, IBusEngine and etc > > classes. In IME extension API, I think it is better to separate them to > > several name spaces too. It will be more developer friendly. > > > > Why I use ime.ui is because I may add some APIs for switching IME, and > > some focus related events in this namespace later. So the name > > candidates is too limited. > > > > How about input.ime.engine for Engine related API, and input.ime.ui for > > UI related API? > > Why do we need the additional "ime" in there? > > Or, to ask it another way, do you see APIs under > "input.<somethingelse>"? What is the <somethingelse>? > > Bryan I think only input.ui and input.engine may confuse developers. input is too general, and my origin idea is input.imeui.
On Mon, Apr 18, 2011 at 11:57 AM, <penghuang@chromium.org> wrote: > On 2011/04/18 15:46:08, bryeung wrote: >> >> On Mon, Apr 18, 2011 at 10:35 AM, <mailto:penghuang@chromium.org> wrote: > > http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... >> >> > chrome/common/extensions/api/extension_api.json:2371: "namespace": >> > "experimental.ime.ui", >> > I think we need to find a good way to organize IME APIs. In ibus, >> > methods are bound with IBusBus, IBusPanelService, IBusEngine and etc >> > classes. In IME extension API, I think it is better to separate them to >> > several name spaces too. It will be more developer friendly. >> > >> > Why I use ime.ui is because I may add some APIs for switching IME, and >> > some focus related events in this namespace later. So the name >> > candidates is too limited. >> > >> > How about input.ime.engine for Engine related API, and input.ime.ui for >> > UI related API? > >> Why do we need the additional "ime" in there? > >> Or, to ask it another way, do you see APIs under >> "input.<somethingelse>"? What is the <somethingelse>? > >> Bryan > > I think only input.ui and input.engine may confuse developers. input is too > general, and my origin idea is input.imeui. > I discussed this with Peng offline last night. I think we could do something like ime.ui and ime.engine. I just want to avoid the double layer of input.ime. In the end, we'll have to get the name reviewed and approved by the extensions tech lead. Bryan
On 2011/04/19 14:49:09, bryeung wrote: > On Mon, Apr 18, 2011 at 11:57 AM, <mailto:penghuang@chromium.org> wrote: > > On 2011/04/18 15:46:08, bryeung wrote: > >> > >> On Mon, Apr 18, 2011 at 10:35 AM, <mailto:penghuang@chromium.org> wrote: > > > > > http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... > >> > >> > chrome/common/extensions/api/extension_api.json:2371: "namespace": > >> > "experimental.ime.ui", > >> > I think we need to find a good way to organize IME APIs. In ibus, > >> > methods are bound with IBusBus, IBusPanelService, IBusEngine and etc > >> > classes. In IME extension API, I think it is better to separate them to > >> > several name spaces too. It will be more developer friendly. > >> > > >> > Why I use ime.ui is because I may add some APIs for switching IME, and > >> > some focus related events in this namespace later. So the name > >> > candidates is too limited. > >> > > >> > How about input.ime.engine for Engine related API, and input.ime.ui for > >> > UI related API? > > > >> Why do we need the additional "ime" in there? > > > >> Or, to ask it another way, do you see APIs under > >> "input.<somethingelse>"? What is the <somethingelse>? > > > >> Bryan > > > > I think only input.ui and input.engine may confuse developers. input is too > > general, and my origin idea is input.imeui. > > > > I discussed this with Peng offline last night. > > I think we could do something like ime.ui and ime.engine. I just want > to avoid the double layer of input.ime. > > In the end, we'll have to get the name reviewed and approved by the > extensions tech lead. > > Bryan Zach, Do you think ime.ui is OK? We could use it temporarily, and let extension leaders review the namespace later.
ime.ui makes sense, but I worry about separating ime and virtual keyboard APIs. Since there are a few functions they should share, it seems a little kludgey to have them in separate namespaces. I would personally prefer to err on the side of excessive nested namespaces than separating related functions. On Thu, Apr 21, 2011 at 5:07 AM, <penghuang@chromium.org> wrote: > On 2011/04/19 14:49:09, bryeung wrote: > >> On Mon, Apr 18, 2011 at 11:57 AM, <mailto:penghuang@chromium.org> wrote: >> > On 2011/04/18 15:46:08, bryeung wrote: >> >> >> >> On Mon, Apr 18, 2011 at 10:35 AM, <mailto:penghuang@chromium.org >> > >> > wrote: > >> > >> > >> > > > http://codereview.chromium.org/6869024/diff/4001/chrome/common/extensions/api... > >> >> >> >> > chrome/common/extensions/api/extension_api.json:2371: "namespace": >> >> > "experimental.ime.ui", >> >> > I think we need to find a good way to organize IME APIs. In ibus, >> >> > methods are bound with IBusBus, IBusPanelService, IBusEngine and etc >> >> > classes. In IME extension API, I think it is better to separate them >> to >> >> > several name spaces too. It will be more developer friendly. >> >> > >> >> > Why I use ime.ui is because I may add some APIs for switching IME, >> and >> >> > some focus related events in this namespace later. So the name >> >> > candidates is too limited. >> >> > >> >> > How about input.ime.engine for Engine related API, and input.ime.ui >> for >> >> > UI related API? >> > >> >> Why do we need the additional "ime" in there? >> > >> >> Or, to ask it another way, do you see APIs under >> >> "input.<somethingelse>"? What is the <somethingelse>? >> >> > >> >> Bryan >> > >> > I think only input.ui and input.engine may confuse developers. input is >> too >> > general, and my origin idea is input.imeui. >> > >> > > I discussed this with Peng offline last night. >> > > I think we could do something like ime.ui and ime.engine. I just want >> to avoid the double layer of input.ime. >> > > In the end, we'll have to get the name reviewed and approved by the >> extensions tech lead. >> > > Bryan >> > > Zach, Do you think ime.ui is OK? We could use it temporarily, and let > extension > leaders review the namespace later. > > > > > > http://codereview.chromium.org/6869024/ >
On Wed, Apr 20, 2011 at 11:36 PM, Zach Kuznia <zork@google.com> wrote: > ime.ui makes sense, but I worry about separating ime and virtual keyboard > APIs. Since there are a few functions they should share, it seems a > little kludgey to have them in separate namespaces. I would personally > prefer to err on the side of excessive nested namespaces than separating > related functions. Zach: I'm a bit confused by these comments. I was expecting to see ime.ui and ime.engine as the two namespaces. The API you're working on I was imagining underneath ime.engine, which would mean that the virtual keyboard API was moved there (since it is part of your API spec). Bryan
Oh, I misunderstood; I was under the impression that we would still have the chrome.input namespace. In that case, this sounds good to me! On Fri, Apr 22, 2011 at 1:57 AM, Bryan Yeung <bryeung@chromium.org> wrote: > On Wed, Apr 20, 2011 at 11:36 PM, Zach Kuznia <zork@google.com> wrote: > > ime.ui makes sense, but I worry about separating ime and virtual keyboard > > APIs. Since there are a few functions they should share, it seems a > > little kludgey to have them in separate namespaces. I would personally > > prefer to err on the side of excessive nested namespaces than separating > > related functions. > > Zach: I'm a bit confused by these comments. > > I was expecting to see ime.ui and ime.engine as the two namespaces. > The API you're working on I was imagining underneath ime.engine, which > would mean that the virtual keyboard API was moved there (since it is > part of your API spec). > > Bryan >
I think we could keep chrome.input for VK currently. And then we might consider how to organize namespaces for input and IME later. Do you think it is OK? Thanks. On 2011/04/22 01:07:36, zork wrote: > Oh, I misunderstood; I was under the impression that we would still have the > chrome.input namespace. In that case, this sounds good to me! > > On Fri, Apr 22, 2011 at 1:57 AM, Bryan Yeung <mailto:bryeung@chromium.org> wrote: > > > On Wed, Apr 20, 2011 at 11:36 PM, Zach Kuznia <mailto:zork@google.com> wrote: > > > ime.ui makes sense, but I worry about separating ime and virtual keyboard > > > APIs. Since there are a few functions they should share, it seems a > > > little kludgey to have them in separate namespaces. I would personally > > > prefer to err on the side of excessive nested namespaces than separating > > > related functions. > > > > Zach: I'm a bit confused by these comments. > > > > I was expecting to see ime.ui and ime.engine as the two namespaces. > > The API you're working on I was imagining underneath ime.engine, which > > would mean that the virtual keyboard API was moved there (since it is > > part of your API spec). > > > > Bryan > >
LGTM http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_input_ui_api.cc (right): http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/extension_input_ui_api.cc:37: private: nit: one vertical space between line 36 and 37, please. http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_input_ui_api.h (right): http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/extension_input_ui_api.h:39: virtual ~ExtensionInputUiEventRouter(); nit: do we have any reason to make the destructor virtual? http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/extension_input_ui_api.h:62: InputUiController* ui_controller_; scoped_ptr<InputUiController> would be better? http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:90: #if defined(TOUCH_UI) #if defined(OS_CHROMEOS) && defined(TOUCH_UI) might be better?
LGTM This API has some conflicts with the IME extension api that's currently in review. I don't think that should prevent you from committing, but we will need to revisit this later this quarter to make certain that the APIs match before leaving experimental.* Cheers, -Zach On Mon, May 2, 2011 at 4:33 PM, <yusukes@chromium.org> wrote: > LGTM > > > > http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... > File chrome/browser/extensions/extension_input_ui_api.cc (right): > > > http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... > chrome/browser/extensions/extension_input_ui_api.cc:37: private: > nit: one vertical space between line 36 and 37, please. > > > http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... > File chrome/browser/extensions/extension_input_ui_api.h (right): > > > http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... > chrome/browser/extensions/extension_input_ui_api.h:39: virtual > ~ExtensionInputUiEventRouter(); > nit: do we have any reason to make the destructor virtual? > > > http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... > chrome/browser/extensions/extension_input_ui_api.h:62: > InputUiController* ui_controller_; > scoped_ptr<InputUiController> would be better? > > > http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... > File chrome/browser/extensions/extension_service.cc (right): > > > http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... > chrome/browser/extensions/extension_service.cc:90: #if defined(TOUCH_UI) > #if defined(OS_CHROMEOS) && defined(TOUCH_UI) might be better? > > > http://codereview.chromium.org/6869024/ >
http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_input_ui_api.cc (right): http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/extension_input_ui_api.cc:37: private: On 2011/05/02 07:33:31, Yusuke Sato wrote: > nit: one vertical space between line 36 and 37, please. Done. http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_input_ui_api.h (right): http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/extension_input_ui_api.h:39: virtual ~ExtensionInputUiEventRouter(); On 2011/05/02 07:33:31, Yusuke Sato wrote: > nit: do we have any reason to make the destructor virtual? Done. http://codereview.chromium.org/6869024/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/extension_input_ui_api.h:62: InputUiController* ui_controller_; On 2011/05/02 07:33:31, Yusuke Sato wrote: > scoped_ptr<InputUiController> would be better? Done.
Presubmit check for 6869024-19009 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/common/extensions/api/extension_api.json, line 2433, 99 chars \ chrome/common/extensions/api/extension_api.json, line 2438, 93 chars \ chrome/common/extensions/api/extension_api.json, line 2446, 82 chars \ chrome/common/extensions/api/extension_api.json, line 2457, 93 chars \ chrome/common/extensions/api/extension_api.json, line 2465, 83 chars This change modifies file(s) which the extension docs depend on. You must rebuild the extension docs. Build by running the build.py script in chrome/common/extensions/docs/build/. Be sure to include any modified resulting static files (/common/extension/docs/*.html) in your final changelist.
Presubmit check for 6869024-19009 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/common/extensions/api/extension_api.json, line 2433, 99 chars \ chrome/common/extensions/api/extension_api.json, line 2438, 93 chars \ chrome/common/extensions/api/extension_api.json, line 2446, 82 chars \ chrome/common/extensions/api/extension_api.json, line 2457, 93 chars \ chrome/common/extensions/api/extension_api.json, line 2465, 83 chars This change modifies file(s) which the extension docs depend on. You must rebuild the extension docs. Build by running the build.py script in chrome/common/extensions/docs/build/. Be sure to include any modified resulting static files (/common/extension/docs/*.html) in your final changelist.
I saw many lines are longer than 80 characters in json file. So could we ignore those warnings? On 2011/05/02 18:20:16, commit-bot wrote: > Presubmit check for 6869024-19009 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit Warnings ** > Found lines longer than 80 characters (first 5 shown). > chrome/common/extensions/api/extension_api.json, line 2433, 99 chars \ > chrome/common/extensions/api/extension_api.json, line 2438, 93 chars \ > chrome/common/extensions/api/extension_api.json, line 2446, 82 chars \ > chrome/common/extensions/api/extension_api.json, line 2457, 93 chars \ > chrome/common/extensions/api/extension_api.json, line 2465, 83 chars > > > This change modifies file(s) which the extension docs depend on. You must > rebuild the extension docs. > > Build by running the build.py script in chrome/common/extensions/docs/build/. > > Be sure to include any modified resulting static files > (/common/extension/docs/*.html) in your final changelist.
Drive-by since I was watching the tree and noticed the ExtensionApiTest.Stubs failure that you got on committing this. Before you re-land please make sure to do 2 things: 1) Fix the ExtensionApiTest.Stubs error. See comments in stubs_apitest.cc about how to do this (basically you need to annotate whether your API is available in content scripts or not). 2) Make sure to run the extension docs generation script - that is the error you see above about "You must rebuild the extension docs." It probably makes sense for you to add someone from the extensions team to help review this (and probably any future changes to the extensions system). I'd be happy to do so, as I'm sure mpcomplete, mihaip, skerner, finnur, etc. would be.
Thanks for your advice. For api docs, I executed build.py. But the changes from
this scripts are not related to this CL. So I did not include it. Is it OK?
Below is the diff in docs:
diff --git a/chrome/common/extensions/docs/experimental.webRequest.html
b/chrome/common/extensions/docs/experimental.webRequest.html
index 5737193..d88b064 100644
--- a/chrome/common/extensions/docs/experimental.webRequest.html
+++ b/chrome/common/extensions/docs/experimental.webRequest.html
@@ -1050,7 +1050,7 @@ unexpected results.
array of <span><span></span></span>
</span>
<span>string</span>
- <span>["statusLine", "responseHeaders",
"redirectRequestLine", "redirectRequestHeaders"]</span>
+ <span>["statusLine", "responseHeaders"]</span>
</span>
</span></span>
</span>
On 2011/05/04 20:29:58, Antony Sargent wrote:
> Drive-by since I was watching the tree and noticed the ExtensionApiTest.Stubs
> failure that you got on committing this.
>
> Before you re-land please make sure to do 2 things:
>
> 1) Fix the ExtensionApiTest.Stubs error. See comments in stubs_apitest.cc
about
> how to do this (basically you need to annotate whether your API is available
in
> content scripts or not).
>
> 2) Make sure to run the extension docs generation script - that is the error
you
> see above about "You must rebuild the extension docs."
>
> It probably makes sense for you to add someone from the extensions team to
help
> review this (and probably any future changes to the extensions system). I'd be
> happy to do so, as I'm sure mpcomplete, mihaip, skerner, finnur, etc. would
be.
That's fine to leave that file out of your CL - probably someone else forgot to run the doc build script on their change. I'll track it down and fix in a separate CL. On Wed, May 4, 2011 at 2:32 PM, <penghuang@chromium.org> wrote: > Thanks for your advice. For api docs, I executed build.py. But the changes > from > this scripts are not related to this CL. So I did not include it. Is it OK? > > Below is the diff in docs: > diff --git a/chrome/common/extensions/docs/experimental.webRequest.html > b/chrome/common/extensions/docs/experimental.webRequest.html > index 5737193..d88b064 100644 > --- a/chrome/common/extensions/docs/experimental.webRequest.html > +++ b/chrome/common/extensions/docs/experimental.webRequest.html > @@ -1050,7 +1050,7 @@ unexpected results. > array of <span><span></span></span> > </span> > <span>string</span> > - <span>["statusLine", "responseHeaders", > "redirectRequestLine", "redirectRequestHeaders"]</span> > + <span>["statusLine", "responseHeaders"]</span> > </span> > </span></span> > </span> > > > > > > On 2011/05/04 20:29:58, Antony Sargent wrote: > >> Drive-by since I was watching the tree and noticed the >> ExtensionApiTest.Stubs >> failure that you got on committing this. >> > > Before you re-land please make sure to do 2 things: >> > > 1) Fix the ExtensionApiTest.Stubs error. See comments in stubs_apitest.cc >> > about > >> how to do this (basically you need to annotate whether your API is >> available >> > in > >> content scripts or not). >> > > 2) Make sure to run the extension docs generation script - that is the >> error >> > you > >> see above about "You must rebuild the extension docs." >> > > It probably makes sense for you to add someone from the extensions team to >> > help > >> review this (and probably any future changes to the extensions system). >> I'd be >> happy to do so, as I'm sure mpcomplete, mihaip, skerner, finnur, etc. >> would >> > be. > > > > http://codereview.chromium.org/6869024/ >
I updated the CL which should fix the build errors. Who can help me to run the trybot again? Thanks. http://codereview.chromium.org/6869024/diff/27010/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/6869024/diff/27010/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:3870: ['include', '^browser/extensions/'], ['include', '^browser/extensions/'] overrides previous exclude. :(
On 2011/05/04 20:29:58, Antony Sargent wrote: > Drive-by since I was watching the tree and noticed the ExtensionApiTest.Stubs > failure that you got on committing this. > > Before you re-land please make sure to do 2 things: > > 1) Fix the ExtensionApiTest.Stubs error. See comments in stubs_apitest.cc about > how to do this (basically you need to annotate whether your API is available in > content scripts or not). I did a little research and found the reason is because namespace experimental.input.ui conflicts with experimental.input. chrome/renderer/resources/renderer_extension_bindings.js can not support two namespaces like a.b and a.b.c at same time. After offline discussion with Antoy, we agree to rename input.ui to inputUI. When IME extension API leave experimental, we will discuss and maybe re-organize it again. > > 2) Make sure to run the extension docs generation script - that is the error you > see above about "You must rebuild the extension docs." > > It probably makes sense for you to add someone from the extensions team to help > review this (and probably any future changes to the extensions system). I'd be > happy to do so, as I'm sure mpcomplete, mihaip, skerner, finnur, etc. would be.
This is pretty different from the IME API discussed at http://dev.chromium.org/developers/design-documents/extensions/input-method-e.... Were there design changes, or is this something else? http://codereview.chromium.org/6869024/diff/27010/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6869024/diff/27010/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:2465: "description": "Notify input method engine cursor up buttion was clicked.", Typo (buttion), shows up a bunch of times.
On 2011/05/05 19:42:13, Mihai Parparita wrote: > This is pretty different from the IME API discussed at > http://dev.chromium.org/developers/design-documents/extensions/input-method-e.... > Were there design changes, or is this something else? That is different. chrome.ime.engine is for developing input method engines. APIs in this CL is for developing input method UI (front-end). > > http://codereview.chromium.org/6869024/diff/27010/chrome/common/extensions/ap... > File chrome/common/extensions/api/extension_api.json (right): > > http://codereview.chromium.org/6869024/diff/27010/chrome/common/extensions/ap... > chrome/common/extensions/api/extension_api.json:2465: "description": "Notify > input method engine cursor up buttion was clicked.", > Typo (buttion), shows up a bunch of times.
http://codereview.chromium.org/6869024/diff/27010/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6869024/diff/27010/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:2465: "description": "Notify input method engine cursor up buttion was clicked.", On 2011/05/05 19:42:13, Mihai Parparita wrote: > Typo (buttion), shows up a bunch of times. Done.
http://codereview.chromium.org/6869024/diff/23006/chrome/browser/extensions/e... File chrome/browser/extensions/extension_input_ui_api.cc (right): http://codereview.chromium.org/6869024/diff/23006/chrome/browser/extensions/e... chrome/browser/extensions/extension_input_ui_api.cc:185: extension_id_ = extension_id; So it looks like this makes it so that only one extension at a time can use this API, and it's essentially a race if more than one extension tries to use it (whichever extension happens to call inputUI.register last wins). In general we usually like to make extension APIs that more than one extension at a time can use, although it may be for your case that it really makes sense that only one extension should be able to. You'll eventually need some strategy for dealing with the problem in the code here. One approach would be to enforce at load/install time that there isn't already another extension loaded/installed using the inputUI permission. No need to block this CL, but probably a good idea to file a bug so you don't forget to deal with it before the API becomes officially supported. http://codereview.chromium.org/6869024/diff/23006/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6869024/diff/23006/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:2427: "nodoc": true, FYI, while we generally haven't *required* people to generate docs for APIs that are just being implemented in experimental, it can be very helpful to outside developers attempting to use the API (and we usually like to try and get outside developer input on our experimental APIs). See this page for a list of other experimental APIs' documentation in the current dev channel: http://code.google.com/chrome/extensions/dev/experimental.html You can also look at the trunk version here: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs... To create your own docs for this API, remove the "nodoc" line here, and then create a template file at: chrome/common/extensions/docs/static/experimental.inputUI.html You can look at other files there to get a sense for the format. Then run the chrome/common/extensions/docs/build/build.py script and the build script will generate chrome/common/extensions/docs/experimental.inputUI.html for you, as well as link to it inside chrome/common/extensions/docs/experimental.html. Anyhow, no need to hold up this CL, but I'd encourage you to do a follow-up CL adding docs. http://codereview.chromium.org/6869024/diff/23006/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:2433: "description": "Register the extension, so the extension can receive the onUpdated event.", nit: I do not see any event named onUpdated. Maybe you meant onSetCursorLocation/onUpdateAuxiliaryText/onUpdateLookupTable ?
http://codereview.chromium.org/6869024/diff/23006/chrome/browser/extensions/e... File chrome/browser/extensions/extension_input_ui_api.cc (right): http://codereview.chromium.org/6869024/diff/23006/chrome/browser/extensions/e... chrome/browser/extensions/extension_input_ui_api.cc:185: extension_id_ = extension_id; On 2011/05/05 23:50:02, Antony Sargent wrote: > So it looks like this makes it so that only one extension at a time can use this > API, and it's essentially a race if more than one extension tries to use it > (whichever extension happens to call inputUI.register last wins). > > In general we usually like to make extension APIs that more than one extension > at a time can use, although it may be for your case that it really makes sense > that only one extension should be able to. > > You'll eventually need some strategy for dealing with the problem in the code > here. One approach would be to enforce at load/install time that there isn't > already another extension loaded/installed using the inputUI permission. > > No need to block this CL, but probably a good idea to file a bug so you don't > forget to deal with it before the API becomes officially supported. > Actually, we can only have one activated input method UI at a moment, and we need switch input method UI on the fly. I am not sure what is the better way for it. I will file a bug for this. http://codereview.chromium.org/6869024/diff/23006/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6869024/diff/23006/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:2427: "nodoc": true, Currently, this API is only used internally and only exists when touchui==1 and chromeos==1. We do not want to public it right now. If we decide to allow external developer develop input method ui extension in future, I will do it later. I added the template file, but did not remove nodoc. On 2011/05/05 23:50:02, Antony Sargent wrote: > FYI, while we generally haven't *required* people to generate docs for APIs that > are just being implemented in experimental, it can be very helpful to outside > developers attempting to use the API (and we usually like to try and get outside > developer input on our experimental APIs). > > See this page for a list of other experimental APIs' documentation in the > current dev channel: > > http://code.google.com/chrome/extensions/dev/experimental.html > > You can also look at the trunk version here: > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs... > > To create your own docs for this API, remove the "nodoc" line here, and then > create a template file at: > > chrome/common/extensions/docs/static/experimental.inputUI.html > > You can look at other files there to get a sense for the format. Then run the > chrome/common/extensions/docs/build/build.py script and the build script will > generate chrome/common/extensions/docs/experimental.inputUI.html for you, as > well as link to it inside chrome/common/extensions/docs/experimental.html. > > Anyhow, no need to hold up this CL, but I'd encourage you to do a follow-up CL > adding docs. http://codereview.chromium.org/6869024/diff/23006/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:2433: "description": "Register the extension, so the extension can receive the onUpdated event.", On 2011/05/05 23:50:02, Antony Sargent wrote: > nit: I do not see any event named onUpdated. Maybe you meant > onSetCursorLocation/onUpdateAuxiliaryText/onUpdateLookupTable ? Done.
ok, LGTM
Presubmit check for 6869024-24009 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/common/extensions/api/extension_api.json, line 2433, 107 chars \ chrome/common/extensions/api/extension_api.json, line 2438, 93 chars \ chrome/common/extensions/api/extension_api.json, line 2446, 82 chars \ chrome/common/extensions/api/extension_api.json, line 2457, 93 chars \ chrome/common/extensions/api/extension_api.json, line 2465, 82 chars This change modifies file(s) which the extension docs depend on. You must rebuild the extension docs. Build by running the build.py script in chrome/common/extensions/docs/build/. Be sure to include any modified resulting static files (/common/extension/docs/*.html) in your final changelist.
Just FYI, you'll need to commit this manually instead of via the commit-queue, because of crbug.com/81826. I will also file a bug about the presubmit check looking at .json files for 80-character width - since these often contain documentation and not code I don't believe it's appropriate to do that check on them. On Mon, May 9, 2011 at 10:38 AM, <commit-bot@chromium.org> wrote: > Presubmit check for 6869024-24009 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit Warnings ** > Found lines longer than 80 characters (first 5 shown). > chrome/common/extensions/api/extension_api.json, line 2433, 107 chars \ > chrome/common/extensions/api/extension_api.json, line 2438, 93 chars \ > chrome/common/extensions/api/extension_api.json, line 2446, 82 chars \ > chrome/common/extensions/api/extension_api.json, line 2457, 93 chars \ > chrome/common/extensions/api/extension_api.json, line 2465, 82 chars > > > This change modifies file(s) which the extension docs depend on. You must > rebuild the extension docs. > > Build by running the build.py script in > chrome/common/extensions/docs/build/. > > Be sure to include any modified resulting static files > (/common/extension/docs/*.html) in your final changelist. > > > > > > http://codereview.chromium.org/6869024/ >
Thanks for the information. Bryan landed it for me. On 2011/05/09 17:45:11, Antony Sargent wrote: > Just FYI, you'll need to commit this manually instead of via the > commit-queue, because of crbug.com/81826. > > I will also file a bug about the presubmit check looking at .json files for > 80-character width - since these often contain documentation and not code I > don't believe it's appropriate to do that check on them. > > > On Mon, May 9, 2011 at 10:38 AM, <mailto:commit-bot@chromium.org> wrote: > > > Presubmit check for 6869024-24009 failed and returned exit status 1. > > > > Running presubmit commit checks ... > > > > ** Presubmit Warnings ** > > Found lines longer than 80 characters (first 5 shown). > > chrome/common/extensions/api/extension_api.json, line 2433, 107 chars \ > > chrome/common/extensions/api/extension_api.json, line 2438, 93 chars \ > > chrome/common/extensions/api/extension_api.json, line 2446, 82 chars \ > > chrome/common/extensions/api/extension_api.json, line 2457, 93 chars \ > > chrome/common/extensions/api/extension_api.json, line 2465, 82 chars > > > > > > This change modifies file(s) which the extension docs depend on. You must > > rebuild the extension docs. > > > > Build by running the build.py script in > > chrome/common/extensions/docs/build/. > > > > Be sure to include any modified resulting static files > > (/common/extension/docs/*.html) in your final changelist. > > > > > > > > > > > > http://codereview.chromium.org/6869024/ > > |
