Allow scroll events to open custom launcher pages.

Allow scroll events to open custom launcher pages.

Child widgets/webcontents like to steal the event, so purge the
clickzone widget and use ShouldDescendIntoChildForEventHandling to
direct the events at the start page view.

BUG=455787
Committed: https://crrev.com/eaa9e93535db9dfc7b54c9882a12e54a2ed284ed
Cr-Commit-Position: refs/heads/master@{#316539}

[calamity, 2015-02-12 04:32:24]

calamity@chromium.org changed reviewers:
+ calamity@chromium.org

[calamity, 2015-02-12 04:32:25]

Hah! Didn't know that existed. This looks great! Here's some initial things I
noticed.

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/app_list_v...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/app_list_v...
ui/app_list/views/app_list_view.cc:347: // the microphone icon.
You can always add SearchBoxView::OnScroll/Gesture and forward it to the active
content view (which is what keystrokes do right now).

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/app_list_v...
ui/app_list/views/app_list_view.cc:350: return child ==
search_box_widget_->GetNativeView();
You're also going to need the google logo webview somewhere here... That can
have a clickable image for doodles.

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/start_page...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/start_page...
ui/app_list/views/start_page_view.cc:322: if (event.y_offset() > 0) {
I don't know if we want mousewheel/gesture everywhere to show the custom
launcher page..

If we do, we might have to look at filtering the events that go to the start
page web contents to get this working for the areas that it covers.

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/start_page...
ui/app_list/views/start_page_view.cc:332: }
I think these all need to be gated by

app_list_main_view_->contents_view()->ShouldShowCustomPageClickzone()

since that determines whether the custom page is showable or not.

[tapted, 2015-02-13 02:58:49]

Patchset #6 (id:100001) has been deleted

[tapted, 2015-02-13 04:18:41]

So this is basically ready. The problem: the test doesn't currently work on
desktop platforms -- just ChromeOS. I think it's a WindowTreeHost thing that's
messing up coordinates. If it's blocking stuff, we could perhaps just
#ifdef/MAYBE the test for now on non-chromeos

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/app_list_v...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/app_list_v...
ui/app_list/views/app_list_view.cc:350: return child ==
search_box_widget_->GetNativeView();
On 2015/02/12 04:32:24, calamity wrote:
> You're also going to need the google logo webview somewhere here... That can
> have a clickable image for doodles.

Yes. Also: darn.

Turns out matching up |child| with a corresponding WebView in ui/app_list is
super-hard, since there is no content dependency. Patchset 5 has some
exploration of what that would look like (only works on aura).

Gave up on that and now just check against |location| here -- seems to work
pretty well.

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/start_page...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/start_page...
ui/app_list/views/start_page_view.cc:322: if (event.y_offset() > 0) {
On 2015/02/12 04:32:24, calamity wrote:
> I don't know if we want mousewheel/gesture everywhere to show the custom
> launcher page..
> 
> If we do, we might have to look at filtering the events that go to the start
> page web contents to get this working for the areas that it covers.

Done.

https://codereview.chromium.org/913133007/diff/1/ui/app_list/views/start_page...
ui/app_list/views/start_page_view.cc:332: }
On 2015/02/12 04:32:24, calamity wrote:
> I think these all need to be gated by
> 
> app_list_main_view_->contents_view()->ShouldShowCustomPageClickzone()
> 
> since that determines whether the custom page is showable or not.

Done.

[tapted, 2015-02-13 05:39:36]

ok - I think I've figured out a fix for desktop platforms. It puts an aura
dependency in the test, but it's one I probably need to nut out elsewhere
anyway.

[tapted, 2015-02-17 00:05:27]

Patchset #8 (id:160001) has been deleted

[tapted, 2015-02-17 00:05:57]

added scroll and did some spot-checks on a touchscreen - all good! ptal

[calamity, 2015-02-17 00:54:10]

lgtm. Hooray for deleting code.

https://codereview.chromium.org/913133007/diff/200001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/913133007/diff/200001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:164: gfx::Point
point_in_clickzone = bounds.CenterPoint();
This returns coordinates in the contents view's coordinates. Aren't the events
generated relative to the widget? (I'm worried about the app list border
throwing off your coordinates by 12px or whatever it is)

https://codereview.chromium.org/913133007/diff/200001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:208: const int
num_fingers = 2;
kStepDelay etc?

https://codereview.chromium.org/913133007/diff/200001/ui/app_list/views/start...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/913133007/diff/200001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:230: if
(!contents_view->ShouldShowCustomPageClickzone())
This ought to get a rename. I'll do it in my CL that goes on top of this.

https://codereview.chromium.org/913133007/diff/200001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:328: }
nit: newline after early return

[tapted, 2015-02-17 02:22:47]

New patchsets have been uploaded after l-g-t-m from calamity@chromium.org

[tapted, 2015-02-17 03:06:01]

https://codereview.chromium.org/913133007/diff/200001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/913133007/diff/200001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:164: gfx::Point
point_in_clickzone = bounds.CenterPoint();
On 2015/02/17 00:54:09, calamity wrote:
> This returns coordinates in the contents view's coordinates. Aren't the events
> generated relative to the widget? (I'm worried about the app list border
> throwing off your coordinates by 12px or whatever it is)

Yep! Fixed. Darn border.

It's 31 pixels (border_image width of 54 minus internal margin of 23). collapsed
height = kCustomPageCollapsedHeight = 78 and since that's > 31*2 it wasn't
affecting things in the test. I've added in the extra conversions.

https://codereview.chromium.org/913133007/diff/200001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:208: const int
num_fingers = 2;
On 2015/02/17 00:54:09, calamity wrote:
> kStepDelay etc?

Yeah I was doing that for ages, and it blew my mind when sky picked me up on it
:p. Style guide [1] says kFooStyle "for constants defined globally or within a
class." and "may optionally be used for compile-time constants of local scope".
So doing kStepDelay would be disobeying the style guide since it's local scope
and not compile-time. Then steps/fingers would be optional, but I went
hacker_style for consistency.

[1] go/cppguide?cl=head#Constant_Names

https://codereview.chromium.org/913133007/diff/200001/ui/app_list/views/start...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/913133007/diff/200001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:230: if
(!contents_view->ShouldShowCustomPageClickzone())
On 2015/02/17 00:54:09, calamity wrote:
> This ought to get a rename. I'll do it in my CL that goes on top of this.

Acknowledged.

https://codereview.chromium.org/913133007/diff/200001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:328: }
On 2015/02/17 00:54:09, calamity wrote:
> nit: newline after early return

Done.

[calamity, 2015-02-17 05:19:29]

https://codereview.chromium.org/913133007/diff/200001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/913133007/diff/200001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:208: const int
num_fingers = 2;
On 2015/02/17 03:06:01, tapted wrote:
> On 2015/02/17 00:54:09, calamity wrote:
> > kStepDelay etc?
> 
> Yeah I was doing that for ages, and it blew my mind when sky picked me up on
it
> :p. Style guide [1] says kFooStyle "for constants defined globally or within a
> class." and "may optionally be used for compile-time constants of local
scope".
> So doing kStepDelay would be disobeying the style guide since it's local scope
> and not compile-time. Then steps/fingers would be optional, but I went
> hacker_style for consistency.
> 
> [1] go/cppguide?cl=head#Constant_Names

Everything I know is a lie.

[tapted, 2015-02-17 06:25:45]

https://codereview.chromium.org/913133007/diff/280001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/913133007/diff/280001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:212: #if
defined(OS_CHROMEOS)
OK.. I'm pretty sure I've cracked these event generator coordinates now - see
comment here. I'll see about a follow-up to the event generator code that might
let this go away -- I'm pretty sure this is the first time someone's tried
EventGenerator+Gestures+Desktop-Widgets, since it was aura-specific for a very
long time.

[tapted, 2015-02-17 07:12:38]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-02-17 07:13:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/913133007/280001

[calamity, 2015-02-17 07:13:56]

https://codereview.chromium.org/913133007/diff/280001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/913133007/diff/280001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:212: #if
defined(OS_CHROMEOS)
On 2015/02/17 06:25:45, tapted wrote:
> OK.. I'm pretty sure I've cracked these event generator coordinates now - see
> comment here. I'll see about a follow-up to the event generator code that
might
> let this go away -- I'm pretty sure this is the first time someone's tried
> EventGenerator+Gestures+Desktop-Widgets, since it was aura-specific for a very
> long time.

Acknowledged.

[commit-bot: I haz the power, 2015-02-17 07:16:08]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2015-02-17 07:17:03]

Patchset 13 (id:??) landed as
https://crrev.com/eaa9e93535db9dfc7b54c9882a12e54a2ed284ed
Cr-Commit-Position: refs/heads/master@{#316539}