Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10825324:
Start of New Autofill UI for Windows (Closed)
|
Created:
8 years, 4 months ago by csharp Modified:
7 years, 11 months ago CC:
chromium-reviews, dhollowa+watch_chromium.org, browser-components-watch_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionStart of New Autofill UI for Windows
Add some very basic functionality for the new Autofill UI system for windows.
It currently just shows an empty popup in the correct location.
Future CLs will add the actual functionality.
BUG=51644
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155476
Patch Set 1 : #
Total comments: 31
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 26
Patch Set 4 : #Patch Set 5 : #
Total comments: 10
Patch Set 6 : #
Total comments: 1
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #
Total comments: 26
Patch Set 12 : Fix test failures #Patch Set 13 : rebading #Patch Set 14 : #Patch Set 15 : rebasing #
Total comments: 1
Messages
Total messages: 46 (0 generated)
Can haz tests? Otherwise lg :) https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (left): https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:348: // none, so all platforms use the default. nit: Please remove the "Currently there are none sentence. Also, maybe it's worth reversing this check to just be checking for platforms that *don't* define the UI? (So, just Mac.) https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:13: autofill_manager); nit: This looks like it fits on the previous line. https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:31: Optional nit: I would omit this blank line, but up to you. https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:41: nit: Ditto https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:63: if (view_.get()) Optional nit: This method would be slightly shorter if you reversed this condition. https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.h (right): https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:11: class AutofillPopupViewViews; Wonderful. :P https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:25: // AutofillExternalDelegate implementations. nit: "implementations" -> "implementation" https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:46: content::WebContents* web_contents_; // Weak reference. nit: The parent class should probably just have a protected accessor to its WebContents, rather than needing to store it here. https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:48: if (popup_ == NULL) { nit: if (!popup_) { https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:52: popup_ = (new AutofillPopupWidget)->AsWeakPtr(); nit: Please add a brief comment here about the lifetime of this object, since it's uncommon in Chrome code to see unprotected |new| calls. When does this popup actually get registered with the OS to be automatically freed? https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:85: popup_bounds.set_y(popup_bounds.y() + element_bounds().height()); nit: It's a little weird to use popup_bounds.y() and element_bounds().height(), since the two are equal before you update the y-coordinate. I'd just use popup_bounds for both. https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:8: #include <vector> nit: This doesn't look like it's used within the header https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:31: virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE; nit: Does this need to be public? What is it overriding? https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:34: // AutofillPopupView implementations. nit: "implementations" -> "implementation" https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:38: virtual void ResizePopup() OVERRIDE; nit: Do these need to be protected, or can they be private? https://chromiumcodereview.appspot.com/10825324/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:49: content::RenderViewHost* render_view_host_; // Weak reference. nit: Why cache this if you're already caching the WebContents?
I don't think that tests would add much here, since this is mostly just adding the implementations of methods already declared in the base class (which handles making sure they are correct called), and I don't think there is a good way with views to test my outputs. sky@ can you review the view changes, thanks. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (left): http://codereview.chromium.org/10825324/diff/2001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:348: // none, so all platforms use the default. On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: Please remove the "Currently there are none sentence. Also, maybe it's > worth reversing this check to just be checking for platforms that *don't* define > the UI? (So, just Mac.) Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:13: autofill_manager); On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: This looks like it fits on the previous line. Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:31: On 2012/08/14 05:12:09, Ilya Sherman wrote: > Optional nit: I would omit this blank line, but up to you. Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:41: On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: Ditto Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:63: if (view_.get()) On 2012/08/14 05:12:09, Ilya Sherman wrote: > Optional nit: This method would be slightly shorter if you reversed this > condition. Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.h (right): http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:25: // AutofillExternalDelegate implementations. On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: "implementations" -> "implementation" Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:46: content::WebContents* web_contents_; // Weak reference. On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: The parent class should probably just have a protected accessor to its > WebContents, rather than needing to store it here. Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:48: if (popup_ == NULL) { On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: if (!popup_) { Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:52: popup_ = (new AutofillPopupWidget)->AsWeakPtr(); On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: Please add a brief comment here about the lifetime of this object, since > it's uncommon in Chrome code to see unprotected |new| calls. When does this > popup actually get registered with the OS to be automatically freed? Comment added, let me know if it needs more details (There are also some notes in the header definition of popup_) http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:85: popup_bounds.set_y(popup_bounds.y() + element_bounds().height()); On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: It's a little weird to use popup_bounds.y() and element_bounds().height(), > since the two are equal before you update the y-coordinate. I'd just use > popup_bounds for both. Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:8: #include <vector> On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: This doesn't look like it's used within the header Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:31: virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE; On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: Does this need to be public? What is it overriding? Moved to private. Overriding the views::View function that handles drawing. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:34: // AutofillPopupView implementations. On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: "implementations" -> "implementation" Done. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:38: virtual void ResizePopup() OVERRIDE; On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: Do these need to be protected, or can they be private? I don't think so, moved. http://codereview.chromium.org/10825324/diff/2001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:49: content::RenderViewHost* render_view_host_; // Weak reference. On 2012/08/14 05:12:09, Ilya Sherman wrote: > nit: Why cache this if you're already caching the WebContents? Removed.
LGTM. Perhaps Scott can comment on the feasibility/best practice of tests for the views code. http://codereview.chromium.org/10825324/diff/5002/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10825324/diff/5002/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:347: // in an autofill_external_delegate_YOUROS.cc. nit: This comment is probably no longer needed. http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:59: if (view_.get() == NULL) nit: if (!view_.get()) http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:51: // The widget is destory by the corresponding NativeWidget, so we use nit: "destory" -> "destroyed" (though I kind of like the word "destory" :)
You can write a browser test (or interactive ui test if necessary), that generates mouse and keyboard events. This should allow you to set up any test configuration you need.
Added a basic browser test to ensure that the popup can open and close without crash (there isn't really anything else in the code to test yet). http://codereview.chromium.org/10825324/diff/5002/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10825324/diff/5002/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:347: // in an autofill_external_delegate_YOUROS.cc. On 2012/08/14 19:27:12, Ilya Sherman wrote: > nit: This comment is probably no longer needed. Done. http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:59: if (view_.get() == NULL) On 2012/08/14 19:27:12, Ilya Sherman wrote: > nit: if (!view_.get()) Done. http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:51: // The widget is destory by the corresponding NativeWidget, so we use On 2012/08/14 19:27:12, Ilya Sherman wrote: > nit: "destory" -> "destroyed" (though I kind of like the word "destory" :) Done.
http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:28: view_->Hide(); Why do you need hide here? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.h (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:17: class AutofillExternalDelegateViews : public AutofillExternalDelegate { Add a dscription. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:43: scoped_ptr<AutofillPopupViewViews> view_; This is fragile. AutofillPopupViewViews is deleted by either the view hierarchy (because its a child of the popup) or here. I think this code would be less fragile if AutofillPopupViewViews was not the actual AutofillPopupView. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc:20: tab_contents_ = chrome::GetActiveTabContents(browser()); Any reason this is here and not in the test? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:35: SetVisible(false); Why do you explicitly hide here? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:48: BrowserView* browser_view = BrowserView::GetBrowserViewForNativeWindow( Why does this need to be browserview? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:49: web_contents_->GetView()->GetTopLevelNativeWindow()); spacing is off. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: popup_->SetBounds(client_area); Where do you close this? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:72: SetVisible(true); I don't understand why you need the explicit visible calls here.
Should the browser test be in a seperate file for consistency? Even though there is only one test now, more may be added later when there are more additions to the code to test. On 2012/08/15 19:39:58, csharp wrote: > Added a basic browser test to ensure that the popup can open and close without > crash (there isn't really anything else in the code to test yet). > > http://codereview.chromium.org/10825324/diff/5002/chrome/browser/autofill/aut... > File chrome/browser/autofill/autofill_external_delegate.cc (right): > > http://codereview.chromium.org/10825324/diff/5002/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_external_delegate.cc:347: // in an > autofill_external_delegate_YOUROS.cc. > On 2012/08/14 19:27:12, Ilya Sherman wrote: > > nit: This comment is probably no longer needed. > > Done. > > http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... > File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc > (right): > > http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:59: if > (view_.get() == NULL) > On 2012/08/14 19:27:12, Ilya Sherman wrote: > > nit: if (!view_.get()) > > Done. > > http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > http://codereview.chromium.org/10825324/diff/5002/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:51: // The widget > is destory by the corresponding NativeWidget, so we use > On 2012/08/14 19:27:12, Ilya Sherman wrote: > > nit: "destory" -> "destroyed" (though I kind of like the word "destory" :) > > Done.
On 2012/08/15 22:03:49, dyu1 wrote: > Should the browser test be in a seperate file for consistency? Even though there > is only one test now, more may be added later when there are more additions to > the code to test. Separate from what?
Sorry for all the views questions, I haven't done any work with views before so I'm not really sure how it's suppose to work :) http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:28: view_->Hide(); On 2012/08/15 21:07:29, sky wrote: > Why do you need hide here? Without this hide the popup doesn't disappear (since we are only holding the weak pointer to it the reset doesn't delete the object) http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.h (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:17: class AutofillExternalDelegateViews : public AutofillExternalDelegate { On 2012/08/15 21:07:29, sky wrote: > Add a dscription. Done. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:43: scoped_ptr<AutofillPopupViewViews> view_; On 2012/08/15 21:07:29, sky wrote: > This is fragile. AutofillPopupViewViews is deleted by either the view hierarchy > (because its a child of the popup) or here. I think this code would be less > fragile if AutofillPopupViewViews was not the actual AutofillPopupView. I'm not sure I understand how to fix that. If AutofillPopupViewViews isn't the actual popup, then who is (And wouldn't this new popup have the same issue). http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc:20: tab_contents_ = chrome::GetActiveTabContents(browser()); On 2012/08/15 21:07:29, sky wrote: > Any reason this is here and not in the test? Just to allow future tests to have these values already setup for them. (I can move it into the test if you don't want this shared function until there are multiple tests). http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:35: SetVisible(false); On 2012/08/15 21:07:29, sky wrote: > Why do you explicitly hide here? Removed. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:48: BrowserView* browser_view = BrowserView::GetBrowserViewForNativeWindow( On 2012/08/15 21:07:29, sky wrote: > Why does this need to be browserview? I'm not sure that it should be. The browser view just seemed to be the correct place to get the parent widget for the autofill popup. Is there a better parent window or widget to use? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:49: web_contents_->GetView()->GetTopLevelNativeWindow()); On 2012/08/15 21:07:29, sky wrote: > spacing is off. Done. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: popup_->SetBounds(client_area); On 2012/08/15 21:07:29, sky wrote: > Where do you close this? I'm not sure what you mean. ResizePopup set the bounds of the autofill popup to the actual size we want (if that's what you mean). http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:72: SetVisible(true); On 2012/08/15 21:07:29, sky wrote: > I don't understand why you need the explicit visible calls here. Ah, I thought I needed it, removed.
http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:28: view_->Hide(); On 2012/08/16 19:16:27, csharp wrote: > On 2012/08/15 21:07:29, sky wrote: > > Why do you need hide here? > > Without this hide the popup doesn't disappear (since we are only holding the > weak pointer to it the reset doesn't delete the object) You're saying the Hide deletes? Doesn't it eventually end up in AutofillPopupViewViews::HideInternal() which does SetVisible(false)? AutofillPopupViewViews is a View, so SetVisible(false) on the view just hides the view not the widget. Maybe I'm missing something? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.h (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:43: scoped_ptr<AutofillPopupViewViews> view_; On 2012/08/16 19:16:27, csharp wrote: > On 2012/08/15 21:07:29, sky wrote: > > This is fragile. AutofillPopupViewViews is deleted by either the view > hierarchy > > (because its a child of the popup) or here. I think this code would be less > > fragile if AutofillPopupViewViews was not the actual AutofillPopupView. > > I'm not sure I understand how to fix that. If AutofillPopupViewViews isn't the > actual popup, then who is (And wouldn't this new popup have the same issue). I'm not sure what you mean by popup here (too many popups). IF you need to deal with two places possibly deleting the object then it seems like you need a way for the popup to tell you its deleted so that this can release the object. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc:20: tab_contents_ = chrome::GetActiveTabContents(browser()); On 2012/08/16 19:16:27, csharp wrote: > On 2012/08/15 21:07:29, sky wrote: > > Any reason this is here and not in the test? > > Just to allow future tests to have these values already setup for them. (I can > move it into the test if you don't want this shared function until there are > multiple tests). I would move it to the test for now. You can promote later. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:48: BrowserView* browser_view = BrowserView::GetBrowserViewForNativeWindow( On 2012/08/16 19:16:27, csharp wrote: > On 2012/08/15 21:07:29, sky wrote: > > Why does this need to be browserview? > > I'm not sure that it should be. The browser view just seemed to be the correct > place to get the parent widget for the autofill popup. Is there a better parent > window or widget to use? params.parent = web_contents_->GetView()->GetTopLevelNativeWindow() Should work equally well and not have a dependency on BrowserView.
http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:28: view_->Hide(); On 2012/08/16 20:00:15, sky wrote: > On 2012/08/16 19:16:27, csharp wrote: > > On 2012/08/15 21:07:29, sky wrote: > > > Why do you need hide here? > > > > Without this hide the popup doesn't disappear (since we are only holding the > > weak pointer to it the reset doesn't delete the object) > > You're saying the Hide deletes? Doesn't it eventually end up in > AutofillPopupViewViews::HideInternal() which does SetVisible(false)? > AutofillPopupViewViews is a View, so SetVisible(false) on the view just hides > the view not the widget. Maybe I'm missing something? I think you've got it right. This will lead to the view getting hidden while the widget remains visible. I've update AutofillPopupViewViews::HideInternal() to also hide the widget. Is this right? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.h (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:43: scoped_ptr<AutofillPopupViewViews> view_; On 2012/08/16 20:00:15, sky wrote: > On 2012/08/16 19:16:27, csharp wrote: > > On 2012/08/15 21:07:29, sky wrote: > > > This is fragile. AutofillPopupViewViews is deleted by either the view > > hierarchy > > > (because its a child of the popup) or here. I think this code would be less > > > fragile if AutofillPopupViewViews was not the actual AutofillPopupView. > > > > I'm not sure I understand how to fix that. If AutofillPopupViewViews isn't the > > actual popup, then who is (And wouldn't this new popup have the same issue). > > I'm not sure what you mean by popup here (too many popups). IF you need to deal > with two places possibly deleting the object then it seems like you need a way > for the popup to tell you its deleted so that this can release the object. Ok, I've added code for the popup to let the delegate know if it has been deleted. I'm just holding it as a raw pointer right now since none of the other types seem like they would work in this situation. Is there a class I should be using to hold that pointer? http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc:20: tab_contents_ = chrome::GetActiveTabContents(browser()); On 2012/08/16 20:00:15, sky wrote: > On 2012/08/16 19:16:27, csharp wrote: > > On 2012/08/15 21:07:29, sky wrote: > > > Any reason this is here and not in the test? > > > > Just to allow future tests to have these values already setup for them. (I can > > move it into the test if you don't want this shared function until there are > > multiple tests). > > I would move it to the test for now. You can promote later. Done. http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/4003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:48: BrowserView* browser_view = BrowserView::GetBrowserViewForNativeWindow( On 2012/08/16 20:00:15, sky wrote: > On 2012/08/16 19:16:27, csharp wrote: > > On 2012/08/15 21:07:29, sky wrote: > > > Why does this need to be browserview? > > > > I'm not sure that it should be. The browser view just seemed to be the correct > > place to get the parent widget for the autofill popup. Is there a better > parent > > window or widget to use? > > params.parent = web_contents_->GetView()->GetTopLevelNativeWindow() > > Should work equally well and not have a dependency on BrowserView. Done.
http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:34: view_->Hide(); I know I asked this before, but how come delete view_ isn't enough? Can't it be? http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc (right): http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc:14: class AutofillExternalDelegateViewsBrowserTest : public InProcessBrowserTest { use a typedef so that you don't need a class. http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:19: class AutofillPopupViewViews::AutofillPopupWidget Why do you need this class? When the popup is deleted it'll delete the view which will notify the delegate and clean up. That said, if this happens HideInternal is never invoked and so no cleanup is done. It would be good to add a test case for this. Specifically in the test get a handle to the widget and invoke close on it and make sure everything is cleaned up. http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:74: popup_->Hide(); You never close the popup_ and therefor its leaking.
http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:34: view_->Hide(); On 2012/08/16 23:02:01, sky wrote: > I know I asked this before, but how come delete view_ isn't enough? Can't it be? Hide calls a virtual function in view_, so I can't merge the call inside the deletion, but I still need to ensure it is called to ensure that the base class hiding code gets executed. http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc (right): http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc:14: class AutofillExternalDelegateViewsBrowserTest : public InProcessBrowserTest { On 2012/08/16 23:02:01, sky wrote: > use a typedef so that you don't need a class. Done. http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:19: class AutofillPopupViewViews::AutofillPopupWidget On 2012/08/16 23:02:01, sky wrote: > Why do you need this class? > > When the popup is deleted it'll delete the view which will notify the delegate > and clean up. > > That said, if this happens HideInternal is never invoked and so no cleanup is > done. > > It would be good to add a test case for this. Specifically in the test get a > handle to the widget and invoke close on it and make sure everything is cleaned > up. I wasn't able to get the popup to be visible without this class and beng@ said I need to add a widget for it to live in (similar to the omnibox code). I've added a test to make sure that hide code is called if the popup is close by the widget getting closed. http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:74: popup_->Hide(); On 2012/08/16 23:02:01, sky wrote: > You never close the popup_ and therefor its leaking. Added close to popup in the deconstructor http://codereview.chromium.org/10825324/diff/6012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc (right): http://codereview.chromium.org/10825324/diff/6012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views_browsertest.cc:102: views::View* popup_view = I tried to just close the parent widget, but that wasn't calling the view's deconstructor, this seemed to be the only way to delete the view from outside of the external delegate.
http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:34: view_->Hide(); On 2012/08/20 20:59:00, csharp wrote: > On 2012/08/16 23:02:01, sky wrote: > > I know I asked this before, but how come delete view_ isn't enough? Can't it > be? > > Hide calls a virtual function in view_, so I can't merge the call inside the > deletion, but I still need to ensure it is called to ensure that the base class > hiding code gets executed. Can't the destructor in the base class take care of whatever cleanup needs to be done?
http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc (right): http://codereview.chromium.org/10825324/diff/3012/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_external_delegate_views.cc:34: view_->Hide(); On 2012/08/21 15:29:41, sky wrote: > On 2012/08/20 20:59:00, csharp wrote: > > On 2012/08/16 23:02:01, sky wrote: > > > I know I asked this before, but how come delete view_ isn't enough? Can't it > > be? > > > > Hide calls a virtual function in view_, so I can't merge the call inside the > > deletion, but I still need to ensure it is called to ensure that the base > class > > hiding code gets executed. > > Can't the destructor in the base class take care of whatever cleanup needs to be > done? Done.
http://codereview.chromium.org/10825324/diff/18015/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/18015/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:39: if (popup_.get()) Did you verify this all works correctly if the widget is destroyed out from under you? In particular because the widget effectively owns this it'll try deleting this and popup_ will still be valid and you'll unnecessarily call out to Close(). The typical pattern for code like this is to subclass WidgetDelegateView and key off WindowClosing or DeleteDelegate. http://codereview.chromium.org/10825324/diff/18015/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:78: SetVisible(false); Why the SetVisible(false) here? Additionally are you sure HideInternal shouldn't be Close (aka delayed delete)?
http://codereview.chromium.org/10825324/diff/18015/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/18015/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:39: if (popup_.get()) On 2012/08/21 22:27:38, sky wrote: > Did you verify this all works correctly if the widget is destroyed out from > under you? In particular because the widget effectively owns this it'll try > deleting this and popup_ will still be valid and you'll unnecessarily call out > to Close(). > > The typical pattern for code like this is to subclass WidgetDelegateView and key > off WindowClosing or DeleteDelegate. Done. http://codereview.chromium.org/10825324/diff/18015/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:78: SetVisible(false); On 2012/08/21 22:27:38, sky wrote: > Why the SetVisible(false) here? Additionally are you sure HideInternal shouldn't > be Close (aka delayed delete)? Good point, changed.
http://codereview.chromium.org/10825324/diff/25001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): http://codereview.chromium.org/10825324/diff/25001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:24: class AutofillPopupViewViews : public views::WidgetDelegateView, As far as I can tell you're never actually setting this as the widget's delegate. Am I missing something?
http://codereview.chromium.org/10825324/diff/25001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): http://codereview.chromium.org/10825324/diff/25001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:24: class AutofillPopupViewViews : public views::WidgetDelegateView, On 2012/08/23 22:34:36, sky wrote: > As far as I can tell you're never actually setting this as the widget's > delegate. Am I missing something? Ah, no I wasn't. Fixed.
http://codereview.chromium.org/10825324/diff/31003/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/31003/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:19: class AutofillPopupViewViews::AutofillPopupWidget The ownership is still slightly wrong here. Remember that the widget owns its child views, so if the widget gets destroyed its going to delete AutofillPopupViewViews. This means you don't need supportsweakptr here. A small example of the pattern can be found here: ash/wm/workspace/system_background_controller.cc . If you really need the View to outlive the widget then you need to invoke set_owned_by_client on it. But in this case because your view is also the delegate it can know the widget is being deleted (WindowClosing or DeleteDelegate) and invoke InvalidateView as necessary.
http://codereview.chromium.org/10825324/diff/31003/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/31003/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:19: class AutofillPopupViewViews::AutofillPopupWidget On 2012/08/28 15:51:37, sky wrote: > The ownership is still slightly wrong here. Remember that the widget owns its > child views, so if the widget gets destroyed its going to delete > AutofillPopupViewViews. This means you don't need supportsweakptr here. A small > example of the pattern can be found here: > ash/wm/workspace/system_background_controller.cc . If you really need the View > to outlive the widget then you need to invoke set_owned_by_client on it. But in > this case because your view is also the delegate it can know the widget is being > deleted (WindowClosing or DeleteDelegate) and invoke InvalidateView as > necessary. Ok, I removed the weak_ptr support. InvalidateView is already called in the deconstructor so I don't think I need to worry about calling it else. I've also modified the delegate so that is never deletes the view direct, it will just hide it (which will close the widget and close it).
I think you got it this time. LGTM http://codereview.chromium.org/10825324/diff/39001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): http://codereview.chromium.org/10825324/diff/39001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:30: virtual ~AutofillPopupViewViews(); If you can, make this private.
http://codereview.chromium.org/10825324/diff/39001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): http://codereview.chromium.org/10825324/diff/39001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:49: bool popup_closing_; nit: I don't see this used anywhere -- is it vestigial?
Thanks for helping me figure out views :) http://codereview.chromium.org/10825324/diff/39001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): http://codereview.chromium.org/10825324/diff/39001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:30: virtual ~AutofillPopupViewViews(); On 2012/08/29 20:56:41, sky wrote: > If you can, make this private. Done. http://codereview.chromium.org/10825324/diff/39001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:49: bool popup_closing_; On 2012/08/29 22:36:44, Ilya Sherman wrote: > nit: I don't see this used anywhere -- is it vestigial? Correct, removed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/43001
Try job failure for 10825324-43001 (retry) on mac_rel for steps "browser_tests, unit_tests". It's a second try, previously, steps "browser_tests, unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.h (right): http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:8: #include "base/memory/scoped_ptr.h" unused here. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:14: class WebContents; unused here. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:22: AutofillExternalDelegateViews(TabContents* tab_contents, did you mean WebContents? http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:31: AutofillPopupViewViews* GetPopup() { return view_; } and then rename this to popup_view()? http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:49: virtual void SetBounds(const gfx::Rect& bounds) OVERRIDE; include compiler_specific.h for OVERRIDE. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:57: AutofillPopupViewViews* view_; |view_| is too generic. How about popup_view_? http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:59: DISALLOW_COPY_AND_ASSIGN(AutofillExternalDelegateViews); include basictypes.h for this.
http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: web_contents_->GetRenderViewHost()->AddKeyboardListener(this); Why are you adding this view as listener? KeyboardListener seems to be gtk specific. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:15: class RenderViewHost; unused here. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:20: class Rect; unused here. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:26: public KeyboardListener { ouch, looks like this isn't in content namespace. I'll make a patch for this and send to John. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:26: public KeyboardListener { Looks like you don't need to inherit from KeyboardListener, unless you are planning to add views support to it, because at the time I'm writing this, it seems to be gtk specific. "virtual bool HandleKeyPressEvent(GdkEventKey* event) = 0;" http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:38: remove extra blank line here. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:47: content::WebContents* web_contents_; // Weak reference. nit: I prefer // Not owned. But that is just my personal preference ;)
http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_external_delegate_views.h (right): http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:8: #include "base/memory/scoped_ptr.h" On 2012/09/02 12:08:04, tfarina wrote: > unused here. Done. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:14: class WebContents; On 2012/09/02 12:08:04, tfarina wrote: > unused here. Done. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:22: AutofillExternalDelegateViews(TabContents* tab_contents, On 2012/09/02 12:08:04, tfarina wrote: > did you mean WebContents? Nope, tabcontents is needed by the base external delegate class. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:31: AutofillPopupViewViews* GetPopup() { return view_; } On 2012/09/02 12:08:04, tfarina wrote: > and then rename this to popup_view()? Done. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:49: virtual void SetBounds(const gfx::Rect& bounds) OVERRIDE; On 2012/09/02 12:08:04, tfarina wrote: > include compiler_specific.h for OVERRIDE. Done. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:57: AutofillPopupViewViews* view_; On 2012/09/02 12:08:04, tfarina wrote: > |view_| is too generic. How about popup_view_? Done. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_external_delegate_views.h:59: DISALLOW_COPY_AND_ASSIGN(AutofillExternalDelegateViews); On 2012/09/02 12:08:04, tfarina wrote: > include basictypes.h for this. Done. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: web_contents_->GetRenderViewHost()->AddKeyboardListener(this); On 2012/09/02 12:13:25, tfarina wrote: > Why are you adding this view as listener? KeyboardListener seems to be gtk > specific. I'm planning on adding views support later. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:15: class RenderViewHost; On 2012/09/02 12:13:25, tfarina wrote: > unused here. Done. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:20: class Rect; On 2012/09/02 12:13:25, tfarina wrote: > unused here. Done. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:26: public KeyboardListener { On 2012/09/02 12:13:25, tfarina wrote: > Looks like you don't need to inherit from KeyboardListener, unless you are > planning to add views support to it, because at the time I'm writing this, it > seems to be gtk specific. > > "virtual bool HandleKeyPressEvent(GdkEventKey* event) = 0;" Yup, I do plan on adding views support later. http://codereview.chromium.org/10825324/diff/43001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:38: On 2012/09/02 12:13:25, tfarina wrote: > remove extra blank line here. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/43004
Failed to apply patch for chrome/chrome_browser.gypi: While running patch -p1 --forward --force; patching file chrome/chrome_browser.gypi Hunk #1 FAILED at 3127. 1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/44011
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/53002
Try job failure for 10825324-53002 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/53002
Try job failure for 10825324-53002 (retry) on mac_rel for step "sync_integration_tests". It's a second try, previously, step "sync_integration_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/53002
Try job failure for 10825324-53002 (retry) on win_rel for step "runhooks". It's a second try, previously, step "sync_integration_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/53002
Failed to apply patch for chrome/chrome_browser.gypi: While running patch -p1 --forward --force; patching file chrome/chrome_browser.gypi Hunk #1 FAILED at 3128. 1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/58002
Try job failure for 10825324-58002 (retry) (retry) on mac_rel for step "sync_integration_tests". It's a second try, previously, step "sync_integration_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10825324/58002
Change committed as 155476
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/10825324/diff/58002/chrome/browser/ui/... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://chromiumcodereview.appspot.com/10825324/diff/58002/chrome/browser/ui/... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:16: const SkColor kPopupBackground = SkColorSetARGB(0xFF, 0x00, 0x00, 0x00); This requires a static initializer for what it's worth. |
