VR: Use ToolbarModel to drive VR URL bar state.

VR: Use ToolbarModel to drive VR URL bar state.

This brings VR more in line with how desktop's omnibox works, and
reduces the potential for VR to exhibit inconsistencies.

Note that ToolbarModel supplies state, but not an indication of when that state
changes. For this reason, we'll still use a WebContentsObserver to
trigger updates.

BUG=736756
Review-Url: https://codereview.chromium.org/2960903002
Cr-Commit-Position: refs/heads/master@{#483383}
Committed: https://chromium.googlesource.com/chromium/src/+/ad4f36ac5736301cbb7c9c47e8810039f6e256e9

[cjgrant, 2017-06-27 21:52:46]

cjgrant@chromium.org changed reviewers:
+ mthiesse@chromium.org

[cjgrant, 2017-06-27 21:52:46]

Michael, PTAL when you can.  This fixes bugs.  :)

[mthiesse, 2017-06-28 00:27:09]

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:228: const
gfx::VectorIcon& icon = *(state_.vector_icon);
nit: unnecessary parens

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc
(right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:49:
ToolbarState state;
Why not just add a 5-param constructor to ToolbarState, then no need for the
assert below, etc. Easier to make sure it's always initialized correctly.

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_helper.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.cc:15: constexpr int
kMaxURLDisplayChars = 1024;
Why 1024? Do we intend for the URL to never get truncated by the
ToolbarModelImpl?

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.cc:29: state.vector_icon =
&(toolbar_model_->GetVectorIcon());
nit: unnecessary parens

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_helper.h (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.h:18: // This class houses an
instance of ToolbarModel, and when asked, passes a
nit: Clarify or remove "when asked"? 

Maybe "This class houses an instance of ToolbarModel, and updates it when
requested, passing a snapshot of the toolbar state to the UI when necessary."

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_state.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_state.cc:18: should_display_url ==
other.should_display_url);
Don't care about secure_verbose_text?

[cjgrant, 2017-06-28 15:14:34]

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:228: const
gfx::VectorIcon& icon = *(state_.vector_icon);
On 2017/06/28 00:27:08, mthiesse wrote:
> nit: unnecessary parens

Done.

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc
(right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture_unittest.cc:49:
ToolbarState state;
On 2017/06/28 00:27:08, mthiesse wrote:
> Why not just add a 5-param constructor to ToolbarState, then no need for the
> assert below, etc. Easier to make sure it's always initialized correctly.

Done.

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_helper.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.cc:15: constexpr int
kMaxURLDisplayChars = 1024;
On 2017/06/28 00:27:08, mthiesse wrote:
> Why 1024? Do we intend for the URL to never get truncated by the
> ToolbarModelImpl?

ToolbarModelImpl can truncate, that's fine.  The constant used in content is
32K.  If we limit to 1K, that's still 10 times more URL that we can fit on our
bar, and our rendering will further truncate.  Is this what you meant?

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.cc:29: state.vector_icon =
&(toolbar_model_->GetVectorIcon());
On 2017/06/28 00:27:08, mthiesse wrote:
> nit: unnecessary parens

Done.

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_helper.h (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.h:18: // This class houses an
instance of ToolbarModel, and when asked, passes a
On 2017/06/28 00:27:08, mthiesse wrote:
> nit: Clarify or remove "when asked"? 
> 
> Maybe "This class houses an instance of ToolbarModel, and updates it when
> requested, passing a snapshot of the toolbar state to the UI when necessary."

Done.  Your wording, except we don't update the ToolbarModel, we query it.

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_state.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_state.cc:18: should_display_url ==
other.should_display_url);
On 2017/06/28 00:27:08, mthiesse wrote:
> Don't care about secure_verbose_text?

Done. Good catch.

[mthiesse, 2017-06-28 15:46:41]

lgtm % comment

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_helper.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.cc:15: constexpr int
kMaxURLDisplayChars = 1024;
On 2017/06/28 15:14:34, cjgrant wrote:
> On 2017/06/28 00:27:08, mthiesse wrote:
> > Why 1024? Do we intend for the URL to never get truncated by the
> > ToolbarModelImpl?
> 
> ToolbarModelImpl can truncate, that's fine.  The constant used in content is
> 32K.  If we limit to 1K, that's still 10 times more URL that we can fit on our
> bar, and our rendering will further truncate.  Is this what you meant?

1K just seems strange. I was wondering if we were trying to make sure truncating
only happens once. Maybe put a comment on why 1K? If it's arbitrary, that's
probably fine, but we should note that :P

[cjgrant, 2017-06-28 15:48:46]

The CQ bit was checked by cjgrant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 15:49:03]

Dry run: 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-06-28 15:52:29]

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

[commit-bot: I haz the power, 2017-06-28 15:52:30]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cjgrant, 2017-06-28 19:41:51]

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_helper.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.cc:15: constexpr int
kMaxURLDisplayChars = 1024;
On 2017/06/28 15:46:41, mthiesse wrote:
> On 2017/06/28 15:14:34, cjgrant wrote:
> > On 2017/06/28 00:27:08, mthiesse wrote:
> > > Why 1024? Do we intend for the URL to never get truncated by the
> > > ToolbarModelImpl?
> > 
> > ToolbarModelImpl can truncate, that's fine.  The constant used in content is
> > 32K.  If we limit to 1K, that's still 10 times more URL that we can fit on
our
> > bar, and our rendering will further truncate.  Is this what you meant?
> 
> 1K just seems strange. I was wondering if we were trying to make sure
truncating
> only happens once. Maybe put a comment on why 1K? If it's arbitrary, that's
> probably fine, but we should note that :P

Is it less strange than 32K that desktop uses?  :)  I'm actually just trying to
avoid linking in another chunk of content to pick up a constant.

[cjgrant, 2017-06-28 19:41:59]

The CQ bit was checked by cjgrant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 19:42:22]

Dry run: CQ is trying da patch.

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

[cjgrant, 2017-06-28 19:46:34]

cjgrant@chromium.org changed reviewers:
+ pkasting@chromium.org

[cjgrant, 2017-06-28 19:46:34]

pkasting@chromium.org: Please review changes in 

components/toolbar

Note that the existing Android Omnibox implementation (as far as I can see) uses
only GetURL() from ToolbarModel.  However, since both Clank and VR use this
class, I'm not sure it makes sense to use a BUILDFLAG(ENABLER_VR) conditional.

[mthiesse, 2017-06-28 19:50:13]

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_helper.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.cc:15: constexpr int
kMaxURLDisplayChars = 1024;
On 2017/06/28 19:41:51, cjgrant wrote:
> On 2017/06/28 15:46:41, mthiesse wrote:
> > On 2017/06/28 15:14:34, cjgrant wrote:
> > > On 2017/06/28 00:27:08, mthiesse wrote:
> > > > Why 1024? Do we intend for the URL to never get truncated by the
> > > > ToolbarModelImpl?
> > > 
> > > ToolbarModelImpl can truncate, that's fine.  The constant used in content
is
> > > 32K.  If we limit to 1K, that's still 10 times more URL that we can fit on
> our
> > > bar, and our rendering will further truncate.  Is this what you meant?
> > 
> > 1K just seems strange. I was wondering if we were trying to make sure
> truncating
> > only happens once. Maybe put a comment on why 1K? If it's arbitrary, that's
> > probably fine, but we should note that :P
> 
> Is it less strange than 32K that desktop uses?  :)  I'm actually just trying
to
> avoid linking in another chunk of content to pick up a constant.

I'm just asking for a comment saying "This is arbitrary", so future readers
don't assume it has to be 1024.

[Peter Kasting, 2017-06-28 20:06:05]

LGTM

[cjgrant, 2017-06-28 20:21:02]

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/toolbar_helper.cc (right):

https://codereview.chromium.org/2960903002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/toolbar_helper.cc:15: constexpr int
kMaxURLDisplayChars = 1024;
On 2017/06/28 19:50:13, mthiesse wrote:
> On 2017/06/28 19:41:51, cjgrant wrote:
> > On 2017/06/28 15:46:41, mthiesse wrote:
> > > On 2017/06/28 15:14:34, cjgrant wrote:
> > > > On 2017/06/28 00:27:08, mthiesse wrote:
> > > > > Why 1024? Do we intend for the URL to never get truncated by the
> > > > > ToolbarModelImpl?
> > > > 
> > > > ToolbarModelImpl can truncate, that's fine.  The constant used in
content
> is
> > > > 32K.  If we limit to 1K, that's still 10 times more URL that we can fit
on
> > our
> > > > bar, and our rendering will further truncate.  Is this what you meant?
> > > 
> > > 1K just seems strange. I was wondering if we were trying to make sure
> > truncating
> > > only happens once. Maybe put a comment on why 1K? If it's arbitrary,
that's
> > > probably fine, but we should note that :P
> > 
> > Is it less strange than 32K that desktop uses?  :)  I'm actually just trying
> to
> > avoid linking in another chunk of content to pick up a constant.
> 
> I'm just asking for a comment saying "This is arbitrary", so future readers
> don't assume it has to be 1024.

SGTM.  Done.

[cjgrant, 2017-06-29 15:05:46]

The CQ bit was checked by cjgrant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 15:06:06]

Dry run: CQ is trying da patch.

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

[cjgrant, 2017-06-29 16:15:22]

The CQ bit was unchecked by cjgrant@chromium.org

[cjgrant, 2017-06-29 16:15:27]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-06-29 16:15:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2960903002/#ps80001
(title: "Update toolbar build file to include vector icons for Android.")

[commit-bot: I haz the power, 2017-06-29 16:15:46]

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-06-29 16:30:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498752927287300,
"parent_rev": "ecbf3090c642226b2c1a9d6ed055ec06bcd84763", "commit_rev":
"ad4f36ac5736301cbb7c9c47e8810039f6e256e9"}

[commit-bot: I haz the power, 2017-06-29 16:30:46]

Description was changed from

==========
VR: Use ToolbarModel to drive VR URL bar state.

This brings VR more in line with how desktop's omnibox works, and
reduces the potential for VR to exhibit inconsistencies.

Note that ToolbarModel supplies state, but not an indication of when that state
changes. For this reason, we'll still use a WebContentsObserver to
trigger updates.

BUG=736756
==========

to

==========
VR: Use ToolbarModel to drive VR URL bar state.

This brings VR more in line with how desktop's omnibox works, and
reduces the potential for VR to exhibit inconsistencies.

Note that ToolbarModel supplies state, but not an indication of when that state
changes. For this reason, we'll still use a WebContentsObserver to
trigger updates.

BUG=736756

Review-Url: https://codereview.chromium.org/2960903002
Cr-Commit-Position: refs/heads/master@{#483383}
Committed:
https://chromium.googlesource.com/chromium/src/+/ad4f36ac5736301cbb7c9c47e881...
==========

[commit-bot: I haz the power, 2017-06-29 16:30:49]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ad4f36ac5736301cbb7c9c47e881...