Move the offline URL trimming to GetFormattedURL.

Move the offline URL trimming to GetFormattedURL.

Changing directly the URL may have side effects beside hiding the
scheme.
Move the formatting to GetFormattedURL.
Also refactor ToolbarModelIOS to inherit ToolbarModel.

BUG=674986
Committed: https://crrev.com/b65b86e30213aec954229d11b48663ca4f894f6e
Cr-Commit-Position: refs/heads/master@{#440955}

[Olivier, 2016-12-28 11:40:30]

olivierrobin@chromium.org changed reviewers:
+ eugenebut@chromium.org, noyau@chromium.org, rohitrao@chromium.org

[Eugene But (OOO till 7-30), 2016-12-28 17:03:00]

Could you please add some tests to toolbar_model_impl_ios_unittest. Other than
that lgtm and thanks a lot for refactoring!

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.cc (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.cc:80: online_url,
base::UTF8ToUTF16(base::StringPrintf(
Optional nit: For the purpose of your DCHECK this should be fully sufficient:

DCHECK(!base::StartsWith(online_url, base::UTF8ToUTF16(url::kHttpScheme));

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.h (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:36: base::string16
StripSchemeFromOnlineUrl(base::string16 online_url);
Optional nit: Camel case Url is correct per C++ Style Guide, but the rest of the
code in this file uses all caps URL. It's up toy you if you want to choose
correctness or consistency, I just wanted to bring your attention.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:36: base::string16
StripSchemeFromOnlineUrl(base::string16 online_url);
Should this be |const base::string16&online_url|?

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils_unittest.cc (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:86:
TEST(OfflineURLUtilsTest, StripSchemeFromOnlineUrlTest) {
Could you please add comments for test method.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:87: base::string16
empty_url = base::UTF8ToUTF16("");
Should this be just |base::string16 empty_url;|?

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.h (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.h:353: - (BOOL)isOffline;
Should this be a property, like
isLinkLoadingPrerenderTab/isVoiceSearchResultsTab:
@property(nonatomic, readonly, getter=isOffline) BOOL offline;

But I guess the bigger question is: "Should we even add anything to Tab, because
this class should be removed?".

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm:55: *url =
ShouldDisplayURL() ? item->GetVirtualURL() : GURL();
I know that you just reverting to the old code, but could you please
s/GURL()/GURL::EmptyURL()

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm:85: Tab* currentTab =
delegate_->GetCurrentTab();
s/currentTab/current_tab

Looks like existing code violates Style Guide.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm:86: if ([currentTab
isOffline] &&
If you choose not to add anything to Tab (which is totally up to you), then you
can do this:

reading_list::IsOfflineURL(currentTab.webState->GetVisibleURL())

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm:87:
GetSecurityLevel(true) == security_state::SecurityLevel::NONE) {
nit: GetSecurityLevel(true /*ignore_editing*/)

[Olivier, 2016-12-29 12:37:20]

Done, thanks

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.cc (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.cc:80: online_url,
base::UTF8ToUTF16(base::StringPrintf(
On 2016/12/28 17:02:59, Eugene But wrote:
> Optional nit: For the purpose of your DCHECK this should be fully sufficient:
> 
> DCHECK(!base::StartsWith(online_url, base::UTF8ToUTF16(url::kHttpScheme));

This would DCHECK on http://http.com.
I will keep the kStandardSchemeSeparator

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.h (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:36: base::string16
StripSchemeFromOnlineUrl(base::string16 online_url);
On 2016/12/28 17:02:59, Eugene But wrote:
> Should this be |const base::string16&online_url|?

Done.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils_unittest.cc (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:86:
TEST(OfflineURLUtilsTest, StripSchemeFromOnlineUrlTest) {
On 2016/12/28 17:02:59, Eugene But wrote:
> Could you please add comments for test method.

Done.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:87: base::string16
empty_url = base::UTF8ToUTF16("");
On 2016/12/28 17:02:59, Eugene But wrote:
> Should this be just |base::string16 empty_url;|?

Done.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab.h (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.h:353: - (BOOL)isOffline;
On 2016/12/28 17:02:59, Eugene But wrote:
> Should this be a property, like
> isLinkLoadingPrerenderTab/isVoiceSearchResultsTab:
> @property(nonatomic, readonly, getter=isOffline) BOOL offline;
> 
> But I guess the bigger question is: "Should we even add anything to Tab,
because
> this class should be removed?".

Removed

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm:55: *url =
ShouldDisplayURL() ? item->GetVirtualURL() : GURL();
On 2016/12/28 17:02:59, Eugene But wrote:
> I know that you just reverting to the old code, but could you please
> s/GURL()/GURL::EmptyURL()

Done.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm (right):

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm:85: Tab* currentTab =
delegate_->GetCurrentTab();
On 2016/12/28 17:02:59, Eugene But wrote:
> s/currentTab/current_tab
> 
> Looks like existing code violates Style Guide.

Done.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm:86: if ([currentTab
isOffline] &&
On 2016/12/28 17:03:00, Eugene But wrote:
> If you choose not to add anything to Tab (which is totally up to you), then
you
> can do this:
> 
> reading_list::IsOfflineURL(currentTab.webState->GetVisibleURL())

We need the URL, not the virtualURL.
Done.

https://codereview.chromium.org/2606873002/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm:87:
GetSecurityLevel(true) == security_state::SecurityLevel::NONE) {
On 2016/12/28 17:02:59, Eugene But wrote:
> nit: GetSecurityLevel(true /*ignore_editing*/)

Done.

[Olivier, 2016-12-29 12:37:25]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-12-29 12:37:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2606873002/#ps80001
(title: "cleaning")

[commit-bot: I haz the power, 2016-12-29 12:37:38]

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, 2016-12-29 12:43:24]

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

[commit-bot: I haz the power, 2016-12-29 12:43:25]

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...)

[Olivier, 2016-12-29 13:05:19]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-12-29 13:05:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2606873002/#ps100001
(title: "fix DEPS")

[commit-bot: I haz the power, 2016-12-29 13:05:25]

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, 2016-12-29 14:39:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483016719060810,
"parent_rev": "3b4b3fb13dcaf546f04e521379ddb28e1b667b27", "commit_rev":
"ab93188658ca47da0358a72514666b906517d1ed"}

[commit-bot: I haz the power, 2016-12-29 14:40:12]

Description was changed from

==========
Move the offline URL trimming to GetFormattedURL.

Changing directly the URL may have side effects beside hiding the
scheme.
Move the formatting to GetFormattedURL.
Also refactor ToolbarModelIOS to inherit ToolbarModel.

BUG=674986
==========

to

==========
Move the offline URL trimming to GetFormattedURL.

Changing directly the URL may have side effects beside hiding the
scheme.
Move the formatting to GetFormattedURL.
Also refactor ToolbarModelIOS to inherit ToolbarModel.

BUG=674986

Review-Url: https://codereview.chromium.org/2606873002
==========

[commit-bot: I haz the power, 2016-12-29 14:40:13]

Committed patchset #6 (id:100001)

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

Description was changed from

==========
Move the offline URL trimming to GetFormattedURL.

Changing directly the URL may have side effects beside hiding the
scheme.
Move the formatting to GetFormattedURL.
Also refactor ToolbarModelIOS to inherit ToolbarModel.

BUG=674986

Review-Url: https://codereview.chromium.org/2606873002
==========

to

==========
Move the offline URL trimming to GetFormattedURL.

Changing directly the URL may have side effects beside hiding the
scheme.
Move the formatting to GetFormattedURL.
Also refactor ToolbarModelIOS to inherit ToolbarModel.

BUG=674986

Committed: https://crrev.com/b65b86e30213aec954229d11b48663ca4f894f6e
Cr-Commit-Position: refs/heads/master@{#440955}
==========

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

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

[rohitrao (ping after 24h), 2017-01-06 13:16:35]

Found the CL I was looking for, but it took some digging =)

https://chromereviews.googleplex.com/19697013/

We made a conscious decision to not use inheritance here, because doing so is
incorrect.

The issue is that ToolbarModel::input_in_progress() is not a virtual function. 
This means that ToolbarModelIOSImpl now has two implementations of that
function, backed by two different ivars.  One version is called when the
compiler thinks you have a ToolbarModelIOS*, and the other is called when the
compiler thinks you have a ToolbarModel*.  There is nothing to keep the two in
sync.