Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8228009:
Adding more bidichecker tests and doing some minor cleanups. (Closed)
DescriptionAdding more bidichecker tests and doing some minor cleanups.
BUG=NONE
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110861
Patch Set 1 #
Total comments: 21
Patch Set 2 : A bunch of cleanups as discussed with Jeremy #
Total comments: 22
Patch Set 3 : Fixing comments #
Total comments: 12
Patch Set 4 : More comments fixes #
Total comments: 4
Patch Set 5 : Style fixes #Patch Set 6 : Bringing things up to date #
Messages
Total messages: 11 (0 generated)
Hi Jeremy, do you mind to take a look? These additional tests found 7 additional bugs.
http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right): http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:56: if (pageURL) What is this line supposed to do? http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:107: IN_PROC_BROWSER_TEST_F(WebUIBidiCheckerBrowserTest, TestMainHistoryPage) { This only tests the page as LTR, right so why rename the test? http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:137: "chrome://bugreport#0?description=%D7%91%D7%93%D7%99%D7%A7%D7%94&issueType=1"); Could you add a comment explaining what the weird escaped characters are? http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:146: RunBidiCheckerOnPage(chrome::kChromeUICrashesURL); I'm pretty sure about:crash isn't HTML and that you can remove this test? http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:195: #define MAYBE_TestSettingsAutofillPage FLAKY_TestSettingsAutofillPage Are these tests still flaky? Do you know what the status of the fix for them is? http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:267: GURL testGURL = GURL("http://ynet.co.il"); Just to be sure - there's no chance we actually try to load the site when this test is run, right? http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:271: browser()->profile()->GetHistoryService(Profile::IMPLICIT_ACCESS); Indent http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:274: // This is the real title of the site nit:End comments with a '.' http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:281: 71, put the above in a char[] and use array_size() here. http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:300: GURL testGURL = GURL("http://google.com"); Since both these tests do the same thing, can you put them into a common function? http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... File chrome/browser/ui/webui/bidi_checker_web_ui_test.h (right): http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.h:22: virtual void RunBidiCheckerOnPage(const char pageURL[]); I would just define 2 functions and make the above function private. Named something like: virtual void RunBidiCheckerOnPageAsLTR(); virtual void RunBidiCheckerOnPageAsRTL(); http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.h:37: virtual void RunBidiCheckerOnPage(const char pageURL[]); specify OVERRIDE in the function definition. http://codereview.chromium.org/8228009/diff/1/chrome/test/data/webui/bidichec... File chrome/test/data/webui/bidichecker_tests.js (right): http://codereview.chromium.org/8228009/diff/1/chrome/test/data/webui/bidichec... chrome/test/data/webui/bidichecker_tests.js:66: // BUG: http://crbug.com/90835 Could you please assign owners for all the new bugs? you can find the right people by doing a git blame on the appropriate pages...
Do you mind to take another look on this? http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right): http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:56: if (pageURL) On 2011/10/16 12:54:59, jeremy wrote: > What is this line supposed to do? Hmm, no idea. Removed. http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:137: "chrome://bugreport#0?description=%D7%91%D7%93%D7%99%D7%A7%D7%94&issueType=1"); On 2011/10/16 12:54:59, jeremy wrote: > Could you add a comment explaining what the weird escaped characters are? Done. http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:146: RunBidiCheckerOnPage(chrome::kChromeUICrashesURL); On 2011/10/16 12:54:59, jeremy wrote: > I'm pretty sure about:crash isn't HTML and that you can remove this test? This is not about:crash but about:crashes, which I just verified as HTML. http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:267: GURL testGURL = GURL("http://ynet.co.il"); On 2011/10/16 12:54:59, jeremy wrote: > Just to be sure - there's no chance we actually try to load the site when this > test is run, right? Right. The only way to get to the site is by manually clicking the link in the history page. http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:271: browser()->profile()->GetHistoryService(Profile::IMPLICIT_ACCESS); On 2011/10/16 12:54:59, jeremy wrote: > Indent Done. http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:281: 71, On 2011/10/16 12:54:59, jeremy wrote: > put the above in a char[] and use array_size() here. No need, found a cleaner alternative. http://codereview.chromium.org/8228009/diff/1/chrome/browser/ui/webui/bidi_ch... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:300: GURL testGURL = GURL("http://google.com"); On 2011/10/16 12:54:59, jeremy wrote: > Since both these tests do the same thing, can you put them into a common > function? Done. http://codereview.chromium.org/8228009/diff/1/chrome/test/data/webui/bidichec... File chrome/test/data/webui/bidichecker_tests.js (right): http://codereview.chromium.org/8228009/diff/1/chrome/test/data/webui/bidichec... chrome/test/data/webui/bidichecker_tests.js:66: // BUG: http://crbug.com/90835 On 2011/10/16 12:54:59, jeremy wrote: > Could you please assign owners for all the new bugs? you can find the right > people by doing a git blame on the appropriate pages... Done.
http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right): http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. General comments for the tests in this file: * Please structure so for each page you have an RTL test followed by an LTR test or vice versa. * For tests which only have an LTR case, please add a comment explaining why you don't need an RTL case. * For string literals that make the lines longer than 80 chars you can use string continuations, e.g. take advantage of the fact that c allows you to continue strings on the following line: "foo" "bar" yields the string literal "foobar". * Comment before each test series with the URL of the page used, e.g.: // about:crashes ...code... // about:plugins ...code... http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:69: // WebUIBidiCheckerBrowserTestFakeBidi I think you can remove this and the above comment. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:107: browser()->profile()->GetHistoryService(Profile::IMPLICIT_ACCESS); Can you split the setup code out into a common function so that you don't have code duplication between this and the RTL case. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:112: 12, Why can't you use the single argument version of UTF8ToUTF16() like the RTL case? Also probably better to use array_size() rather than hardcoding string length. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:136: "chrome://bugreport#0?description=%D7%91%D7%93%D7%99%D7%A7%D7%94&issueType=1"); >80 chars split into 2 lines: "foo" "bar" http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:187: // http://crbug.com/94642 Any updates on this bug? http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:225: PersonalDataManagerFactory::GetForProfile(profile); indent http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:237: "\xD7\x91\xD7\x93\xD7\x99\xD7\xA7\xD7\x94\x20\xD7\x91\xD7\xA2\xD7\x9E", >80 chars, use string continuation, also bellow. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:251: TestSettingsAutofillPage) { MAYBE_Test... ? http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:287: TestSettingsBorwserOptionsPage) { *Browser http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... File chrome/browser/ui/webui/bidi_checker_web_ui_test.h (right): http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.h:32: virtual void RunBidiCheckerOnPage(const char pageURL[]); Why is this virtual?
Fixed comments. Do you mind to take a look? http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right): http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/10/25 09:19:53, jeremy wrote: > General comments for the tests in this file: > * Please structure so for each page you have an RTL test followed by an LTR test > or vice versa. > * For tests which only have an LTR case, please add a comment explaining why you > don't need an RTL case. > * For string literals that make the lines longer than 80 chars you can use > string continuations, e.g. take advantage of the fact that c allows you to > continue strings on the following line: > > "foo" > "bar" > > yields the string literal "foobar". > * Comment before each test series with the URL of the page used, e.g.: > > // about:crashes > ...code... > > // about:plugins > ...code... Done. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:69: // WebUIBidiCheckerBrowserTestFakeBidi On 2011/10/25 09:19:53, jeremy wrote: > I think you can remove this and the above comment. Done. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:107: browser()->profile()->GetHistoryService(Profile::IMPLICIT_ACCESS); On 2011/10/25 09:19:53, jeremy wrote: > Can you split the setup code out into a common function so that you don't have > code duplication between this and the RTL case. Done. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:112: 12, On 2011/10/25 09:19:53, jeremy wrote: > Why can't you use the single argument version of UTF8ToUTF16() like the RTL > case? Also probably better to use array_size() rather than hardcoding string > length. Done. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:136: "chrome://bugreport#0?description=%D7%91%D7%93%D7%99%D7%A7%D7%94&issueType=1"); On 2011/10/25 09:19:53, jeremy wrote: > >80 chars split into 2 lines: > "foo" > "bar" Done. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:187: // http://crbug.com/94642 On 2011/10/25 09:19:53, jeremy wrote: > Any updates on this bug? Checking. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:225: PersonalDataManagerFactory::GetForProfile(profile); On 2011/10/25 09:19:53, jeremy wrote: > indent Done. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:237: "\xD7\x91\xD7\x93\xD7\x99\xD7\xA7\xD7\x94\x20\xD7\x91\xD7\xA2\xD7\x9E", On 2011/10/25 09:19:53, jeremy wrote: > >80 chars, use string continuation, also bellow. Done. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:251: TestSettingsAutofillPage) { On 2011/10/25 09:19:53, jeremy wrote: > MAYBE_Test... ? No problem with the RTL version http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:287: TestSettingsBorwserOptionsPage) { On 2011/10/25 09:19:53, jeremy wrote: > *Browser Done. http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... File chrome/browser/ui/webui/bidi_checker_web_ui_test.h (right): http://codereview.chromium.org/8228009/diff/6001/chrome/browser/ui/webui/bidi... chrome/browser/ui/webui/bidi_checker_web_ui_test.h:32: virtual void RunBidiCheckerOnPage(const char pageURL[]); On 2011/10/25 09:19:53, jeremy wrote: > Why is this virtual? Leftover. Fixed.
http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right): http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:101: // chrome://history //=== lines are missing for this one. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:103: static void SetupTestHistoryPage(Browser* browser, nit: I think this would be clearer if all these functions were named: SetupFooTest rather than SetupTestFoo . http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:105: std::string page_title) { nit: pass these in as const references. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:144: "&issueType=1"); I think string continuations should be lined up without extra 4 space indenty. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... File chrome/browser/ui/webui/bidi_checker_web_ui_test.h (right): http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.h:31: public: one space before public. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.h:37: // active language. I think you can remove lines 2-3 of this comment.
Any other comments? http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right): http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:101: // chrome://history On 2011/10/30 09:59:19, jeremy wrote: > //=== lines are missing for this one. Done. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:103: static void SetupTestHistoryPage(Browser* browser, On 2011/10/30 09:59:19, jeremy wrote: > nit: I think this would be clearer if all these functions were named: > SetupFooTest rather than SetupTestFoo . Done. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:105: std::string page_title) { On 2011/10/30 09:59:19, jeremy wrote: > nit: pass these in as const references. Done. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:144: "&issueType=1"); On 2011/10/30 09:59:19, jeremy wrote: > I think string continuations should be lined up without extra 4 space indenty. Done. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... File chrome/browser/ui/webui/bidi_checker_web_ui_test.h (right): http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.h:31: public: On 2011/10/30 09:59:19, jeremy wrote: > one space before public. Done. http://codereview.chromium.org/8228009/diff/11001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.h:37: // active language. On 2011/10/30 09:59:19, jeremy wrote: > I think you can remove lines 2-3 of this comment. Done.
LGTM http://codereview.chromium.org/8228009/diff/17001/chrome/browser/ui/webui/bid... File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right): http://codereview.chromium.org/8228009/diff/17001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:116: // Test an Israeli news site with a Hebrew title nit: . at end of comment. http://codereview.chromium.org/8228009/diff/17001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:324: "\x79\x6E\x65\x74\x20\xD7\x97\xD7\x93\xD7\xA9" >80 chars ?
http://codereview.chromium.org/8228009/diff/17001/chrome/browser/ui/webui/bid... File chrome/browser/ui/webui/bidi_checker_web_ui_test.cc (right): http://codereview.chromium.org/8228009/diff/17001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:116: // Test an Israeli news site with a Hebrew title On 2011/10/30 13:08:56, jeremy wrote: > nit: . at end of comment. Done. http://codereview.chromium.org/8228009/diff/17001/chrome/browser/ui/webui/bid... chrome/browser/ui/webui/bidi_checker_web_ui_test.cc:324: "\x79\x6E\x65\x74\x20\xD7\x97\xD7\x93\xD7\xA9" On 2011/10/30 13:08:56, jeremy wrote: > >80 chars ? It's exactly 80 but broke it down more anyhow.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ofri@chromium.org/8228009/23001
Presubmit check for 8228009-23001 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/bidichecker_tests.js, line 64, 85 chars \ chrome/test/data/webui/bidichecker_tests.js, line 79, 110 chars Presubmit checks took 1.3s to calculate. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
