Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11270018:
[autofill] Adding new API to request an interactive autocomplete UI flow. (Closed)
|
Created:
8 years, 1 month ago by Dan Beam Modified:
8 years, 1 month ago CC:
chromium-reviews, dyu1, dhollowa+watch_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, browser-components-watch_chromium.org, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[autofill] Adding new API to request an interactive autocomplete UI flow.
BUG=157661
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166019
Patch Set 1 #
Total comments: 17
Patch Set 2 : slice #Patch Set 3 : do a barrel roll! #
Total comments: 15
Patch Set 4 : review feedback #
Total comments: 8
Patch Set 5 : jam@ review #Patch Set 6 : moar review comments #Patch Set 7 : newest #
Total comments: 27
Patch Set 8 : review #
Total comments: 2
Patch Set 9 : remove forward #
Total comments: 6
Patch Set 10 : isherman@ review #
Total comments: 2
Patch Set 11 : thestig@ review #Patch Set 12 : webkit moved #Patch Set 13 : 80 col #
Total comments: 9
Patch Set 14 : form_ -> currently_requesting_form_ #Patch Set 15 : the winrar #Patch Set 16 : if() -> DCHECK() #Patch Set 17 : smaller codez == better codez #
Messages
Total messages: 44 (0 generated)
reviewers/watchers, please ignore until further clarifications have been made regarding the design/API https://codereview.chromium.org/11270018/diff/1/content/browser/web_contents/... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/11270018/diff/1/content/browser/web_contents/... content/browser/web_contents/web_contents_impl.cc:1535: delegate_->RequestAutocomplete(this); not sure what to do without a delegate here...
https://codereview.chromium.org/11270018/diff/1/chrome/browser/autofill/autof... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11270018/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.h:230: void OnRequestAutocomplete(const FormData& form); I don't know if this belongs here. You have to implement RequestAutocomplete in browser.cc because it's the WebContentsDelegate. For now that's the end of the line. https://codereview.chromium.org/11270018/diff/1/chrome/renderer/autofill/auto... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/1/chrome/renderer/autofill/auto... chrome/renderer/autofill/autofill_agent.cc:219: // TODO(dbeam): should popups be hidden here? Maybe after a view message from what popups? https://codereview.chromium.org/11270018/diff/1/chrome/renderer/autofill/auto... File chrome/renderer/autofill/autofill_agent.h (right): https://codereview.chromium.org/11270018/diff/1/chrome/renderer/autofill/auto... chrome/renderer/autofill/autofill_agent.h:72: // WebKit::WebAutofillClient: this looks for all the world like the place RequestAutocomplete belongs https://codereview.chromium.org/11270018/diff/1/content/browser/web_contents/... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/11270018/diff/1/content/browser/web_contents/... content/browser/web_contents/web_contents_impl.cc:1535: delegate_->RequestAutocomplete(this); On 2012/10/24 20:48:35, Dan Beam wrote: > not sure what to do without a delegate here... as above in RequestMediaAccessPermission, you should probably trigger the "cancel" response. https://codereview.chromium.org/11270018/diff/1/content/public/renderer/rende... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/11270018/diff/1/content/public/renderer/rende... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, this doesn't look like it belongs here
ptal https://chromiumcodereview.appspot.com/11270018/diff/1/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:219: // TODO(dbeam): should popups be hidden here? Maybe after a view message from On 2012/10/24 21:59:44, Evan Stade wrote: > what popups? any existing autocomplete suggestion popups https://chromiumcodereview.appspot.com/11270018/diff/1/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.h:72: // WebKit::WebAutofillClient: On 2012/10/24 21:59:44, Evan Stade wrote: > this looks for all the world like the place RequestAutocomplete belongs I thought so at first as well, but it ended up being not particularly where I wanted in webkit. I asked isherman@ about it and he agreed. He also mentioned that csharp@ is moving much of this to the browser for greater control of the UI? https://chromiumcodereview.appspot.com/11270018/diff/1/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/1/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1535: delegate_->RequestAutocomplete(this); On 2012/10/24 21:59:44, Evan Stade wrote: > On 2012/10/24 20:48:35, Dan Beam wrote: > > not sure what to do without a delegate here... > > as above in RequestMediaAccessPermission, you should probably trigger the > "cancel" response. triggering a cancel now https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, On 2012/10/24 21:59:44, Evan Stade wrote: > this doesn't look like it belongs here this can stay, now, right?
https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 03:06:05, Dan Beam wrote: > On 2012/10/24 21:59:44, Evan Stade wrote: > > this doesn't look like it belongs here > > this can stay, now, right? no, I still don't believe this belongs. This one is not like the others; it's not a "change to the frame". Then again, neither is PrintPage --- that one doesn't seem to fit in either. Maybe you can ask jam@ for his opinion. I wonder if adding a RenderViewDelegate which dispatches calls like this on the chrome side would make sense? https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/common/autof... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/common/autof... chrome/common/autofill_messages.h:140: bool /* whether the user pressed cancel */) "whether the user pressed cancel" is very specific. What if the user presses escape or presses the [x]? What if we run into some other kind of error that prevents handling the request? For example, the request could originate from an instant-loaded page, in which case we'd want to ignore it instead of showing a UI. Therefore the bool should be described as "success/failure". Also I think true should correlate to success. https://chromiumcodereview.appspot.com/11270018/diff/8001/content/browser/ren... File content/browser/renderer_host/render_view_host_delegate.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/content/browser/ren... content/browser/renderer_host/render_view_host_delegate.h:420: virtual void RequestAutocomplete() {} what's this for?
Looks pretty good! https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/browser/auto... File chrome/browser/autofill/autofill_manager.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.h:230: void OnRequestAutocomplete(const FormData& form); You'll also want to pass a message id so that the renderer can discard stale messages. https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/common/autof... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/common/autof... chrome/common/autofill_messages.h:140: bool /* whether the user pressed cancel */) nit: Boolean parameters are hard to make sense of at the call site. We generally prefer enumerated values with descriptive names. https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/renderer/aut... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:222: // the browser to say whether the interactive autocomplete UI will show? Yes, we should hide popups and cancel any pending requests (by incrementing the autofill_query_id_ so that any incoming responses are discarded as stale). In fact, we could probably just rename autofill_query_id_ to something more like ipc_message_id_, and reuse it here. https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:229: EXTRACT_VALUE | EXTRACT_OPTION_TEXT), nit: I don't think we need to extract values or option text. These are useful for heuristics, but probably aren't needed for the interactive dialog UI. https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:607: form_.dispatchAutocompleteEvent(success); We should do some sort of checking here to make sure the form still exists. It would be bad if the form could be removed from the DOM while we were processing the request, and if that caused us to crash. https://chromiumcodereview.appspot.com/11270018/diff/8001/content/public/rend... File content/public/renderer/render_view_observer.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/content/public/rend... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, nit: I think "DidRequestAutocomplete" better matches the other method names in this class.
https://chromiumcodereview.appspot.com/11270018/diff/1/chrome/browser/autofil... File chrome/browser/autofill/autofill_manager.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/chrome/browser/autofil... chrome/browser/autofill/autofill_manager.h:230: void OnRequestAutocomplete(const FormData& form); On 2012/10/24 21:59:44, Evan Stade wrote: > I don't know if this belongs here. You have to implement RequestAutocomplete in > browser.cc because it's the WebContentsDelegate. For now that's the end of the > line. This does belong here. We shouldn't need to touch browser.cc. In an ideal world, though, the AutofillManager should be split up into multiple classes, with IPC message handling probably being handled by a different class from the one that actually fills out the forms. https://chromiumcodereview.appspot.com/11270018/diff/1/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.h:72: // WebKit::WebAutofillClient: On 2012/10/26 03:06:05, Dan Beam wrote: > On 2012/10/24 21:59:44, Evan Stade wrote: > > this looks for all the world like the place RequestAutocomplete belongs > > I thought so at first as well, but it ended up being not particularly where I > wanted in webkit. I asked isherman@ about it and he agreed. He also mentioned > that csharp@ is moving much of this to the browser for greater control of the > UI? Yeah, the WebAutofillClient is all about the existing popup UI, and is being deleted because the UI is moving into the browser process. https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 03:23:02, Evan Stade wrote: > On 2012/10/26 03:06:05, Dan Beam wrote: > > On 2012/10/24 21:59:44, Evan Stade wrote: > > > this doesn't look like it belongs here > > > > this can stay, now, right? > > no, I still don't believe this belongs. This one is not like the others; it's > not a "change to the frame". Then again, neither is PrintPage --- that one > doesn't seem to fit in either. Maybe you can ask jam@ for his opinion. I wonder > if adding a RenderViewDelegate which dispatches calls like this on the chrome > side would make sense? Really? This seems like the right place to me. Certainly we'll want jam@ to review this though.
https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 03:35:28, Ilya Sherman wrote: > On 2012/10/26 03:23:02, Evan Stade wrote: > > On 2012/10/26 03:06:05, Dan Beam wrote: > > > On 2012/10/24 21:59:44, Evan Stade wrote: > > > > this doesn't look like it belongs here > > > > > > this can stay, now, right? > > > > no, I still don't believe this belongs. This one is not like the others; it's > > not a "change to the frame". Then again, neither is PrintPage --- that one > > doesn't seem to fit in either. Maybe you can ask jam@ for his opinion. I > wonder > > if adding a RenderViewDelegate which dispatches calls like this on the chrome > > side would make sense? > perhaps ContentRendererClient instead. > Really? This seems like the right place to me. Certainly we'll want jam@ to > review this though. do you expect that more than one thing will want to "Observe" this API call? Does this API call represent a change to the frame?
https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 03:52:53, Evan Stade wrote: > On 2012/10/26 03:35:28, Ilya Sherman wrote: > > On 2012/10/26 03:23:02, Evan Stade wrote: > > > On 2012/10/26 03:06:05, Dan Beam wrote: > > > > On 2012/10/24 21:59:44, Evan Stade wrote: > > > > > this doesn't look like it belongs here > > > > > > > > this can stay, now, right? > > > > > > no, I still don't believe this belongs. This one is not like the others; > it's > > > not a "change to the frame". Then again, neither is PrintPage --- that one > > > doesn't seem to fit in either. Maybe you can ask jam@ for his opinion. I > > wonder > > > if adding a RenderViewDelegate which dispatches calls like this on the > chrome > > > side would make sense? > > > > perhaps ContentRendererClient instead. > > > Really? This seems like the right place to me. Certainly we'll want jam@ to > > review this though. > > do you expect that more than one thing will want to "Observe" this API call? > Does this API call represent a change to the frame? Lots of the methods in RenderViewObserver are only observed by one class. The advantage of adding this method to this class is that the RenderView only needs to keep track of one list of observers. If we create a new AutofillObserver class, the RenderView needs to keep track of a list of AutofillObservers. The whole point of the RenderViewObserver class initially was to decouple the RenderView from having to know about all the different types of observers it has. Anyway, we can see what John says, but the above is why I think it probably makes sense to keep this method in this class.
https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... File content/public/renderer/render_view_observer.h (right): https://chromiumcodereview.appspot.com/11270018/diff/1/content/public/rendere... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 04:03:33, Ilya Sherman wrote: > On 2012/10/26 03:52:53, Evan Stade wrote: > > On 2012/10/26 03:35:28, Ilya Sherman wrote: > > > On 2012/10/26 03:23:02, Evan Stade wrote: > > > > On 2012/10/26 03:06:05, Dan Beam wrote: > > > > > On 2012/10/24 21:59:44, Evan Stade wrote: > > > > > > this doesn't look like it belongs here > > > > > > > > > > this can stay, now, right? > > > > > > > > no, I still don't believe this belongs. This one is not like the others; > > it's > > > > not a "change to the frame". Then again, neither is PrintPage --- that one > > > > doesn't seem to fit in either. Maybe you can ask jam@ for his opinion. I > > > wonder > > > > if adding a RenderViewDelegate which dispatches calls like this on the > > chrome > > > > side would make sense? > > > > > > > perhaps ContentRendererClient instead. > > > > > Really? This seems like the right place to me. Certainly we'll want jam@ > to > > > review this though. > > > > do you expect that more than one thing will want to "Observe" this API call? > > Does this API call represent a change to the frame? > > Lots of the methods in RenderViewObserver are only observed by one class. The > advantage of adding this method to this class is that the RenderView only needs > to keep track of one list of observers. If we create a new AutofillObserver > class, the RenderView needs to keep track of a list of AutofillObservers. The > whole point of the RenderViewObserver class initially was to decouple the > RenderView from having to know about all the different types of observers it > has. I didn't suggest that. > > Anyway, we can see what John says, but the above is why I think it probably > makes sense to keep this method in this class. yes, it will be up to John.
https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/browser/auto... File chrome/browser/autofill/autofill_manager.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.h:230: void OnRequestAutocomplete(const FormData& form); On 2012/10/26 03:30:52, Ilya Sherman wrote: > You'll also want to pass a message id so that the renderer can discard stale > messages. Done. https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/common/autof... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/common/autof... chrome/common/autofill_messages.h:140: bool /* whether the user pressed cancel */) On 2012/10/26 03:30:52, Ilya Sherman wrote: > nit: Boolean parameters are hard to make sense of at the call site. We > generally prefer enumerated values with descriptive names. I see 3 examples of bools in messages in this file that could be enums. Is this different? Should we update the others? https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/renderer/aut... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:222: // the browser to say whether the interactive autocomplete UI will show? On 2012/10/26 03:30:52, Ilya Sherman wrote: > Yes, we should hide popups and cancel any pending requests (by incrementing the > autofill_query_id_ so that any incoming responses are discarded as stale). In > fact, we could probably just rename autofill_query_id_ to something more like > ipc_message_id_, and reuse it here. creating separate counter for this request like we talked about https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:229: EXTRACT_VALUE | EXTRACT_OPTION_TEXT), On 2012/10/26 03:30:52, Ilya Sherman wrote: > nit: I don't think we need to extract values or option text. These are useful > for heuristics, but probably aren't needed for the interactive dialog UI. Done. https://chromiumcodereview.appspot.com/11270018/diff/8001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:607: form_.dispatchAutocompleteEvent(success); On 2012/10/26 03:30:52, Ilya Sherman wrote: > We should do some sort of checking here to make sure the form still exists. It > would be bad if the form could be removed from the DOM while we were processing > the request, and if that caused us to crash. why would it being removed from the DOM cause us to crash? alternatively, do you know how I'd check if this is in the DOM still (didn't see any examples in this class)? https://chromiumcodereview.appspot.com/11270018/diff/8001/content/browser/ren... File content/browser/renderer_host/render_view_host_delegate.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/content/browser/ren... content/browser/renderer_host/render_view_host_delegate.h:420: virtual void RequestAutocomplete() {} On 2012/10/26 03:23:02, Evan Stade wrote: > what's this for? a red herring, sorry, removed... https://chromiumcodereview.appspot.com/11270018/diff/8001/content/public/rend... File content/public/renderer/render_view_observer.h (right): https://chromiumcodereview.appspot.com/11270018/diff/8001/content/public/rend... content/public/renderer/render_view_observer.h:85: virtual void RequestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 03:30:52, Ilya Sherman wrote: > nit: I think "DidRequestAutocomplete" better matches the other method names in > this class. I'll wait until we've decided where this is going (as if it moves it might have a different convention).
jam@: content/ isherman@: chrome/browser/autofill/, chrome/renderer/autofill/
+tsepez for security FYI (adding new IPC)
https://codereview.chromium.org/11270018/diff/11002/chrome/common/autofill_me... File chrome/common/autofill_messages.h (right): https://codereview.chromium.org/11270018/diff/11002/chrome/common/autofill_me... chrome/common/autofill_messages.h:140: int /* id of this message */, nit: here and below, by convention what's inside the comment is the name of the parameter in the dispatcher. in this case, that'd be query_id, success https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... content/renderer/render_view_impl.h:458: virtual void requestAutocomplete(WebKit::WebFrame* frame, which interface is this from (I couldn't find it on trac)? autofill related methods should be on WebAutofillClient. that's why that interface got added, so that autofill callbacks from webkit go straight to chrome instead of hopping through content
IPC LGTM.
https://codereview.chromium.org/11270018/diff/11002/chrome/common/autofill_me... File chrome/common/autofill_messages.h (right): https://codereview.chromium.org/11270018/diff/11002/chrome/common/autofill_me... chrome/common/autofill_messages.h:140: int /* id of this message */, On 2012/10/26 20:09:38, John Abd-El-Malek wrote: > nit: here and below, by convention what's inside the comment is the name of the > parameter in the dispatcher. in this case, that'd be query_id, success Done. https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... content/renderer/render_view_impl.h:458: virtual void requestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 20:09:38, John Abd-El-Malek wrote: > which interface is this from (I couldn't find it on trac)? sorry, the WebKit stuff isn't in yet, it's in review - https://bugs.webkit.org/show_bug.cgi?id=100557 > autofill related methods should be on WebAutofillClient. that's why that > interface got added, so that autofill callbacks from webkit go straight to > chrome instead of hopping through content the only thing that implements this interface right now is AutofillPopupMenuClient. This isn't really related to popup menus. I asked isherman@ if he'd like me to make AutofillPopupMenuClient more generic (i.e. AutofillClient or something like that) so I could include this method there, but he said that AutofillPopupMenuClient is deprecated as it's in the process of being moved. isherman@, is this correct?
https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... content/renderer/render_view_impl.h:458: virtual void requestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 20:58:39, Dan Beam wrote: > On 2012/10/26 20:09:38, John Abd-El-Malek wrote: > > which interface is this from (I couldn't find it on trac)? > > sorry, the WebKit stuff isn't in yet, it's in review - > https://bugs.webkit.org/show_bug.cgi?id=100557 > > > autofill related methods should be on WebAutofillClient. that's why that > > interface got added, so that autofill callbacks from webkit go straight to > > chrome instead of hopping through content > > the only thing that implements this interface right now is > AutofillPopupMenuClient. you need to look in the chromium repo (not webkit), see http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/autofill/a... AutofillAgent (which you're modifying to get the call through RenderViewObserver) implements WebAutofillClient. > This isn't really related to popup menus. I asked > isherman@ if he'd like me to make AutofillPopupMenuClient more generic (i.e. > AutofillClient or something like that) so I could include this method there, but > he said that AutofillPopupMenuClient is deprecated as it's in the process of > being moved. isherman@, is this correct?
https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... content/renderer/render_view_impl.h:458: virtual void requestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 21:06:04, John Abd-El-Malek wrote: > On 2012/10/26 20:58:39, Dan Beam wrote: > > On 2012/10/26 20:09:38, John Abd-El-Malek wrote: > > > which interface is this from (I couldn't find it on trac)? > > > > sorry, the WebKit stuff isn't in yet, it's in review - > > https://bugs.webkit.org/show_bug.cgi?id=100557 > > > > > autofill related methods should be on WebAutofillClient. that's why that > > > interface got added, so that autofill callbacks from webkit go straight to > > > chrome instead of hopping through content > > > > the only thing that implements this interface right now is > > AutofillPopupMenuClient. > > you need to look in the chromium repo (not webkit), see > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/autofill/a... > > AutofillAgent (which you're modifying to get the call through > RenderViewObserver) implements WebAutofillClient. > > > This isn't really related to popup menus. I asked > > isherman@ if he'd like me to make AutofillPopupMenuClient more generic (i.e. > > AutofillClient or something like that) so I could include this method there, > but > > he said that AutofillPopupMenuClient is deprecated as it's in the process of > > being moved. isherman@, is this correct? > John, the WebAutofillClient class is currently primarily responsible for implementing a UI that lives in the renderer. Chris Sharp is currently working on moving that UI into the browser process. Once that work is complete, the WebAutofillClient will only have three methods: virtual void textFieldDidEndEditing(const WebInputElement&) virtual void textFieldDidChange(const WebInputElement&) virtual void textFieldDidReceiveKeyDown(const WebInputElement&, const WebKeyboardEvent&) (plus possibly this new one). These methods are really not any more Autofill-specific than is e.g. WillSubmitForm() from the RenderViewObserver class (unless I'm just unaware of other clients of that method). Given that the WebAutofillClient class is shrinking to just handling these events, is it still appropriate to add this new method to that class, rather than simply removing the class once the UI migration work is finished? Put another way: When should methods be added to the RenderViewObserver, rather than routed via a more specific WebFooClient class? Should WillSubmitForm() be moved into this class as well?
https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... content/renderer/render_view_impl.h:458: virtual void requestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 23:18:42, Ilya Sherman wrote: > On 2012/10/26 21:06:04, John Abd-El-Malek wrote: > > On 2012/10/26 20:58:39, Dan Beam wrote: > > > On 2012/10/26 20:09:38, John Abd-El-Malek wrote: > > > > which interface is this from (I couldn't find it on trac)? > > > > > > sorry, the WebKit stuff isn't in yet, it's in review - > > > https://bugs.webkit.org/show_bug.cgi?id=100557 > > > > > > > autofill related methods should be on WebAutofillClient. that's why that > > > > interface got added, so that autofill callbacks from webkit go straight to > > > > chrome instead of hopping through content > > > > > > the only thing that implements this interface right now is > > > AutofillPopupMenuClient. > > > > you need to look in the chromium repo (not webkit), see > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/autofill/a... > > > > AutofillAgent (which you're modifying to get the call through > > RenderViewObserver) implements WebAutofillClient. > > > > > This isn't really related to popup menus. I asked > > > isherman@ if he'd like me to make AutofillPopupMenuClient more generic (i.e. > > > AutofillClient or something like that) so I could include this method there, > > but > > > he said that AutofillPopupMenuClient is deprecated as it's in the process of > > > being moved. isherman@, is this correct? > > > > John, the WebAutofillClient class is currently primarily responsible for > implementing a UI that lives in the renderer. Chris Sharp is currently working > on moving that UI into the browser process. Once that work is complete, the > WebAutofillClient will only have three methods: > > virtual void textFieldDidEndEditing(const WebInputElement&) > virtual void textFieldDidChange(const WebInputElement&) > virtual void textFieldDidReceiveKeyDown(const WebInputElement&, const > WebKeyboardEvent&) > > (plus possibly this new one). These methods are really not any more > Autofill-specific than is e.g. WillSubmitForm() from the RenderViewObserver > class (unless I'm just unaware of other clients of that method). Given that the > WebAutofillClient class is shrinking to just handling these events, is it still > appropriate to add this new method to that class, rather than simply removing > the class once the UI migration work is finished? Put another way: When should > methods be added to the RenderViewObserver, rather than routed via a more > specific WebFooClient class? Should WillSubmitForm() be moved into this class > as well? When I created WebAutofillClient, and the other related interfaces (spellcheck, devtools), the criteria was that each interface would get a set of related callbacks from WebKit that were currently on WebViewClient/WebFrameClient. These callbacks were only used by one feature. So in the example above, willSubmitForm needs to stay in content because RenderViewImpl does do stuff in that callback. So yes, I think the function should move, and stay with the methods that are remaining in that interface.
https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/11002/content/renderer/render_v... content/renderer/render_view_impl.h:458: virtual void requestAutocomplete(WebKit::WebFrame* frame, On 2012/10/26 23:48:47, John Abd-El-Malek wrote: > On 2012/10/26 23:18:42, Ilya Sherman wrote: > > On 2012/10/26 21:06:04, John Abd-El-Malek wrote: > > > On 2012/10/26 20:58:39, Dan Beam wrote: > > > > On 2012/10/26 20:09:38, John Abd-El-Malek wrote: > > > > > which interface is this from (I couldn't find it on trac)? > > > > > > > > sorry, the WebKit stuff isn't in yet, it's in review - > > > > https://bugs.webkit.org/show_bug.cgi?id=100557 > > > > > > > > > autofill related methods should be on WebAutofillClient. that's why that > > > > > interface got added, so that autofill callbacks from webkit go straight > to > > > > > chrome instead of hopping through content > > > > > > > > the only thing that implements this interface right now is > > > > AutofillPopupMenuClient. > > > > > > you need to look in the chromium repo (not webkit), see > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/autofill/a... > > > > > > AutofillAgent (which you're modifying to get the call through > > > RenderViewObserver) implements WebAutofillClient. > > > > > > > This isn't really related to popup menus. I asked > > > > isherman@ if he'd like me to make AutofillPopupMenuClient more generic > (i.e. > > > > AutofillClient or something like that) so I could include this method > there, > > > but > > > > he said that AutofillPopupMenuClient is deprecated as it's in the process > of > > > > being moved. isherman@, is this correct? > > > > > > > John, the WebAutofillClient class is currently primarily responsible for > > implementing a UI that lives in the renderer. Chris Sharp is currently > working > > on moving that UI into the browser process. Once that work is complete, the > > WebAutofillClient will only have three methods: > > > > virtual void textFieldDidEndEditing(const WebInputElement&) > > virtual void textFieldDidChange(const WebInputElement&) > > virtual void textFieldDidReceiveKeyDown(const WebInputElement&, const > > WebKeyboardEvent&) > > > > (plus possibly this new one). These methods are really not any more > > Autofill-specific than is e.g. WillSubmitForm() from the RenderViewObserver > > class (unless I'm just unaware of other clients of that method). Given that > the > > WebAutofillClient class is shrinking to just handling these events, is it > still > > appropriate to add this new method to that class, rather than simply removing > > the class once the UI migration work is finished? Put another way: When > should > > methods be added to the RenderViewObserver, rather than routed via a more > > specific WebFooClient class? Should WillSubmitForm() be moved into this class > > as well? > > When I created WebAutofillClient, and the other related interfaces (spellcheck, > devtools), the criteria was that each interface would get a set of related > callbacks from WebKit that were currently on WebViewClient/WebFrameClient. These > callbacks were only used by one feature. So in the example above, willSubmitForm > needs to stay in content because RenderViewImpl does do stuff in that callback. > > So yes, I think the function should move, and stay with the methods that are > remaining in that interface. Ok, I didn't realize that was the history for this interface -- this makes more sense now. Thanks for explaining!
ok, now skipping content / renderview* stuff https://codereview.chromium.org/11270018/diff/16010/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/16010/content/renderer/render_v... content/renderer/render_view_impl.h:107: class WebFormElement; just left this here as it was being transitively forwarded or included before...
https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/browser/aut... File chrome/browser/autofill/autofill_manager.h (right): https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/browser/aut... chrome/browser/autofill/autofill_manager.h:232: void OnRequestAutocomplete(int unique_id, const FormData& form); nit: Perhaps "ipc_message_id" rather than "unique_id"? "unique_id" in Autofill code is usually referring to a profile's unique id in the database. https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/common/auto... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/common/auto... chrome/common/autofill_messages.h:201: int /* query_id */, nit: Update this param name (this isn't really a query, at least in the sense that the other id is a query id) https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:59: int request_autocomplete_query_id_ = 0; This should be a private member of the class. Also, it doesn't seem like a single id will work if there can be multiple tabs in a single renderer, each showing a dialog. https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:624: NOTREACHED() << "Unknown autocomplete result."; nit: Please remove the logged text; you can add it as a comment instead. (It's inferable just from the line number for the NOTREACHED(), and the logged string adds unnecessary weight to the binary, or something like that.) https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:748: web_view->hidePopups(); You should also hide any popups that might be showing in the browser process's UI. https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.h:172: // Hides any currently showing autofill popups. nit: "autofill" -> "Autofill" https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.h:188: WebKit::WebFormElement form_; It doesn't seem like this will work if multiple tabs in a single renderer can have separate dialogs open simultaneously.
https://codereview.chromium.org/11270018/diff/16010/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11270018/diff/16010/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.cc:812: // (https://bugs.webkit.org/show_bug.cgi?id=100557). nit: it doesn't depend on webkit. It's more like "until we have an implementation". https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:224: request_autocomplete_query_id_ += 1; ++ https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:750: extra newline https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.h (right): https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.h:188: WebKit::WebFormElement form_; On 2012/10/27 07:44:11, Ilya Sherman wrote: > It doesn't seem like this will work if multiple tabs in a single renderer can > have separate dialogs open simultaneously. there's only one AutofillAgent per RenderView. RenderView 1:1 tab. Render view many:1 renderer process.
https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.h (right): https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.h:188: WebKit::WebFormElement form_; On 2012/10/29 17:54:28, Evan Stade wrote: > On 2012/10/27 07:44:11, Ilya Sherman wrote: > > It doesn't seem like this will work if multiple tabs in a single renderer can > > have separate dialogs open simultaneously. > > there's only one AutofillAgent per RenderView. RenderView 1:1 tab. Render view > many:1 renderer process. Ok, that makes this much easier to reason about -- thanks for clarifying that. Given that this is the case, I don't think we actually need a separate variable to track the IPC message id for this set of IPCs. Do y'all agree, or is there still a reason to separate the two?
https://codereview.chromium.org/11270018/diff/16010/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11270018/diff/16010/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.cc:812: // (https://bugs.webkit.org/show_bug.cgi?id=100557). On 2012/10/29 17:54:28, Evan Stade wrote: > nit: it doesn't depend on webkit. It's more like "until we have an > implementation". Done. https://codereview.chromium.org/11270018/diff/16010/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11270018/diff/16010/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:232: void OnRequestAutocomplete(int unique_id, const FormData& form); On 2012/10/27 07:44:11, Ilya Sherman wrote: > nit: Perhaps "ipc_message_id" rather than "unique_id"? "unique_id" in Autofill > code is usually referring to a profile's unique id in the database. Done. https://codereview.chromium.org/11270018/diff/16010/chrome/common/autofill_me... File chrome/common/autofill_messages.h (right): https://codereview.chromium.org/11270018/diff/16010/chrome/common/autofill_me... chrome/common/autofill_messages.h:201: int /* query_id */, On 2012/10/27 07:44:11, Ilya Sherman wrote: > nit: Update this param name (this isn't really a query, at least in the sense > that the other id is a query id) how is this not a query_id? it's a magically auto-incrementing IPC id just like the others. https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:59: int request_autocomplete_query_id_ = 0; On 2012/10/27 07:44:11, Ilya Sherman wrote: > This should be a private member of the class. Also, it doesn't seem like a > single id will work if there can be multiple tabs in a single renderer, each > showing a dialog. I don't think this is relevant any more. https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:224: request_autocomplete_query_id_ += 1; On 2012/10/29 17:54:28, Evan Stade wrote: > ++ Done. https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:624: NOTREACHED() << "Unknown autocomplete result."; On 2012/10/27 07:44:11, Ilya Sherman wrote: > nit: Please remove the logged text; you can add it as a comment instead. (It's > inferable just from the line number for the NOTREACHED(), and the logged string > adds unnecessary weight to the binary, or something like that.) Done. https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:748: web_view->hidePopups(); On 2012/10/27 07:44:11, Ilya Sherman wrote: > You should also hide any popups that might be showing in the browser process's > UI. The code wasn't doing this before and I'm simply pulling this out into a helper method. https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:750: On 2012/10/29 17:54:28, Evan Stade wrote: > extra newline Done. https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.h (right): https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.h:172: // Hides any currently showing autofill popups. On 2012/10/27 07:44:11, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" Done. https://codereview.chromium.org/11270018/diff/16010/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.h:188: WebKit::WebFormElement form_; On 2012/10/29 17:59:23, Ilya Sherman wrote: > On 2012/10/29 17:54:28, Evan Stade wrote: > > On 2012/10/27 07:44:11, Ilya Sherman wrote: > > > It doesn't seem like this will work if multiple tabs in a single renderer > can > > > have separate dialogs open simultaneously. > > > > there's only one AutofillAgent per RenderView. RenderView 1:1 tab. Render view > > many:1 renderer process. > > Ok, that makes this much easier to reason about -- thanks for clarifying that. > Given that this is the case, I don't think we actually need a separate variable > to track the IPC message id for this set of IPCs. Do y'all agree, or is there > still a reason to separate the two? Currently, the webkit implementation will hit the request *sending* code every time formEl.requestAutocomplete() is called. It seems like we could do 1 of two things to address multiple requests - 1) keep a boolean and ignore pending requests and remove the IPC query ID, or 2) keep query ID and queue requests or ignore older requests. I lean toward #1 unless we think it's a valid use case to show Infinity autocomplete UIs back to back (which I think is an easy way to end up in a rick roll alert() loop).
(removing myself as a reviewer) https://codereview.chromium.org/11270018/diff/12002/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/12002/content/renderer/render_v... content/renderer/render_view_impl.h:106: class WebFormElement; nit: not needed anymore. remove this and then no more changes in content
https://codereview.chromium.org/11270018/diff/12002/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11270018/diff/12002/content/renderer/render_v... content/renderer/render_view_impl.h:106: class WebFormElement; On 2012/10/29 19:20:17, John Abd-El-Malek wrote: > nit: not needed anymore. remove this and then no more changes in content in a previous set of comments I mention that this is still needed for include what you use (before it was being transitively forwarded or included somewhere else).
https://codereview.chromium.org/11270018/diff/16010/chrome/common/autofill_me... File chrome/common/autofill_messages.h (right): https://codereview.chromium.org/11270018/diff/16010/chrome/common/autofill_me... chrome/common/autofill_messages.h:201: int /* query_id */, On 2012/10/29 19:17:05, Dan Beam wrote: > On 2012/10/27 07:44:11, Ilya Sherman wrote: > > nit: Update this param name (this isn't really a query, at least in the sense > > that the other id is a query id) > > how is this not a query_id? it's a magically auto-incrementing IPC id just like > the others. btw, this was removed in favor of a bool to show whether we're requesting an autocomplete or not, so if that sticks around this isn't necessary as a param.
removed forward and any changes in content/
LGTM https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/common/auto... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/common/auto... chrome/common/autofill_messages.h:201: int /* query_id */, On 2012/10/29 19:17:05, Dan Beam wrote: > On 2012/10/27 07:44:11, Ilya Sherman wrote: > > nit: Update this param name (this isn't really a query, at least in the sense > > that the other id is a query id) > > how is this not a query_id? it's a magically auto-incrementing IPC id just like > the others. query_id was probably just not a very good name to begin with; it was originally the IPC message id for the Query... method, though I guess it's used for more than that now. https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:748: web_view->hidePopups(); On 2012/10/29 19:17:05, Dan Beam wrote: > On 2012/10/27 07:44:11, Ilya Sherman wrote: > > You should also hide any popups that might be showing in the browser process's > > UI. > > The code wasn't doing this before and I'm simply pulling this out into a helper > method. Feel free to file a bug against csharp@ and annotate this code with that bug # if you prefer. https://chromiumcodereview.appspot.com/11270018/diff/32001/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/32001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:223: // Cancel any pending autofill requests. nit: "autofill" -> "Autofill" https://chromiumcodereview.appspot.com/11270018/diff/32001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:607: return; nit: Can this be a DCHECK instead? https://chromiumcodereview.appspot.com/11270018/diff/32001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:617: } Optional nit: Maybe write this as a DCHECK() + remaining code, rather than as an if/else + NOTREACHED()?
https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/16010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:748: web_view->hidePopups(); On 2012/10/29 21:27:47, Ilya Sherman wrote: > On 2012/10/29 19:17:05, Dan Beam wrote: > > On 2012/10/27 07:44:11, Ilya Sherman wrote: > > > You should also hide any popups that might be showing in the browser > process's > > > UI. > > > > The code wasn't doing this before and I'm simply pulling this out into a > helper > > method. > > Feel free to file a bug against csharp@ and annotate this code with that bug # > if you prefer. https://chromiumcodereview.appspot.com/11273096/ https://chromiumcodereview.appspot.com/11270018/diff/32001/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/11270018/diff/32001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:223: // Cancel any pending autofill requests. On 2012/10/29 21:27:47, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" D'oh, sorry, done. https://chromiumcodereview.appspot.com/11270018/diff/32001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:607: return; On 2012/10/29 21:27:47, Ilya Sherman wrote: > nit: Can this be a DCHECK instead? Done. https://chromiumcodereview.appspot.com/11270018/diff/32001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:617: } On 2012/10/29 21:27:47, Ilya Sherman wrote: > Optional nit: Maybe write this as a DCHECK() + remaining code, rather than as an > if/else + NOTREACHED()? Done.
+thestig@ for chrome/common, btw
https://chromiumcodereview.appspot.com/11270018/diff/31003/chrome/common/auto... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/11270018/diff/31003/chrome/common/auto... chrome/common/autofill_messages.h:136: int /* result */) I think you can use AutocompleteResult here.
https://chromiumcodereview.appspot.com/11270018/diff/31003/chrome/common/auto... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/11270018/diff/31003/chrome/common/auto... chrome/common/autofill_messages.h:136: int /* result */) On 2012/10/30 00:37:11, Lei Zhang wrote: > I think you can use AutocompleteResult here. Done.
lgtm
FYI: this is still waiting on the webkit review, but it has changed ever so
slightly in autofill_agent.{h,cc} if anybody wants to re-review (isherman@?)
https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:227: WebFormElement non_const_form(form); nit: Seems like we shouldn't be passing in a const form to this method if we're just going to cast the constness away. https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:607: return; When can this happen?
https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:227: WebFormElement non_const_form(form); On 2012/11/02 02:56:57, Ilya Sherman wrote: > nit: Seems like we shouldn't be passing in a const form to this method if we're > just going to cast the constness away. So I removed const but there were issues with this, IIRC. We're only copying a pointer here, I think. Is there a better way? Just removing const didn't work very well and passing by value wasn't so hot either, I think. https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:607: return; On 2012/11/02 02:56:57, Ilya Sherman wrote: > When can this happen? I'd think it's possible that the form *could* be removed from the DOM and set = null, but maybe this doesn't cause it to be null if it's wrapped in a WebFormElement? Also what would happen if the tab's closed during this time - would this ever fire? Or what if a tab is refreshed mid-IPC? We've had issues with this in the past in WebUI code.
https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:227: WebFormElement non_const_form(form); On 2012/11/02 03:10:15, Dan Beam wrote: > On 2012/11/02 02:56:57, Ilya Sherman wrote: > > nit: Seems like we shouldn't be passing in a const form to this method if > we're > > just going to cast the constness away. > > So I removed const but there were issues with this, IIRC. We're only copying a > pointer here, I think. Is there a better way? Just removing const didn't work > very well and passing by value wasn't so hot either, I think. What are the issues with removing const? I agree that the pointer copy is safe, but it seems rather weird to have "const WebFormElement" in the signature of the method if we treat the form as non-const. https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:607: return; On 2012/11/02 03:10:15, Dan Beam wrote: > On 2012/11/02 02:56:57, Ilya Sherman wrote: > > When can this happen? > > I'd think it's possible that the form *could* be removed from the DOM and set = > null, but maybe this doesn't cause it to be null if it's wrapped in a > WebFormElement? The form could be removed from the DOM, but I don't think that would cause it to become null, since we still have a reference to it from the event. > Also what would happen if the tab's closed during this time - > would this ever fire? Or what if a tab is refreshed mid-IPC? We've had issues > with this in the past in WebUI code. Can either of these actually happen, given that the dialog is modal? Even if they can, I think that either the AutofillAgent will be torn down, or the form will remain non-null, since we're holding a reference to it.
https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:607: return; On 2012/11/02 03:24:45, Ilya Sherman wrote: > On 2012/11/02 03:10:15, Dan Beam wrote: > > On 2012/11/02 02:56:57, Ilya Sherman wrote: > > > When can this happen? > > > > I'd think it's possible that the form *could* be removed from the DOM and set > = > > null, but maybe this doesn't cause it to be null if it's wrapped in a > > WebFormElement? > > The form could be removed from the DOM, but I don't think that would cause it to > become null, since we still have a reference to it from the event. True, I definitely think you're right here and rescind that original guess. > > > Also what would happen if the tab's closed during this time - > > would this ever fire? Or what if a tab is refreshed mid-IPC? We've had > issues > > with this in the past in WebUI code. > > Can either of these actually happen, given that the dialog is modal? Even if > they can, I think that either the AutofillAgent will be torn down, or the form > will remain non-null, since we're holding a reference to it. The issue in WebUI is that while IPC was in flight the WebUI handlers were complete destroyed and recreated so it looked like a valid IPC message coming at a totally weird time. It seems possible if somebody in their script does: form.requestAutocomplete(); location.reload(); (which I just did many times in various ways by slamming my CPU and form.requestAutocomplete(); and didn't do anything) I'll make a DCHECK() for now and change if people start hitting it.
https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:227: WebFormElement non_const_form(form); On 2012/11/02 03:24:45, Ilya Sherman wrote: > On 2012/11/02 03:10:15, Dan Beam wrote: > > On 2012/11/02 02:56:57, Ilya Sherman wrote: > > > nit: Seems like we shouldn't be passing in a const form to this method if > > we're > > > just going to cast the constness away. > > > > So I removed const but there were issues with this, IIRC. We're only copying > a > > pointer here, I think. Is there a better way? Just removing const didn't > work > > very well and passing by value wasn't so hot either, I think. > > What are the issues with removing const? I agree that the pointer copy is safe, > but it seems rather weird to have "const WebFormElement" in the signature of the > method if we treat the form as non-const. So it looks like this is already being done every time you see: element_ = element; in this file (3 times) as well as someone pointed out to me that even WebNode::appendChild() is passed a const WebNode& (which seems that it'll *always* be changed). I know this is every-so-slightly misleading, but can you find any counter-examples that'd actually make it worth going against this seemingly pervasive convention?
https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11270018/diff/40003/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:227: WebFormElement non_const_form(form); On 2012/11/02 22:37:55, Dan Beam wrote: > On 2012/11/02 03:24:45, Ilya Sherman wrote: > > On 2012/11/02 03:10:15, Dan Beam wrote: > > > On 2012/11/02 02:56:57, Ilya Sherman wrote: > > > > nit: Seems like we shouldn't be passing in a const form to this method if > > > we're > > > > just going to cast the constness away. > > > > > > So I removed const but there were issues with this, IIRC. We're only > copying > > a > > > pointer here, I think. Is there a better way? Just removing const didn't > > work > > > very well and passing by value wasn't so hot either, I think. > > > > What are the issues with removing const? I agree that the pointer copy is > safe, > > but it seems rather weird to have "const WebFormElement" in the signature of > the > > method if we treat the form as non-const. > > So it looks like this is already being done every time you see: > > element_ = element; > > in this file (3 times) as well as someone pointed out to me that even > WebNode::appendChild() is passed a const WebNode& (which seems that it'll > *always* be changed). > > I know this is every-so-slightly misleading, but can you find any > counter-examples that'd actually make it worth going against this seemingly > pervasive convention? No counter-examples that I'm aware of, no.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/11270018/47002
Change committed as 166019 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
