ToolbarModel: Supply offline page status.

ToolbarModel: Supply offline page status.

This allows users of ToolbarModel to determine whether the current page
is an offline page or not.  In the future, it'd be interesting if the
security level, vector icon and verbose security text could simply
indicate that the page is offline, rather than using a separate bit of
information.

BUG=735770
Review-Url: https://codereview.chromium.org/2965043003
Cr-Commit-Position: refs/heads/master@{#484789}
Committed: https://chromium.googlesource.com/chromium/src/+/d0af1dee22043449ff2076b1ba361a47b1803b3d

[cjgrant, 2017-07-05 20:07:49]

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

[cjgrant, 2017-07-05 20:07:50]

Filip, here's a first cut that puts plumbing in place.  It works for us.  I'm
not sure if there's a layer violation in the chrome delegate tapping Android
code, but it's not the first OS_ANDROID check in that file.

Please let me know what you think.

[fgorski, 2017-07-05 20:27:16]

On 2017/07/05 20:07:50, cjgrant wrote:
> Filip, here's a first cut that puts plumbing in place.  It works for us.  I'm
> not sure if there's a layer violation in the chrome delegate tapping Android
> code, but it's not the first OS_ANDROID check in that file.
> 
> Please let me know what you think.

Overall looks OK.

If there is a layering violation, we'll need to move some code around (looking
at it there is more than a single method to extract, because of OPTabHelper
dependencies), but we can make it in a couple of days.

[cjgrant, 2017-07-05 20:34:12]

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

[cjgrant, 2017-07-05 20:34:12]

pkasting@chromium.org: Please review changes in ToolbarModel and delegates.

As per the bug, now that VR is a ToolbarModel fan, I'm hoping to squeeze it for
more.  Does this look acceptable?

Feel free to comment on the "possible future improvement" comment in the CL
description.

[Peter Kasting, 2017-07-06 06:08:08]

The code is OK, but I suggest you follow your own proposed change now and simply
modify the icon and text rather than returning this separate bit at all, unless
you actually need the separate bit for some other reason.

LGTM if there's a good reason you can't do this immediately.

https://codereview.chromium.org/2965043003/diff/1/chrome/browser/ui/toolbar/c...
File chrome/browser/ui/toolbar/chrome_toolbar_model_delegate.cc (right):

https://codereview.chromium.org/2965043003/diff/1/chrome/browser/ui/toolbar/c...
chrome/browser/ui/toolbar/chrome_toolbar_model_delegate.cc:140: web_contents) !=
nullptr;
Nit: Simpler:

  return web_contents &&
         offline_pages::OfflinePageUtils::GetOfflinePageFromWebContents(
             web_contents);

(Also, side remark... that name is unfortunate in its redundancy.  If we already
had a namespace for "offline_pages", then we presumably didn't need an
OfflinePageUtils class inside it at all, and if we did, it could have been
called just Utils, since "offline page" is implied by the enclosing namespace. 
Feel free to fix separately!)

[cjgrant, 2017-07-06 15:04:19]

On 2017/07/06 06:08:08, Peter Kasting wrote:
> The code is OK, but I suggest you follow your own proposed change now and
simply
> modify the icon and text rather than returning this separate bit at all,
unless
> you actually need the separate bit for some other reason.

I'll proceed with the bigger change as suggested.  However, I think we still
want the IsOfflinePage() method, in addition to the icon and text.  If we were
to introduce a new security_level representing OFFLINE, then the need for the
extra method goes away, but I'm not sure that's a value that belongs in the
enum.

The specific reason is that VR doesn't show the security chip by default.  We
want to selectively show it only if there's a danger.  "Offline" has been
flagged as needing the text, so I need to be able to reliably detect this
condition.

[Peter Kasting, 2017-07-06 16:08:25]

On 2017/07/06 15:04:19, cjgrant wrote:
> On 2017/07/06 06:08:08, Peter Kasting wrote:
> > The code is OK, but I suggest you follow your own proposed change now and
> simply
> > modify the icon and text rather than returning this separate bit at all,
> unless
> > you actually need the separate bit for some other reason.
> 
> I'll proceed with the bigger change as suggested.  However, I think we still
> want the IsOfflinePage() method, in addition to the icon and text.  If we were
> to introduce a new security_level representing OFFLINE, then the need for the
> extra method goes away, but I'm not sure that's a value that belongs in the
> enum.
> 
> The specific reason is that VR doesn't show the security chip by default.  We
> want to selectively show it only if there's a danger.  "Offline" has been
> flagged as needing the text, so I need to be able to reliably detect this
> condition.

I agree that as described, the SecurityLevel enum should not get a state for
this.  For views, the kind of logic you're describing currently lives in
LocationBarView::ShouldShowLocationIconText().

[cjgrant, 2017-07-06 16:20:57]

On 2017/07/06 16:08:25, Peter Kasting wrote:
> On 2017/07/06 15:04:19, cjgrant wrote:
> > On 2017/07/06 06:08:08, Peter Kasting wrote:
> > > The code is OK, but I suggest you follow your own proposed change now and
> > simply
> > > modify the icon and text rather than returning this separate bit at all,
> > unless
> > > you actually need the separate bit for some other reason.
> > 
> > I'll proceed with the bigger change as suggested.  However, I think we still
> > want the IsOfflinePage() method, in addition to the icon and text.  If we
were
> > to introduce a new security_level representing OFFLINE, then the need for
the
> > extra method goes away, but I'm not sure that's a value that belongs in the
> > enum.
> > 
> > The specific reason is that VR doesn't show the security chip by default. 
We
> > want to selectively show it only if there's a danger.  "Offline" has been
> > flagged as needing the text, so I need to be able to reliably detect this
> > condition.
> 
> I agree that as described, the SecurityLevel enum should not get a state for
> this.  For views, the kind of logic you're describing currently lives in
> LocationBarView::ShouldShowLocationIconText().

Peter, I see a snag with doing this in the same CL.  Our Offline chip fix is for
M-61.  But to have ToolbarModel supply the text, the string needs to move from
an Android string file to compoments/toolbar.  As far as I know, this is a
string change, which would have had to be in by feature freeze.  To keep risk
low, may I proceed as this CL is, land our visual side to complete the fix, then
refactor it into ToolbarModel separately?

[Peter Kasting, 2017-07-06 16:26:06]

On 2017/07/06 16:20:57, cjgrant wrote:
> On 2017/07/06 16:08:25, Peter Kasting wrote:
> > On 2017/07/06 15:04:19, cjgrant wrote:
> > > On 2017/07/06 06:08:08, Peter Kasting wrote:
> > > > The code is OK, but I suggest you follow your own proposed change now
and
> > > simply
> > > > modify the icon and text rather than returning this separate bit at all,
> > > unless
> > > > you actually need the separate bit for some other reason.
> > > 
> > > I'll proceed with the bigger change as suggested.  However, I think we
still
> > > want the IsOfflinePage() method, in addition to the icon and text.  If we
> were
> > > to introduce a new security_level representing OFFLINE, then the need for
> the
> > > extra method goes away, but I'm not sure that's a value that belongs in
the
> > > enum.
> > > 
> > > The specific reason is that VR doesn't show the security chip by default. 
> We
> > > want to selectively show it only if there's a danger.  "Offline" has been
> > > flagged as needing the text, so I need to be able to reliably detect this
> > > condition.
> > 
> > I agree that as described, the SecurityLevel enum should not get a state for
> > this.  For views, the kind of logic you're describing currently lives in
> > LocationBarView::ShouldShowLocationIconText().
> 
> Peter, I see a snag with doing this in the same CL.  Our Offline chip fix is
for
> M-61.  But to have ToolbarModel supply the text, the string needs to move from
> an Android string file to compoments/toolbar.  As far as I know, this is a
> string change, which would have had to be in by feature freeze.  To keep risk
> low, may I proceed as this CL is, land our visual side to complete the fix,
then
> refactor it into ToolbarModel separately?

Yeah, that sounds like a good plan as far as being able to merge properly.

[cjgrant, 2017-07-06 17:04:03]

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

[commit-bot: I haz the power, 2017-07-06 17:04:24]

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-07-06 17:21:10]

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

[commit-bot: I haz the power, 2017-07-06 17:21:12]

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

[cjgrant, 2017-07-06 18:06:22]

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

[commit-bot: I haz the power, 2017-07-06 18:06:41]

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-07-06 18:50:25]

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

[commit-bot: I haz the power, 2017-07-06 18:50:26]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[cjgrant, 2017-07-06 19:56:34]

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

[cjgrant, 2017-07-06 19:56:35]

rohitrao@chromium.org: Please review changes in 

ios (the ToolbarModelDelegateIOS stub)

[cjgrant, 2017-07-06 20:13:58]

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

[commit-bot: I haz the power, 2017-07-06 20:14:21]

Dry run: CQ is trying da patch.

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

[Ian Vollick, 2017-07-06 20:38:18]

On 2017/07/06 19:56:35, cjgrant wrote:
> mailto:rohitrao@chromium.org: Please review changes in 
> 
> ios (the ToolbarModelDelegateIOS stub)

lgtm!

[cjgrant, 2017-07-06 20:50:26]

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

[cjgrant, 2017-07-06 20:50:27]

Justin, also adding you for:

ios (the ToolbarModelDelegateIOS stub)

[justincohen, 2017-07-06 21:44:08]

ToolbarModelDelegateIOS LGTM

[commit-bot: I haz the power, 2017-07-06 21:56:24]

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

[commit-bot: I haz the power, 2017-07-06 21:56:25]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[cjgrant, 2017-07-07 01:01:15]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-07-07 01:01:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2965043003/#ps60001
(title: "And, ios-simulator.")

[commit-bot: I haz the power, 2017-07-07 01:01:36]

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-07-07 02:01:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499389275696270,
"parent_rev": "0e569358d483e0c2b8eb1a4f4868267589d96d17", "commit_rev":
"d0af1dee22043449ff2076b1ba361a47b1803b3d"}

[commit-bot: I haz the power, 2017-07-07 02:01:52]

Description was changed from

==========
ToolbarModel: Supply offline page status.

This allows users of ToolbarModel to determine whether the current page
is an offline page or not.  In the future, it'd be interesting if the
security level, vector icon and verbose security text could simply
indicate that the page is offline, rather than using a separate bit of
information.

BUG=735770
==========

to

==========
ToolbarModel: Supply offline page status.

This allows users of ToolbarModel to determine whether the current page
is an offline page or not.  In the future, it'd be interesting if the
security level, vector icon and verbose security text could simply
indicate that the page is offline, rather than using a separate bit of
information.

BUG=735770

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

[commit-bot: I haz the power, 2017-07-07 02:01:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d0af1dee22043449ff2076b1ba36...