Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8676008:
Add automated test for the edit search engine dialog. (Closed)
|
Created:
9 years, 1 month ago by kevers Modified:
9 years ago CC:
chromium-reviews, arv (Not doing code reviews), Paweł Hajdan Jr., Evan Stade Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd automated test for the edit search engine dialog.
BUG=chromium:97846
TEST=browser_tests -gtest_filter=EditSearchEngineDialogUITest*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113022
Patch Set 1 #
Total comments: 8
Patch Set 2 : Consolidate tests. #
Total comments: 21
Patch Set 3 : Add accessors for dialog content. #Patch Set 4 : Fix save.onclick. #
Total comments: 6
Patch Set 5 : File renames, address nits and add documentation. #
Total comments: 12
Patch Set 6 : Update to new test API. #
Total comments: 14
Patch Set 7 : Add TODO comment and fix typos. #Patch Set 8 : Fix typo. #
Total comments: 12
Patch Set 9 : Style fixes. #Patch Set 10 : Use preLoad to address race condition in asynchronous test. #
Total comments: 10
Patch Set 11 : Add pendingDetails to prototype. #Patch Set 12 : Style fixes. #
Messages
Total messages: 30 (0 generated)
Hi Rob, Could you please have a look at this CL. It is a carryover from Issue 8524007 (Edit Search Engine Dialog Test). -- Kevin
http://codereview.chromium.org/8676008/diff/1/chrome/browser/resources/edit_s... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/1/chrome/browser/resources/edit_s... chrome/browser/resources/edit_search_engine_dialog.js:141: armRevalidation_ = true; Is there any reason we can't set isValidating_ here and do away with armRevalidation_? http://codereview.chromium.org/8676008/diff/1/chrome/test/data/webui/edit_sea... File chrome/test/data/webui/edit_search_engine_dialog_test.js (right): http://codereview.chromium.org/8676008/diff/1/chrome/test/data/webui/edit_sea... chrome/test/data/webui/edit_search_engine_dialog_test.js:81: }); Can these three be done as one test now? I suppose you have to remove the initial check for validity (possibly separate test) and have valid data for the other fields. http://codereview.chromium.org/8676008/diff/1/chrome/test/data/webui/edit_sea... chrome/test/data/webui/edit_search_engine_dialog_test.js:92: function runFieldInvalidationTest(test, baseFieldName, testData) { Change the name to something suggesting that it tests this particular thing, rather than runs the test. The test is run from the TEST_F call so this name is somewhat confusing. Perhaps testFieldInvalidation. http://codereview.chromium.org/8676008/diff/1/chrome/test/data/webui/edit_sea... chrome/test/data/webui/edit_search_engine_dialog_test.js:108: setValidation(details); Indent 2 from editSearchEngineDialog.setValidation.
http://codereview.chromium.org/8676008/diff/1/chrome/browser/resources/edit_s... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/1/chrome/browser/resources/edit_s... chrome/browser/resources/edit_search_engine_dialog.js:141: armRevalidation_ = true; On 2011/11/23 15:22:37, flackr wrote: > Is there any reason we can't set isValidating_ here and do away with > armRevalidation_? Done. http://codereview.chromium.org/8676008/diff/1/chrome/test/data/webui/edit_sea... File chrome/test/data/webui/edit_search_engine_dialog_test.js (right): http://codereview.chromium.org/8676008/diff/1/chrome/test/data/webui/edit_sea... chrome/test/data/webui/edit_search_engine_dialog_test.js:81: }); On 2011/11/23 15:22:37, flackr wrote: > Can these three be done as one test now? I suppose you have to remove the > initial check for validity (possibly separate test) and have valid data for the > other fields. Done. http://codereview.chromium.org/8676008/diff/1/chrome/test/data/webui/edit_sea... chrome/test/data/webui/edit_search_engine_dialog_test.js:92: function runFieldInvalidationTest(test, baseFieldName, testData) { On 2011/11/23 15:22:37, flackr wrote: > Change the name to something suggesting that it tests this particular thing, > rather than runs the test. The test is run from the TEST_F call so this name is > somewhat confusing. Perhaps testFieldInvalidation. Done. http://codereview.chromium.org/8676008/diff/1/chrome/test/data/webui/edit_sea... chrome/test/data/webui/edit_search_engine_dialog_test.js:108: setValidation(details); On 2011/11/23 15:22:37, flackr wrote: > Indent 2 from editSearchEngineDialog.setValidation. Done.
http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_test.js (right): http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:63: var fieldNames = []; Does var fieldNames = Object.keys(invalidData) work? http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:68: } If above works, then you can probably remove this loop :)
http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:2749: 'test/data/webui/edit_serach_engine_dialog_ui_test-inl.h', Please move these to chrome/browser/ui/webui/edit_search_engine_dialog_browsertest.* http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_test.js (right): http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:47: expectEquals('short0', $('description-text').value); Please decouple the presentation from tests - one technique is exporting getDescritionText or getDescritionTextElement from your resource/...js http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:70: var setValidation = editSearchEngineDialog.setValidation; I'm kind of confused by what you're doing here - maybe some comments would help. Are you trying to save the setValidation for use in the validityChecker, but then change editSearchEngineDialog.setValidation elsewhere? Maybe renaming to originalSetValidation would make it clearer without comments. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:80: 'field = ' + fieldNames[i]); Would index = be helpful too? http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:83: expectFalse($('save').disabled); provide/use accessor here too & below a la getSaveButton() http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h (right): http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h:46: void EditSearchEngineDialogUITest::SetUpOnMainThread() { Can you have a look at http://codereview.chromium.org/8586009/ and factor this out into its own non-virtual method and call it with a testGenPreamble GEN line like in http://codereview.chromium.org/8586009/diff/19001/chrome/test/data/webui/hung... - when it's refactored you won't need to call WebUIBrowserTest::SetUpOnMainThread(). Depending on which CL lands first, the only change will be to make the following take a |this| parameter for injecting javascript in the preLoad phase. TestHtmlDialogObserver dialog_observer(this); -SCR
http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:2749: 'test/data/webui/edit_serach_engine_dialog_ui_test-inl.h', On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > Please move these to > chrome/browser/ui/webui/edit_search_engine_dialog_browsertest.* Should the certificate viewer and hung renderer tests be moved as well? If so, perhaps this can be done in a separate CL. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_test.js (right): http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:47: expectEquals('short0', $('description-text').value); On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > Please decouple the presentation from tests - one technique is exporting > getDescritionText or getDescritionTextElement from your resource/...js Added accesssors for each entry field to the dialog class with get/set for the text input and field validity. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:63: var fieldNames = []; On 2011/11/23 20:22:00, flackr wrote: > Does var fieldNames = Object.keys(invalidData) work? Done. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:68: } On 2011/11/23 20:22:00, flackr wrote: > If above works, then you can probably remove this loop :) Done. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:70: var setValidation = editSearchEngineDialog.setValidation; On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > I'm kind of confused by what you're doing here - maybe some comments would help. > Are you trying to save the setValidation for use in the validityChecker, but > then change editSearchEngineDialog.setValidation elsewhere? Maybe renaming to > originalSetValidation would make it clearer without comments. Renamed the variable and added documentation. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:80: 'field = ' + fieldNames[i]); On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > Would index = be helpful too? Done. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:83: expectFalse($('save').disabled); On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > provide/use accessor here too & below a la getSaveButton() Done. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h (right): http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h:46: void EditSearchEngineDialogUITest::SetUpOnMainThread() { On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > Can you have a look at http://codereview.chromium.org/8586009/ and factor this > out into its own non-virtual method and call it with a testGenPreamble GEN line > like in > http://codereview.chromium.org/8586009/diff/19001/chrome/test/data/webui/hung... > - when it's refactored you won't need to call > WebUIBrowserTest::SetUpOnMainThread(). > > Depending on which CL lands first, the only change will be to make the following > take a |this| parameter for injecting javascript in the preLoad phase. > > TestHtmlDialogObserver dialog_observer(this); > > -SCR See two options here: wait until 8586009 lands and then make the suggested changes, or create a separate open issue to fix once the patch lands and assign to myself. OK with either approach.
Hi, delayed by the holiday - here are some comments/nits. http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:2749: 'test/data/webui/edit_serach_engine_dialog_ui_test-inl.h', I'm fine with the move part being a separate CL. However, please name them _browsertest.* and don't use the -inl and if you need/want you can have a .cc as well (the gyp rules add -gen to the js-generated file, so there's no conflict). -SCR On 2011/11/24 20:03:25, kevers wrote: > On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > > Please move these to > > chrome/browser/ui/webui/edit_search_engine_dialog_browsertest.* > > Should the certificate viewer and hung renderer tests be moved as well? If so, > perhaps this can be done in a separate CL. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h (right): http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h:46: void EditSearchEngineDialogUITest::SetUpOnMainThread() { I'll back off on doing in this CL, if you just can put a TODO comment referencing the other CL. On 2011/11/24 20:03:25, kevers wrote: > On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > > Can you have a look at http://codereview.chromium.org/8586009/ and factor this > > out into its own non-virtual method and call it with a testGenPreamble GEN > line > > like in > > > http://codereview.chromium.org/8586009/diff/19001/chrome/test/data/webui/hung... > > - when it's refactored you won't need to call > > WebUIBrowserTest::SetUpOnMainThread(). > > > > Depending on which CL lands first, the only change will be to make the > following > > take a |this| parameter for injecting javascript in the preLoad phase. > > > > TestHtmlDialogObserver dialog_observer(this); > > > > -SCR > > See two options here: wait until 8586009 lands and then make the suggested > changes, or create a separate open issue to fix once the patch lands and assign > to myself. OK with either approach. > http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... chrome/browser/resources/edit_search_engine_dialog.js:85: * content is loaded. nit: @type http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... chrome/browser/resources/edit_search_engine_dialog.js:87: var InputFields = {}; nit: inputFields. http://codereview.chromium.org/8676008/diff/9002/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_test.js (right): http://codereview.chromium.org/8676008/diff/9002/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:88: if (index == 0) This use of |index| is confusing to me because it doesn't appear to be incremented before it's checked, but after a double-take, it appears to be incremented when the validityChecker function is called. Can you explain how it works in a comment? Also, are the callbacks guaranteed to be called in order? Could you use mocking to expect one call for each expected field?
http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi#new... chrome/chrome_tests.gypi:2749: 'test/data/webui/edit_serach_engine_dialog_ui_test-inl.h', On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > I'm fine with the move part being a separate CL. However, please name them > _browsertest.* and don't use the -inl and if you need/want you can have a .cc as > well (the gyp rules add -gen to the js-generated file, so there's no conflict). > > -SCR > > On 2011/11/24 20:03:25, kevers wrote: > > On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > > > Please move these to > > > chrome/browser/ui/webui/edit_search_engine_dialog_browsertest.* > > > > Should the certificate viewer and hung renderer tests be moved as well? If > so, > > perhaps this can be done in a separate CL. > Done. http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h (right): http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h:46: void EditSearchEngineDialogUITest::SetUpOnMainThread() { On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > I'll back off on doing in this CL, if you just can put a TODO comment > referencing the other CL. > > On 2011/11/24 20:03:25, kevers wrote: > > On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > > > Can you have a look at http://codereview.chromium.org/8586009/ and factor > this > > > out into its own non-virtual method and call it with a testGenPreamble GEN > > line > > > like in > > > > > > http://codereview.chromium.org/8586009/diff/19001/chrome/test/data/webui/hung... > > > - when it's refactored you won't need to call > > > WebUIBrowserTest::SetUpOnMainThread(). > > > > > > Depending on which CL lands first, the only change will be to make the > > following > > > take a |this| parameter for injecting javascript in the preLoad phase. > > > > > > TestHtmlDialogObserver dialog_observer(this); > > > > > > -SCR > > > > See two options here: wait until 8586009 lands and then make the suggested > > changes, or create a separate open issue to fix once the patch lands and > assign > > to myself. OK with either approach. > > > Done. http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... chrome/browser/resources/edit_search_engine_dialog.js:85: * content is loaded. On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > nit: @type Done. http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... chrome/browser/resources/edit_search_engine_dialog.js:87: var InputFields = {}; On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > nit: inputFields. Done. http://codereview.chromium.org/8676008/diff/9002/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_test.js (right): http://codereview.chromium.org/8676008/diff/9002/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_test.js:88: if (index == 0) On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > This use of |index| is confusing to me because it doesn't appear to be > incremented before it's checked, but after a double-take, it appears to be > incremented when the validityChecker function is called. > > Can you explain how it works in a comment? > > Also, are the callbacks guaranteed to be called in order? Could you use mocking > to expect one call for each expected field? Added a comment before the invalidate and retest code. Not clear what you are suggesting regarding mocking. The asynchronous callbacks are relatively quick, and the tests are run sequentially in a fully deterministic fashion. If we short-circuit the process via mocks we could make the data flow simpler, but at the cost of watering down the test.
I was suggesting using mocking with the side effect of calling the real thing at least so that expectations could be registered, but I don't think the mocking approach for this would add anything except for verbosity; thanks for the discussion. -SCR On 2011/11/29 17:36:37, kevers wrote: > http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi > File chrome/chrome_tests.gypi (right): > > http://codereview.chromium.org/8676008/diff/1006/chrome/chrome_tests.gypi#new... > chrome/chrome_tests.gypi:2749: > 'test/data/webui/edit_serach_engine_dialog_ui_test-inl.h', > On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > > I'm fine with the move part being a separate CL. However, please name them > > _browsertest.* and don't use the -inl and if you need/want you can have a .cc > as > > well (the gyp rules add -gen to the js-generated file, so there's no > conflict). > > > > -SCR > > > > On 2011/11/24 20:03:25, kevers wrote: > > > On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > > > > Please move these to > > > > chrome/browser/ui/webui/edit_search_engine_dialog_browsertest.* > > > > > > Should the certificate viewer and hung renderer tests be moved as well? If > > so, > > > perhaps this can be done in a separate CL. > > > > Done. > > http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... > File chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h (right): > > http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... > chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h:46: void > EditSearchEngineDialogUITest::SetUpOnMainThread() { > On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > > I'll back off on doing in this CL, if you just can put a TODO comment > > referencing the other CL. > > > > On 2011/11/24 20:03:25, kevers wrote: > > > On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > > > > Can you have a look at http://codereview.chromium.org/8586009/ and factor > > this > > > > out into its own non-virtual method and call it with a testGenPreamble GEN > > > line > > > > like in > > > > > > > > > > http://codereview.chromium.org/8586009/diff/19001/chrome/test/data/webui/hung... > > > > - when it's refactored you won't need to call > > > > WebUIBrowserTest::SetUpOnMainThread(). > > > > > > > > Depending on which CL lands first, the only change will be to make the > > > following > > > > take a |this| parameter for injecting javascript in the preLoad phase. > > > > > > > > TestHtmlDialogObserver dialog_observer(this); > > > > > > > > -SCR > > > > > > See two options here: wait until 8586009 lands and then make the suggested > > > changes, or create a separate open issue to fix once the patch lands and > > assign > > > to myself. OK with either approach. > > > > > > > Done. > > http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... > File chrome/browser/resources/edit_search_engine_dialog.js (right): > > http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... > chrome/browser/resources/edit_search_engine_dialog.js:85: * content is loaded. > On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > > nit: @type > > Done. > > http://codereview.chromium.org/8676008/diff/9002/chrome/browser/resources/edi... > chrome/browser/resources/edit_search_engine_dialog.js:87: var InputFields = {}; > On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > > nit: inputFields. > > Done. > > http://codereview.chromium.org/8676008/diff/9002/chrome/test/data/webui/edit_... > File chrome/test/data/webui/edit_search_engine_dialog_test.js (right): > > http://codereview.chromium.org/8676008/diff/9002/chrome/test/data/webui/edit_... > chrome/test/data/webui/edit_search_engine_dialog_test.js:88: if (index == 0) > On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > > This use of |index| is confusing to me because it doesn't appear to be > > incremented before it's checked, but after a double-take, it appears to be > > incremented when the validityChecker function is called. > > > > Can you explain how it works in a comment? > > > > Also, are the callbacks guaranteed to be called in order? Could you use > mocking > > to expect one call for each expected field? > > Added a comment before the invalidate and retest code. > > Not clear what you are suggesting regarding mocking. The asynchronous callbacks > are relatively quick, and the tests are run sequentially in a fully > deterministic fashion. If we short-circuit the process via mocks we could make > the data flow simpler, but at the cost of watering down the test.
LGTM with nits. http://codereview.chromium.org/8676008/diff/16001/chrome/browser/resources/ed... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/16001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:52: set value(text) { nit: @param {string} text <description...> http://codereview.chromium.org/8676008/diff/16001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:68: set valid(state) { nit: @type should be @param {boolean} state <description...> http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc (right): http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc:35: } // namesapce nit: typo http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.h (right): http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.h:14: virtual ~EditSearchEngineDialogUITest(); nit: Make these protected. http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.h:21: }; DISALLOW_COPY_AND_ASSIGN(EditSearchEngineDialogUITest); http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.js (right): http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.js:6: * Test fixture for generated tests. nit: s/generated/search engine dialog/
http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... File chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h (right): http://codereview.chromium.org/8676008/diff/1006/chrome/test/data/webui/edit_... chrome/test/data/webui/edit_search_engine_dialog_ui_test-inl.h:46: void EditSearchEngineDialogUITest::SetUpOnMainThread() { On 2011/11/29 17:36:37, kevers wrote: > On 2011/11/28 22:32:45, Sheridan Rawlins wrote: > > I'll back off on doing in this CL, if you just can put a TODO comment > > referencing the other CL. > > > > On 2011/11/24 20:03:25, kevers wrote: > > > On 2011/11/23 20:36:26, Sheridan Rawlins wrote: > > > > Can you have a look at http://codereview.chromium.org/8586009/ and factor > > this > > > > out into its own non-virtual method and call it with a testGenPreamble GEN > > > line > > > > like in > > > > > > > > > > http://codereview.chromium.org/8586009/diff/19001/chrome/test/data/webui/hung... > > > > - when it's refactored you won't need to call > > > > WebUIBrowserTest::SetUpOnMainThread(). > > > > > > > > Depending on which CL lands first, the only change will be to make the > > > following > > > > take a |this| parameter for injecting javascript in the preLoad phase. > > > > > > > > TestHtmlDialogObserver dialog_observer(this); > > > > > > > > -SCR > > > > > > See two options here: wait until 8586009 lands and then make the suggested > > > changes, or create a separate open issue to fix once the patch lands and > > assign > > > to myself. OK with either approach. > > > > > > > Done. I see that http://codereview.chromium.org/8586009/ has been committed. API updated as requested. http://codereview.chromium.org/8676008/diff/16001/chrome/browser/resources/ed... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/16001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:52: set value(text) { On 2011/11/30 00:14:01, Sheridan Rawlins wrote: > nit: @param {string} text <description...> Based on Arv's feedback in http://codereview.chromium.org/8437004, @type should be used for getters and setters; however, it looks like there should still be a description. http://codereview.chromium.org/8676008/diff/16001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:68: set valid(state) { On 2011/11/30 00:14:01, Sheridan Rawlins wrote: > nit: @type should be @param {boolean} state <description...> See comment above. http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc (right): http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc:35: } // namesapce On 2011/11/30 00:14:01, Sheridan Rawlins wrote: > nit: typo Done. http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.h (right): http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.h:21: }; On 2011/11/30 00:14:01, Sheridan Rawlins wrote: > DISALLOW_COPY_AND_ASSIGN(EditSearchEngineDialogUITest); Done. http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.js (right): http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.js:6: * Test fixture for generated tests. On 2011/11/30 00:14:01, Sheridan Rawlins wrote: > nit: s/generated/search engine dialog/ Done. http://codereview.chromium.org/8676008/diff/16001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.js:6: * Test fixture for generated tests. On 2011/11/30 00:14:01, Sheridan Rawlins wrote: > nit: s/generated/search engine dialog/ Done.
http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:9: * Flag inidicating if we are in the process of validating input. While s/inidicating/indicating http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:17: * Accessor for in entry field in the search engine dialog. s/for in/for an http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:18: * @param {string} baseName Name of the field, which servers as a base name s/servers/serves http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:54: this.text_.value = text; This could invalidate the field, or if not setting the className to invalid just remove the 'valid' className so as to not suggest the input is still valid. http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:115: function setDetails(details) { It seems really odd that we send details from the C++ side only to request validation of those details from the C++ side. I realize this isn't necessary as part of this change but perhaps leave a TODO for validating the inputs prior to passing them in and pass in validity or assume that passed in values are valid? http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:194: chrome.send('requestDetails'); We should pass all of these details in as dialog arguments. This would avoid the back and forth message passing and having the dialog not ready immediately. Leave a TODO for this? http://codereview.chromium.org/8676008/diff/20001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.js (right): http://codereview.chromium.org/8676008/diff/20001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.js:99: // each test is asynchronous, we are gauranteed to get the calls in the s/gauranteed/guaranteed
http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:9: * Flag inidicating if we are in the process of validating input. While On 2011/11/30 16:30:24, flackr wrote: > s/inidicating/indicating Done. http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:17: * Accessor for in entry field in the search engine dialog. On 2011/11/30 16:30:24, flackr wrote: > s/for in/for an Done. http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:18: * @param {string} baseName Name of the field, which servers as a base name On 2011/11/30 16:30:24, flackr wrote: > s/servers/serves Done. http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:54: this.text_.value = text; On 2011/11/30 16:30:24, flackr wrote: > This could invalidate the field, or if not setting the className to invalid just > remove the 'valid' className so as to not suggest the input is still valid. Done. http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:115: function setDetails(details) { On 2011/11/30 16:30:24, flackr wrote: > It seems really odd that we send details from the C++ side only to request > validation of those details from the C++ side. I realize this isn't necessary as > part of this change but perhaps leave a TODO for validating the inputs prior to > passing them in and pass in validity or assume that passed in values are valid? Added a TODO at the end of the initialize method. http://codereview.chromium.org/8676008/diff/20001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:194: chrome.send('requestDetails'); On 2011/11/30 16:30:24, flackr wrote: > We should pass all of these details in as dialog arguments. This would avoid the > back and forth message passing and having the dialog not ready immediately. > Leave a TODO for this? Done. http://codereview.chromium.org/8676008/diff/20001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.js (right): http://codereview.chromium.org/8676008/diff/20001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.js:99: // each test is asynchronous, we are gauranteed to get the calls in the On 2011/11/30 16:30:24, flackr wrote: > s/gauranteed/guaranteed Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kevers@chromium.org/8676008/27001
Presubmit check for 8676008-27001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/test/data/webui/edit_search_engine_dialog_browsertest.js, line 43, 81 chars ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/resources/edit_search_engine_dialog.js Presubmit checks took 1.3s to calculate.
Hi James, Can you please have a look at this CL. Missing a review from an OWNER.
Is isValidating only used in the test? http://codereview.chromium.org/8676008/diff/27001/chrome/browser/resources/ed... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/27001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:31: Remove blank line. http://codereview.chromium.org/8676008/diff/27001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:57: this.icon_.classList = ''; s/classList/className/ http://codereview.chromium.org/8676008/diff/27001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:199: // multiple callback to C++. The call to |requestDetails| Remove this comment indentation. http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc (right): http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc:19: namespace { Blank line after this. http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc:35: } // namespace Blank line before this. http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.h (right): http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.h:12: protected: You have two protected sections.
Yes, isValidating is only used in testing. The construction of the dialog is asynchronous creating a race condition in setting up the tests. The check is used to determine if initialization completed before getting the chance to override the setValidation call. In a previous iteration on the review, flackr@ suggested that we revisit the dialog in order to make the initialization process synchronous, which would in turn simplify testing. http://codereview.chromium.org/8676008/diff/27001/chrome/browser/resources/ed... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/27001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:31: On 2011/12/01 17:07:34, James Hawkins wrote: > Remove blank line. Done. http://codereview.chromium.org/8676008/diff/27001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:57: this.icon_.classList = ''; On 2011/12/01 17:07:34, James Hawkins wrote: > s/classList/className/ Done. http://codereview.chromium.org/8676008/diff/27001/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:199: // multiple callback to C++. The call to |requestDetails| On 2011/12/01 17:07:34, James Hawkins wrote: > Remove this comment indentation. Done. http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc (right): http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc:19: namespace { On 2011/12/01 17:07:34, James Hawkins wrote: > Blank line after this. Done. http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.cc:35: } // namespace On 2011/12/01 17:07:34, James Hawkins wrote: > Blank line before this. Done. http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.h (right): http://codereview.chromium.org/8676008/diff/27001/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.h:12: protected: On 2011/12/01 17:07:34, James Hawkins wrote: > You have two protected sections. Done.
LGTM. Yes, I think something is amiss because we need that variable only for testing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kevers@chromium.org/8676008/32002
Presubmit check for 8676008-32002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/test/data/webui/edit_search_engine_dialog_browsertest.js, line 43, 81 chars Presubmit checks took 1.8s to calculate.
So, why don't you use the preLoad method to set up your override so that you won't have this race (now that crrev.com/112146 has landed to support preLoad for WebUI HTML dialogs)? testfixture.preLoad() is guaranteed to be called before the resource js' DOMContentLoad events fire. On 2011/12/01 17:57:27, kevers wrote: > Yes, isValidating is only used in testing. The construction of the dialog is > asynchronous creating a race condition in setting up the tests. The check is > used to determine if initialization completed before getting the chance to > override the setValidation call. In a previous iteration on the review, flackr@ > suggested that we revisit the dialog in order to make the initialization process > synchronous, which would in turn simplify testing.
Switched to using preLoad instead of isValidating to address the race condition during initialization. James, can you please do a sanity check on the latest patch.
http://codereview.chromium.org/8676008/diff/31002/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.js (right): http://codereview.chromium.org/8676008/diff/31002/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.js:129: if (this.pendingDetails) This is effectively uninitialized. Please add pendingDetails member to prototype set to null with JSDoc description.
More nits. http://codereview.chromium.org/8676008/diff/31002/chrome/browser/resources/ed... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/31002/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:42: * @type {string} New content for the input field. one space after {string} http://codereview.chromium.org/8676008/diff/31002/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:81: * @type{Object.<string,SearchEngineDialogEntryField>} space after @type. http://codereview.chromium.org/8676008/diff/31002/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:121: chrome.send('requestValidation', [inputFields.description.value, This wrapping doesn't seem right to me as things should align with the [; not the (. http://codereview.chromium.org/8676008/diff/31002/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.h (right): http://codereview.chromium.org/8676008/diff/31002/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.h:15: void ShowSearchEngineDialog(); I'm ok with no whitespace between constructor/destructor, but I'd like whitespace and documentation before ShowSearchEngineDialog().
http://codereview.chromium.org/8676008/diff/31002/chrome/browser/resources/ed... File chrome/browser/resources/edit_search_engine_dialog.js (right): http://codereview.chromium.org/8676008/diff/31002/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:42: * @type {string} New content for the input field. On 2011/12/02 17:58:23, Sheridan Rawlins wrote: > one space after {string} Done. http://codereview.chromium.org/8676008/diff/31002/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:81: * @type{Object.<string,SearchEngineDialogEntryField>} On 2011/12/02 17:58:23, Sheridan Rawlins wrote: > space after @type. Done. http://codereview.chromium.org/8676008/diff/31002/chrome/browser/resources/ed... chrome/browser/resources/edit_search_engine_dialog.js:121: chrome.send('requestValidation', [inputFields.description.value, On 2011/12/02 17:58:23, Sheridan Rawlins wrote: > This wrapping doesn't seem right to me as things should align with the [; not > the (. Done. http://codereview.chromium.org/8676008/diff/31002/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.h (right): http://codereview.chromium.org/8676008/diff/31002/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.h:15: void ShowSearchEngineDialog(); On 2011/12/02 17:58:23, Sheridan Rawlins wrote: > I'm ok with no whitespace between constructor/destructor, but I'd like > whitespace and documentation before ShowSearchEngineDialog(). Done. http://codereview.chromium.org/8676008/diff/31002/chrome/test/data/webui/edit... File chrome/test/data/webui/edit_search_engine_dialog_browsertest.js (right): http://codereview.chromium.org/8676008/diff/31002/chrome/test/data/webui/edit... chrome/test/data/webui/edit_search_engine_dialog_browsertest.js:129: if (this.pendingDetails) On 2011/12/02 16:37:50, Sheridan Rawlins wrote: > This is effectively uninitialized. Please add pendingDetails member to > prototype set to null with JSDoc description. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kevers@chromium.org/8676008/32012
SLGTM
Change committed as 113022 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
