Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13264002:
Requery the autofill server when forms and input fields are dynamically added. (Closed)
|
Created:
7 years, 8 months ago by Dane Wallinga Modified:
7 years, 7 months ago CC:
chromium-reviews, Raman Kakilate, benquan, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRequery the autofill server when forms and input fields are dynamically added.
BUG=224802
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197479
Patch Set 1 #
Total comments: 30
Patch Set 2 : some style fixes, pulled apart tests. #Patch Set 3 : and again, except this time remembering to hit save first. #
Total comments: 4
Patch Set 4 : replace bools in formsSeen message with enum, other style fixes #Patch Set 5 : minor style fix #
Total comments: 21
Patch Set 6 : fix operand ordering on ternary operators #
Total comments: 1
Patch Set 7 : refactors, style fixes, comments, etc #Patch Set 8 : and an indent fix #
Total comments: 7
Patch Set 9 : more fixes. you get the drill. #
Total comments: 24
Patch Set 10 : rename some things, move around others #
Total comments: 4
Patch Set 11 : rename FormsSeenParam, member variables #Patch Set 12 : Some bug fixes to make sure things happen in the right order, and when we want them to. #Patch Set 13 : And a bit of cleanup #
Total comments: 10
Patch Set 14 : address comments #Patch Set 15 : Do away with PARTIAL_AND_DYNAMIC_FORMS_SEEN #Patch Set 16 : Fun with merging #Messages
Total messages: 32 (0 generated)
https://chromiumcodereview.appspot.com/13264002/diff/1/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:127: autofill_agent_->OnWhitelistedForAutocheckout(); Might want to make this a new TEST_F https://chromiumcodereview.appspot.com/13264002/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:129: ExecuteJavaScript("newInput=document.createElement(\"input\");" nit but should this be "var newInput=" https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_manager.cc:453: // if new forms are the result of AJAX or DHML, treat as new page. "If new forms were added via AJAX or DHTML..." https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_manager.cc:461: if (!is_post_document_load) curlies since this is multiline https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... File components/autofill/browser/autofill_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_manager_unittest.cc:891: // Test that browser asks for all forms after dom changes when Autocheckout is enabled. DOM https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_manager_unittest.cc:901: ASSERT_TRUE(HasSeenAutofillGetAllFormsMessage()); EXPECT_TRUE https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... File components/autofill/browser/autofill_metrics_unittest.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_metrics_unittest.cc:790: autofill_manager_->OnFormsSeen(forms, TimeTicks(), false, false); Might want to add a TODO somewhere to collect metrics for partial and dynamic forms. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:219: has_more_forms, Might be good to collapse these bools into an enum. PARTIAL_FORM_SEEN. DYNAMIC_FORM_SEEN, PARTIAL_AND_DYNAMIC_FORM_SEEN or something like that and do some bitwise stuff to get these bools out. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:735: forms_seen_timestamp_, false, false)); same comment? https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:951: for (size_t i = 0; i < nodes.size(); ++i) { Can't use an iterator? https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:954: forms_have_changed_since_load_ = true; I think you only want to do this once if any of hte nodes was affect: for (...) { if (node.document().frame() == top_most_frame_) { forms_have_changed_since_load_ = true; break; } } if (forms_have_changed_since_load_ && is_whitelisted_for_autocheckoutt) { // do the other stuff } But I could be completely wrong. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:969: return; Oh. There's a return. That's why this does the right thing. Doh. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... File components/autofill/renderer/autofill_agent.h (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.h:259: // Whether or not new forms/fields have been dynamical added post-document load dynamically and add punctuation
https://chromiumcodereview.appspot.com/13264002/diff/1/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:127: autofill_agent_->OnWhitelistedForAutocheckout(); On 2013/03/29 19:48:31, ahutter wrote: > Might want to make this a new TEST_F Done. https://chromiumcodereview.appspot.com/13264002/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:129: ExecuteJavaScript("newInput=document.createElement(\"input\");" On 2013/03/29 19:48:31, ahutter wrote: > nit but should this be "var newInput=" Done. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_manager.cc:453: // if new forms are the result of AJAX or DHML, treat as new page. On 2013/03/29 19:48:31, ahutter wrote: > "If new forms were added via AJAX or DHTML..." Done. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_manager.cc:461: if (!is_post_document_load) On 2013/03/29 19:48:31, ahutter wrote: > curlies since this is multiline Even if it's just one statement with wrapping? Chromium makes no sense... https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... File components/autofill/browser/autofill_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_manager_unittest.cc:891: // Test that browser asks for all forms after dom changes when Autocheckout is enabled. On 2013/03/29 19:48:31, ahutter wrote: > DOM Done. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_manager_unittest.cc:901: ASSERT_TRUE(HasSeenAutofillGetAllFormsMessage()); On 2013/03/29 19:48:31, ahutter wrote: > EXPECT_TRUE Done. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... File components/autofill/browser/autofill_metrics_unittest.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_metrics_unittest.cc:790: autofill_manager_->OnFormsSeen(forms, TimeTicks(), false, false); On 2013/03/29 19:48:31, ahutter wrote: > Might want to add a TODO somewhere to collect metrics for partial and dynamic > forms. Is it just that the current code path doesn't distinguish? https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:219: has_more_forms, On 2013/03/29 19:48:31, ahutter wrote: > Might be good to collapse these bools into an enum. > PARTIAL_FORM_SEEN. > DYNAMIC_FORM_SEEN, > PARTIAL_AND_DYNAMIC_FORM_SEEN > > or something like that and do some bitwise stuff to get these bools out. Bitwise stuff? You mean like in the enum define PARTIAL_FORM_SEEN=1, DYNAMIC_FORM_SEEN=2, and then have has_more_forms = (parameterValue & PARTIAL_FORM_SEEN) and is_post_document_load = (parameterValue & DYNAMIC_FORM_SEEN)? Those would live in autofill/common? https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:951: for (size_t i = 0; i < nodes.size(); ++i) { On 2013/03/29 19:48:31, ahutter wrote: > Can't use an iterator? Well at least in WebKit, it's preferred that indexing is used when iterating over vectors. Can't find any reference to it at all in the Chrome style guide. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:954: forms_have_changed_since_load_ = true; On 2013/03/29 19:48:31, ahutter wrote: > I think you only want to do this once if any of hte nodes was affect: > > for (...) { > if (node.document().frame() == top_most_frame_) { > forms_have_changed_since_load_ = true; > break; > } > } > > if (forms_have_changed_since_load_ && is_whitelisted_for_autocheckoutt) { > // do the other stuff > } > > But I could be completely wrong. I have a return further down. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:969: return; On 2013/03/29 19:48:31, ahutter wrote: > Oh. There's a return. That's why this does the right thing. Doh. Yeah, this one. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... File components/autofill/renderer/autofill_agent.h (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.h:259: // Whether or not new forms/fields have been dynamical added post-document load On 2013/03/29 19:48:31, ahutter wrote: > dynamically and add punctuation Done.
https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... File components/autofill/browser/autofill_metrics_unittest.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_metrics_unittest.cc:790: autofill_manager_->OnFormsSeen(forms, TimeTicks(), false, false); On 2013/03/29 21:41:12, dgwallinga wrote: > On 2013/03/29 19:48:31, ahutter wrote: > > Might want to add a TODO somewhere to collect metrics for partial and dynamic > > forms. > Is it just that the current code path doesn't distinguish? Just no UMA metrics for these new kinds of forms were filling. You should probably ask Ilya if it's worth tracking when he reviews. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:219: has_more_forms, On 2013/03/29 21:41:12, dgwallinga wrote: > On 2013/03/29 19:48:31, ahutter wrote: > > Might be good to collapse these bools into an enum. > > PARTIAL_FORM_SEEN. > > DYNAMIC_FORM_SEEN, > > PARTIAL_AND_DYNAMIC_FORM_SEEN > > > > or something like that and do some bitwise stuff to get these bools out. > Bitwise stuff? You mean like in the enum define PARTIAL_FORM_SEEN=1, > DYNAMIC_FORM_SEEN=2, and then have has_more_forms = (parameterValue & > PARTIAL_FORM_SEEN) and is_post_document_load = (parameterValue & > DYNAMIC_FORM_SEEN)? Yes. > Those would live in autofill/common? Yes Yeah seems like it might be cleaner than having the multiple bool params and make the code more readable. https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:954: forms_have_changed_since_load_ = true; On 2013/03/29 21:41:12, dgwallinga wrote: > On 2013/03/29 19:48:31, ahutter wrote: > > I think you only want to do this once if any of hte nodes was affect: > > > > for (...) { > > if (node.document().frame() == top_most_frame_) { > > forms_have_changed_since_load_ = true; > > break; > > } > > } > > > > if (forms_have_changed_since_load_ && is_whitelisted_for_autocheckoutt) { > > // do the other stuff > > } > > > > But I could be completely wrong. > > I have a return further down. I get that this works but it's more fragile to future changes then it needs to be in its current form. https://chromiumcodereview.appspot.com/13264002/diff/13001/components/autofil... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/13001/components/autofil... components/autofill/browser/autofill_manager.cc:453: // if new forms were added via AJAX or DHML, treat as new page. nit: If https://chromiumcodereview.appspot.com/13264002/diff/13001/components/autofil... components/autofill/browser/autofill_manager.cc:455: Reset(); Maybe a newline after this.
https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... File components/autofill/browser/autofill_metrics_unittest.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/br... components/autofill/browser/autofill_metrics_unittest.cc:790: autofill_manager_->OnFormsSeen(forms, TimeTicks(), false, false); On 2013/03/29 21:53:08, ahutter wrote: > On 2013/03/29 21:41:12, dgwallinga wrote: > > On 2013/03/29 19:48:31, ahutter wrote: > > > Might want to add a TODO somewhere to collect metrics for partial and > dynamic > > > forms. > > Is it just that the current code path doesn't distinguish? > > Just no UMA metrics for these new kinds of forms were filling. You should > probably ask Ilya if it's worth tracking when he reviews. will do https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/1/components/autofill/re... components/autofill/renderer/autofill_agent.cc:954: forms_have_changed_since_load_ = true; On 2013/03/29 21:53:08, ahutter wrote: > On 2013/03/29 21:41:12, dgwallinga wrote: > > On 2013/03/29 19:48:31, ahutter wrote: > > > I think you only want to do this once if any of hte nodes was affect: > > > > > > for (...) { > > > if (node.document().frame() == top_most_frame_) { > > > forms_have_changed_since_load_ = true; > > > break; > > > } > > > } > > > > > > if (forms_have_changed_since_load_ && is_whitelisted_for_autocheckoutt) { > > > // do the other stuff > > > } > > > > > > But I could be completely wrong. > > > > I have a return further down. > > I get that this works but it's more fragile to future changes then it needs to > be in its current form. Done. https://chromiumcodereview.appspot.com/13264002/diff/13001/components/autofil... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/13001/components/autofil... components/autofill/browser/autofill_manager.cc:453: // if new forms were added via AJAX or DHML, treat as new page. On 2013/03/29 21:53:08, ahutter wrote: > nit: If Done. https://chromiumcodereview.appspot.com/13264002/diff/13001/components/autofil... components/autofill/browser/autofill_manager.cc:455: Reset(); On 2013/03/29 21:53:08, ahutter wrote: > Maybe a newline after this. Done.
https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/common/autofill_messages.h:183: autofill::FormsSeenParam /* partial or dynamic forms */) I think this should be the argument name not a description. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:217: autofill::NO_SPECIAL_FORMS_SEEN : autofill::PARTIAL_FORMS_SEEN; Is this logic right? https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:790: autofill::DYNAMIC_FORMS_SEEN : autofill::PARTIAL_AND_DYNAMIC_FORMS_SEEN; Is this backwards? https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:962: } new line. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:964: forms_have_changed_since_load_ = false; Extract into a function maybe?
https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/common/forms_seen_param.h (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/common/forms_seen_param.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. No (c) and year is wrong. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/common/forms_seen_param.h:9: Comments for enum overall and what each value specifies.
https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/browser/autofill_manager.cc:454: // if new forms were added via AJAX or DHML, treat as new page. nit: *I*f new forms... https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/common/autofill_messages.h:183: autofill::FormsSeenParam /* partial or dynamic forms */) comment section is used to specify param name rather than actual comment. This file has inconsistencies - but thats what I was told to do. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/common/forms_seen_param.h (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/common/forms_seen_param.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2013 https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:793: base::TimeTicks::Now(), timestamp should be the one at which this form is seen by the renderer. This timestamp is used to record metrics. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:974: base::TimeTicks::Now(), record the timestamp at the start of the function. and this code block is similar to 782-796, can we extract a method? https://chromiumcodereview.appspot.com/13264002/diff/23001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/23001/components/autofil... components/autofill/renderer/autofill_agent.cc:788: *topmost_frame_, kRequiredAutofillFields, &forms, &form_elements_); s/kRequiredAutofillFields/0 since we already know the fact that this page is whitelisted, we should extract all the forms, instead of waiting for IPC roundtrip.
https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/browser/autofill_manager.cc:454: // if new forms were added via AJAX or DHML, treat as new page. On 2013/04/02 20:30:19, Raman Kakilate wrote: > nit: *I*f new forms... Done. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/common/autofill_messages.h:183: autofill::FormsSeenParam /* partial or dynamic forms */) On 2013/04/02 20:11:47, ahutter wrote: > I think this should be the argument name not a description. Done. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/common/forms_seen_param.h (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/common/forms_seen_param.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2013/04/02 20:30:19, Raman Kakilate wrote: > 2013 Done. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/common/forms_seen_param.h:9: On 2013/04/02 20:27:33, ahutter wrote: > Comments for enum overall and what each value specifies. Done. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:217: autofill::NO_SPECIAL_FORMS_SEEN : autofill::PARTIAL_FORMS_SEEN; On 2013/04/02 20:11:47, ahutter wrote: > Is this logic right? nope https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:790: autofill::DYNAMIC_FORMS_SEEN : autofill::PARTIAL_AND_DYNAMIC_FORMS_SEEN; On 2013/04/02 20:11:47, ahutter wrote: > Is this backwards? most certainly https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:793: base::TimeTicks::Now(), On 2013/04/02 20:30:19, Raman Kakilate wrote: > timestamp should be the one at which this form is seen by the renderer. This > timestamp is used to record metrics. Done. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:962: } On 2013/04/02 20:11:47, ahutter wrote: > new line. Done. https://chromiumcodereview.appspot.com/13264002/diff/17001/components/autofil... components/autofill/renderer/autofill_agent.cc:974: base::TimeTicks::Now(), On 2013/04/02 20:30:19, Raman Kakilate wrote: > record the timestamp at the start of the function. > > and this code block is similar to 782-796, can we extract a method? Done.
A few nits but otherwise LGTM https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... components/autofill/renderer/autofill_agent.cc:781: if (forms_have_changed_since_load_) { no curlies https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... components/autofill/renderer/autofill_agent.cc:952: if (forms_have_changed_since_load_ && is_whitelisted_for_autocheckout_) { no curlies. https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... components/autofill/renderer/autofill_agent.cc:952: if (forms_have_changed_since_load_ && is_whitelisted_for_autocheckout_) { no curlies
https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... components/autofill/renderer/autofill_agent.cc:957: void AutofillAgent::SendDynamicFormsSeen(base::TimeTicks forms_seen_timestamp) { forms_seen_timestamp is always a member variable forms_seen_timestamp_. We could avoid this arg completely.
https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... components/autofill/renderer/autofill_agent.cc:781: if (forms_have_changed_since_load_) { On 2013/04/02 21:34:53, ahutter wrote: > no curlies Done. https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... components/autofill/renderer/autofill_agent.cc:952: if (forms_have_changed_since_load_ && is_whitelisted_for_autocheckout_) { On 2013/04/02 21:34:53, ahutter wrote: > no curlies Done. https://chromiumcodereview.appspot.com/13264002/diff/31001/components/autofil... components/autofill/renderer/autofill_agent.cc:957: void AutofillAgent::SendDynamicFormsSeen(base::TimeTicks forms_seen_timestamp) { On 2013/04/02 21:39:46, Raman Kakilate wrote: > forms_seen_timestamp is always a member variable forms_seen_timestamp_. We could > avoid this arg completely. works for me
please take a look
When pinging a CL with multiple reviewers, please indicate which reviewer you'd like to review the CL. Unfortunately, the Rietveld code review tool doesn't make it easy to see when you've been added as a reviewer. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/browser/autofill_manager.cc:453: bool has_more_forms = param & autofill::PARTIAL_FORMS_SEEN; Since you're using this as a bitmask, please define the enum constants as bitshifts. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/browser/autofill_manager.cc:456: Reset(); What happens if the user is already interacting with the page? Will this do the right thing? If so, why? https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/browser/autofill_manager.h (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/browser/autofill_manager.h:243: autofill::FormsSeenParam param); nit: "param" is probably the most generic name you could choose. Can you name this something more distinctive? https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/common/forms_seen_param.h (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/common/forms_seen_param.h:10: // Params to be sent with a FormsSeen message to AutofillManager. nit: "Params" -> "Param", though I would actually drop this entire sentence. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/common/forms_seen_param.h:12: enum FormsSeenParam { nit: Perhaps "FormsSeenStatus"? https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.cc:955: void AutofillAgent::SendDynamicFormsSeen() { nit: Please name this MaybeSendDynamicFormsSeen(), since it doesn't always send an IPC message. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.cc:961: *topmost_frame_, kRequiredAutofillFields, &forms, &form_elements_); nit: Write these six lines like so: forms_have_changed_since_load_ = false; form_elements_.clear(); std::vector<FormData> forms; bool has_more_forms = form_cache_.ExtractFormsAndFormElements( *topmost_frame_, kRequiredAutofillFields, &forms, &form_elements_); https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/renderer/autofill_agent.h (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.h:264: bool forms_have_changed_since_load_; You should make sure to initialize this in the initializer list for the class's constructor. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.h:264: bool forms_have_changed_since_load_; nit: This variable is actually tracking whether or not the AutofillManager has been alerted of the latest state of the forms. Please consider renaming accordingly. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.h:277: const WebKit::WebVector<WebKit::WebNode>& nodes) OVERRIDE; nit: This should be grouped with whatever set of methods it's overriding, under a "// Foo implementation." section. It should also be above the instance variables for this class.
https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/browser/autofill_manager.cc:453: bool has_more_forms = param & autofill::PARTIAL_FORMS_SEEN; On 2013/04/06 00:29:39, Ilya Sherman wrote: > Since you're using this as a bitmask, please define the enum constants as > bitshifts. Done. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/browser/autofill_manager.cc:456: Reset(); On 2013/04/06 00:29:39, Ilya Sherman wrote: > What happens if the user is already interacting with the page? Will this do the > right thing? If so, why? Hmm, that depends very much on what we think the 'right thing' is. The page will be treated as though it's a new page. For the purposes of correctly filling out an ajaxy flow, that's what we want. For the purposes of recording metrics (and it's mostly metric related vars that get reset in that call) it's less clear. If most of the page changes, then it makes sense to count it is a new page. If it's just a new field that pops up, maybe not. If absolutely necessary, we could probably add more nuance to the metrics we're logging to distinguish these types of flows, but I don't know exactly how we'd want to structure that. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/common/forms_seen_param.h (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/common/forms_seen_param.h:12: enum FormsSeenParam { On 2013/04/06 00:29:39, Ilya Sherman wrote: > nit: Perhaps "FormsSeenStatus"? Doesn't feel right. AutofillManager needs to know how the forms were created, but I wouldn't describe that as having anything to do with the 'status' of the forms. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.cc:955: void AutofillAgent::SendDynamicFormsSeen() { On 2013/04/06 00:29:39, Ilya Sherman wrote: > nit: Please name this MaybeSendDynamicFormsSeen(), since it doesn't always send > an IPC message. Done. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.cc:961: *topmost_frame_, kRequiredAutofillFields, &forms, &form_elements_); On 2013/04/06 00:29:39, Ilya Sherman wrote: > nit: Write these six lines like so: > > forms_have_changed_since_load_ = false; > form_elements_.clear(); > > std::vector<FormData> forms; > bool has_more_forms = form_cache_.ExtractFormsAndFormElements( > *topmost_frame_, kRequiredAutofillFields, &forms, &form_elements_); Done. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/renderer/autofill_agent.h (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.h:264: bool forms_have_changed_since_load_; On 2013/04/06 00:29:39, Ilya Sherman wrote: > nit: This variable is actually tracking whether or not the AutofillManager has > been alerted of the latest state of the forms. Please consider renaming > accordingly. Done. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.h:264: bool forms_have_changed_since_load_; On 2013/04/06 00:29:39, Ilya Sherman wrote: > You should make sure to initialize this in the initializer list for the class's > constructor. Done. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/renderer/autofill_agent.h:277: const WebKit::WebVector<WebKit::WebNode>& nodes) OVERRIDE; On 2013/04/06 00:29:39, Ilya Sherman wrote: > nit: This should be grouped with whatever set of methods it's overriding, under > a "// Foo implementation." section. It should also be above the instance > variables for this class. Done.
https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/browser/autofill_manager.cc:456: Reset(); On 2013/04/08 22:55:50, dgwallinga wrote: > On 2013/04/06 00:29:39, Ilya Sherman wrote: > > What happens if the user is already interacting with the page? Will this do > the > > right thing? If so, why? > Hmm, that depends very much on what we think the 'right thing' is. The page will > be treated as though it's a new page. For the purposes of correctly filling out > an ajaxy flow, that's what we want. For the purposes of recording metrics (and > it's mostly metric related vars that get reset in that call) it's less clear. If > most of the page changes, then it makes sense to count it is a new page. If it's > just a new field that pops up, maybe not. If absolutely necessary, we could > probably add more nuance to the metrics we're logging to distinguish these types > of flows, but I don't know exactly how we'd want to structure that. Reset() also throws away the list of form_structures_, which means that we lose any server-side data that we might have had for these in the case that just a field or two was changed. Note that this is not actually that rare a case: websites will often change the form fields that are shown based on the user's choice of country from a dropdown (e.g. should a "state" field be shown?). Maybe that's ok, though, since the server will tend to have data for forms as they look when they're submitted, rather than when they're loaded. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/common/forms_seen_param.h (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/common/forms_seen_param.h:12: enum FormsSeenParam { On 2013/04/08 22:55:50, dgwallinga wrote: > On 2013/04/06 00:29:39, Ilya Sherman wrote: > > nit: Perhaps "FormsSeenStatus"? > Doesn't feel right. AutofillManager needs to know how the forms were created, > but I wouldn't describe that as having anything to do with the 'status' of the > forms. It's not the status of the forms; it's the status (or "state", if you prefer) of the renderer-side form manager (a.k.a. the AutofillAgent). Feel free to come up with a better name. Just don't leave it as "Param", which is too generic. https://chromiumcodereview.appspot.com/13264002/diff/40001/components/autofil... File components/autofill/renderer/autofill_agent.h (right): https://chromiumcodereview.appspot.com/13264002/diff/40001/components/autofil... components/autofill/renderer/autofill_agent.h:265: // since the last time AutofillManager was sent form info. nit: Something like "since the last loaded forms were sent to the browser process." https://chromiumcodereview.appspot.com/13264002/diff/40001/components/autofil... components/autofill/renderer/autofill_agent.h:266: bool has_new_forms_for_manager_; nit: has_new_forms_for_browser_
https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/browser/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/browser/autofill_manager.cc:456: Reset(); On 2013/04/09 02:17:46, Ilya Sherman wrote: > On 2013/04/08 22:55:50, dgwallinga wrote: > > On 2013/04/06 00:29:39, Ilya Sherman wrote: > > > What happens if the user is already interacting with the page? Will this do > > the > > > right thing? If so, why? > > Hmm, that depends very much on what we think the 'right thing' is. The page > will > > be treated as though it's a new page. For the purposes of correctly filling > out > > an ajaxy flow, that's what we want. For the purposes of recording metrics (and > > it's mostly metric related vars that get reset in that call) it's less clear. > If > > most of the page changes, then it makes sense to count it is a new page. If > it's > > just a new field that pops up, maybe not. If absolutely necessary, we could > > probably add more nuance to the metrics we're logging to distinguish these > types > > of flows, but I don't know exactly how we'd want to structure that. > > Reset() also throws away the list of form_structures_, which means that we lose > any server-side data that we might have had for these in the case that just a > field or two was changed. Note that this is not actually that rare a case: > websites will often change the form fields that are shown based on the user's > choice of country from a dropdown (e.g. should a "state" field be shown?). > > Maybe that's ok, though, since the server will tend to have data for forms as > they look when they're submitted, rather than when they're loaded. If we don't throw the list of form_structures_ away, then the post-ajax forms just get appended which results in duplicates, and then the mappings back from the server don't line up properly with the form_structures_, and all manner of annoying things can happen. As you say, though - the autofill server is going to have data based on the submitted state, which will probably include any dom changes. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... File components/autofill/common/forms_seen_param.h (right): https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/common/forms_seen_param.h:10: // Params to be sent with a FormsSeen message to AutofillManager. On 2013/04/06 00:29:39, Ilya Sherman wrote: > nit: "Params" -> "Param", though I would actually drop this entire sentence. Done. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/common/forms_seen_param.h:10: // Params to be sent with a FormsSeen message to AutofillManager. On 2013/04/06 00:29:39, Ilya Sherman wrote: > nit: "Params" -> "Param", though I would actually drop this entire sentence. Done. https://chromiumcodereview.appspot.com/13264002/diff/31002/components/autofil... components/autofill/common/forms_seen_param.h:12: enum FormsSeenParam { On 2013/04/09 02:17:46, Ilya Sherman wrote: > On 2013/04/08 22:55:50, dgwallinga wrote: > > On 2013/04/06 00:29:39, Ilya Sherman wrote: > > > nit: Perhaps "FormsSeenStatus"? > > Doesn't feel right. AutofillManager needs to know how the forms were created, > > but I wouldn't describe that as having anything to do with the 'status' of the > > forms. > > It's not the status of the forms; it's the status (or "state", if you prefer) of > the renderer-side form manager (a.k.a. the AutofillAgent). Feel free to come up > with a better name. Just don't leave it as "Param", which is too generic. Done. https://chromiumcodereview.appspot.com/13264002/diff/40001/components/autofil... File components/autofill/renderer/autofill_agent.h (right): https://chromiumcodereview.appspot.com/13264002/diff/40001/components/autofil... components/autofill/renderer/autofill_agent.h:265: // since the last time AutofillManager was sent form info. On 2013/04/09 02:17:46, Ilya Sherman wrote: > nit: Something like "since the last loaded forms were sent to the browser > process." Done. https://chromiumcodereview.appspot.com/13264002/diff/40001/components/autofil... components/autofill/renderer/autofill_agent.h:266: bool has_new_forms_for_manager_; On 2013/04/09 02:17:46, Ilya Sherman wrote: > nit: has_new_forms_for_browser_ Done.
LGTM, thanks.
@isherman,ahutter - made some small changes in autofill_manager.cc and autofill.cc to resolve a couple bugs I came across in manual testing. Please take another glance when you have a chance. @jschuh - can you glance at my changes in autofill_messages.h?
https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:966: // save an IPC nit: Please end the sentence with a period. https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:968: *topmost_frame_, 0, &forms, &form_elements_); Can calling this method with a threshold of 0 ever cause has_more_forms to be true? https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:974: click_timer_.Stop(); Why is this part MaybeSendDynamicFormsSeen() and not didAssociateFormControls()?
https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:966: // save an IPC On 2013/04/18 22:19:08, Ilya Sherman wrote: > nit: Please end the sentence with a period. Done. https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:968: *topmost_frame_, 0, &forms, &form_elements_); On 2013/04/18 22:19:08, Ilya Sherman wrote: > Can calling this method with a threshold of 0 ever cause has_more_forms to be > true? gah, knew I missed something. https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:974: click_timer_.Stop(); On 2013/04/18 22:19:08, Ilya Sherman wrote: > Why is this part MaybeSendDynamicFormsSeen() and not didAssociateFormControls()? I don't want to clear the timer until I know that AutofillManager is going to try to check the field mappings again, and I don't know that until after that !forms.empty() check. If I cleared the timer and AutofillManager didn't check the field mappings, there could be some (admittedly strange) error case where some form element gets immediately added, and then removed, the buyflow doesn't advance correctly, and then nothing notices and so we leave the UI open indefinitely.
https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:970: autofill::PARTIAL_AND_DYNAMIC_FORMS_SEEN : autofill::DYNAMIC_FORMS_SEEN; Is the PARTIAL_AND_DYNAMIC_FORMS_SEEN case reachable anymore? https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:974: click_timer_.Stop(); On 2013/04/18 23:56:44, dgwallinga wrote: > On 2013/04/18 22:19:08, Ilya Sherman wrote: > > Why is this part MaybeSendDynamicFormsSeen() and not > didAssociateFormControls()? > > I don't want to clear the timer until I know that AutofillManager is going to > try to check the field mappings again, and I don't know that until after that > !forms.empty() check. If I cleared the timer and AutofillManager didn't check > the field mappings, there could be some (admittedly strange) error case where > some form element gets immediately added, and then removed, the buyflow doesn't > advance correctly, and then nothing notices and so we leave the UI open > indefinitely. That doesn't match how the logic in DidStartProvisionalLoad() works. IMO the two should be parallel. Also, looking at that method, it seems that you likely should be setting autocheckout_click_in_progress_ to false here.
https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... File components/autofill/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:970: autofill::PARTIAL_AND_DYNAMIC_FORMS_SEEN : autofill::DYNAMIC_FORMS_SEEN; On 2013/04/19 01:38:42, Ilya Sherman wrote: > Is the PARTIAL_AND_DYNAMIC_FORMS_SEEN case reachable anymore? Not presently. It would be applicable if you ever want to support normal autofill for dynamic forms, but if you're hinting that I should remove it from FromsSeenState until that eventuality arises, I have no objection. https://chromiumcodereview.appspot.com/13264002/diff/67001/components/autofil... components/autofill/renderer/autofill_agent.cc:974: click_timer_.Stop(); On 2013/04/19 01:38:42, Ilya Sherman wrote: > On 2013/04/18 23:56:44, dgwallinga wrote: > > On 2013/04/18 22:19:08, Ilya Sherman wrote: > > > Why is this part MaybeSendDynamicFormsSeen() and not > > didAssociateFormControls()? > > > > I don't want to clear the timer until I know that AutofillManager is going to > > try to check the field mappings again, and I don't know that until after that > > !forms.empty() check. If I cleared the timer and AutofillManager didn't check > > the field mappings, there could be some (admittedly strange) error case where > > some form element gets immediately added, and then removed, the buyflow > doesn't > > advance correctly, and then nothing notices and so we leave the UI open > > indefinitely. > > That doesn't match how the logic in DidStartProvisionalLoad() works. IMO the > two should be parallel. > > Also, looking at that method, it seems that you likely should be setting > autocheckout_click_in_progress_ to false here. If the provisional load fails, we'll call ClickFailed. If not, we'll send FormsSeen when the document load completes (which we always do for the topmost frame whether there are new forms or not), leaving AutofillManager to check for problems. Now I suppose we could always send the IPC for dynamic forms as well, but the flow is different enough that we can get by with only sending the IPC for new forms *if* we don't clear the timer until we know we're sending something. autocheckout_click_in_progress_ should already be false here. It gets set true in DidStartProvisionalLoad() if the timer is running as a way of noting that a click was in progress in case the provisional load fails. Doesn't really apply in this case.
LGTM, thanks
@jschuh - can you please take a glance?
@cdn - could you please take a quick glance at the changes to autofill_messages.h?
IPC security review lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgwallinga@chromium.org/13264002/74002
Failed to apply patch for components/autofill/browser/autofill_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file components/autofill/browser/autofill_manager.cc
Hunk #1 FAILED at 455.
1 out of 1 hunk FAILED -- saving rejects to file
components/autofill/browser/autofill_manager.cc.rej
Patch: components/autofill/browser/autofill_manager.cc
Index: components/autofill/browser/autofill_manager.cc
diff --git a/components/autofill/browser/autofill_manager.cc
b/components/autofill/browser/autofill_manager.cc
index
ceecddb17fa24df776146692322544e1e01bedff..d0106b49b9cc1903707fb8694cb5186b2557738d
100644
--- a/components/autofill/browser/autofill_manager.cc
+++ b/components/autofill/browser/autofill_manager.cc
@@ -455,19 +455,28 @@ bool AutofillManager::OnFormSubmitted(const FormData&
form,
void AutofillManager::OnFormsSeen(const std::vector<FormData>& forms,
const TimeTicks& timestamp,
- bool has_more_forms) {
+ autofill::FormsSeenState state) {
+ bool is_post_document_load = state == autofill::DYNAMIC_FORMS_SEEN;
+ bool has_more_forms = state == autofill::PARTIAL_FORMS_SEEN;
+ // If new forms were added via AJAX or DHML, treat as new page.
+ if (is_post_document_load)
+ Reset();
+
RenderViewHost* host = web_contents()->GetRenderViewHost();
if (!host)
return;
if (!GetAutocheckoutURLPrefix().empty()) {
- host->Send(
- new AutofillMsg_WhitelistedForAutocheckout(host->GetRoutingID()));
// If whitelisted URL, fetch all the forms.
- if (has_more_forms) {
+ if (has_more_forms)
host->Send(new AutofillMsg_GetAllForms(host->GetRoutingID()));
- return;
+ if (!is_post_document_load) {
+ host->Send(
+ new AutofillMsg_WhitelistedForAutocheckout(host->GetRoutingID()));
}
+ // Now return early, as OnFormsSeen will get called again with all forms.
+ if (has_more_forms)
+ return;
}
autocheckout_manager_.OnFormsSeen();
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgwallinga@chromium.org/13264002/94001
Retried try job too often on win7_aura for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgwallinga@chromium.org/13264002/94001
Message was sent while issue was closed.
Change committed as 197479 |
