Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
446843002:
Browser Test: Autocomplete confused about direction if inline style and inherited dir attribute… (Closed)
|
Created:
6 years, 4 months ago by Sunil Ratnu Modified:
6 years ago CC:
chromium-reviews, benquan, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionBrowser Test for Autocomplete's confusion about direction if inline style and inherited dir attribute are mixed.
This patch provides browser_tests for the corresponding blink patch:
https://codereview.chromium.org/419023007/ which deals with the Bug:Autocomplete
confused about direction if inline style and inherited dir attribute is mixed.
BUG=397831
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288318
Patch Set 1 #
Total comments: 4
Patch Set 2 : Incorporate review comments #
Total comments: 6
Patch Set 3 : Incorporated review Comments #
Total comments: 10
Patch Set 4 : Incorporated review Comments #Messages
Total messages: 10 (0 generated)
PTAL.Thanks!
Thanks! https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... chrome/renderer/autofill/form_autofill_browsertest.cc:686: WebFormControlElementToFormFieldAutocompleteDirection) { nit: This line should be indented more, like so: TEST_F(FormAutofillTest, WebFormControlElementToFormFieldAutocompleteDirection) { https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... chrome/renderer/autofill/form_autofill_browsertest.cc:690: " <INPUT type=\"text\" id=\"element\" value=\"RTL\"/>" nit: It might be cleaner to use single quotes to surround attribute values, rather than double quotes, since single quotes don't need to be escaped. (I realize that the existing tests use double quotes. It would be great to clean this up at some point :) https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... chrome/renderer/autofill/form_autofill_browsertest.cc:690: " <INPUT type=\"text\" id=\"element\" value=\"RTL\"/>" nit: Please indent by two spaces, rather than by one. Ditto below. https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... chrome/renderer/autofill/form_autofill_browsertest.cc:708: "</FORM>"); Please write separate TEST_F test cases for each HTML form, rather than grouping them all into a single tests. Short test cases are easier to understand and to maintain. (Again, I realize the existing code is not always good about following this practice; again, this is something that would be good to clean up in this file :)
On 2014/08/06 20:45:21, Ilya Sherman wrote: > Thanks! > > https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... > File chrome/renderer/autofill/form_autofill_browsertest.cc (right): > > https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... > chrome/renderer/autofill/form_autofill_browsertest.cc:686: > WebFormControlElementToFormFieldAutocompleteDirection) { > nit: This line should be indented more, like so: > > TEST_F(FormAutofillTest, > WebFormControlElementToFormFieldAutocompleteDirection) { > > https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... > chrome/renderer/autofill/form_autofill_browsertest.cc:690: " <INPUT > type=\"text\" id=\"element\" value=\"RTL\"/>" > nit: It might be cleaner to use single quotes to surround attribute values, > rather than double quotes, since single quotes don't need to be escaped. (I > realize that the existing tests use double quotes. It would be great to clean > this up at some point :) > > https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... > chrome/renderer/autofill/form_autofill_browsertest.cc:690: " <INPUT > type=\"text\" id=\"element\" value=\"RTL\"/>" > nit: Please indent by two spaces, rather than by one. Ditto below. > > https://codereview.chromium.org/446843002/diff/1/chrome/renderer/autofill/for... > chrome/renderer/autofill/form_autofill_browsertest.cc:708: "</FORM>"); > Please write separate TEST_F test cases for each HTML form, rather than grouping > them all into a single tests. Short test cases are easier to understand and to > maintain. (Again, I realize the existing code is not always good about > following this practice; again, this is something that would be good to clean up > in this file :) Thanks Ilya for the review. I have uploaded a new patch as per your review comments. Please have a look. And regarding some of the clean ups, I will upload another patch. :)
On 2014/08/07 04:30:36, Sunil Ratnu wrote: > And regarding some of the clean ups, I will upload another patch. :) Thanks! :) https://codereview.chromium.org/446843002/diff/20001/chrome/renderer/autofill... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/446843002/diff/20001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:684: // Both text input and autocomplete should have same direction: test_case#1 Rather than just numbering the test cases, please describe what situation each test case is testing. For example, you might name this test WebFormControlElementToFormField_TextDirectionDetectedFromDirectStyle. Admittedly, that name is very long, mostly because the method name is very long. I personally think the suffix is the more important part; so just naming the test something like "TextDirectionDetectedFromDirectStyle" would be fine. (If you can find a way to fit both the method name and a descriptive test name, even better!) https://codereview.chromium.org/446843002/diff/20001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:688: "<FORM autocomplete='on'>" nit: You shouldn't need to explicitly specify autocomplete='on' -- the default state should be equally good, and is less code. In tests, the less superfluous code you have, the easier it is to see at a glance exactly what is being tested :) https://codereview.chromium.org/446843002/diff/20001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:689: " <INPUT type='text' id='element' value='RTL'/>" nit: Would this test be equally valid without a value specified on the input element? If so, please omit the value.
Thanks for the review. I've made the suggested changes and tried to cover all the different cases. Please do let me know if I've missed something. https://codereview.chromium.org/446843002/diff/20001/chrome/renderer/autofill... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/446843002/diff/20001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:684: // Both text input and autocomplete should have same direction: test_case#1 On 2014/08/07 19:53:01, Ilya Sherman wrote: > Rather than just numbering the test cases, please describe what situation each > test case is testing. For example, you might name this test > > WebFormControlElementToFormField_TextDirectionDetectedFromDirectStyle. > > Admittedly, that name is very long, mostly because the method name is very long. > I personally think the suffix is the more important part; so just naming the > test something like "TextDirectionDetectedFromDirectStyle" would be fine. (If > you can find a way to fit both the method name and a descriptive test name, even > better!) Done. https://codereview.chromium.org/446843002/diff/20001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:688: "<FORM autocomplete='on'>" On 2014/08/07 19:53:01, Ilya Sherman wrote: > nit: You shouldn't need to explicitly specify autocomplete='on' -- the default > state should be equally good, and is less code. In tests, the less superfluous > code you have, the easier it is to see at a glance exactly what is being tested > :) Done. https://codereview.chromium.org/446843002/diff/20001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:689: " <INPUT type='text' id='element' value='RTL'/>" On 2014/08/07 19:53:00, Ilya Sherman wrote: > nit: Would this test be equally valid without a value specified on the input > element? If so, please omit the value. Done.
LGTM % the remaining nits. Thanks! :) https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:720: TEST_F(FormAutofillTest, DetectTextDirectionFromDirectParentStyle) { nit: "DirectParent" -> "Parent" https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:738: TEST_F(FormAutofillTest, DetectTextDirectionFromDirectParentDIRAttribute) { nit: Ditto https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:772: // Both text input and autocomplete should have same direction: test_case#6 nit: I'd either expand these comments to be more specific about what exactly is being tested in each test case, or just omit them entirely. https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:774: DetectTextDirectionWhenParentHaveBothDIRAttributeAndStyle) { nit: "Have" -> "Has" https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:792: TEST_F(FormAutofillTest, DetectTextDirectionWhenAncestorHaveInlineStyle) { nit: "Have" -> "Has"
https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:720: TEST_F(FormAutofillTest, DetectTextDirectionFromDirectParentStyle) { On 2014/08/08 04:59:31, Ilya Sherman wrote: > nit: "DirectParent" -> "Parent" Done. https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:738: TEST_F(FormAutofillTest, DetectTextDirectionFromDirectParentDIRAttribute) { On 2014/08/08 04:59:31, Ilya Sherman wrote: > nit: Ditto Done. https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:772: // Both text input and autocomplete should have same direction: test_case#6 Removing them entirely as the names are self explantory. On 2014/08/08 04:59:32, Ilya Sherman wrote: > nit: I'd either expand these comments to be more specific about what exactly is > being tested in each test case, or just omit them entirely. https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:774: DetectTextDirectionWhenParentHaveBothDIRAttributeAndStyle) { On 2014/08/08 04:59:31, Ilya Sherman wrote: > nit: "Have" -> "Has" Done. https://codereview.chromium.org/446843002/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/form_autofill_browsertest.cc:792: TEST_F(FormAutofillTest, DetectTextDirectionWhenAncestorHaveInlineStyle) { On 2014/08/08 04:59:32, Ilya Sherman wrote: > nit: "Have" -> "Has" Done.
The CQ bit was checked by sunil.ratnu@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sunil.ratnu@samsung.com/446843002/60001
Message was sent while issue was closed.
Change committed as 288318 |
