Remove usage of Tab's |url| property from TabModel.

Remove usage of Tab's |url| property from TabModel.

BUG=546208
Review-Url: https://codereview.chromium.org/2823623002
Cr-Commit-Position: refs/heads/master@{#476404}
Committed: https://chromium.googlesource.com/chromium/src/+/2a192b8c579d1666bbe1fca1c5eda3ff9b6b3a32

[kkhorimoto, 2017-04-14 20:37:40]

kkhorimoto@chromium.org changed reviewers:
+ eugenebut@chromium.org, rohitrao@chromium.org, sdefresne@chromium.org

[kkhorimoto, 2017-04-14 20:37:41]

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:615: NSString* fileName =
base::SysUTF8ToNSString(URL.ExtractFileName());
A file is currently reference if it's in the content area, which would be the
last committed.

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:776: if (tab.lastCommittedURL ==
GURL(kChromeUINewTabURL)) {
Loads aren't occurring during session restoration, so last committed is
sufficient.

[Eugene But (OOO till 7-30), 2017-04-14 21:46:10]

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:615: NSString* fileName =
base::SysUTF8ToNSString(URL.ExtractFileName());
On 2017/04/14 20:37:40, kkhorimoto_ wrote:
> A file is currently reference if it's in the content area, which would be the
> last committed.
Should we include both lastCommitted and pending?

[sdefresne, 2017-04-18 15:54:34]

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:776: if (tab.lastCommittedURL ==
GURL(kChromeUINewTabURL)) {
On 2017/04/14 20:37:40, kkhorimoto_ wrote:
> Loads aren't occurring during session restoration, so last committed is
> sufficient.

This is also called when restoring a session after a crash. There can be a
pending tab if the user started a navigation and then immediately tapped the
"Restore tabs" button from the infobar. I think we also need to check pending
here as we do not want to close the pending tab in that case.

[kkhorimoto, 2017-05-31 22:30:04]

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:615: NSString* fileName =
base::SysUTF8ToNSString(URL.ExtractFileName());
On 2017/04/14 21:46:10, Eugene But wrote:
> On 2017/04/14 20:37:40, kkhorimoto_ wrote:
> > A file is currently reference if it's in the content area, which would be
the
> > last committed.
> Should we include both lastCommitted and pending?

Done.

https://codereview.chromium.org/2823623002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:776: if (tab.lastCommittedURL ==
GURL(kChromeUINewTabURL)) {
On 2017/04/18 15:54:34, sdefresne wrote:
> On 2017/04/14 20:37:40, kkhorimoto_ wrote:
> > Loads aren't occurring during session restoration, so last committed is
> > sufficient.
> 
> This is also called when restoring a session after a crash. There can be a
> pending tab if the user started a navigation and then immediately tapped the
> "Restore tabs" button from the infobar. I think we also need to check pending
> here as we do not want to close the pending tab in that case.

Done.

[Eugene But (OOO till 7-30), 2017-05-31 22:43:11]

https://codereview.chromium.org/2823623002/diff/20001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2823623002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model.mm:603: NSString* fileName =
base::SysUTF8ToNSString(URL.ExtractFileName());
Does it compile? I can't fine |URL| variable.

[kkhorimoto, 2017-06-01 17:55:33]

https://codereview.chromium.org/2823623002/diff/20001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2823623002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model.mm:603: NSString* fileName =
base::SysUTF8ToNSString(URL.ExtractFileName());
On 2017/05/31 22:43:11, Eugene But wrote:
> Does it compile? I can't fine |URL| variable.

Whoop, I think I accidentally reverted or something when trying to upload this. 
I uploaded an actual fix.

[Eugene But (OOO till 7-30), 2017-06-01 18:32:09]

lgtm

https://codereview.chromium.org/2823623002/diff/40001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2823623002/diff/40001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model.mm:605: if
(UrlIsExternalFileReference(pendingURL)) {
nit: How about |if (pendingItem &&
UrlIsExternalFileReference(pendingItem->GetURL()))| ?

[kkhorimoto, 2017-06-01 19:40:43]

test + formatting fixes

[kkhorimoto, 2017-06-01 19:42:12]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-06-01 19:42:12]

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/2823623002/#ps60001
(title: "test + formatting fixes")

[kkhorimoto, 2017-06-01 19:42:30]

https://codereview.chromium.org/2823623002/diff/40001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2823623002/diff/40001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model.mm:605: if
(UrlIsExternalFileReference(pendingURL)) {
On 2017/06/01 18:32:09, Eugene But wrote:
> nit: How about |if (pendingItem &&
> UrlIsExternalFileReference(pendingItem->GetURL()))| ?

Done.

[commit-bot: I haz the power, 2017-06-01 19:42:59]

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-01 20:11:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496346132247880,
"parent_rev": "02f951db0b77eddb5c896b36f494eb7407a899ff", "commit_rev":
"2a192b8c579d1666bbe1fca1c5eda3ff9b6b3a32"}

[commit-bot: I haz the power, 2017-06-01 20:11:19]

Description was changed from

==========
Remove usage of Tab's |url| property from TabModel.

BUG=546208
==========

to

==========
Remove usage of Tab's |url| property from TabModel.

BUG=546208

Review-Url: https://codereview.chromium.org/2823623002
Cr-Commit-Position: refs/heads/master@{#476404}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a192b8c579d1666bbe1fca1c5ed...
==========

[commit-bot: I haz the power, 2017-06-01 20:11:21]

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