Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(7)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10580039: Adds ability to render omnibox as a view above the page. (Closed)

Created:
8 years, 6 months ago by sky
Modified:
8 years, 6 months ago
CC:
chromium-reviews, James Su, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org, gideonwald, tfarina, Use pkasting(at)chromium.org
Visibility:
Public.

Description

Adds ability to render omnibox as a view above the page. BUG=none TEST=none R=pkasting@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=143473

Patch Set 1 #

Patch Set 2 : cleanup #

Total comments: 11

Patch Set 3 : nits #

Patch Set 4 : Merge with trunk (new names) #

Patch Set 5 : Fix windows #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+533 lines, -97 lines) Patch
M chrome/browser/chromeos/login/simple_web_view_dialog.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/frame/browser_view.h View 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 3 4 chunks +3 lines, -7 lines 0 comments Download
M chrome/browser/ui/views/frame/contents_container.h View 1 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/contents_container.cc View 1 2 chunks +44 lines, -4 lines 2 comments Download
M chrome/browser/ui/views/location_bar/location_bar_view.h View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/location_bar_view.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
A + chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.h View 1 2 3 10 chunks +23 lines, -52 lines 1 comment Download
A chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc View 1 2 3 1 chunk +395 lines, -0 lines 2 comments Download
M chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h View 1 2 3 1 chunk +6 lines, -7 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc View 1 2 3 2 chunks +16 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.cc View 1 2 3 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_win.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_win.cc View 1 2 3 4 1 chunk +5 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_views.h View 1 chunk +6 lines, -1 line 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_views.cc View 1 2 3 4 1 chunk +5 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/toolbar_view.h View 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/toolbar_view.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
sky
8 years, 6 months ago (2012-06-20 17:23:00 UTC) #1
tfarina
http://codereview.chromium.org/10580039/diff/2001/chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.h File chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.h (right): http://codereview.chromium.org/10580039/diff/2001/chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.h#newcode29 chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.h:29: // A view representing the contents of the autocomplete ...
8 years, 6 months ago (2012-06-21 06:28:55 UTC) #2
tfarina
http://codereview.chromium.org/10580039/diff/2001/chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc File chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc (right): http://codereview.chromium.org/10580039/diff/2001/chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc#newcode11 chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc:11: #include "chrome/browser/themes/theme_service.h" nit: is this used in this file? ...
8 years, 6 months ago (2012-06-21 06:38:37 UTC) #3
sky
I incorporated the rest of your suggestions. http://codereview.chromium.org/10580039/diff/2001/chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc File chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc (right): http://codereview.chromium.org/10580039/diff/2001/chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc#newcode211 chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc:211: if (!HasMatchAt(index)) ...
8 years, 6 months ago (2012-06-21 18:34:24 UTC) #4
sky
Punting to Ben. If you the time Peter, take a look.
8 years, 6 months ago (2012-06-21 21:45:07 UTC) #5
Ben Goodger (Google)
lgtm
8 years, 6 months ago (2012-06-21 21:56:00 UTC) #6
Peter Kasting
8 years, 6 months ago (2012-06-22 20:33:22 UTC) #7 
Not many comments, but a few.

http://codereview.chromium.org/10580039/diff/10002/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/contents_container.cc (right):

http://codereview.chromium.org/10580039/diff/10002/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/contents_container.cc:16: bool should_show() {
return child_count() && child_at(0)->visible(); }
Nit: const

http://codereview.chromium.org/10580039/diff/10002/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/contents_container.cc:23: bool child_visibile_;
Nit: Maybe call this child_was_visible_ and add some comments about it?

http://codereview.chromium.org/10580039/diff/10002/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc (right):

http://codereview.chromium.org/10580039/diff/10002/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
It would have been nice to copy this file from omnibox_popup_contents_view.cc so
it was clear what the changes were.  Then I could have reviewed those more
closely...

http://codereview.chromium.org/10580039/diff/10002/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.cc:279:
model_->SetHoveredLine(GetIndexForPoint(event.location()));
Nit: Call OnMouseMoved() instead?

http://codereview.chromium.org/10580039/diff/10002/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.h (right):

http://codereview.chromium.org/10580039/diff/10002/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/inline_omnibox_popup_view.h:49: // Overridden
from AutocompletePopupView:
Nit: You branched off before tfarina renamed a bunch of Autocomplete classes to
Omnibox; please update the references (mostly comments) to match (e.g. here,
line 111, .cc file, etc.)

Powered by Google App Engine
This is Rietveld 408576698