Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8490017:
Setting up external delegate calls to allow a future delegate to handle autofill (Closed)
|
Created:
9 years, 1 month ago by csharp Modified:
9 years, 1 month ago CC:
chromium-reviews, GeorgeY, brettw-cc_chromium.org, darin-cc_chromium.org, dyu1, Paweł Hajdan Jr., Ilya Sherman, dhollowa Visibility:
Public. |
DescriptionSetting up external delegate calls to allow a future delegate to handle autofill
The external delegate IPC calls in autofill_agent are setup so that a future
change that adds an external delegate should already have its code called at the
correct times and with the correct values.
BUG=51644
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110506
Patch Set 1 #
Total comments: 32
Patch Set 2 : Adding code review changes #
Total comments: 16
Patch Set 3 : Removing unneeded IPC calls #
Total comments: 14
Patch Set 4 : Changing to get window bounds from WebKit #
Total comments: 1
Patch Set 5 : Hard coding input element position #Messages
Total messages: 15 (0 generated)
This change list just handles modifying the files that we currently have to call external delegates in the right places and with the right values. I'm not sure if all of the changes in the delegate make sense, but they seem to be the best choice for desktop chrome (I'm not sure if a desktop delegate class should be made to keep the delegate "pure"). I do have a few questions that I wasn't able to figure out, so hopefully I can get some answers and change the corresponding code if required. -I have an IPC call to tell autofill_agent when an external delegate exists, but I couldn't find a better home for where to send the message other than in AutocompleteManagerHistory::SendSuggestions -All the IPC calls from autofill_agent to the delegate go through the autofill_manager, should the delegate have it's own IPC handler? -Are there any tests I can add? Most of my changes affect autofill_agent.cc, but it doesn't seem to have a unit test file Thanks
Also as a quick note, this change can't be submitted until I get my API change for exposing a function in webkit submitted, https://bugs.webkit.org/show_bug.cgi?id=71579 On 2011/11/07 20:25:09, csharp wrote: > This change list just handles modifying the files that we currently have to call > external delegates in the right places and with the right values. > > I'm not sure if all of the changes in the delegate make sense, but they seem to > be the best choice for desktop chrome (I'm not sure if a desktop delegate class > should be made to keep the delegate "pure"). > > I do have a few questions that I wasn't able to figure out, so hopefully I can > get some answers and change the corresponding code if required. > -I have an IPC call to tell autofill_agent when an external delegate exists, but > I couldn't find a better home for where to send the message other than in > AutocompleteManagerHistory::SendSuggestions > -All the IPC calls from autofill_agent to the delegate go through the > autofill_manager, should the delegate have it's own IPC handler? > -Are there any tests I can add? Most of my changes affect autofill_agent.cc, but > it doesn't seem to have a unit test file > > Thanks
On 2011/11/07 20:25:09, csharp wrote: > -I have an IPC call to tell autofill_agent when an external delegate exists, > but I couldn't find a better home for where to send the message other than in > AutocompleteManagerHistory::SendSuggestions Offered a suggestion in the inline comments. > -All the IPC calls from autofill_agent to the delegate go through the > autofill_manager, should the delegate have it's own IPC handler? For now, there aren't very many IPC calls targeted at the delegate, so I think it's fine to have AutofillManager be the one class responsible for listening for these messages. If we end up with more delegate-oriented IPCs, we can think about handling those in a separate place. > -Are there any tests I can add? Most of my changes affect autofill_agent.cc, > but it doesn't seem to have a unit test file Yeah, the lack of test coverage for AutofillAgent is my bad: [ http://crbug.com/63573 ]. I tried to write test for this class at one point, but basically got stuck on trying to write something more fine-grained than the interactive ui tests in chrome/browser/autofill/autofill_browsertest.cc. If you want to try to add some tests for AutofillAgent, feel free to snag that bug from me. Otherwise, I don't think the external delegate changes required in AutofillAgent should end up being big enough to where I'd be too worried about a lack of test coverage for them -- AutofillAgent primarily wires up a bunch of classes that are already independently tested. http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... File chrome/browser/autocomplete_history_manager.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... chrome/browser/autocomplete_history_manager.cc:268: Send(new AutofillMsg_HasExternalDelegate(routing_id(), true)); How about instead calling this within AutofillManager::SetExternalDelegate()? http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... File chrome/browser/autocomplete_history_manager_unittest.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... chrome/browser/autocomplete_history_manager_unittest.cc:162: virtual void HideAutofillPopup() {} nit: Please annotate these with OVERRIDE. http://codereview.chromium.org/8490017/diff/1/chrome/browser/autofill/autofil... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autofill/autofil... chrome/browser/autofill/autofill_manager.h:164: void HideAutofillPopup(); nit: These should be prefixed with "On", e.g. "OnHideAutofillPopup" http://codereview.chromium.org/8490017/diff/1/chrome/browser/autofill/autofil... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autofill/autofil... chrome/browser/autofill/autofill_manager_unittest.cc:2881: virtual void HideAutofillPopup() {} nit: Please annotate these with OVERRIDE http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages.h File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:96: // that will display the auotfill popup. nit: auotfill -> Autofill http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:98: bool /* has_external_delegate */) nit: _SetHasExternalDelegate http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:172: std::vector<int> /* unique_ids */) This should probably be a method call within the browser process, called from AutofillManager::OnQueryFormFieldAutofill(). http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:179: int /* height */) Can these coordinates simply be passed as an additional gfx::Rect parameter to the QueryFormFieldAutofill message, or do the two messages need to be sent at distinct times? http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:182: IPC_MESSAGE_ROUTED0(AutofillDelegateMsg_HideAutofillPopup) nit: This should be named AutofillHostMsg_*. The general pattern we try to follow in the Chrome code is that there is a Foo object in the renderer and a FooHost object in the browser that communicate with one another. To keep things simple, we generally avoid exposing further implementation details in the message name's prefix. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:263: Send(new AutofillDelegateMsg_HideAutofillPopup(routing_id())); This call shouldn't be necessary -- all the relevant information is already available to the browser process. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:342: routing_id(), query_id, v, l, i, ids)); We'll need to rework how warnings are handled; but otherwise, all of the relevant information is already available on the browser-side. Rather than routing through the renderer, let's keep all this logic browser-side. If you'd like to ignore getting warnings working just right for now, feel free to file a bug to tackle this later. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... File chrome/renderer/autofill/autofill_agent.h (right): http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.h:108: void HasExternalDelegate(bool has_external_delegate); nit: OnSetHasExternalDelegate http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.h:181: // popup. This deciedes if we communicate with the delegate or webkit. nit: "popup. " -> "popup? " (period becomes question mark, one space becomes two spaces); "deciedes if" -> "determines whether"; "webkit" -> "WebKit".
http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... File chrome/browser/autocomplete_history_manager.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... chrome/browser/autocomplete_history_manager.cc:268: Send(new AutofillMsg_HasExternalDelegate(routing_id(), true)); That doesn't seem to work. Although the Send function does return true when calling from SetExternalDelegate, Autofill_Agent does not seem to get the message. I think this might happen because when we assign the delegate, autofill_agent isn't yet initialized. On 2011/11/07 21:48:49, Ilya Sherman wrote: > How about instead calling this within AutofillManager::SetExternalDelegate()? http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... File chrome/browser/autocomplete_history_manager_unittest.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... chrome/browser/autocomplete_history_manager_unittest.cc:162: virtual void HideAutofillPopup() {} On 2011/11/07 21:48:49, Ilya Sherman wrote: > nit: Please annotate these with OVERRIDE. Done. http://codereview.chromium.org/8490017/diff/1/chrome/browser/autofill/autofil... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autofill/autofil... chrome/browser/autofill/autofill_manager.h:164: void HideAutofillPopup(); On 2011/11/07 21:48:49, Ilya Sherman wrote: > nit: These should be prefixed with "On", e.g. "OnHideAutofillPopup" Done. http://codereview.chromium.org/8490017/diff/1/chrome/browser/autofill/autofil... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autofill/autofil... chrome/browser/autofill/autofill_manager_unittest.cc:2881: virtual void HideAutofillPopup() {} On 2011/11/07 21:48:49, Ilya Sherman wrote: > nit: Please annotate these with OVERRIDE Done. http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages.h File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:96: // that will display the auotfill popup. On 2011/11/07 21:48:49, Ilya Sherman wrote: > nit: auotfill -> Autofill Done. http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:98: bool /* has_external_delegate */) On 2011/11/07 21:48:49, Ilya Sherman wrote: > nit: _SetHasExternalDelegate Done. http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:172: std::vector<int> /* unique_ids */) Ok, with some of autofill_agents logic repeated in the browser we can even avoid adding a new call to ShowAutofillSuggestions, the call is already made by AutocompleteHistoryManager (which is called at the end of OnQueryFormFieldAutofill()) On 2011/11/07 21:48:49, Ilya Sherman wrote: > This should probably be a method call within the browser process, called from > AutofillManager::OnQueryFormFieldAutofill(). http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:179: int /* height */) QueryFormFieldAutofill looks like it would be a prefect place to pass this information. However I don't think we can pass a gfx::Rect with the IPC calls since its data members are private, and as far as I can tell we can only pass structs/classes that have the members we want to copy as public. WebKit::WebRect could be used, since it's members are public, but I assume we don't want to be using WebKit classes too much internally. Should we create a new rect class to pass or am I overlooking some way of being able to pass gfx::Rect. On 2011/11/07 21:48:49, Ilya Sherman wrote: > Can these coordinates simply be passed as an additional gfx::Rect parameter to > the QueryFormFieldAutofill message, or do the two messages need to be sent at > distinct times? http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:182: IPC_MESSAGE_ROUTED0(AutofillDelegateMsg_HideAutofillPopup) On 2011/11/07 21:48:49, Ilya Sherman wrote: > nit: This should be named AutofillHostMsg_*. The general pattern we try to > follow in the Chrome code is that there is a Foo object in the renderer and a > FooHost object in the browser that communicate with one another. To keep things > simple, we generally avoid exposing further implementation details in the > message name's prefix. Done. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:263: Send(new AutofillDelegateMsg_HideAutofillPopup(routing_id())); On 2011/11/07 21:48:49, Ilya Sherman wrote: > This call shouldn't be necessary -- all the relevant information is already > available to the browser process. Done. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:342: routing_id(), query_id, v, l, i, ids)); So if I understand correctly, you are suggesting that we have a copy of this logic browser-side to prevent the call? If so, would this make sense to place the external_delegate base class? On 2011/11/07 21:48:49, Ilya Sherman wrote: > We'll need to rework how warnings are handled; but otherwise, all of the > relevant information is already available on the browser-side. Rather than > routing through the renderer, let's keep all this logic browser-side. If you'd > like to ignore getting warnings working just right for now, feel free to file a > bug to tackle this later. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... File chrome/renderer/autofill/autofill_agent.h (right): http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.h:108: void HasExternalDelegate(bool has_external_delegate); On 2011/11/07 21:48:49, Ilya Sherman wrote: > nit: OnSetHasExternalDelegate Done. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.h:181: // popup. This deciedes if we communicate with the delegate or webkit. On 2011/11/07 21:48:49, Ilya Sherman wrote: > nit: "popup. " -> "popup? " (period becomes question mark, one space becomes > two spaces); "deciedes if" -> "determines whether"; "webkit" -> "WebKit". Done.
I like the progress seen here. http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autocomplete_... File chrome/browser/autocomplete_history_manager.cc (right): http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autocomplete_... chrome/browser/autocomplete_history_manager.cc:224: void SetExternalDelegate(AutofillExternalDelegate* delegate); This doesn't look like valid code. http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autocomplete_... chrome/browser/autocomplete_history_manager.cc:273: Send(new AutofillMsg_SetHasExternalDelegate(routing_id(), true)); You probably want to make this call at SetExternalDelegate() time. http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_external_delegate.h:52: // Set the bounds of the element we are providing the Autofill data Be more specific. E.g. state clearly this is the bounds of the text field to be autocompleted. Clearly state if the coordinates are page-relative or screen-relative. http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_external_delegate.h:57: virtual void ShowAutofillPopup() = 0; HideAutofillPopup() seems fine since it should be safe to call it no matter what (e.g. when text field loses focus). But ShowAutofillPopup() might be poorly named. If all autofill logic is moved to the browser, webkit wouldn't even know if the autofill popup is open, or can be opened because we have suggestions. http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_manager.h:163: void OnSetAutofillElementBounds(int x, int y, int width, int height); Label coordinates as screen or document relative. http://codereview.chromium.org/8490017/diff/6001/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/8490017/diff/6001/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:131: if (has_external_delegate_) Can you confirm this is called when, for example, a page reload happens? What happens to the autofill window if the element maintains focus but scrolls off the page?
http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... File chrome/browser/autocomplete_history_manager.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... chrome/browser/autocomplete_history_manager.cc:268: Send(new AutofillMsg_HasExternalDelegate(routing_id(), true)); Ah, that's irksome. Now that I think about this a bit more, though... do we even need to tell the AutofillAgent that there's an external delegate? When there is an external delegate, the browser process can avoid ever calling into AutofillAgent::OnSuggestionsReturned(). Within AutofillAgent::InputElementLostFocus(), the renderer can just always send the HideAutofillPopup message -- the AutofillManager can disregard any such message if it doesn't have an external delegate. On 2011/11/08 19:59:00, csharp1 wrote: > That doesn't seem to work. Although the Send function does return true when > calling from SetExternalDelegate, Autofill_Agent does not seem to get the > message. I think this might happen because when we assign the delegate, > autofill_agent isn't yet initialized. > > On 2011/11/07 21:48:49, Ilya Sherman wrote: > > How about instead calling this within AutofillManager::SetExternalDelegate()? > http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages.h File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:179: int /* height */) On 2011/11/08 19:59:00, csharp1 wrote: > QueryFormFieldAutofill looks like it would be a prefect place to pass this > information. However I don't think we can pass a gfx::Rect with the IPC calls > since its data members are private, and as far as I can tell we can only pass > structs/classes that have the members we want to copy as public. You can pass a gfx::Rect by adding serialization/deserialization code for the IPC macros to use. For some examples, take a look at how src/chrome/common/automation_messages does this for ContextMenuModel's, and how src/ipc/ipc_message_utils does this for strings, vectors, etc. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:342: routing_id(), query_id, v, l, i, ids)); On 2011/11/08 19:59:00, csharp1 wrote: > So if I understand correctly, you are suggesting that we have a copy of this > logic browser-side to prevent the call? Right. Once the transition to the browser-side UI is complete, we can remove the copy of this code that lives in autofill_agent. > If so, would this make sense to place the external_delegate base class? Yeah, I think that's a reasonable place for this code to live. > On 2011/11/07 21:48:49, Ilya Sherman wrote: > > We'll need to rework how warnings are handled; but otherwise, all of the > > relevant information is already available on the browser-side. Rather than > > routing through the renderer, let's keep all this logic browser-side. If > you'd > > like to ignore getting warnings working just right for now, feel free to file > a > > bug to tackle this later. > http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autocomplete_... File chrome/browser/autocomplete_history_manager.cc (right): http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autocomplete_... chrome/browser/autocomplete_history_manager.cc:224: void SetExternalDelegate(AutofillExternalDelegate* delegate); I don't think this is quite the change you wanted to make ;) http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_manager.h:162: const std::vector<int>& autofill_unique_ids); nit: I think this method is no longer used -- right?
http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... File chrome/browser/autocomplete_history_manager.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/browser/autocomplete_his... chrome/browser/autocomplete_history_manager.cc:268: Send(new AutofillMsg_HasExternalDelegate(routing_id(), true)); Seems like a good solution, changes made. On 2011/11/08 21:06:16, Ilya Sherman wrote: > Ah, that's irksome. Now that I think about this a bit more, though... do we > even need to tell the AutofillAgent that there's an external delegate? When > there is an external delegate, the browser process can avoid ever calling into > AutofillAgent::OnSuggestionsReturned(). Within > AutofillAgent::InputElementLostFocus(), the renderer can just always send the > HideAutofillPopup message -- the AutofillManager can disregard any such message > if it doesn't have an external delegate. > > On 2011/11/08 19:59:00, csharp1 wrote: > > That doesn't seem to work. Although the Send function does return true when > > calling from SetExternalDelegate, Autofill_Agent does not seem to get the > > message. I think this might happen because when we assign the delegate, > > autofill_agent isn't yet initialized. > > > > On 2011/11/07 21:48:49, Ilya Sherman wrote: > > > How about instead calling this within > AutofillManager::SetExternalDelegate()? > > > http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages.h File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/8490017/diff/1/chrome/common/autofill_messages... chrome/common/autofill_messages.h:179: int /* height */) As I started looking there I saw a case where gfx::Rect was just used with no problem, so it turns out it just works :) On 2011/11/08 21:06:16, Ilya Sherman wrote: > On 2011/11/08 19:59:00, csharp1 wrote: > > QueryFormFieldAutofill looks like it would be a prefect place to pass this > > information. However I don't think we can pass a gfx::Rect with the IPC calls > > since its data members are private, and as far as I can tell we can only pass > > structs/classes that have the members we want to copy as public. > > You can pass a gfx::Rect by adding serialization/deserialization code for the > IPC macros to use. For some examples, take a look at how > src/chrome/common/automation_messages does this for ContextMenuModel's, and how > src/ipc/ipc_message_utils does this for strings, vectors, etc. http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/8490017/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:342: routing_id(), query_id, v, l, i, ids)); Ok, I've added a todo to copy the logic over. I quickly looked at it and it looks like it might be enough work to deserve its own cl since it has several member variables and functions that I'll need to figure out how to correctly move over. On 2011/11/08 21:06:16, Ilya Sherman wrote: > On 2011/11/08 19:59:00, csharp1 wrote: > > So if I understand correctly, you are suggesting that we have a copy of this > > logic browser-side to prevent the call? > > Right. Once the transition to the browser-side UI is complete, we can remove > the copy of this code that lives in autofill_agent. > > > If so, would this make sense to place the external_delegate base class? > > Yeah, I think that's a reasonable place for this code to live. > > > On 2011/11/07 21:48:49, Ilya Sherman wrote: > > > We'll need to rework how warnings are handled; but otherwise, all of the > > > relevant information is already available on the browser-side. Rather than > > > routing through the renderer, let's keep all this logic browser-side. If > > you'd > > > like to ignore getting warnings working just right for now, feel free to > file > > a > > > bug to tackle this later. > > > http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autocomplete_... File chrome/browser/autocomplete_history_manager.cc (right): http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autocomplete_... chrome/browser/autocomplete_history_manager.cc:224: void SetExternalDelegate(AutofillExternalDelegate* delegate); Fixed. Surprisingly this code did compile without trouble. On 2011/11/08 21:06:16, Ilya Sherman wrote: > I don't think this is quite the change you wanted to make ;) http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autocomplete_... chrome/browser/autocomplete_history_manager.cc:273: Send(new AutofillMsg_SetHasExternalDelegate(routing_id(), true)); We no longer make SetExternalDelegate calls, so problem removed. On 2011/11/08 20:46:02, John Grabowski wrote: > You probably want to make this call at SetExternalDelegate() time. http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_external_delegate.h:52: // Set the bounds of the element we are providing the Autofill data On 2011/11/08 20:46:02, John Grabowski wrote: > Be more specific. E.g. state clearly this is the bounds of the text field to be > autocompleted. Clearly state if the coordinates are page-relative or > screen-relative. Done. http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_external_delegate.h:57: virtual void ShowAutofillPopup() = 0; True, I had removed the Webkit call to it already but had forgotten to take it out to. OnSuggestionReturned should be the only signal we need to know when to show the popup. On 2011/11/08 20:46:02, John Grabowski wrote: > HideAutofillPopup() seems fine since it should be safe to call it no matter what > > (e.g. when text field loses focus). But ShowAutofillPopup() might be poorly > named. If all autofill logic is moved to the browser, webkit wouldn't even know > if the autofill popup is open, or can be opened because we have suggestions. http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_manager.h:162: const std::vector<int>& autofill_unique_ids); Correct and removed. On 2011/11/08 21:06:16, Ilya Sherman wrote: > nit: I think this method is no longer used -- right? http://codereview.chromium.org/8490017/diff/6001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_manager.h:163: void OnSetAutofillElementBounds(int x, int y, int width, int height); No longer used, but I added comment to new function that gets gfx::Rect to say that it is document-relative. On 2011/11/08 20:46:02, John Grabowski wrote: > Label coordinates as screen or document relative. http://codereview.chromium.org/8490017/diff/6001/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/8490017/diff/6001/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:131: if (has_external_delegate_) This function is not called in both of these cases, good catch. I've added a bug to myself for the page refresh issue, since the input field does lose focus. In the scrolling case the input field doesn't lost focus, so this shouldn't get called, but the popup should hide so I've add another bug to myself to fix that issue, but I'm not sure right now where the correct place to fix that would be. On 2011/11/08 20:46:02, John Grabowski wrote: > Can you confirm this is called when, for example, a page reload happens? > What happens to the autofill window if the element maintains focus but scrolls > off the page?
http://codereview.chromium.org/8490017/diff/6001/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/8490017/diff/6001/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:131: if (has_external_delegate_) On 2011/11/09 16:18:37, csharp wrote: > In the scrolling case the input field doesn't lost focus, so > this shouldn't get called, but the popup should hide so I've add another bug to > myself to fix that issue, but I'm not sure right now where the correct place to > fix that would be. My hunch is that we'll want to add something along the lines of OnFrameDidScroll to the RenderView::Observer class to handle scrolling. http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:46: // AutofillAgent::OnSuggestionsReturned. nit: Please add a link to http://crbug.com/51644 here (or file a new, more focused bug for this, if you prefer). http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:55: // for. The bounds are page-relative. nit: Two spaces after a sentence-final period, please (in Autofill code, at least). http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:453: external_delegate_->SetAutofillElementBounds(bounding_box); nit: Are you sure you want these to be two separate methods? Would they ever be called separately? http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:26: #include "ui/gfx/rect.h" nit: Let's forward-declare gfx::Rect here and move the #include to the implementation file. http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:148: // The bounding_box is a page_relative value. nit: "bounding_box" -> "|bounding_box|"
LGTM from me, but please wait for LGTM from Ilya. http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:148: // The bounding_box is a page_relative value. page_relative --> page relative (is english) http://codereview.chromium.org/8490017/diff/12001/chrome/common/autofill_mess... File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/8490017/diff/12001/chrome/common/autofill_mess... chrome/common/autofill_messages.h:130: gfx::Rect /* input field bounds, page-relative */) Nit: I think "document relative" is more common than "page relative"
I found a better WebKit function to use, which gets the window relative position so it is now used. I updated the comments to show the values are now window relative. http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:46: // AutofillAgent::OnSuggestionsReturned. On 2011/11/09 20:34:19, Ilya Sherman wrote: > nit: Please add a link to http://crbug.com/51644 here (or file a new, more > focused bug for this, if you prefer). Done. http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:55: // for. The bounds are page-relative. Done. What is the reason for the two spaces? I don't remember seeing it in the style guide. On 2011/11/09 20:34:19, Ilya Sherman wrote: > nit: Two spaces after a sentence-final period, please (in Autofill code, at > least). http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:453: external_delegate_->SetAutofillElementBounds(bounding_box); Hmmm, I don't think we would need to call them separately at any time, assuming that we hide the popup any time the page moves like we currently do, so I'll merge them. On 2011/11/09 20:34:19, Ilya Sherman wrote: > nit: Are you sure you want these to be two separate methods? Would they ever be > called separately? http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:26: #include "ui/gfx/rect.h" On 2011/11/09 20:34:19, Ilya Sherman wrote: > nit: Let's forward-declare gfx::Rect here and move the #include to the > implementation file. Done. http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:148: // The bounding_box is a page_relative value. On 2011/11/09 20:34:19, Ilya Sherman wrote: > nit: "bounding_box" -> "|bounding_box|" Done. http://codereview.chromium.org/8490017/diff/12001/chrome/common/autofill_mess... File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/8490017/diff/12001/chrome/common/autofill_mess... chrome/common/autofill_messages.h:130: gfx::Rect /* input field bounds, page-relative */) The WebKit function is now getting the window relative value, so the name is updated. I'm not if viewport would be a better choice, but I stuck with window since that is the WebKit function's name On 2011/11/09 22:18:34, John Grabowski wrote: > Nit: I think "document relative" is more common than "page relative"
LGTM, thanks :) http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/8490017/diff/12001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:55: // for. The bounds are page-relative. Yeah, it's not defined one way or the other in the style guide, so different parts of the Chrome codebase do this differently. It doesn't matter particularly much, but it's nice to have consistency within a file. For the Autofill code, we've generally preferred two spaces to one. (This is, admittedly, largely a carry-over from typewriters and early computer fonts. Modern computer fonts tend to be designed so that the period already has a bit of extra space after it, which arguably makes the second space unnecessary. But David and I both think that two spaces still looks better in code, which uses a monospaced font.) On 2011/11/10 18:09:32, csharp wrote: > What is the reason for the two spaces? I don't remember seeing it in the style > guide. > > On 2011/11/09 20:34:19, Ilya Sherman wrote: > > nit: Two spaces after a sentence-final period, please (in Autofill code, at > > least). >
Ok, there will be one more set of changes uploaded by me once the WebKit change has been submitted. If anything other than the function name changes I will note it when uploading, otherwise I will just commit this change.
http://codereview.chromium.org/8490017/diff/17001/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/8490017/diff/17001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:420: gfx::Rect bounding_box(autofill_query_element_.boundsInWindowSpace()); So that you're not blocked by the WebKit work... How about just passing some default, fixed value here for now; committing this CL; and creating a new CL to update this to a real value once the WebKit work lands? For example, you could compute what the correct value should be for your favorite test page, and then use that test page to develop the browser-side UI while waiting for the WebKit API change.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/8490017/25001
Change committed as 110506 |
