Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2077783002:
Make limitations for input.ime.sendKeyEvents API. (Closed)
DescriptionMake limitations for input.ime.sendKeyEvents API.
1) The API works only on text fields
2) ENTER et al. works only on http:, https: etc.
3) onKeyEvent no work in password field.
BUG=517773
TEST=Verified on local build.
Committed: https://crrev.com/06cf902e7ac4cc27399399219ad6d37042719752
Cr-Commit-Position: refs/heads/master@{#403250}
Patch Set 1 #Patch Set 2 : Make limitations for input.ime.sendKeyEvents API. #
Total comments: 13
Patch Set 3 #Patch Set 4 : Blacklist extension window. #
Total comments: 17
Patch Set 5 : Fix testing memory leak. #Patch Set 6 : Check lastError of sendKeyEvents. #
Total comments: 20
Patch Set 7 : Addressed Devlin's comments. #
Total comments: 15
Patch Set 8 : Addressed comments. #Patch Set 9 #
Total comments: 6
Patch Set 10 #Patch Set 11 #Patch Set 12 : onKeyEvent should not work in password. #
Total comments: 2
Patch Set 13 : Use find than count. #Patch Set 14 #Patch Set 15 : Fix chromeos error. #Messages
Total messages: 57 (18 generated)
Description was changed from ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. doesn't work on chrome:// pages. BUT=517773 TEST=Verified on local build. ========== to ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. doesn't work on chrome:// pages. BUT=517773 TEST=Verified on local build. ==========
azurewei@chromium.org changed reviewers: + shuchen@chromium.org
Please review this cl. Thanks!
azurewei@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
R: +rdevlin.cronin@. Devlin, can you help review this cl? Thanks!
Note that I don't own ui/input_method, so you'll probably still want Shu to review this. Also, this strikes me as something that would be good to test. https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:203: Browser* browser = chrome::FindLastActive(); I'm a little dubious of this - might it be possible that the last active browser (or active web contents) changed since the input was sent? https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:210: GURL url = web_contents->GetController().GetActiveEntry()->GetURL(); GetActiveEntry() is deprecated, and can also return null. I think this would probably be better as web_contents->GetLastCommittedURL(). https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:220: bool InputMethodEngine::IsValidKeyForSpecialPage(const std::string& url, why not pass as a gurl? https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:226: pages.push_back(std::string("chrome://")); use the scheme constant. https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:226: pages.push_back(std::string("chrome://")); What about e.g. chrome-devtools? https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:234: if (url.substr(0, page.size()) == page) { prefer GURL::SchemeIs()
meacer@chromium.org changed reviewers: + meacer@chromium.org
https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:226: pages.push_back(std::string("chrome://")); On 2016/06/17 16:22:43, Devlin wrote: > use the scheme constant. Drive-by: I'd rather use a whitelist here than a blacklist. Adding http and https to the allowed schemes list is a good start.
https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:231: keycodes.push_back(ui::VKEY_RETURN); I suggested these keys in the thread but I'm not sure if they are the only keys that we should restrict. I'm a bit concerned about the scenario where the extension triggers a shortcut that opens a page or somehow clicks on a confirmation dialog. Can we be more restrictive here and disable more keys? We can loosen up the requirements later on, but it's safer to start with a smaller set. Or even better, can we whitelist allowed keys? Is there any reason the IME API should send a, say, F2 key?
On 2016/06/21 01:06:37, Mustafa Emre Acer wrote: > https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... > File chrome/browser/ui/input_method/input_method_engine.cc (right): > > https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... > chrome/browser/ui/input_method/input_method_engine.cc:226: > pages.push_back(std::string("chrome://")); > On 2016/06/17 16:22:43, Devlin wrote: > > use the scheme constant. > > Drive-by: I'd rather use a whitelist here than a blacklist. Adding http and > https to the allowed schemes list is a good start. Except for Http/Https schemes, the key events should be able to work under some other situations, like null new tab/about blank/file/data/ftp/chrome-extension/chrome dev tools/... Should all these schemes be in the white list?
On 2016/06/21 01:13:10, Mustafa Emre Acer wrote: > https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... > File chrome/browser/ui/input_method/input_method_engine.cc (right): > > https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... > chrome/browser/ui/input_method/input_method_engine.cc:231: > keycodes.push_back(ui::VKEY_RETURN); > I suggested these keys in the thread but I'm not sure if they are the only keys > that we should restrict. I'm a bit concerned about the scenario where the > extension triggers a shortcut that opens a page or somehow clicks on a > confirmation dialog. > > Can we be more restrictive here and disable more keys? We can loosen up the > requirements later on, but it's safer to start with a smaller set. > > Or even better, can we whitelist allowed keys? Is there any reason the IME API > should send a, say, F2 key? How about whitelisting all the character keys (with no Ctrl modifier), backspace key and arrow keys(left, right, etc.) ?
> Except for Http/Https schemes, the key events should be able to work under some > other situations, like null new tab/about > blank/file/data/ftp/chrome-extension/chrome dev tools/... Should all these > schemes be in the white list? about:blank and ftp are certainly fine, but why do we need the others? chrome-devtools and chrome-extension are particularly dangerous, and I don't see a good reason to use IME on a developer facing and mostly English page. > How about whitelisting all the character keys (with no Ctrl modifier), backspace > key and arrow keys(left, right, etc.) ? SGTM!
On 2016/06/21 04:44:07, Mustafa Emre Acer wrote: > > Except for Http/Https schemes, the key events should be able to work under > some > > other situations, like null new tab/about > > blank/file/data/ftp/chrome-extension/chrome dev tools/... Should all these > > schemes be in the white list? > > about:blank and ftp are certainly fine, but why do we need the others? > chrome-devtools and chrome-extension are particularly dangerous, and I don't see > a good reason to use IME on a developer facing and mostly English page. > Should the IME extensions be able to work on extension windows (AppWindow), such as hangouts? There is a thorny problem on the AppWindow that we can't get the url of the AppWindow, which is supposed to return true with SchemeIs(kExtensionScheme), from last active browser. The AppWindow has not been set as last active browser view or web contents. While, the input method changes to current focused window no matter whether is active or not. As a result, we get the last non-AppWindow url, and the key events are still sent to the AppWindows. The behavior of the special keys on AppWindow become unpredictable. I don't find a good way to solve this problem. But we can bypass this by whitelist or the windows whose input method is different from the input context handler. Currently, only the AppWindow will meet this situation. WDYT?
https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:203: Browser* browser = chrome::FindLastActive(); On 2016/06/17 16:22:43, Devlin wrote: > I'm a little dubious of this - might it be possible that the last active browser > (or active web contents) changed since the input was sent? The last active browser/web content could be different from the current input method, for example AppWindow (Please also see last comment). If so, the last committed url is unreliable. I just white list this case for now. https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:210: GURL url = web_contents->GetController().GetActiveEntry()->GetURL(); On 2016/06/17 16:22:43, Devlin wrote: > GetActiveEntry() is deprecated, and can also return null. I think this would > probably be better as web_contents->GetLastCommittedURL(). Updated with GetLastCommittedURL(). Thanks. https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:220: bool InputMethodEngine::IsValidKeyForSpecialPage(const std::string& url, On 2016/06/17 16:22:43, Devlin wrote: > why not pass as a gurl? Done. https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:226: pages.push_back(std::string("chrome://")); On 2016/06/21 01:06:37, Mustafa Emre Acer wrote: > On 2016/06/17 16:22:43, Devlin wrote: > > use the scheme constant. > > Drive-by: I'd rather use a whitelist here than a blacklist. Adding http and > https to the allowed schemes list is a good start. Done. https://codereview.chromium.org/2077783002/diff/20001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:234: if (url.substr(0, page.size()) == page) { On 2016/06/17 16:22:43, Devlin wrote: > prefer GURL::SchemeIs() Done.
On 2016/06/21 08:07:47, Azure Wei wrote: > On 2016/06/21 04:44:07, Mustafa Emre Acer wrote: > > > Except for Http/Https schemes, the key events should be able to work under > > some > > > other situations, like null new tab/about > > > blank/file/data/ftp/chrome-extension/chrome dev tools/... Should all these > > > schemes be in the white list? > > > > about:blank and ftp are certainly fine, but why do we need the others? > > chrome-devtools and chrome-extension are particularly dangerous, and I don't > see > > a good reason to use IME on a developer facing and mostly English page. > > > > Should the IME extensions be able to work on extension windows (AppWindow), such > as hangouts? For now I think it should not. The initial experience will admittedly be inconsistent given extension pages look like normal web pages, but I think it's better to observe and understand abuse patterns before this API gets too powerful. > > There is a thorny problem on the AppWindow that we can't get the url of the > AppWindow, which is supposed to return true with SchemeIs(kExtensionScheme), > from last active browser. > The AppWindow has not been set as last active browser view or web contents. > While, the input method changes to current focused window no matter whether is > active or not. > > As a result, we get the last non-AppWindow url, and the key events are still > sent to the AppWindows. The behavior of the special keys on AppWindow become > unpredictable. > > I don't find a good way to solve this problem. But we can bypass this by > whitelist or the windows whose input method is different from the input context > handler. Currently, only the AppWindow will meet this situation. WDYT? I don't have a good suggestion here, but whitelisting to http, https, about:blank and ftp will solve this, right?
On 2016/06/21 22:29:46, Mustafa Emre Acer wrote: > On 2016/06/21 08:07:47, Azure Wei wrote: > > On 2016/06/21 04:44:07, Mustafa Emre Acer wrote: > > > > Except for Http/Https schemes, the key events should be able to work under > > > some > > > > other situations, like null new tab/about > > > > blank/file/data/ftp/chrome-extension/chrome dev tools/... Should all these > > > > schemes be in the white list? > > > > > > about:blank and ftp are certainly fine, but why do we need the others? > > > chrome-devtools and chrome-extension are particularly dangerous, and I don't > > see > > > a good reason to use IME on a developer facing and mostly English page. > > > > > > > Should the IME extensions be able to work on extension windows (AppWindow), > such > > as hangouts? > > For now I think it should not. The initial experience will admittedly be > inconsistent given extension pages look like normal web pages, but I think it's > better to observe and understand abuse patterns before this API gets too > powerful. OK. Moved the extension pages out from whitelist. > > > > There is a thorny problem on the AppWindow that we can't get the url of the > > AppWindow, which is supposed to return true with SchemeIs(kExtensionScheme), > > from last active browser. > > The AppWindow has not been set as last active browser view or web contents. > > While, the input method changes to current focused window no matter whether is > > active or not. > > > > As a result, we get the last non-AppWindow url, and the key events are still > > sent to the AppWindows. The behavior of the special keys on AppWindow become > > unpredictable. > > > > I don't find a good way to solve this problem. But we can bypass this by > > whitelist or the windows whose input method is different from the input > context > > handler. Currently, only the AppWindow will meet this situation. WDYT? > > I don't have a good suggestion here, but whitelisting to http, https, > about:blank and ftp will solve this, right? Yes. It won't be an issue if we don't want to whitelist the extension pages. The codes are updated.
https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:237: whitelist_schemes.push_back(url::kFtpScheme); Let's use c++11 initialization syntax here. https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:240: meacer@, do you think we can add file:// urls here? https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:241: std::vector<const char*> whitelist_urls; why not std::vector<GURL>? https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:252: if (url == GURL(whitelist_url)) This wouldn't work with e.g. fragments. I don't think we ever do something like chrome://newtab#foo, but if we did, I'd expect it to work. https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:270: !ui_event->IsCommandDown()) { What about other keyboard modifier keys? Alt, Meta/Search on CrOS.... https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:271: for (auto invalid_key : invalid_character_keycodes) { Are tab and return considered character keys? If not, then this isn't needed here, right? https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc (right): https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:74: ASSERT_EQ(key_events.size(), 2U); switch the order here: expected, actual https://codereview.chromium.org/2077783002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js (right): https://codereview.chromium.org/2077783002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js:73: if (chrome.runtime.lastError) {} Can we not tell whether or not we expect these to succeed and either assertNoLastError or check the last error is what we expect? https://codereview.chromium.org/2077783002/diff/60001/ui/base/ime/input_metho... File ui/base/ime/input_method_base.cc (right): https://codereview.chromium.org/2077783002/diff/60001/ui/base/ime/input_metho... ui/base/ime/input_method_base.cc:202: key_events_for_test.push_back(new KeyEvent(*event)); This never gets cleared in production, right? And over the course of thousands of keystrokes, that's non-trivial. Also, it looks like this will always leak memory, since these are never cleaned up.
https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:237: whitelist_schemes.push_back(url::kFtpScheme); On 2016/06/22 16:33:12, Devlin wrote: > Let's use c++11 initialization syntax here. Done. https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:241: std::vector<const char*> whitelist_urls; On 2016/06/22 16:33:12, Devlin wrote: > why not std::vector<GURL>? Done. https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:252: if (url == GURL(whitelist_url)) On 2016/06/22 16:33:12, Devlin wrote: > This wouldn't work with e.g. fragments. I don't think we ever do something like > chrome://newtab#foo, but if we did, I'd expect it to work. Done. https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:270: !ui_event->IsCommandDown()) { On 2016/06/22 16:33:12, Devlin wrote: > What about other keyboard modifier keys? Alt, Meta/Search on CrOS.... Other modifier keys like Meta/Search doesn't have flag in KeyEvent. They will be send as two key events. And the first modifier key event will not be sent. https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... chrome/browser/ui/input_method/input_method_engine.cc:271: for (auto invalid_key : invalid_character_keycodes) { On 2016/06/22 16:33:12, Devlin wrote: > Are tab and return considered character keys? If not, then this isn't needed > here, right? Yes, tab and return are also character keys. https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc (right): https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:74: ASSERT_EQ(key_events.size(), 2U); On 2016/06/22 16:33:12, Devlin wrote: > switch the order here: expected, actual Done. https://codereview.chromium.org/2077783002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js (right): https://codereview.chromium.org/2077783002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js:73: if (chrome.runtime.lastError) {} On 2016/06/22 16:33:12, Devlin wrote: > Can we not tell whether or not we expect these to succeed and either > assertNoLastError or check the last error is what we expect? Done. https://codereview.chromium.org/2077783002/diff/60001/ui/base/ime/input_metho... File ui/base/ime/input_method_base.cc (right): https://codereview.chromium.org/2077783002/diff/60001/ui/base/ime/input_metho... ui/base/ime/input_method_base.cc:202: key_events_for_test.push_back(new KeyEvent(*event)); On 2016/06/22 16:33:12, Devlin wrote: > This never gets cleared in production, right? And over the course of thousands > of keystrokes, that's non-trivial. > > Also, it looks like this will always leak memory, since these are never cleaned > up. Fixed with adding test flag in InputMethod.
https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:219: // If |input_context->GetInputMethod() != input_method|, treats |url| as Should this ever happen? If not, can we just never send the key event? https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:222: if (!url.is_empty() && !IsValidKeyForSpecialPage(url, url_trustable, event)) same question as above for url.is_empty() https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:248: if (url.spec().substr(0, whitelist_url.spec().size()) == probably compare GURL::GetOrigin() here. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:255: ui::VKEY_BACK, ui::VKEY_LEFT, ui::VKEY_RIGHT, ui::VKEY_UP, ui::VKEY_DOWN}; is this git cl formatted? https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc (right): https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:63: ASSERT_EQ(ui::VKEY_A, key_events[2]->key_code()); out of curiosity, what about key_events[1] and [3]? https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:73: BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser()); I don't think you actually need BrowserView here - you could just use browser()->window()->GetNativeWindow(). https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:80: InputImeActivateFunction::disable_bubble_for_testing_ = true; use a base::AutoReset here. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:87: ASSERT_EQ(ui::VKEY_A, key_events[0]->key_code()); what about key_events[1]? Also some of these ASSERTs should probably be EXPECTs. https://codereview.chromium.org/2077783002/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js (right): https://codereview.chromium.org/2077783002/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js:97: chrome.test.assertEq(failToSendKeyEvents, nit: indentation chrome.test.assertEq(failToSendKeyEvents, chrome.runtime.lastError.message); or if that doesn't fit chrome.test.assertEq( failToSendKeyEvents, chrome.runtime.lastError.message); https://codereview.chromium.org/2077783002/diff/100001/ui/base/ime/input_meth... File ui/base/ime/input_method_base.cc (right): https://codereview.chromium.org/2077783002/diff/100001/ui/base/ime/input_meth... ui/base/ime/input_method_base.cc:32: if (!key_events_for_testing_.empty()) { Why not just std::vector<std::unique_ptr<KeyEvent>>? Or even std::vector<KeyEvent>?
https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:219: // If |input_context->GetInputMethod() != input_method|, treats |url| as On 2016/06/23 18:43:22, Devlin wrote: > Should this ever happen? If not, can we just never send the key event? This could happen when the current top input view has not been set as last active browser, like panel AppWindow. I don't think we should not send the key events for all these cases, since the input method that will handle the events is still the right window. Not sending the key events will make the API unable to work under lots of cases like chrome Apps. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:222: if (!url.is_empty() && !IsValidKeyForSpecialPage(url, url_trustable, event)) On 2016/06/23 18:43:21, Devlin wrote: > same question as above for url.is_empty() Removed this check as it should not happen. And if it happens, it will be treated as 'special url'. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:248: if (url.spec().substr(0, whitelist_url.spec().size()) == On 2016/06/23 18:43:22, Devlin wrote: > probably compare GURL::GetOrigin() here. Done. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:255: ui::VKEY_BACK, ui::VKEY_LEFT, ui::VKEY_RIGHT, ui::VKEY_UP, ui::VKEY_DOWN}; On 2016/06/23 18:43:22, Devlin wrote: > is this git cl formatted? Yes. It's formatted. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc (right): https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:63: ASSERT_EQ(ui::VKEY_A, key_events[2]->key_code()); On 2016/06/23 18:43:22, Devlin wrote: > out of curiosity, what about key_events[1] and [3]? Added check for all events. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:73: BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser()); On 2016/06/23 18:43:22, Devlin wrote: > I don't think you actually need BrowserView here - you could just use > browser()->window()->GetNativeWindow(). Done. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:80: InputImeActivateFunction::disable_bubble_for_testing_ = true; On 2016/06/23 18:43:22, Devlin wrote: > use a base::AutoReset here. Done. https://codereview.chromium.org/2077783002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:87: ASSERT_EQ(ui::VKEY_A, key_events[0]->key_code()); On 2016/06/23 18:43:22, Devlin wrote: > what about key_events[1]? Also some of these ASSERTs should probably be > EXPECTs. Done. https://codereview.chromium.org/2077783002/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js (right): https://codereview.chromium.org/2077783002/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js:97: chrome.test.assertEq(failToSendKeyEvents, On 2016/06/23 18:43:22, Devlin wrote: > nit: indentation > chrome.test.assertEq(failToSendKeyEvents, > chrome.runtime.lastError.message); > or if that doesn't fit > chrome.test.assertEq( > failToSendKeyEvents, > chrome.runtime.lastError.message); Done. Thanks for the comments. https://codereview.chromium.org/2077783002/diff/100001/ui/base/ime/input_meth... File ui/base/ime/input_method_base.cc (right): https://codereview.chromium.org/2077783002/diff/100001/ui/base/ime/input_meth... ui/base/ime/input_method_base.cc:32: if (!key_events_for_testing_.empty()) { On 2016/06/23 18:43:22, Devlin wrote: > Why not just std::vector<std::unique_ptr<KeyEvent>>? Or even > std::vector<KeyEvent>? Done. Updated with ScopedVector.
Thanks for adding the whitelisting. I'm happy with the security restrictions, but I'll defer the code review to Devlin. In the meanwhile, please remember to update the CL description (ENTER et al. works only on http:, https: etc. pages)
Description was changed from ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. doesn't work on chrome:// pages. BUT=517773 TEST=Verified on local build. ========== to ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. BUT=517773 TEST=Verified on local build. ==========
Description was changed from ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. BUT=517773 TEST=Verified on local build. ========== to ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. BUG=517773 TEST=Verified on local build. ==========
On 2016/06/24 17:21:05, Mustafa Emre Acer wrote: > Thanks for adding the whitelisting. I'm happy with the security restrictions, > but I'll defer the code review to Devlin. > > In the meanwhile, please remember to update the CL description (ENTER et al. > works only on http:, https: etc. pages) I've updated the CL description. Thanks for the review. Devlin raised a question in one of his comments that whether we should whitelist file url, quoted as follows: > https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... > chrome/browser/ui/input_method/input_method_engine.cc:240: > meacer@, do you think we can add file:// urls here?
On 2016/06/25 08:46:59, Azure Wei wrote: > On 2016/06/24 17:21:05, Mustafa Emre Acer wrote: > > Thanks for adding the whitelisting. I'm happy with the security restrictions, > > but I'll defer the code review to Devlin. > > > > In the meanwhile, please remember to update the CL description (ENTER et al. > > works only on http:, https: etc. pages) > > I've updated the CL description. Thanks for the review. > > Devlin raised a question in one of his comments that whether we should whitelist > file url, quoted as follows: > > > > https://codereview.chromium.org/2077783002/diff/60001/chrome/browser/ui/input... > > chrome/browser/ui/input_method/input_method_engine.cc:240: > > meacer@, do you think we can add file:// urls here? I don't think we should whitelist file:// URLs. They are more privileged than web pages (they can include other file:// URLs as subresources, iframe them, enumerate them etc). I also don't see a compelling use case for enabling IMEs in file URLs.
Pinging... Devlin, does the latest patch set look good to you? Thank.
https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:230: bool url_trustable, The more I look at the "url_trustable" bool, the more I think it sounds weird. :) Can we pull the whitelisted logic out into SendKeyEvent(), and change this signature to take bool is_special_page, ui::KeyEvent* ui_event? https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:259: for (auto invalid_key : invalid_character_keycodes) { nit: I find it slightly more readable to make keycodes sets so you can do: return invalid_character_keycodes.count(ui_event->key_code()) == 0; https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:266: // Checks if the key event is white listed key. and here return whitelist_keycodes.count(ui_event->key_code()) != 0; https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc (right): https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:83: ScopedVector<ui::KeyEvent> key_events; ScopedVector is deprecated. Use std::vector<std::unique_ptr<>> instead. Although here, just a vanilla std::vector<> should work. https://codereview.chromium.org/2077783002/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js (right): https://codereview.chromium.org/2077783002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js:104: 'contextID': 1, nit: no need for '' around property names. https://codereview.chromium.org/2077783002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js:108: 'key': '\u0009', comment what this key is. https://codereview.chromium.org/2077783002/diff/120001/ui/base/ime/input_meth... File ui/base/ime/input_method_base.cc (right): https://codereview.chromium.org/2077783002/diff/120001/ui/base/ime/input_meth... ui/base/ime/input_method_base.cc:213: const ScopedVector<ui::KeyEvent>& InputMethodBase::GetKeyEventsForTesting() { (same comment about ScopedVector)
Pinging... Devlin, does the latest patch set look good to you? Thank.
On 2016/06/29 00:33:42, Shu Chen wrote: > Pinging... > > Devlin, does the latest patch set look good to you? > > Thank. Sorry, this message was pending last night due to network disconnection.
https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:230: bool url_trustable, On 2016/06/28 20:56:44, Devlin wrote: > The more I look at the "url_trustable" bool, the more I think it sounds weird. > :) Can we pull the whitelisted logic out into SendKeyEvent(), and change this > signature to take bool is_special_page, ui::KeyEvent* ui_event? Do you mean put all the logic of checking the url is special url into SendKeyEvent(). If so, I think we don't need the |bool is_special_page| parameter. https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:259: for (auto invalid_key : invalid_character_keycodes) { On 2016/06/28 20:56:44, Devlin wrote: > nit: I find it slightly more readable to make keycodes sets so you can do: > return invalid_character_keycodes.count(ui_event->key_code()) == 0; Done. https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:266: // Checks if the key event is white listed key. On 2016/06/28 20:56:44, Devlin wrote: > and here > return whitelist_keycodes.count(ui_event->key_code()) != 0; Done. https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc (right): https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/ime/input_ime_apitest_nonchromeos.cc:83: ScopedVector<ui::KeyEvent> key_events; On 2016/06/28 20:56:44, Devlin wrote: > ScopedVector is deprecated. Use std::vector<std::unique_ptr<>> instead. > Although here, just a vanilla std::vector<> should work. Done. https://codereview.chromium.org/2077783002/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js (right): https://codereview.chromium.org/2077783002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js:104: 'contextID': 1, On 2016/06/28 20:56:44, Devlin wrote: > nit: no need for '' around property names. Done. https://codereview.chromium.org/2077783002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/input_ime_nonchromeos/background.js:108: 'key': '\u0009', On 2016/06/28 20:56:44, Devlin wrote: > comment what this key is. Done. https://codereview.chromium.org/2077783002/diff/120001/ui/base/ime/input_meth... File ui/base/ime/input_method_base.cc (right): https://codereview.chromium.org/2077783002/diff/120001/ui/base/ime/input_meth... ui/base/ime/input_method_base.cc:213: const ScopedVector<ui::KeyEvent>& InputMethodBase::GetKeyEventsForTesting() { On 2016/06/28 20:56:44, Devlin wrote: > (same comment about ScopedVector) Done.
https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:210: Browser* browser = chrome::FindLastActive(); IMO, IsValidKeyForSpecialPage() may run faster than the following code. So, maybe it worths to check IsValidKeyForSpecialPage() earlier at here? https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.h (right): https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.h:14: #include "url/gurl.h" move this to .cc file.
lgtm with one last nit and once Shu signs off. https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/120001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:230: bool url_trustable, On 2016/06/29 02:53:46, Azure Wei wrote: > On 2016/06/28 20:56:44, Devlin wrote: > > The more I look at the "url_trustable" bool, the more I think it sounds weird. > > :) Can we pull the whitelisted logic out into SendKeyEvent(), and change this > > signature to take bool is_special_page, ui::KeyEvent* ui_event? > > Do you mean put all the logic of checking the url is special url into > SendKeyEvent(). If so, I think we don't need the |bool is_special_page| > parameter. Ah, right, because otherwise we send all key events. https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:225: is_special_page = true; Hmm... sorry, this still looks a little weird to me. It's just hard to parse all the times these bools change at a quick glance, and a little hard to understand what the code is actually doing. What if we actually move the IsSpecialPage logic into its own method (and keep the IsValidKeyForSpecialPage in its own method too). So: bool IsSpecialPage(content::WebContents* web_contents, InputMethod* method) { if (!web_contents) return true; // If |input_context->GetInputMethod() != input_method|, treats |url| as // special page no mater what it is. if (web_contents->window()->GetNativeWindow()->GetHost()->GetInputMethod() != input_method) return true; std::vector<const char*> whitelist_schemes{ url::kFtpScheme, url::kHttpScheme, url::kHttpsScheme}; GURL url = web_contents->GetLastCommittedURL(); // If we can't determine the last committed url, treat it as special. if (!url.is_valid()) return true; for (auto scheme : whitelist_schemes) { if (url.SchemeIs(scheme)) return false; } Origin origin(url); std::vector<GURL> whitelist_urls{GURL(url::kAboutBlankURL), GURL(chrome::kChromeUINewTabURL)}; for (const GURL& whitelist_url : whitelist_urls) { if (Origin(whitelist_url).IsSameOriginWith(origin)) return false; } return true; } and then here: if (!IsValidKeyForSpecialPage(event)) { <GetWebContents> if (IsSpecialPage(web_contents, input_method)) return false; } input_context->SendKeyEvent(event); return true; Sorry for the back-and-forth on this - it's just a pretty subtle method, and I wanna maximize readability so that we can understand what's going on (and be less likely to break it in the future :)).
https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:210: Browser* browser = chrome::FindLastActive(); On 2016/06/29 13:18:19, Shu Chen wrote: > IMO, IsValidKeyForSpecialPage() may run faster than the following code. > > So, maybe it worths to check IsValidKeyForSpecialPage() earlier at here? Done. And renamed as IsValidKeyForAllPages(). https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:225: is_special_page = true; On 2016/06/29 16:07:23, Devlin wrote: > Hmm... sorry, this still looks a little weird to me. It's just hard to parse > all the times these bools change at a quick glance, and a little hard to > understand what the code is actually doing. What if we actually move the > IsSpecialPage logic into its own method (and keep the IsValidKeyForSpecialPage > in its own method too). So: > > bool IsSpecialPage(content::WebContents* web_contents, InputMethod* method) { > if (!web_contents) > return true; > // If |input_context->GetInputMethod() != input_method|, treats |url| as > // special page no mater what it is. > if (web_contents->window()->GetNativeWindow()->GetHost()->GetInputMethod() != > input_method) > return true; > std::vector<const char*> whitelist_schemes{ > url::kFtpScheme, url::kHttpScheme, url::kHttpsScheme}; > GURL url = web_contents->GetLastCommittedURL(); > // If we can't determine the last committed url, treat it as special. > if (!url.is_valid()) > return true; > for (auto scheme : whitelist_schemes) { > if (url.SchemeIs(scheme)) > return false; > } > Origin origin(url); > std::vector<GURL> whitelist_urls{GURL(url::kAboutBlankURL), > GURL(chrome::kChromeUINewTabURL)}; > for (const GURL& whitelist_url : whitelist_urls) { > if (Origin(whitelist_url).IsSameOriginWith(origin)) > return false; > } > return true; > } > > > and then here: > > if (!IsValidKeyForSpecialPage(event)) { > <GetWebContents> > if (IsSpecialPage(web_contents, input_method)) > return false; > } > > input_context->SendKeyEvent(event); > return true; > > > Sorry for the back-and-forth on this - it's just a pretty subtle method, and I > wanna maximize readability so that we can understand what's going on (and be > less likely to break it in the future :)). Done. Added the new method IsSpecialPage(ui::InputMethod* input_method), since all the work of getting web contents and url could be pulled out from SendKeyEvent. https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.h (right): https://codereview.chromium.org/2077783002/diff/160001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.h:14: #include "url/gurl.h" On 2016/06/29 13:18:19, Shu Chen wrote: > move this to .cc file. Done.
Description was changed from ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. BUG=517773 TEST=Verified on local build. ========== to ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. 3) onKeyEvent no work in password field. BUG=517773 TEST=Verified on local build. ==========
(slgtm) https://codereview.chromium.org/2077783002/diff/220001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/220001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:267: return std::count(invalid_character_keycodes.begin(), nitty nit: prefer find() for vectors so that it can short-circuit once it finds the item, rather than iterating over the full collection (count is fine for maps and sets, where the maximum number is 1).
lgtm
https://codereview.chromium.org/2077783002/diff/220001/chrome/browser/ui/inpu... File chrome/browser/ui/input_method/input_method_engine.cc (right): https://codereview.chromium.org/2077783002/diff/220001/chrome/browser/ui/inpu... chrome/browser/ui/input_method/input_method_engine.cc:267: return std::count(invalid_character_keycodes.begin(), On 2016/06/30 14:35:46, Devlin wrote: > nitty nit: prefer find() for vectors so that it can short-circuit once it finds > the item, rather than iterating over the full collection (count is fine for maps > and sets, where the maximum number is 1). Done. Many thanks.
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, shuchen@chromium.org Link to the patchset: https://codereview.chromium.org/2077783002/#ps240001 (title: "Use find than count.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2077783002/#ps260001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2077783002/#ps280001 (title: "Fix chromeos error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. 3) onKeyEvent no work in password field. BUG=517773 TEST=Verified on local build. ========== to ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. 3) onKeyEvent no work in password field. BUG=517773 TEST=Verified on local build. ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. 3) onKeyEvent no work in password field. BUG=517773 TEST=Verified on local build. ========== to ========== Make limitations for input.ime.sendKeyEvents API. 1) The API works only on text fields 2) ENTER et al. works only on http:, https: etc. 3) onKeyEvent no work in password field. BUG=517773 TEST=Verified on local build. Committed: https://crrev.com/06cf902e7ac4cc27399399219ad6d37042719752 Cr-Commit-Position: refs/heads/master@{#403250} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/06cf902e7ac4cc27399399219ad6d37042719752 Cr-Commit-Position: refs/heads/master@{#403250}
Message was sent while issue was closed.
creis@chromium.org changed reviewers: + creis@chromium.org
Message was sent while issue was closed.
This CL broke the Site Isolation Linux FYI bot. See https://crbug.com/624958. Can you take a look? |
