Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
247883002:
Adds browser test framework for the IME keyboard, and some basic typing tests. (Closed)
|
Created:
6 years, 8 months ago by rsadam Modified:
6 years, 7 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdds browser test framework for the IME keyboard, and some basic typing tests.
BUG=353857
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269328
Patch Set 1 #Patch Set 2 : Adds some missing comments. #
Total comments: 4
Patch Set 3 : Fixed nits. #
Total comments: 4
Patch Set 4 : Fixed nits. #Patch Set 5 : #
Total comments: 31
Patch Set 6 : #Patch Set 7 : Run tests on full layout. #
Total comments: 17
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Messages
Total messages: 25 (0 generated)
Kevers PTAL!
Fixed some nits.
lgtm lgtm with some minor changes. https://codereview.chromium.org/247883002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_view_browsertest.cc:69: ime->ChangeInputMethod("_comp_ime_" + extensionId_ + "xkb:us::eng"); I think you can do this here to get the input method id: chromeos::extension_ime_util:GetComponentInputMethodID(extensionId_, "xkb:us::eng"); https://codereview.chromium.org/247883002/diff/40001/chrome/test/data/chromeo... File chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js (right): https://codereview.chromium.org/247883002/diff/40001/chrome/test/data/chromeo... chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js:90: generateMouseEvent(key, 'click', true, true); You probably want to add: generateMouseEvent(key, 'mouseover', true, true); generateMouseEvent(key, 'mouseout', true, true);
Addressed nits! https://codereview.chromium.org/247883002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_view_browsertest.cc:69: ime->ChangeInputMethod("_comp_ime_" + extensionId_ + "xkb:us::eng"); On 2014/04/22 19:14:43, Shu Chen wrote: > I think you can do this here to get the input method id: > chromeos::extension_ime_util:GetComponentInputMethodID(extensionId_, > "xkb:us::eng"); Done. https://codereview.chromium.org/247883002/diff/40001/chrome/test/data/chromeo... File chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js (right): https://codereview.chromium.org/247883002/diff/40001/chrome/test/data/chromeo... chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js:90: generateMouseEvent(key, 'click', true, true); On 2014/04/22 19:14:43, Shu Chen wrote: > You probably want to add: > generateMouseEvent(key, 'mouseover', true, true); > generateMouseEvent(key, 'mouseout', true, true); Done.
lgtm with nits. https://codereview.chromium.org/247883002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_view_browsertest.cc:36: base::FilePath(FILE_PATH_LITERAL("chromeos/virtual_keyboard/inputview/")); Nit: insert blank line before closing brace for namespace https://codereview.chromium.org/247883002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h (right): https://codereview.chromium.org/247883002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Nit: remove (c) and update year.
Fixed nits! +koz for OWNERS https://codereview.chromium.org/247883002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_view_browsertest.cc:36: base::FilePath(FILE_PATH_LITERAL("chromeos/virtual_keyboard/inputview/")); On 2014/04/23 13:39:05, kevers wrote: > Nit: insert blank line before closing brace for namespace Done. https://codereview.chromium.org/247883002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h (right): https://codereview.chromium.org/247883002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/04/23 13:39:05, kevers wrote: > Nit: remove (c) and update year. Done.
Just realized that koz is in AUS, +jyasskin for OWNERS.
Removed an unneeded LOG message, fixed a typo.
On 2014/04/24 21:41:58, rsadam wrote: > Removed an unneeded LOG message, fixed a typo. ping @jyasskin
https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:2: * Copyright 2014 The Chromium Authors. All rights reserved. http://www.chromium.org/developers/coding-style says: As of May 2013, the header should look like this: // Copyright $YEAR The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. It's generally better to copy that than copy an existing file. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:29: const base::FilePath kBaseKeyboardTestFramework = The style guide discourages global/static objects with non-trivial constructors or destructors. You can store the FILE_PATH_LITERALs here (as const base::FilePath::CharType[]), but construct the FilePaths inline. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:33: base::FilePath(FILE_PATH_LITERAL("GoogleKeyboardInput-xkb.crx")); It's not great to check in the built .crx. Is its source in the Chrome tree, or elsewhere? How do you make sure that it stays up to date? https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:53: extensions::CrxInstaller::CreateSilent(service); It'd be nice to share the common code here with https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex.... https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:89: RunTest(base::FilePath(FILE_PATH_LITERAL("typing_test.js"))); It would likely be easier to read this test if the typing_test javascript were inlined in this .cc file. Since RunTest uses ReadFileToString() to read the file anyway, it should be straightforward to let it take a string literal. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h (right): https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:22: // Injects javascript in |file| into the keyboard page and runs test methods. Please make "test methods" more precise. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:28: virtual std::string GetKeyboardExtensionId(); For things that you expect to be static values, it tends to make the test easier to read if they're clearly stored in variables instead of computed by virtual functions. Try to make these functions protected setters, and store their arguments into private member variables. Once that's done, could this class become a configuration struct that you pass to a helper function instead of a class your users have to inherit their tests from? https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:40: virtual content::WebContents* NavigateToWebContents(); This isn't named well since you're navigating to a URL, not a WebContents. It's also only used in one place, so it might be better just to inline it. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:42: virtual GURL GetURL(); Remember to comment which URL. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:49: // Injects javascript into the keyboard page. The test |file| is in The implementation of this looks like it just accumulates the javascript into a member variable, until the test starts. https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... File chrome/test/data/chromeos/virtual_keyboard/end_to_end_test.js (right): https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... chrome/test/data/chromeos/virtual_keyboard/end_to_end_test.js:31: } Revert whitespace changes to files that you're not changing in other ways. https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... File chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js (right): https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js:49: chrome.input.ime.setComposition = setComposition; This looks suspicious: you're mocking setComposition, saving the mock, overriding the mock, resetting setComposition to its original value, and then resetting it back to the mock. https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js:62: * Generates a mouse event and dispatches it on the target. Mention that this function takes extra arguments beyond the declared ones, and describe what they do. Or switch to the MouseEventInit dictionary style so you don't need to play with |arguments|. https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js:67: var e = document.createEvent('MouseEvents'); https://developer.mozilla.org/en-US/docs/Web/API/document.createEvent suggests that this function is deprecated in favor of calls like: new MouseEvent('mouseover', {bubbles: true, cancelable: true}); https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm#constructor-mouseevent http://dom.spec.whatwg.org/#eventinit https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... File chrome/test/data/chromeos/virtual_keyboard/inputview/typing_test.js (right): https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... chrome/test/data/chromeos/virtual_keyboard/inputview/typing_test.js:8: * Tests typing in the lowecase keyset. spelling: lowercase?
Will have a patch to address most of the feedback tomorrow - however with respect to 3 of the points I'm not sure what the best route is... https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:33: base::FilePath(FILE_PATH_LITERAL("GoogleKeyboardInput-xkb.crx")); On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > It's not great to check in the built .crx. Is its source in the Chrome tree, or > elsewhere? How do you make sure that it stays up to date? The keyboard's language model is not open source and is built as part of the official chrome build. There's work in progress to split the layout information from the layout, however this will not be done until the M37 time frame. For M36, the IME keyboard will be deployed, and so having the CRX allows at least basic testing of the keyboard in the mean time. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:53: extensions::CrxInstaller::CreateSilent(service); On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > It'd be nice to share the common code here with > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex.... The ExtensionBrowserTest doesn't seem to support the set_allow_silent_install flag - one alternative would be to modify it to make that customizable. Would this be a better alternative? https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:89: RunTest(base::FilePath(FILE_PATH_LITERAL("typing_test.js"))); On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > It would likely be easier to read this test if the typing_test javascript were > inlined in this .cc file. Since RunTest uses ReadFileToString() to read the file > anyway, it should be straightforward to let it take a string literal. I have several follow patches that is likely to make the js file very large. My worry about inlining it would be that it would make reading it harder... We'll also be adding other tests such as keyset transition tests in M36.
https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:2: * Copyright 2014 The Chromium Authors. All rights reserved. On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > http://www.chromium.org/developers/coding-style says: > > As of May 2013, the header should look like this: > // Copyright $YEAR The Chromium Authors. All rights reserved. > // Use of this source code is governed by a BSD-style license that can be > // found in the LICENSE file. > > It's generally better to copy that than copy an existing file. Done. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:29: const base::FilePath kBaseKeyboardTestFramework = On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > The style guide discourages global/static objects with non-trivial constructors > or destructors. You can store the FILE_PATH_LITERALs here (as const > base::FilePath::CharType[]), but construct the FilePaths inline. Done. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h (right): https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:22: // Injects javascript in |file| into the keyboard page and runs test methods. On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > Please make "test methods" more precise. Done. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:28: virtual std::string GetKeyboardExtensionId(); On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > For things that you expect to be static values, it tends to make the test easier > to read if they're clearly stored in variables instead of computed by virtual > functions. Try to make these functions protected setters, and store their > arguments into private member variables. > > Once that's done, could this class become a configuration struct that you pass > to a helper function instead of a class your users have to inherit their tests > from? Done. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:40: virtual content::WebContents* NavigateToWebContents(); On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > This isn't named well since you're navigating to a URL, not a WebContents. It's > also only used in one place, so it might be better just to inline it. Done. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:42: virtual GURL GetURL(); On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > Remember to comment which URL. Done. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:49: // Injects javascript into the keyboard page. The test |file| is in On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > The implementation of this looks like it just accumulates the javascript into a > member variable, until the test starts. Done. https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... File chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js (right): https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js:49: chrome.input.ime.setComposition = setComposition; On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > This looks suspicious: you're mocking setComposition, saving the mock, > overriding the mock, resetting setComposition to its original value, and then > resetting it back to the mock. Oops. We don't need to mock it using a controller since we don't have any expectations on it. Done! https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js:62: * Generates a mouse event and dispatches it on the target. On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > Mention that this function takes extra arguments beyond the declared ones, and > describe what they do. Or switch to the MouseEventInit dictionary style so you > don't need to play with |arguments|. Done. https://codereview.chromium.org/247883002/diff/100001/chrome/test/data/chrome... chrome/test/data/chromeos/virtual_keyboard/inputview/test_base.js:67: var e = document.createEvent('MouseEvents'); On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > https://developer.mozilla.org/en-US/docs/Web/API/document.createEvent suggests > that this function is deprecated in favor of calls like: > > new MouseEvent('mouseover', {bubbles: true, cancelable: true}); > > https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm#constructor-mouseevent > http://dom.spec.whatwg.org/#eventinit Done.
https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:33: base::FilePath(FILE_PATH_LITERAL("GoogleKeyboardInput-xkb.crx")); On 2014/04/29 23:10:12, rsadam wrote: > On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > > It's not great to check in the built .crx. Is its source in the Chrome tree, > or > > elsewhere? How do you make sure that it stays up to date? > > The keyboard's language model is not open source and is built as part of the > official chrome build. There's work in progress to split the layout information > from the layout, however this will not be done until the M37 time frame. For > M36, the IME keyboard will be deployed, and so having the CRX allows at least > basic testing of the keyboard in the mean time. Ok. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:53: extensions::CrxInstaller::CreateSilent(service); On 2014/04/29 23:10:12, rsadam wrote: > On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > > It'd be nice to share the common code here with > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex.... > > The ExtensionBrowserTest doesn't seem to support the set_allow_silent_install > flag - one alternative would be to modify it to make that customizable. Would > this be a better alternative? You'd have to refactor it into a free function anyway, since ExtensionBrowserTest insists you inherit from it to use its helper functions. InstallOrUpdateExtension() uses a MockAutoConfirmExtensionInstallPrompt to avoid needing the silent option. I'm not sure how the INSTALL_UI_TYPE_NONE option works without that, but maybe we don't need all the knobs that have accumulated. https://codereview.chromium.org/247883002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:89: RunTest(base::FilePath(FILE_PATH_LITERAL("typing_test.js"))); On 2014/04/29 23:10:12, rsadam wrote: > On 2014/04/28 23:14:50, Jeffrey Yasskin wrote: > > It would likely be easier to read this test if the typing_test javascript were > > inlined in this .cc file. Since RunTest uses ReadFileToString() to read the > file > > anyway, it should be straightforward to let it take a string literal. > > I have several follow patches that is likely to make the js file very large. My > worry about inlining it would be that it would make reading it harder... We'll > also be adding other tests such as keyset transition tests in M36. Hm, ok. I guess the startup overhead of a browser test makes it worthwhile to combine a bunch into a single RunTest() invocation. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:31: const std::string kInputViewTestDir = "chromeos/virtual_keyboard/inputview/"; std::string is also an object with a non-trivial constructor and destructor. Use "const char kInputViewTestDir[]" instead. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:52: base::FilePath(FILE_PATH_LITERAL(kInputViewTestDir)), This won't compile on windows. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc:45: VirtualKeyboardBrowserTestHelper::VirtualKeyboardBrowserTestHelper() The header file should mention the default values, or maybe say "see the .cc for default values". https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc:91: const std::string id) { Take std::strings as "const std::string&", unless you modify the argument inside the function. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc:93: std::string url = "chrome-extension://" + id + "/"; This should probably be "Extension::GetBaseURLFromExtensionId(id)" https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h (right): https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:22: class VirtualKeyboardBrowserTestHelper { "Helper" isn't a great suffix, especially for a class with no behavior. What's the meaning of the data this incorporates? This might be "...TestConfig" if there's no better description. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:26: virtual ~VirtualKeyboardBrowserTestHelper(); Doesn't need to be virtual anymore. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:48: std::string base_framework_; It looks like this could just be a struct: struct VirtualKeyboardBrowserTestHelper { VirtualKeyboardBrowserTestHelper(); ~VirtualKeyboardBrowserTestHelper(); // The filename of the base framework. This file should be in |test_dir_|. std::string base_framework_; // The virtual keyboard's extension id. std::string extension_id_; ... }; The setter and getter methods don't really add much to a pure data object.
Okay, so the vote is to leave the extension stuff as is? Addressed the other suggestions! https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:31: const std::string kInputViewTestDir = "chromeos/virtual_keyboard/inputview/"; On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > std::string is also an object with a non-trivial constructor and destructor. Use > "const char kInputViewTestDir[]" instead. Done. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:52: base::FilePath(FILE_PATH_LITERAL(kInputViewTestDir)), On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > This won't compile on windows. These tests are chromeos only so I don't think that's an issue. (They're in the directory is browser/chromeos/extensions) https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc:45: VirtualKeyboardBrowserTestHelper::VirtualKeyboardBrowserTestHelper() On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > The header file should mention the default values, or maybe say "see the .cc for > default values". Done. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc:91: const std::string id) { On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > Take std::strings as "const std::string&", unless you modify the argument inside > the function. Done. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc:93: std::string url = "chrome-extension://" + id + "/"; On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > This should probably be "Extension::GetBaseURLFromExtensionId(id)" Done. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h (right): https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:22: class VirtualKeyboardBrowserTestHelper { On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > "Helper" isn't a great suffix, especially for a class with no behavior. What's > the meaning of the data this incorporates? This might be "...TestConfig" if > there's no better description. Done. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:26: virtual ~VirtualKeyboardBrowserTestHelper(); On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > Doesn't need to be virtual anymore. Done. https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.h:48: std::string base_framework_; On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > It looks like this could just be a struct: > > struct VirtualKeyboardBrowserTestHelper { > VirtualKeyboardBrowserTestHelper(); > ~VirtualKeyboardBrowserTestHelper(); > // The filename of the base framework. This file should be in |test_dir_|. > std::string base_framework_; > // The virtual keyboard's extension id. > std::string extension_id_; > ... > }; > > The setter and getter methods don't really add much to a pure data object. Done.
ping
lgtm https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/input_view_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/input_view_browsertest.cc:52: base::FilePath(FILE_PATH_LITERAL(kInputViewTestDir)), On 2014/05/05 17:44:42, rsadam wrote: > On 2014/05/02 22:09:37, Jeffrey Yasskin wrote: > > This won't compile on windows. > > These tests are chromeos only so I don't think that's an issue. (They're in the > directory is browser/chromeos/extensions) You should probably do these the same way you did kExtensionName: use base::FilePath::CharType in the global constant instead of char. If you're not trying to be cross-platform at all, just don't use the FILE_PATH_LITERAL() macro. https://codereview.chromium.org/247883002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc:94: std::string url = extensions::Extension::GetBaseURLFromExtensionId(id).spec(); You can remove both .spec()s if you make this a GURL.
> If you're not trying to be cross-platform at all, just don't use the > FILE_PATH_LITERAL() macro. Done. https://codereview.chromium.org/247883002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc (right): https://codereview.chromium.org/247883002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/virtual_keyboard_browsertest.cc:94: std::string url = extensions::Extension::GetBaseURLFromExtensionId(id).spec(); On 2014/05/08 00:04:16, Jeffrey Yasskin wrote: > You can remove both .spec()s if you make this a GURL. Done.
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsadam@chromium.org/247883002/220001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by rsadam@chromium.org
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsadam@chromium.org/247883002/220001
Message was sent while issue was closed.
Change committed as 269328 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
