Issue 10915217: Hook up SetInstantPreviewHeight for ChromeOS.

Issue 10915217: Hook up SetInstantPreviewHeight for ChromeOS. (Closed)

Created:
8 years, 3 months ago by Jered

Modified:
8 years, 3 months ago

Reviewers:
sreeram, sky

CC:
chromium-reviews, tfarina, gideonwald, dominich, David Black, Shishir

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Hook up SetInstantPreviewHeight for ChromeOS. I tested this by specifying the fake instant page with --instant-url and adding some calls to setNonNativeDropdownHeight(). BUG= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157670

Patch Set 1 #

Patch Set 2 : Minor fixes. #

Total comments: 4

Patch Set 3 : Rebase. #

Patch Set 4 : Address comments. #

Total comments: 8

Patch Set 5 : More comments. #

Total comments: 4

Patch Set 6 : Move height to ShowPreview. #

Total comments: 2

Patch Set 7 : Don't cache height in InstantController. #

Total comments: 2

Patch Set 8 : Move hidden notification, too. #

Created: 8 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+143 lines, -46 lines)			Patch
M	chrome/browser/instant/instant_controller.h	View	1 2 3 4 5 6	2 chunks	+6 lines, -2 lines	0 comments	Download
M	chrome/browser/instant/instant_controller.cc	View	1 2 3 4 5 6 7	6 chunks	+26 lines, -5 lines	0 comments	Download
M	chrome/browser/instant/instant_controller_delegate.h	View	1 2 3 4 5	1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/instant/instant_loader.cc	View	1 2 3 4	3 chunks	+24 lines, -0 lines	0 comments	Download
M	chrome/browser/instant/instant_loader_delegate.h	View		1 chunk	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/browser_instant_controller.h	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/browser_instant_controller.cc	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -12 lines	0 comments	Download
M	chrome/browser/ui/browser_window.h	View	1 2 3 4 5	2 chunks	+6 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/cocoa/browser_window_cocoa.h	View	1 2 3 4 5	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/ui/cocoa/browser_window_cocoa.mm	View	1 2 3 4 5	1 chunk	+5 lines, -1 line	0 comments	Download
M	chrome/browser/ui/gtk/browser_window_gtk.h	View	1 2 3 4 5	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/ui/gtk/browser_window_gtk.cc	View	1 2 3 4 5	1 chunk	+5 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/frame/browser_view.h	View	1 2 3 4 5	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/frame/browser_view.cc	View	1 2 3 4 5	2 chunks	+6 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/views/frame/contents_container.h	View	1 2 3 4 5	3 chunks	+12 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/frame/contents_container.cc	View	1 2 3 4 5 6	3 chunks	+31 lines, -11 lines	0 comments	Download
M	chrome/test/base/test_browser_window.h	View	1 2 3 4 5	1 chunk	+3 lines, -1 line	0 comments	Download

Messages

Total messages: 19 (0 generated)

Expand Messages | Collapse Messages

sreeram

I think you need to pass the units all the way to the view code, ...

8 years, 3 months ago (2012-09-17 17:26:12 UTC) #2

Jered

Ok, please have another look; I'll add sky@ once this looks good to you. http://codereview.chromium.org/10915217/diff/3001/chrome/browser/instant/instant_controller.cc ...

8 years, 3 months ago (2012-09-18 22:12:00 UTC) #3

sreeram

LGTM http://codereview.chromium.org/10915217/diff/11001/chrome/browser/instant/instant_loader.cc File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/10915217/diff/11001/chrome/browser/instant/instant_loader.cc#newcode18 chrome/browser/instant/instant_loader.cc:18: #include "chrome/common/instant_types.h" No need for this (already brought ...

8 years, 3 months ago (2012-09-18 22:24:24 UTC) #4

sreeram

http://codereview.chromium.org/10915217/diff/11001/chrome/browser/ui/views/frame/contents_container.h File chrome/browser/ui/views/frame/contents_container.h (right): http://codereview.chromium.org/10915217/diff/11001/chrome/browser/ui/views/frame/contents_container.h#newcode9 chrome/browser/ui/views/frame/contents_container.h:9: #include "chrome/common/instant_types.h" Sort.

8 years, 3 months ago (2012-09-18 22:28:08 UTC) #5

Jered

Thanks, +sky http://codereview.chromium.org/10915217/diff/11001/chrome/browser/instant/instant_loader.cc File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/10915217/diff/11001/chrome/browser/instant/instant_loader.cc#newcode18 chrome/browser/instant/instant_loader.cc:18: #include "chrome/common/instant_types.h" On 2012/09/18 22:24:24, sreeram wrote: ...

8 years, 3 months ago (2012-09-18 22:46:24 UTC) #6

sky

I believed we discussed this before. At what point are we setting the preview height? ...

8 years, 3 months ago (2012-09-19 00:23:17 UTC) #7

sreeram

On 2012/09/19 00:23:17, sky wrote: > I believed we discussed this before. At what point ...

8 years, 3 months ago (2012-09-19 01:11:44 UTC) #8

On 2012/09/19 00:23:17, sky wrote:
> I believed we discussed this before. At what point are we setting the preview
> height? We need to ensure we don't bounce the size around. Ideally SetPreview
> would include the height.

Agreed. Jered, let's pass in the height starting with the
delegate_->ShowInstant() call. Quoting Scott from an email thread:
<quote>
Layout changes the bounds immediately. Painting is typically delayed
though. In some cases this is fine, but for web content changing the
bounds results in a bunch of messages being sent to the renderer. So
that even if you ultimately change the bounds to the right size later
on, its too late. You have no control over how the renderer is going
to deal with the separate changes.
</quote>

In other words, we want to avoid doing something like this:
Show();  // uses the default 100%
SetHeight("50%");  // immediately after the line above.

This is bouncy, and we should instead have done:
SetHeight("50%");
Show();

Even better:
Show("50%");

Now, SetPreviewHeight will be used in three cases:

1. When the currently selected match is a URL, we'll kick off a prerender. After
the prerender has completed, we'll notify the search page, and the page will
then call SetPreviewHeight so that it only shows the suggestions. So, by this
time, the search page has already been shown.

Aside: We need SetPreviewHeight here so that we can know where to position the
URL-prerendered view (just below the suggestions, in y-axis). We don't actually
need to resize the search preview. We could just position the URL overlay so
that it obscures the search overlay (in z-order), but to keep things consistent
with #2 and #3 below, we'll resize the search overlay anyway.

2. To show the doodle on the NTP.
3. To show zero suggest (suggestions even when the user hasn't typed anything in
to the omnibox).

In case #1, we will always do a Show("100%") first, and only then (at some later
time, when the page tells us so), we'll do Show("50%"). So, changing the API
will not benefit this case. However, for cases #2 and #3, the search overlay is
not yet showing, so Scott's proposal will help avoid accidental bounce.

Jered

Is this what you had in mind? http://codereview.chromium.org/10915217/diff/16004/chrome/browser/ui/browser_window.h File chrome/browser/ui/browser_window.h (right): http://codereview.chromium.org/10915217/diff/16004/chrome/browser/ui/browser_window.h#newcode338 chrome/browser/ui/browser_window.h:338: // Resize ...

8 years, 3 months ago (2012-09-19 18:52:09 UTC) #9

sreeram

There's no need to cache the preview height/units in InstantController. Just have InstantController::Show() take the ...

8 years, 3 months ago (2012-09-19 19:59:52 UTC) #10

sreeram

Forgot to add, you also need to unconditionally call delegate_->ShowInstant(). This means we'll send duplicate ...

8 years, 3 months ago (2012-09-19 20:05:04 UTC) #11

sreeram

On 2012/09/19 20:05:04, sreeram wrote: > Forgot to add, you also need to unconditionally call ...

8 years, 3 months ago (2012-09-19 20:10:58 UTC) #12

Jered

Ok, here's a new patchset. On 2012/09/19 20:10:58, sreeram wrote: > On 2012/09/19 20:05:04, sreeram ...

8 years, 3 months ago (2012-09-19 20:22:10 UTC) #13

Jered

http://codereview.chromium.org/10915217/diff/9021/chrome/browser/ui/views/frame/contents_container.cc File chrome/browser/ui/views/frame/contents_container.cc (right): http://codereview.chromium.org/10915217/diff/9021/chrome/browser/ui/views/frame/contents_container.cc#newcode74 chrome/browser/ui/views/frame/contents_container.cc:74: Layout(); On 2012/09/19 19:59:52, sreeram wrote: > This method ...

8 years, 3 months ago (2012-09-19 20:22:18 UTC) #14

sreeram

LGTM http://codereview.chromium.org/10915217/diff/12025/chrome/browser/ui/browser_instant_controller.cc File chrome/browser/ui/browser_instant_controller.cc (right): http://codereview.chromium.org/10915217/diff/12025/chrome/browser/ui/browser_instant_controller.cc#newcode103 chrome/browser/ui/browser_instant_controller.cc:103: content::NotificationService::NoDetails()); For consistency, let's move this notification also ...

8 years, 3 months ago (2012-09-19 20:52:03 UTC) #15

Jered

Thanks for reviewing. http://codereview.chromium.org/10915217/diff/12025/chrome/browser/ui/browser_instant_controller.cc File chrome/browser/ui/browser_instant_controller.cc (right): http://codereview.chromium.org/10915217/diff/12025/chrome/browser/ui/browser_instant_controller.cc#newcode103 chrome/browser/ui/browser_instant_controller.cc:103: content::NotificationService::NoDetails()); On 2012/09/19 20:52:03, sreeram wrote: ...

8 years, 3 months ago (2012-09-19 21:18:14 UTC) #17

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jered@chromium.org/10915217/15003

8 years, 3 months ago (2012-09-19 21:50:24 UTC) #18

Change committed as 157670

Expand Messages | Collapse Messages