Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6033010:
Support autocompletion for HTMl5 tags:"email", "month" and "tel". (Closed)
|
Created:
9 years, 11 months ago by honten.org Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr., darin-cc_chromium.org, brettw-cc_chromium.org, James Hawkins, dhollowa, Ilya Sherman Visibility:
Public. |
DescriptionSupport autocompletion for HTMl5 tags:"email", "month" and "tel".
BUG=65654
TEST=1.make HTML5 forms with the following input types: - Email input:
<input type="email"> - Date picker: <input type="month"> for credit card
expiration. - Telephone input: <input type="tel"> - Placeholder text:
<input name="address" placeholder="1 Main Street"/> 2.Try to fill.
Implement tests for email, month and tel input type.
Also this patch needs WebKit patch, please refer to
https://bugs.webkit.org/show_bug.cgi?id=51809
Actually, original issue item says placeholder problem, but I cannot reproduce manually.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73660
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix typo, format errors. #
Total comments: 5
Patch Set 3 : Add more tests, fix some format errors and change parsing. #
Total comments: 29
Patch Set 4 : unroll tests. #
Total comments: 5
Patch Set 5 : Fix format errors and other naming issues. #
Total comments: 26
Patch Set 6 : Fix format errors and change to use toWebInputElement(). #
Total comments: 8
Patch Set 7 : Fix format errors. #
Total comments: 2
Patch Set 8 : Delete static in anonymous namespace. #Patch Set 9 : Update the latest source. #Patch Set 10 : Fix merge error. #Messages
Total messages: 62 (0 generated)
Could you review my change?
-estade, +dhollowa
http://codereview.chromium.org/6033010/diff/1/chrome/browser/autofill/autofil... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6033010/diff/1/chrome/browser/autofill/autofil... chrome/browser/autofill/autofill_manager.cc:692: // THML5 input="month" consisnts of year-month. Spelling errors in this comment. http://codereview.chromium.org/6033010/diff/1/chrome/browser/autofill/autofil... chrome/browser/autofill/autofill_manager.cc:694: credit_card->GetFieldText(AutoFillType(CREDIT_CARD_EXP_4_DIGIT_YEAR)) + Please extract a variable from these lines and send that to set_value. Note that the style guide says each continuation of one line must be indented four spaces from the previous line. Personally, I'd do: string16 month = credit_card->GetFieldText(...); month += ASCIIToUTF16("-"); month += credit_card->GetFieldText(...); http://codereview.chromium.org/6033010/diff/1/chrome/browser/autofill/autofil... chrome/browser/autofill/autofill_manager.cc:695: string16(ASCIIToUTF16("-")) + No need to wrap with string16(). http://codereview.chromium.org/6033010/diff/1/chrome/renderer/form_manager.cc File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/1/chrome/renderer/form_manager.cc... chrome/renderer/form_manager.cc:276: // In HTML5, email,month and tel are text input field to autocomplete. space after comma
Thanks, James I updated some format and typo issues. Please review my change. On 2011/01/03 19:50:00, James Hawkins wrote: > http://codereview.chromium.org/6033010/diff/1/chrome/browser/autofill/autofil... > File chrome/browser/autofill/autofill_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/1/chrome/browser/autofill/autofil... > chrome/browser/autofill/autofill_manager.cc:692: // THML5 input="month" > consisnts of year-month. > Spelling errors in this comment. > > http://codereview.chromium.org/6033010/diff/1/chrome/browser/autofill/autofil... > chrome/browser/autofill/autofill_manager.cc:694: > credit_card->GetFieldText(AutoFillType(CREDIT_CARD_EXP_4_DIGIT_YEAR)) + > Please extract a variable from these lines and send that to set_value. Note > that the style guide says each continuation of one line must be indented four > spaces from the previous line. > > Personally, I'd do: > > string16 month = credit_card->GetFieldText(...); > month += ASCIIToUTF16("-"); > month += credit_card->GetFieldText(...); > > http://codereview.chromium.org/6033010/diff/1/chrome/browser/autofill/autofil... > chrome/browser/autofill/autofill_manager.cc:695: string16(ASCIIToUTF16("-")) + > No need to wrap with string16(). > > http://codereview.chromium.org/6033010/diff/1/chrome/renderer/form_manager.cc > File chrome/renderer/form_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/1/chrome/renderer/form_manager.cc... > chrome/renderer/form_manager.cc:276: // In HTML5, email,month and tel are text > input field to autocomplete. > space after comma
It looks like the WebKit code has changed. I'm noticing the method HTMLInputElement::setSuggestedValue() uses a canSetSuggestedValue() method now. http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_manager.cc:692: // HTML5 input="month" consisnts of year-month. It is possible that that the |credit_card| has empty year and/or empty month. We should fill "month" control type only if the |credit_card| has both year and month present. Also, please add unit tests for each case, that is: 1. year empty, month empty 2. year empty, month non-empty 3. year non-empty, month empty 4. both non-emtpy http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_manager.cc:692: // HTML5 input="month" consisnts of year-month. nit: s/consisnts/consists/ http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... chrome/browser/autofill/credit_card_field.cc:135: if ((!credit_card_field->expiration_month_ || The handling of input type="month" should be split out separately. We can match purely on field type in this case. I.e. pseudocode: if (*q->form_control_type() == "month") { credit_card_field->expiration_month_ = *q++; } else { ...normal logic for text fields, lines 118:148... } and then we'll have to add special parsing for month fields that have "month" type, otherwise callers of |CreditCard::ConvertDate| will be parsing the month incorrectly. For example, if you submit a form currently you'll get a DCHECK at the following stack: CreditCard::ConvertDate at credit_card.cc:616 CreditCard::SetExpirationMonthFromString at credit_card.cc:487 CreditCard::SetInfo at credit_card.cc:300 PersonalDataManager::ImportFormData at personal_data_manager.cc:172 AutoFillManager::HandleSubmit at autofill_manager.cc:478 http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... chrome/browser/autofill/credit_card_field.cc:188: credit_card_field->expiration_month_->form_control_type(), "month")))) { nit: indentation seems off here. line 188 should be indented in another 4 spaces, and then the "month" param will wrap.
http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... chrome/browser/autofill/credit_card_field.cc:135: if ((!credit_card_field->expiration_month_ || Ok, So can I add new AutoFillType CREDIT_CARD_EXP_YEAR_MONTH? for "month" input type. Or should I share CREDIT_CARD_EXP_MONTH? even if "month" input type? On 2011/01/05 03:08:26, dhollowa wrote: > The handling of input type="month" should be split out separately. We can match > purely on field type in this case. I.e. pseudocode: > > if (*q->form_control_type() == "month") { > credit_card_field->expiration_month_ = *q++; > } else { > ...normal logic for text fields, lines 118:148... > } > > and then we'll have to add special parsing for month fields that have "month" > type, otherwise callers of |CreditCard::ConvertDate| will be parsing the month > incorrectly. > > For example, if you submit a form currently you'll get a DCHECK at the following > stack: > > > CreditCard::ConvertDate at credit_card.cc:616 > CreditCard::SetExpirationMonthFromString at credit_card.cc:487 > CreditCard::SetInfo at credit_card.cc:300 > PersonalDataManager::ImportFormData at personal_data_manager.cc:172 > AutoFillManager::HandleSubmit at autofill_manager.cc:478
AutoFillType's are shared with server data, so they're difficult to change (or add to). If we can use the context of the type="month" of the field in all cases where we need it, then it should be fine to use CREDIT_CARD_EXP_MONTH, and determine formatting from context. On 2011/01/05 08:09:09, honten wrote: > http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... > File chrome/browser/autofill/credit_card_field.cc (right): > > http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... > chrome/browser/autofill/credit_card_field.cc:135: if > ((!credit_card_field->expiration_month_ || > Ok, > > So can I add new AutoFillType CREDIT_CARD_EXP_YEAR_MONTH? for "month" input > type. > Or should I share CREDIT_CARD_EXP_MONTH? even if "month" input type? > > On 2011/01/05 03:08:26, dhollowa wrote: > > The handling of input type="month" should be split out separately. We can > match > > purely on field type in this case. I.e. pseudocode: > > > > if (*q->form_control_type() == "month") { > > credit_card_field->expiration_month_ = *q++; > > } else { > > ...normal logic for text fields, lines 118:148... > > } > > > > and then we'll have to add special parsing for month fields that have "month" > > type, otherwise callers of |CreditCard::ConvertDate| will be parsing the month > > incorrectly. > > > > For example, if you submit a form currently you'll get a DCHECK at the > following > > stack: > > > > > > CreditCard::ConvertDate at credit_card.cc:616 > > CreditCard::SetExpirationMonthFromString at credit_card.cc:487 > > CreditCard::SetInfo at credit_card.cc:300 > > PersonalDataManager::ImportFormData at personal_data_manager.cc:172 > > AutoFillManager::HandleSubmit at autofill_manager.cc:478
I see, So I'll add extra parser into CreditCard::SetExpirationMonthFromString(), and chare CREDIT_CARD_EXP_MONTH. Thanks, On Wed, Jan 5, 2011 at 7:44 AM, <dhollowa@chromium.org> wrote: > AutoFillType's are shared with server data, so they're difficult to change > (or > add to). If we can use the context of the type="month" of the field in all > cases where we need it, then it should be fine to use CREDIT_CARD_EXP_MONTH, > and > determine formatting from context. > > On 2011/01/05 08:09:09, honten wrote: > > http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... >> >> File chrome/browser/autofill/credit_card_field.cc (right): > > > http://codereview.chromium.org/6033010/diff/7001/chrome/browser/autofill/cred... >> >> chrome/browser/autofill/credit_card_field.cc:135: if >> ((!credit_card_field->expiration_month_ || >> Ok, > >> So can I add new AutoFillType CREDIT_CARD_EXP_YEAR_MONTH? for "month" >> input >> type. >> Or should I share CREDIT_CARD_EXP_MONTH? even if "month" input type? > >> On 2011/01/05 03:08:26, dhollowa wrote: >> > The handling of input type="month" should be split out separately. We >> > can >> match >> > purely on field type in this case. I.e. pseudocode: >> > >> > if (*q->form_control_type() == "month") { >> > credit_card_field->expiration_month_ = *q++; >> > } else { >> > ...normal logic for text fields, lines 118:148... >> > } >> > >> > and then we'll have to add special parsing for month fields that have > > "month" >> >> > type, otherwise callers of |CreditCard::ConvertDate| will be parsing the > > month >> >> > incorrectly. >> > >> > For example, if you submit a form currently you'll get a DCHECK at the >> following >> > stack: >> > >> > >> > CreditCard::ConvertDate at credit_card.cc:616 >> > CreditCard::SetExpirationMonthFromString at credit_card.cc:487 >> > CreditCard::SetInfo at credit_card.cc:300 >> > PersonalDataManager::ImportFormData at personal_data_manager.cc:172 >> > AutoFillManager::HandleSubmit at autofill_manager.cc:478 > > > > http://codereview.chromium.org/6033010/ >
Please review my change. I don't check the new WebKit source yet, so don't know we have to change WebKit source. It looks Ok because TextInputType::canSetSuggestedValue() returns always true. But I don't confirm it. Still I'm building the new source code, so I can notice until tomorrow. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:704: field->set_value(credit_card->GetFieldText(type)); As you pointed out, only both are not empty, set the value. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:156: } Make four credit cards for year month combination checking. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card.cc:480: Use regular expression to check yyyy-mm. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card.h (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card.h:78: Implemented special method to parse month input type. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:172: imported_credit_card_->SetMonthInputInfo(value); Only "month" input type, call SetMonthInputInfo().
dhollowa, Unfortunately, still I need to change in WebKit, because I notice I have to change MonthInputType, EmailInputType and TelInputType need to be changed. But it's not big deal. Anyway, except that, it should be Ok. So please review it. I'll update WebKit patch tonight again. Thanks, On Wed, Jan 5, 2011 at 11:04 PM, <Takano.Naoki@gmail.com> wrote: > Please review my change. > > I don't check the new WebKit source yet, so don't know we have to change > WebKit > source. > It looks Ok because TextInputType::canSetSuggestedValue() returns always > true. > But I don't confirm it. > > Still I'm building the new source code, so I can notice until tomorrow. > > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... > File chrome/browser/autofill/autofill_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_manager.cc:704: > field->set_value(credit_card->GetFieldText(type)); > As you pointed out, only both are not empty, set the value. > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... > File chrome/browser/autofill/autofill_manager_unittest.cc (right): > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_manager_unittest.cc:156: } > Make four credit cards for year month combination checking. > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... > File chrome/browser/autofill/credit_card.cc (right): > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... > chrome/browser/autofill/credit_card.cc:480: > Use regular expression to check yyyy-mm. > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... > File chrome/browser/autofill/credit_card.h (right): > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... > chrome/browser/autofill/credit_card.h:78: > Implemented special method to parse month input type. > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/per... > File chrome/browser/autofill/personal_data_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/per... > chrome/browser/autofill/personal_data_manager.cc:172: > imported_credit_card_->SetMonthInputInfo(value); > Only "month" input type, call SetMonthInputInfo(). > > http://codereview.chromium.org/6033010/ >
http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:143: for (int i = 0; i < 4; i++) { I'm a lazy reader. It'd be easier on me if we unroll this loop and set the values explicitly. I like to save the thinking for "real" code. ;-) http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1357: for (int i = 0; i < 4; ++i) { Same here. I'd prefer to unroll the loop with these. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card.cc:480: Nice. On 2011/01/06 07:04:54, honten wrote: > Use regular expression to check yyyy-mm. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:118: if ((*q) && LowerCaseEqualsASCII((*q)->form_control_type(), "month")) { This is looking good. However, I tried it against a form with input element: <label for="random_id">HTML5 M-o-n-t-h:</label> <input type="month" id="random_id"><br> And it wasn't matching. I would have thought it should... See: http://www/~dhollowa/autofill/html5_65654b.html For the full example. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:142: // If type="month", year is included into the expiration_month_. This comment can be removed now. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:174: imported_credit_card_->SetInfo( nit: according to style guide we need braces {} around if/else bodies due to line-wrap.
http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:118: if ((*q) && LowerCaseEqualsASCII((*q)->form_control_type(), "month")) { I can't see your full example. Did you put your example in Google office? Sorry, I'm not Googler... Could you attach it? On 2011/01/07 03:47:15, dhollowa wrote: > This is looking good. > > However, I tried it against a form with input element: > > <label for="random_id">HTML5 M-o-n-t-h:</label> > <input type="month" id="random_id"><br> > > And it wasn't matching. I would have thought it should... See: > > http://www/%7Edhollowa/autofill/html5_65654b.html > > For the full example. >
I update, but I cannot revise credit_card_field.cc correctly because I cannot reproduce. Also I renewed WebKit patch and uploaded it. Please find it. Thanks, http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:122: credit_card_field->expiration_month_ = *q++; I cannot reproduce your bug. I made form as following, <form id="gaia_loginform" action="https://www.google.com" method="post"> <label for="nameoncard">Card Holder</label> <input type="text" name="nameoncard" /><br /> <label for="CardNumber">Card Number</label> <input type="text" name="card_number" /><br /> <label for="random_id">HTML5 M-o-n-t-h:</label> <input type="month" id="random_id"><br> <input type="submit" name="submit" value="ok"><br /> Could you give me your full sample?
http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:699: // Because |credit_card| might not have both info. nit: This comment might be clearer as "Fill the value only if |credit_card| includes both year and month information." http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:51: explicit TestPersonalDataManager(bool four_credit_cards_with_year_month) { Rather than adding a boolean here, please add an |AddCreditCard()| method and use that + the ClearCreditCards() method. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:304: int use_month_type) { nit: bool rather than int http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:385: std::string year_month; nit: "date" might be a better name http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1357: for (int i = 0; i < 4; ++i) { On 2011/01/07 03:47:15, dhollowa wrote: > Same here. I'd prefer to unroll the loop with these. I think it's reasonable to preserve the loop, but unroll the credit card creation. One way to do this is to create an array of the four credit cards and the corresponding expectations, and loop over that. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card.h (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card.h:77: void SetMonthInputInfo(const string16& value); nit: "SetInfoForMonthInputType" might be a slightly clearer (though also slightly longer) name for this function. Also, I think it would be better to declare this closer to where SetInfo() is declared. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:151: } nit: This is a one-line if-stmt, so no need for braces. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:172: imported_credit_card_->SetMonthInputInfo(value); It might be good to add a DCHECK here that the |field_type| is what we expect. http://codereview.chromium.org/6033010/diff/15001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:278: static const char* type_names[] = {"text", "email", "tel", "month"}; Can we just return |element.isTextField() && !element.isPasswordField()| here? (The function would then need to take a WebInputElement rather than a WebFormControlElement as the parameter)
Ilya, Thank you for your comment. Could you give me your thought for my question? Thanks, http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1357: for (int i = 0; i < 4; ++i) { I see... I'll change it again. On 2011/01/07 07:28:28, Ilya Sherman wrote: > On 2011/01/07 03:47:15, dhollowa wrote: > > Same here. I'd prefer to unroll the loop with these. > > I think it's reasonable to preserve the loop, but unroll the credit card > creation. One way to do this is to create an array of the four credit cards and > the corresponding expectations, and loop over that. http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card.h (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card.h:77: void SetMonthInputInfo(const string16& value); Yes, I wanted to declare it right after SetInfo(), but SetInfo() is virtual function, as you know. That's why I declared here. Or we don't have to care about "virtual" here? On 2011/01/07 07:28:28, Ilya Sherman wrote: > nit: "SetInfoForMonthInputType" might be a slightly clearer (though also > slightly longer) name for this function. Also, I think it would be better to > declare this closer to where SetInfo() is declared.
http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card.h (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card.h:77: void SetMonthInputInfo(const string16& value); On 2011/01/07 08:06:17, honten wrote: > Yes, I wanted to declare it right after SetInfo(), but SetInfo() is virtual > function, as you know. > That's why I declared here. > > Or we don't have to care about "virtual" here? I would declare it right below that block of virtual functions, like so: virtual string16 GetPreviewText(const AutoFillType& type) const; virtual void SetInfo(const AutoFillType& type, const string16& value); virtual const string16 Label() const; void SetInfoForMonthInputField(const string16& value); > > On 2011/01/07 07:28:28, Ilya Sherman wrote: > > nit: "SetInfoForMonthInputType" might be a slightly clearer (though also > > slightly longer) name for this function. Also, I think it would be better to > > declare this closer to where SetInfo() is declared. >
Ok, that makes sense to me. Thanks, On 2011/01/07 08:15:48, Ilya Sherman wrote: > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... > File chrome/browser/autofill/credit_card.h (right): > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... > chrome/browser/autofill/credit_card.h:77: void SetMonthInputInfo(const string16& > value); > On 2011/01/07 08:06:17, honten wrote: > > Yes, I wanted to declare it right after SetInfo(), but SetInfo() is virtual > > function, as you know. > > That's why I declared here. > > > > Or we don't have to care about "virtual" here? > > I would declare it right below that block of virtual functions, like so: > > virtual string16 GetPreviewText(const AutoFillType& type) const; > virtual void SetInfo(const AutoFillType& type, const string16& value); > virtual const string16 Label() const; > > void SetInfoForMonthInputField(const string16& value); > > > > > On 2011/01/07 07:28:28, Ilya Sherman wrote: > > > nit: "SetInfoForMonthInputType" might be a slightly clearer (though also > > > slightly longer) name for this function. Also, I think it would be better > to > > > declare this closer to where SetInfo() is declared. > >
Try here: http://www.slugworth.com/html5_65654b.html On 2011/01/07 04:05:59, honten wrote: > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... > File chrome/browser/autofill/credit_card_field.cc (right): > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... > chrome/browser/autofill/credit_card_field.cc:118: if ((*q) && > LowerCaseEqualsASCII((*q)->form_control_type(), "month")) { > I can't see your full example. Did you put your example in Google office? > Sorry, I'm not Googler... > > Could you attach it? > > On 2011/01/07 03:47:15, dhollowa wrote: > > This is looking good. > > > > However, I tried it against a form with input element: > > > > <label for="random_id">HTML5 M-o-n-t-h:</label> > > <input type="month" id="random_id"><br> > > > > And it wasn't matching. I would have thought it should... See: > > > > http://www/%257Edhollowa/autofill/html5_65654b.html > > > > For the full example. > >
Great!! Thanks, On Fri, Jan 7, 2011 at 10:20 AM, <dhollowa@chromium.org> wrote: > Try here: > > http://www.slugworth.com/html5_65654b.html > > > On 2011/01/07 04:05:59, honten wrote: > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... >> >> File chrome/browser/autofill/credit_card_field.cc (right): > > > http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/cre... >> >> chrome/browser/autofill/credit_card_field.cc:118: if ((*q) && >> LowerCaseEqualsASCII((*q)->form_control_type(), "month")) { >> I can't see your full example. Did you put your example in Google office? >> Sorry, I'm not Googler... > >> Could you attach it? > >> On 2011/01/07 03:47:15, dhollowa wrote: >> > This is looking good. >> > >> > However, I tried it against a form with input element: >> > >> > <label for="random_id">HTML5 M-o-n-t-h:</label> >> > <input type="month" id="random_id"><br> >> > >> > And it wasn't matching. I would have thought it should... See: >> > >> > http://www/%257Edhollowa/autofill/html5_65654b.html >> > >> > For the full example. >> > > > > > http://codereview.chromium.org/6033010/ >
http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1357: for (int i = 0; i < 4; ++i) { No, don't! No loops! On 2011/01/07 08:06:17, honten wrote: > I see... I'll change it again. > > On 2011/01/07 07:28:28, Ilya Sherman wrote: > > On 2011/01/07 03:47:15, dhollowa wrote: > > > Same here. I'd prefer to unroll the loop with these. > > > > I think it's reasonable to preserve the loop, but unroll the credit card > > creation. One way to do this is to create an array of the four credit cards > and > > the corresponding expectations, and loop over that. >
http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1357: for (int i = 0; i < 4; ++i) { So... which should I choose???? Could you discuss between you guys? On 2011/01/07 18:44:13, dhollowa wrote: > No, don't! No loops! > > On 2011/01/07 08:06:17, honten wrote: > > I see... I'll change it again. > > > > On 2011/01/07 07:28:28, Ilya Sherman wrote: > > > On 2011/01/07 03:47:15, dhollowa wrote: > > > > Same here. I'd prefer to unroll the loop with these. > > > > > > I think it's reasonable to preserve the loop, but unroll the credit card > > > creation. One way to do this is to create an array of the four credit cards > > and > > > the corresponding expectations, and loop over that. > > >
http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1357: for (int i = 0; i < 4; ++i) { BTW, if the no-loop is fine, is my latest change Ok, dhollowa? I separate this loop to four test functions. Is this fine? Please refer to the latest patch. Thanks, On 2011/01/07 18:45:52, honten wrote: > So... which should I choose???? > > Could you discuss between you guys? > > On 2011/01/07 18:44:13, dhollowa wrote: > > No, don't! No loops! > > > > On 2011/01/07 08:06:17, honten wrote: > > > I see... I'll change it again. > > > > > > On 2011/01/07 07:28:28, Ilya Sherman wrote: > > > > On 2011/01/07 03:47:15, dhollowa wrote: > > > > > Same here. I'd prefer to unroll the loop with these. > > > > > > > > I think it's reasonable to preserve the loop, but unroll the credit card > > > > creation. One way to do this is to create an array of the four credit > cards > > > and > > > > the corresponding expectations, and loop over that. > > > > > >
http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:139: pattern = ASCIIToUTF16("expir|exp.*month|exp date|ccmonth"); These regexes should be pulled from resources. It looks like you're merge from trunk lost some of these changes.
http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1357: for (int i = 0; i < 4; ++i) { Yes, I like the latest unrolled version. Thanks. On 2011/01/07 18:48:31, honten wrote: > BTW, if the no-loop is fine, is my latest change Ok, dhollowa? > > I separate this loop to four test functions. Is this fine? Please refer to the > latest patch. > > Thanks, > > On 2011/01/07 18:45:52, honten wrote: > > So... which should I choose???? > > > > Could you discuss between you guys? > > > > On 2011/01/07 18:44:13, dhollowa wrote: > > > No, don't! No loops! > > > > > > On 2011/01/07 08:06:17, honten wrote: > > > > I see... I'll change it again. > > > > > > > > On 2011/01/07 07:28:28, Ilya Sherman wrote: > > > > > On 2011/01/07 03:47:15, dhollowa wrote: > > > > > > Same here. I'd prefer to unroll the loop with these. > > > > > > > > > > I think it's reasonable to preserve the loop, but unroll the credit card > > > > > creation. One way to do this is to create an array of the four credit > > cards > > > > and > > > > > the corresponding expectations, and loop over that. > > > > > > > > > >
http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:122: credit_card_field->expiration_month_ = *q++; I tried again with this latest patch and things seem to be working. On 2011/01/07 05:33:49, honten wrote: > I cannot reproduce your bug. > > I made form as following, > > <form id="gaia_loginform" action="https://www.google.com" method="post"> > <label for="nameoncard">Card Holder</label> > <input type="text" name="nameoncard" /><br /> > <label for="CardNumber">Card Number</label> > <input type="text" name="card_number" /><br /> > <label for="random_id">HTML5 M-o-n-t-h:</label> > <input type="month" id="random_id"><br> > <input type="submit" name="submit" value="ok"><br /> > > Could you give me your full sample?
http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:211: FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, Indentation seems to be off here. http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/31001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:120: "4234567890123456", // Visa nit: indentation should be at open paren. And below.
dhollowa, Could you review again? This change includes your and Ilya's recommendation. Thanks,
LGTM. Thanks! For the future: it is helpful to click the "Reply" or "Done" button in the review comment so the reviewer can quickly glance and see what has and hasn't been addressed.
Thanks for your dedication and LGTM!! I'll press "Done" or "Reply" from next time. Could you commit my patch? Because I'm not a commiter. On Mon, Jan 10, 2011 at 10:48 AM, <dhollowa@chromium.org> wrote: > LGTM. Thanks! > > For the future: it is helpful to click the "Reply" or "Done" button in the > review comment so the reviewer can quickly glance and see what has and > hasn't > been addressed. > > http://codereview.chromium.org/6033010/ >
Let's wait for LGTM from Ilya. And for WebKit patch to land (let me know). And then I'll be happy to land this one, yes. On 2011/01/10 18:51:49, takano.naoki_gmail.com wrote: > Thanks for your dedication and LGTM!! > > I'll press "Done" or "Reply" from next time. > > Could you commit my patch? Because I'm not a commiter. > > On Mon, Jan 10, 2011 at 10:48 AM, <mailto:dhollowa@chromium.org> wrote: > > LGTM. Thanks! > > > > For the future: it is helpful to click the "Reply" or "Done" button in the > > review comment so the reviewer can quickly glance and see what has and > > hasn't > > been addressed. > > > > http://codereview.chromium.org/6033010/ > >
Ok, For now, in WebKit side, I have to add ChangeLog. Except that, I already got LGTM from isherman in WebKit. Of course, I want to wait LGTM from isherman in Chrome side too. Thanks, On Mon, Jan 10, 2011 at 11:16 AM, <dhollowa@chromium.org> wrote: > Let's wait for LGTM from Ilya. And for WebKit patch to land (let me know). > And > then I'll be happy to land this one, yes. > > On 2011/01/10 18:51:49, takano.naoki_gmail.com wrote: >> >> Thanks for your dedication and LGTM!! > >> I'll press "Done" or "Reply" from next time. > >> Could you commit my patch? Because I'm not a commiter. > >> On Mon, Jan 10, 2011 at 10:48 AM, <mailto:dhollowa@chromium.org> wrote: >> > LGTM. Thanks! >> > >> > For the future: it is helpful to click the "Reply" or "Done" button in >> > the >> > review comment so the reviewer can quickly glance and see what has and >> > hasn't >> > been addressed. >> > >> > http://codereview.chromium.org/6033010/ >> > > > > > http://codereview.chromium.org/6033010/ >
Whoops, thought I'd mailed these out already: http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/15001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1357: for (int i = 0; i < 4; ++i) { On 2011/01/07 18:45:52, honten wrote: > So... which should I choose???? > > Could you discuss between you guys? Unrolled is fine. I prefer loops to extra boilerplate, but I don't feel strongly about it. =) http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:700: month = year + ASCIIToUTF16("-") + month; nit: Naming this "month" is confusing. Please either use a new variable name -- e.g. "date" -- or just directly call field->set_value(year + ASCIIToUTF16("-") + month); http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:90: void CreateTestCreditCardsYearMonth() { As long as you're unrolling the loops, it would be clearer to create each of these cards where they're used, rather than all at once (see also comment further down). http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:152: "04", "2012"); nit: The indentation here was correct previously (also below). http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:398: date.c_str(), "month", &field); nit: Please use the same style as elsewhere in this function: all arguments on a single line. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1368: test_personal_data_->CreateTestCreditCardsYearMonth(); Rather than creating all four cards within CreateTestCreditCardsYearMonth(), it would be clearer to create the one relevant card here. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1378: autofill_manager_->PackGUIDs(guid, std::string()))); nit: Each of these lines should be indented four spaces (rather than six). Same for the three tests below. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:191: "month")))) { nit: These two lines should be indented one more space each -- they should be indented relative to the "L" rather than to the parenthesis. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:172: DCHECK_EQ(field_type.field_type(), CREDIT_CARD_EXP_MONTH); nit: I believe that the macro expects the arguments to be in the reverse order. This feels backward when you're writing the code, but it makes the error messages come out looking more sensible when the DCHECK fails. http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:276: // In HTML5, email, all text fields except password are text input fields to nit: I think the fragment "email, " in this sentence is left over from a previous comment ;) http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); I might be mistaken, but I think this conversion will fail for <select> elements, and potentially others as well. I'll try to remember to take another look at this when I get back into the office on Monday. http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:427: control_element.toConst<WebInputElement>(); nit: If this conversion is legal (see above), you should just call this |input_element| and remove the declaration of |input_element| within the if-stmt.
http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); On 2011/01/10 20:08:48, Ilya Sherman wrote: > I might be mistaken, but I think this conversion will fail for <select> > elements, and potentially others as well. I'll try to remember to take another > look at this when I get back into the office on Monday. Yep, this conversion is just a static_cast, which is not guaranteed to work here. In the realm of WebKit, it looks like dom/InputElement::toInputElement() is the typesafe way to perform this conversion. I don't think we have something like this exported in the Chromium WebKit API. Darin Fisher (fishd) might be a good person to ask about whether this already exists; or if not, where to add it.
http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); Ok, I'll ask him. On 2011/01/10 21:08:37, Ilya Sherman wrote: > On 2011/01/10 20:08:48, Ilya Sherman wrote: > > I might be mistaken, but I think this conversion will fail for <select> > > elements, and potentially others as well. I'll try to remember to take > another > > look at this when I get back into the office on Monday. > > Yep, this conversion is just a static_cast, which is not guaranteed to work > here. > > In the realm of WebKit, it looks like dom/InputElement::toInputElement() is the > typesafe way to perform this conversion. I don't think we have something like > this exported in the Chromium WebKit API. Darin Fisher (fishd) might be a good > person to ask about whether this already exists; or if not, where to add it.
http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); Darin, Could you tell me if there is a method Ilya mentions here? If there is not, do you have any suggestion or thought? Thanks, On 2011/01/10 22:50:35, honten wrote: > Ok, I'll ask him. > > On 2011/01/10 21:08:37, Ilya Sherman wrote: > > On 2011/01/10 20:08:48, Ilya Sherman wrote: > > > I might be mistaken, but I think this conversion will fail for <select> > > > elements, and potentially others as well. I'll try to remember to take > > another > > > look at this when I get back into the office on Monday. > > > > Yep, this conversion is just a static_cast, which is not guaranteed to work > > here. > > > > In the realm of WebKit, it looks like dom/InputElement::toInputElement() is > the > > typesafe way to perform this conversion. I don't think we have something like > > this exported in the Chromium WebKit API. Darin Fisher (fishd) might be a > good > > person to ask about whether this already exists; or if not, where to add it. >
Darin, Could you tell me if there is a method Ilya mentions here? If there is not, do you have any suggestion or thought? On Mon, Jan 10, 2011 at 2:57 PM, <Takano.Naoki@gmail.com> wrote: > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > File chrome/renderer/form_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > chrome/renderer/form_manager.cc:317: const WebInputElement& > input_element = element.toConst<WebInputElement>(); > Darin, > > Could you tell me if there is a method Ilya mentions here? > > If there is not, do you have any suggestion or thought? > > Thanks, > > On 2011/01/10 22:50:35, honten wrote: >> >> Ok, I'll ask him. > >> On 2011/01/10 21:08:37, Ilya Sherman wrote: >> > On 2011/01/10 20:08:48, Ilya Sherman wrote: >> > > I might be mistaken, but I think this conversion will fail for > > <select> >> >> > > elements, and potentially others as well. I'll try to remember to > > take >> >> > another >> > > look at this when I get back into the office on Monday. >> > >> > Yep, this conversion is just a static_cast, which is not guaranteed > > to work >> >> > here. >> > >> > In the realm of WebKit, it looks like > > dom/InputElement::toInputElement() is >> >> the >> > typesafe way to perform this conversion. I don't think we have > > something like >> >> > this exported in the Chromium WebKit API. Darin Fisher (fishd) > > might be a >> >> good >> > person to ask about whether this already exists; or if not, where to > > add it. > > > http://codereview.chromium.org/6033010/ >
> Darin Fisher (fishd) might be a good person to ask about whether this already exists; > or if not, where to add it. Oops, he's fishd on IRC but his email addy is darin@chromium.org. I've forwarded your question on to him.
Thanks, If you have time, please also take a look at https://bugs.webkit.org/show_bug.cgi?id=51382 On Mon, Jan 10, 2011 at 5:15 PM, <isherman@chromium.org> wrote: >> Darin Fisher (fishd) might be a good person to ask about whether this >> already > > exists; >> >> or if not, where to add it. > > Oops, he's fishd on IRC but his email addy is darin@chromium.org. I've > forwarded your question on to him. > > > > http://codereview.chromium.org/6033010/ >
Ilya, I don't get any response from Darin. Could you make sure? Thanks, On Mon, Jan 10, 2011 at 9:20 PM, Naoki Takano <takano.naoki@gmail.com> wrote: > Thanks, > > If you have time, please also take a look at > https://bugs.webkit.org/show_bug.cgi?id=51382 > > On Mon, Jan 10, 2011 at 5:15 PM, <isherman@chromium.org> wrote: >>> Darin Fisher (fishd) might be a good person to ask about whether this >>> already >> >> exists; >>> >>> or if not, where to add it. >> >> Oops, he's fishd on IRC but his email addy is darin@chromium.org. I've >> forwarded your question on to him. >> >> >> >> http://codereview.chromium.org/6033010/ >> >
Darin, Could you answer following question? http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); Could you tell me if there is a method Ilya mentions here? If there is not, do you have any suggestion or thought? Thanks, On Tue, Jan 11, 2011 at 1:08 PM, Naoki Takano <takano.naoki@gmail.com> wrote: > Ilya, > > I don't get any response from Darin. > > Could you make sure? > > Thanks, > > On Mon, Jan 10, 2011 at 9:20 PM, Naoki Takano <takano.naoki@gmail.com> wrote: >> Thanks, >> >> If you have time, please also take a look at >> https://bugs.webkit.org/show_bug.cgi?id=51382 >> >> On Mon, Jan 10, 2011 at 5:15 PM, <isherman@chromium.org> wrote: >>>> Darin Fisher (fishd) might be a good person to ask about whether this >>>> already >>> >>> exists; >>>> >>>> or if not, where to add it. >>> >>> Oops, he's fishd on IRC but his email addy is darin@chromium.org. I've >>> forwarded your question on to him. >>> >>> >>> >>> http://codereview.chromium.org/6033010/ >>> >> >
Darin, Could you give me any suggestion? On 2011/01/13 06:00:26, takano.naoki_gmail.com wrote: > Darin, > > Could you answer following question? > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > File chrome/renderer/form_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > chrome/renderer/form_manager.cc:317: const WebInputElement& > input_element = element.toConst<WebInputElement>(); > > Could you tell me if there is a method Ilya mentions here? > > If there is not, do you have any suggestion or thought? > > Thanks, > > On Tue, Jan 11, 2011 at 1:08 PM, Naoki Takano <mailto:takano.naoki@gmail.com> wrote: > > Ilya, > > > > I don't get any response from Darin. > > > > Could you make sure? > > > > Thanks, > > > > On Mon, Jan 10, 2011 at 9:20 PM, Naoki Takano <mailto:takano.naoki@gmail.com> wrote: > >> Thanks, > >> > >> If you have time, please also take a look at > >> https://bugs.webkit.org/show_bug.cgi?id=51382 > >> > >> On Mon, Jan 10, 2011 at 5:15 PM, mailto: <isherman@chromium.org> wrote: > >>>> Darin Fisher (fishd) might be a good person to ask about whether this > >>>> already > >>> > >>> exists; > >>>> > >>>> or if not, where to add it. > >>> > >>> Oops, he's fishd on IRC but his email addy is mailto:darin@chromium.org. I've > >>> forwarded your question on to him. > >>> > >>> > >>> > >>> http://codereview.chromium.org/6033010/ > >>> > >> > >
Ilya, Sorry for bothering you. But I cannot get any response from Darin, almost for a week. Could you ask him again? Thanks, On Mon, Jan 17, 2011 at 10:14 PM, <Takano.Naoki@gmail.com> wrote: > Darin, > > Could you give me any suggestion? > > On 2011/01/13 06:00:26, takano.naoki_gmail.com wrote: >> >> Darin, > >> Could you answer following question? > > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... >> >> File chrome/renderer/form_manager.cc (right): > > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... >> >> chrome/renderer/form_manager.cc:317: const WebInputElement& >> input_element = element.toConst<WebInputElement>(); > >> Could you tell me if there is a method Ilya mentions here? > >> If there is not, do you have any suggestion or thought? > >> Thanks, > >> On Tue, Jan 11, 2011 at 1:08 PM, Naoki Takano >> <mailto:takano.naoki@gmail.com> > > wrote: >> >> > Ilya, >> > >> > I don't get any response from Darin. >> > >> > Could you make sure? >> > >> > Thanks, >> > >> > On Mon, Jan 10, 2011 at 9:20 PM, Naoki Takano > > <mailto:takano.naoki@gmail.com> wrote: >> >> >> Thanks, >> >> >> >> If you have time, please also take a look at >> >> https://bugs.webkit.org/show_bug.cgi?id=51382 >> >> >> >> On Mon, Jan 10, 2011 at 5:15 PM, mailto: <isherman@chromium.org> wrote: >> >>>> Darin Fisher (fishd) might be a good person to ask about whether this >> >>>> already >> >>> >> >>> exists; >> >>>> >> >>>> or if not, where to add it. >> >>> >> >>> Oops, he's fishd on IRC but his email addy is >> >>> mailto:darin@chromium.org. > > I've >> >> >>> forwarded your question on to him. >> >>> >> >>> >> >>> >> >>> http://codereview.chromium.org/6033010/ >> >>> >> >> >> > > > > > http://codereview.chromium.org/6033010/ >
Is there anybody who can review this part? Especially I want to know this part, as Ilya mentioned, http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); Darin, Darin looks very busy, I'm waiting for more than a week. On 2011/01/18 20:14:21, takano.naoki_gmail.com wrote: > Ilya, > > Sorry for bothering you. > But I cannot get any response from Darin, almost for a week. > Could you ask him again? > > Thanks, > > On Mon, Jan 17, 2011 at 10:14 PM, <mailto:Takano.Naoki@gmail.com> wrote: > > Darin, > > > > Could you give me any suggestion? > > > > On 2011/01/13 06:00:26, http://takano.naoki_gmail.com wrote: > >> > >> Darin, > > > >> Could you answer following question? > > > > > > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > >> > >> File chrome/renderer/form_manager.cc (right): > > > > > > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > >> > >> chrome/renderer/form_manager.cc:317: const WebInputElement& > >> input_element = element.toConst<WebInputElement>(); > > > >> Could you tell me if there is a method Ilya mentions here? > > > >> If there is not, do you have any suggestion or thought? > > > >> Thanks, > > > >> On Tue, Jan 11, 2011 at 1:08 PM, Naoki Takano > >> <mailto:takano.naoki@gmail.com> > > > > wrote: > >> > >> > Ilya, > >> > > >> > I don't get any response from Darin. > >> > > >> > Could you make sure? > >> > > >> > Thanks, > >> > > >> > On Mon, Jan 10, 2011 at 9:20 PM, Naoki Takano > > > > <mailto:takano.naoki@gmail.com> wrote: > >> > >> >> Thanks, > >> >> > >> >> If you have time, please also take a look at > >> >> https://bugs.webkit.org/show_bug.cgi?id=51382 > >> >> > >> >> On Mon, Jan 10, 2011 at 5:15 PM, mailto: <isherman@chromium.org> wrote: > >> >>>> Darin Fisher (fishd) might be a good person to ask about whether this > >> >>>> already > >> >>> > >> >>> exists; > >> >>>> > >> >>>> or if not, where to add it. > >> >>> > >> >>> Oops, he's fishd on IRC but his email addy is > >> >>> mailto:darin@chromium.org. > > > > I've > >> > >> >>> forwarded your question on to him. > >> >>> > >> >>> > >> >>> > >> >>> http://codereview.chromium.org/6033010/ > >> >>> > >> >> > >> > > > > > > > > > http://codereview.chromium.org/6033010/ > >
http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); The test: element.hasTagName("input") should be sufficient to safely test that the downcast is ok. On 2011/01/10 22:57:13, honten wrote: > Darin, > > Could you tell me if there is a method Ilya mentions here? > > If there is not, do you have any suggestion or thought? > > Thanks, > > On 2011/01/10 22:50:35, honten wrote: > > Ok, I'll ask him. > > > > On 2011/01/10 21:08:37, Ilya Sherman wrote: > > > On 2011/01/10 20:08:48, Ilya Sherman wrote: > > > > I might be mistaken, but I think this conversion will fail for <select> > > > > elements, and potentially others as well. I'll try to remember to take > > > another > > > > look at this when I get back into the office on Monday. > > > > > > Yep, this conversion is just a static_cast, which is not guaranteed to work > > > here. > > > > > > In the realm of WebKit, it looks like dom/InputElement::toInputElement() is > > the > > > typesafe way to perform this conversion. I don't think we have something > like > > > this exported in the Chromium WebKit API. Darin Fisher (fishd) might be a > > good > > > person to ask about whether this already exists; or if not, where to add it. > > >
http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); I think you can use WebElement::hasTagName("input") to check if casting to WebInputElement is valid. However, it would probably be better if we had a way to leverage toInputElement(). I support adding a toWebInputElement method in WebInputElement.h. If we do that, we should also do the same for all of the other WebNode subclasses.
Darin, Thank you for your comment. If you suggest me how to implement WebInputElement method, I follow it. Add send you the patch to WebKit side. Or do you have any plan to add the method? Thanks, On Wed, Jan 19, 2011 at 2:04 PM, <darin@chromium.org> wrote: > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > File chrome/renderer/form_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > chrome/renderer/form_manager.cc:317: const WebInputElement& > input_element = element.toConst<WebInputElement>(); > I think you can use WebElement::hasTagName("input") to check if casting > to WebInputElement is valid. > > However, it would probably be better if we had a way to leverage > toInputElement(). > > I support adding a toWebInputElement method in WebInputElement.h. If we > do that, we should also do the same for all of the other WebNode > subclasses. > > http://codereview.chromium.org/6033010/ >
Darin, I tried to implement toWebInputElement() in https://bugs.webkit.org/show_bug.cgi?id=51809 Please review it. Thanks, On 2011/01/19 22:07:52, takano.naoki_gmail.com wrote: > Darin, > > Thank you for your comment. > > If you suggest me how to implement WebInputElement method, I follow it. > Add send you the patch to WebKit side. > > Or do you have any plan to add the method? > > Thanks, > > On Wed, Jan 19, 2011 at 2:04 PM, <mailto:darin@chromium.org> wrote: > > > > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > > File chrome/renderer/form_manager.cc (right): > > > > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > > chrome/renderer/form_manager.cc:317: const WebInputElement& > > input_element = element.toConst<WebInputElement>(); > > I think you can use WebElement::hasTagName("input") to check if casting > > to WebInputElement is valid. > > > > However, it would probably be better if we had a way to leverage > > toInputElement(). > > > > I support adding a toWebInputElement method in WebInputElement.h. If we > > do that, we should also do the same for all of the other WebNode > > subclasses. > > > > http://codereview.chromium.org/6033010/ > >
http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:700: month = year + ASCIIToUTF16("-") + month; On 2011/01/10 20:08:48, Ilya Sherman wrote: > nit: Naming this "month" is confusing. Please either use a new variable name -- > e.g. "date" -- or just directly call > > field->set_value(year + ASCIIToUTF16("-") + month); Done. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:90: void CreateTestCreditCardsYearMonth() { On 2011/01/10 20:08:48, Ilya Sherman wrote: > As long as you're unrolling the loops, it would be clearer to create each of > these cards where they're used, rather than all at once (see also comment > further down). Done. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:152: "04", "2012"); On 2011/01/10 20:08:48, Ilya Sherman wrote: > nit: The indentation here was correct previously (also below). Done. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:398: date.c_str(), "month", &field); On 2011/01/10 20:08:48, Ilya Sherman wrote: > nit: Please use the same style as elsewhere in this function: all arguments on a > single line. Done. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1368: test_personal_data_->CreateTestCreditCardsYearMonth(); On 2011/01/10 20:08:48, Ilya Sherman wrote: > Rather than creating all four cards within CreateTestCreditCardsYearMonth(), it > would be clearer to create the one relevant card here. Done. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:1378: autofill_manager_->PackGUIDs(guid, std::string()))); On 2011/01/10 20:08:48, Ilya Sherman wrote: > nit: Each of these lines should be indented four spaces (rather than six). Same > for the three tests below. Done. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/cre... File chrome/browser/autofill/credit_card_field.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/cre... chrome/browser/autofill/credit_card_field.cc:191: "month")))) { On 2011/01/10 20:08:48, Ilya Sherman wrote: > nit: These two lines should be indented one more space each -- they should be > indented relative to the "L" rather than to the parenthesis. Done. http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:172: DCHECK_EQ(field_type.field_type(), CREDIT_CARD_EXP_MONTH); On 2011/01/10 20:08:48, Ilya Sherman wrote: > nit: I believe that the macro expects the arguments to be in the reverse order. > This feels backward when you're writing the code, but it makes the error > messages come out looking more sensible when the DCHECK fails. Done. http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:276: // In HTML5, email, all text fields except password are text input fields to On 2011/01/10 20:08:48, Ilya Sherman wrote: > nit: I think the fragment "email, " in this sentence is left over from a > previous comment ;) Done. http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = element.toConst<WebInputElement>(); Now I changed to use toWebInputElement(). On 2011/01/19 22:04:18, darin wrote: > I think you can use WebElement::hasTagName("input") to check if casting to > WebInputElement is valid. > > However, it would probably be better if we had a way to leverage > toInputElement(). > > I support adding a toWebInputElement method in WebInputElement.h. If we do > that, we should also do the same for all of the other WebNode subclasses.
LGTM once the below nits are addressed. Thanks for your perseverance on this patch! http://codereview.chromium.org/6033010/diff/77001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/77001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:93: void CreateTestCreditCardsYearMonth(const char* year, const char* month) { nit: This only creates a single credit card, so perhaps "CreateTestCreditCardWithYearAndMonth()" http://codereview.chromium.org/6033010/diff/77001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:95: // Create four credit cards with year month combination as following, nit: Now only creates one credit card. http://codereview.chromium.org/6033010/diff/77001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/77001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:432: if (!input_element->autoComplete()) nit: No need for nested if-stmts here; you can combine them into a single stmt. http://codereview.chromium.org/6033010/diff/77001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:929: input_element->suggestedValue().length()); nit: Please indent this line so that the leading "i" aligns with the "0" from the previous line.
Updated. http://codereview.chromium.org/6033010/diff/77001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/6033010/diff/77001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:93: void CreateTestCreditCardsYearMonth(const char* year, const char* month) { On 2011/01/25 06:21:30, Ilya Sherman wrote: > nit: This only creates a single credit card, so perhaps > "CreateTestCreditCardWithYearAndMonth()" Done. http://codereview.chromium.org/6033010/diff/77001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager_unittest.cc:95: // Create four credit cards with year month combination as following, On 2011/01/25 06:21:30, Ilya Sherman wrote: > nit: Now only creates one credit card. Done. http://codereview.chromium.org/6033010/diff/77001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/77001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:432: if (!input_element->autoComplete()) On 2011/01/25 06:21:30, Ilya Sherman wrote: > nit: No need for nested if-stmts here; you can combine them into a single stmt. Done. http://codereview.chromium.org/6033010/diff/77001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:929: input_element->suggestedValue().length()); On 2011/01/25 06:21:30, Ilya Sherman wrote: > nit: Please indent this line so that the leading "i" aligns with the "0" from > the previous line. Done.
Darin, Could you give us your thought in https://bugs.webkit.org/show_bug.cgi?id=51809 ? We really need const_cast<> in either chrome or webkit code. Could you decide it? Thanks, On 2011/01/19 22:04:18, darin wrote: > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > File chrome/renderer/form_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = > element.toConst<WebInputElement>(); > I think you can use WebElement::hasTagName("input") to check if casting to > WebInputElement is valid. > > However, it would probably be better if we had a way to leverage > toInputElement(). > > I support adding a toWebInputElement method in WebInputElement.h. If we do > that, we should also do the same for all of the other WebNode subclasses.
Ilya, Could you help me to contact Darin? If I cannot reach him, I cannot submit WebKit source code... Thanks, On 2011/01/26 03:05:06, honten wrote: > Darin, > > Could you give us your thought in > https://bugs.webkit.org/show_bug.cgi?id=51809 > ? > > We really need const_cast<> in either chrome or webkit code. > > Could you decide it? > > Thanks, > On 2011/01/19 22:04:18, darin wrote: > > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > > File chrome/renderer/form_manager.cc (right): > > > > > http://codereview.chromium.org/6033010/diff/53001/chrome/renderer/form_manage... > > chrome/renderer/form_manager.cc:317: const WebInputElement& input_element = > > element.toConst<WebInputElement>(); > > I think you can use WebElement::hasTagName("input") to check if casting to > > WebInputElement is valid. > > > > However, it would probably be better if we had a way to leverage > > toInputElement(). > > > > I support adding a toWebInputElement method in WebInputElement.h. If we do > > that, we should also do the same for all of the other WebNode subclasses.
http://codereview.chromium.org/6033010/diff/84001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/84001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:278: static bool IsTextInput(const WebInputElement* element) { nit: Just noticed: no need for "static" here, as it's already in an anonymous namespace.
http://codereview.chromium.org/6033010/diff/84001/chrome/renderer/form_manage... File chrome/renderer/form_manager.cc (right): http://codereview.chromium.org/6033010/diff/84001/chrome/renderer/form_manage... chrome/renderer/form_manager.cc:278: static bool IsTextInput(const WebInputElement* element) { On 2011/01/28 08:15:52, Ilya Sherman wrote: > nit: Just noticed: no need for "static" here, as it's already in an anonymous > namespace. Done.
Finally, WebKit patch is landed. So I updated this patch according to the latest. Please review it. Thanks, On 2011/01/29 03:43:30, honten wrote: > http://codereview.chromium.org/6033010/diff/84001/chrome/renderer/form_manage... > File chrome/renderer/form_manager.cc (right): > > http://codereview.chromium.org/6033010/diff/84001/chrome/renderer/form_manage... > chrome/renderer/form_manager.cc:278: static bool IsTextInput(const > WebInputElement* element) { > On 2011/01/28 08:15:52, Ilya Sherman wrote: > > nit: Just noticed: no need for "static" here, as it's already in an anonymous > > namespace. > > Done.
Running this past the trybots... *fingers crossed*
Looks like a few of the browser tests are failing: http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/10959
isherman, Thank you for your try. I'll look into it. On Wed, Feb 2, 2011 at 9:29 PM, <isherman@chromium.org> wrote: > Looks like a few of the browser tests are failing: > http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/10959 > > http://codereview.chromium.org/6033010/ >
iserman, There was a defect when I merged it. If fixed it, ran all browser_tests. I got three test errors, as following, FormAutocompleteTest.FAILS_DynamicAutoCompleteOffFormSubmit PrerenderBrowserTest.PrerenderDelayLoadPlugin ExtensionDevToolsBrowserTest.FLAKY_TimelineApi But they look all failed already before I change. I checked with 5ddf8648ca36da2ae0bb8df4979183b95cf9f77a version. Please review it again. Thanks, On 2011/02/03 05:30:19, takano.naoki_gmail.com wrote: > isherman, > > Thank you for your try. > > I'll look into it. > > On Wed, Feb 2, 2011 at 9:29 PM, <mailto:isherman@chromium.org> wrote: > > Looks like a few of the browser tests are failing: > > http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/10959 > > > > http://codereview.chromium.org/6033010/ > >
Try runs look green now. David, if you're happy with this CL, could you land it?
isherman, Thank you for your dedication!! On Wed, Feb 2, 2011 at 11:57 PM, <isherman@chromium.org> wrote: > Try runs look green now. David, if you're happy with this CL, could you > land > it? > > http://codereview.chromium.org/6033010/ >
It has landed. Many thanks! |
