🏠 onExitPeekState no longer only swipe triggered

[Home] onExitPeekState no longer only swipe triggered

Previously the onExitPeekState event was only triggered when a user
swiped up on the bottom sheet. This was to prevent content from
loading if the sheet was expanding for omnibox focus. This change
has the sheet call the even for any action that expands the sheet out
of the peeking state.

BUG=671361
Review-Url: https://codereview.chromium.org/2676663004
Cr-Commit-Position: refs/heads/master@{#449711}
Committed: https://chromium.googlesource.com/chromium/src/+/0ddb4374a9221dba28e9ea8a1edaacb32dcc0861

[mdjones, 2017-02-03 18:36:48]

Description was changed from

==========
[Home] onExitPeekState no longer only swipe triggered

Previously the onExitPeekState event was only triggered when a user
swiped up on the bottom sheet. This was to prevent content from
loading if the sheet was expanding for omnibox focus. This change
has the sheet call the even for any action that expands the sheet out
of the peeking state.

BUG=671361
==========

to

==========
[Home] onExitPeekState no longer only swipe triggered

Previously the onExitPeekState event was only triggered when a user
swiped up on the bottom sheet. This was to prevent content from
loading if the sheet was expanding for omnibox focus. This change
has the sheet call the even for any action that expands the sheet out
of the peeking state.

BUG=671361
==========

[mdjones, 2017-02-03 18:39:17]

mdjones@chromium.org changed reviewers:
+ dfalcantara@chromium.org, dgn@chromium.org

[mdjones, 2017-02-03 18:39:18]

The problem that this change introduces is that focusing the omnibox for the
first time loads the ntp content, causing jank. Though this is more correct for
the event name, I think a larger discussion needs to take place about when to
load content.

[dgn, 2017-02-06 10:42:56]

On 2017/02/03 18:39:18, mdjones wrote:
> The problem that this change introduces is that focusing the omnibox for the
> first time loads the ntp content, causing jank. Though this is more correct
for
> the event name, I think a larger discussion needs to take place about when to
> load content.

Thanks!

Can we maybe initialise and inflate (there is an AsyncLayoutInflater) the bottom
sheet in the background on omnibox focus or after main startup tasks are
completed? I saw that jank even when pulling up the omnibox, since it takes a
bit of time to create the content.

[mdjones, 2017-02-06 22:13:25]

On 2017/02/06 10:42:56, dgn wrote:
> On 2017/02/03 18:39:18, mdjones wrote:
> > The problem that this change introduces is that focusing the omnibox for the
> > first time loads the ntp content, causing jank. Though this is more correct
> for
> > the event name, I think a larger discussion needs to take place about when
to
> > load content.
> 
> Thanks!
> 
> Can we maybe initialise and inflate (there is an AsyncLayoutInflater) the
bottom
> sheet in the background on omnibox focus or after main startup tasks are
> completed? I saw that jank even when pulling up the omnibox, since it takes a
> bit of time to create the content.

I tried using the AsyncLayoutInflater but there was still noticeable jank. I'm
considering creating the NTP content on startup, but I need a tab to be ready
before I can do that. Is it possible to remove the tab requirement from the
suggestions classes?

[dgn, 2017-02-07 11:04:39]

On 2017/02/06 22:13:25, mdjones wrote:
> On 2017/02/06 10:42:56, dgn wrote:
> > On 2017/02/03 18:39:18, mdjones wrote:
> > > The problem that this change introduces is that focusing the omnibox for
the
> > > first time loads the ntp content, causing jank. Though this is more
correct
> > for
> > > the event name, I think a larger discussion needs to take place about when
> to
> > > load content.
> > 
> > Thanks!
> > 
> > Can we maybe initialise and inflate (there is an AsyncLayoutInflater) the
> bottom
> > sheet in the background on omnibox focus or after main startup tasks are
> > completed? I saw that jank even when pulling up the omnibox, since it takes
a
> > bit of time to create the content.
> 
> I tried using the AsyncLayoutInflater but there was still noticeable jank. I'm
> considering creating the NTP content on startup, but I need a tab to be ready
> before I can do that. Is it possible to remove the tab requirement from the
> suggestions classes?

I think so yes. If we have the selector it should be enough. Having a dependency
on a specific tab is wrong anyway, as the bottom sheet will be used across
different tabs.

[dgn, 2017-02-07 12:13:44]

On 2017/02/07 11:04:39, dgn wrote:
> On 2017/02/06 22:13:25, mdjones wrote:
> > On 2017/02/06 10:42:56, dgn wrote:
> > > On 2017/02/03 18:39:18, mdjones wrote:
> > > > The problem that this change introduces is that focusing the omnibox for
> the
> > > > first time loads the ntp content, causing jank. Though this is more
> correct
> > > for
> > > > the event name, I think a larger discussion needs to take place about
when
> > to
> > > > load content.
> > > 
> > > Thanks!
> > > 
> > > Can we maybe initialise and inflate (there is an AsyncLayoutInflater) the
> > bottom
> > > sheet in the background on omnibox focus or after main startup tasks are
> > > completed? I saw that jank even when pulling up the omnibox, since it
takes
> a
> > > bit of time to create the content.
> > 
> > I tried using the AsyncLayoutInflater but there was still noticeable jank.
I'm
> > considering creating the NTP content on startup, but I need a tab to be
ready
> > before I can do that. Is it possible to remove the tab requirement from the
> > suggestions classes?
> 
> I think so yes. If we have the selector it should be enough. Having a
dependency
> on a specific tab is wrong anyway, as the bottom sheet will be used across
> different tabs.

Shouldn't your NativePageHost remove the dependency on a Tab?

[dgn, 2017-02-10 14:53:23]

ping, can this land? Or should I add it to my CL?

[gone, 2017-02-10 18:19:01]

lgtm

[mdjones, 2017-02-10 18:46:35]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2017-02-10 18:47:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 18:50:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 18:50:05]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mdjones, 2017-02-10 18:59:30]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-02-10 18:59:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2676663004/#ps40001
(title: "rebase correctly")

[commit-bot: I haz the power, 2017-02-10 19:00:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 20:09:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486753170269330,
"parent_rev": "71423d74c1786cbd7c436368cca4481b946cc12b", "commit_rev":
"0ddb4374a9221dba28e9ea8a1edaacb32dcc0861"}

[commit-bot: I haz the power, 2017-02-10 20:10:24]

Description was changed from

==========
[Home] onExitPeekState no longer only swipe triggered

Previously the onExitPeekState event was only triggered when a user
swiped up on the bottom sheet. This was to prevent content from
loading if the sheet was expanding for omnibox focus. This change
has the sheet call the even for any action that expands the sheet out
of the peeking state.

BUG=671361
==========

to

==========
[Home] onExitPeekState no longer only swipe triggered

Previously the onExitPeekState event was only triggered when a user
swiped up on the bottom sheet. This was to prevent content from
loading if the sheet was expanding for omnibox focus. This change
has the sheet call the even for any action that expands the sheet out
of the peeking state.

BUG=671361

Review-Url: https://codereview.chromium.org/2676663004
Cr-Commit-Position: refs/heads/master@{#449711}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ddb4374a9221dba28e9ea8a1eda...
==========

[commit-bot: I haz the power, 2017-02-10 20:10:24]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0ddb4374a9221dba28e9ea8a1eda...