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

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

BUG=546208
Review-Url: https://codereview.chromium.org/2824543002
Cr-Commit-Position: refs/heads/master@{#476168}
Committed: https://chromium.googlesource.com/chromium/src/+/64c801e7a62896e59ab8337ae53dd10c8b4d7126

[kkhorimoto, 2017-04-14 21:03:48]

visible => lastCommitted for |-webWillAddPendingURL:transition:|

[kkhorimoto, 2017-04-14 21:05:15]

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

[kkhorimoto, 2017-04-14 21:05:16]

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

https://codereview.chromium.org/2824543002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:593: self.visibleURL.spec().c_str()];
It seems to make the most sense to use the URL that would be displayed in the
omnibox.

https://codereview.chromium.org/2824543002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:668: self.visibleURL,
url_formatter::kFormatUrlOmitNothing,
It seems to make the most sense to use the URL that would be displayed in the
omnibox.

https://codereview.chromium.org/2824543002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:697: const GURL& url = self.visibleURL;
Since favicon fetching occurs during a load, we should be fetching the favicon
for the page that is being loaded.

https://codereview.chromium.org/2824543002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:897: base::SysNSStringToUTF16(title));
|title| is fetched via WebState::GetTitle(), which uses the last committed URL.

https://codereview.chromium.org/2824543002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:1182: tabURL:self.lastCommittedURL
If an external URL is being opened by a page, then we want the xcallback to
return to the page that is opening the URL.  Since a navigation needs to be
committed before opening a URL, we should use the last committed here.

https://codereview.chromium.org/2824543002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:1290: const GURL& committedURL =
self.lastCommittedURL;
If a file is being opened by a page, then we want the xcallback to
return to the page that is opening the URL.  Since a navigation needs to be
committed before opening a URL, we should use the last committed here.

https://codereview.chromium.org/2824543002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:1304:
self.lastCommittedURL.SchemeIs(kChromeUIScheme)) {
If we're counting a main frame load, then we can assume that the load has
finished and that the navigation is committed.

https://codereview.chromium.org/2824543002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab.mm:1365: if (url == self.visibleURL)
Reader mode HTML is loaded when a load is taking too long, so we should use the
visible URL to reflect the navigation that is occurring.

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

https://codereview.chromium.org/2824543002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1478: url != self.lastCommitted) {
If this condition passes the !GetPendingItem() check, then I think we can safely
use the last committed.  Technically, it could have been possible that there was
a transient item, but in practice this never occurs without there also being a
pending item.

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

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

https://codereview.chromium.org/2824543002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1365: if (url == self.visibleURL)
This checks that URL did not change, so lastCommittedURL seems to be more
appropriate.

https://codereview.chromium.org/2824543002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1478: url != self.lastCommitted) {
On 2017/04/14 21:05:16, kkhorimoto_ wrote:
> If this condition passes the !GetPendingItem() check, then I think we can
safely
> use the last committed.  Technically, it could have been possible that there
was
> a transient item, but in practice this never occurs without there also being a
> pending item.
I don't understand this code. Rohit, by any chance do you know what this this
does?

[kkhorimoto, 2017-05-31 22:45:46]

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

https://codereview.chromium.org/2824543002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1365: if (url == self.visibleURL)
On 2017/04/14 21:53:47, Eugene But wrote:
> This checks that URL did not change, so lastCommittedURL seems to be more
> appropriate.

Done.

https://codereview.chromium.org/2824543002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab.mm:1478: url != self.lastCommitted) {
On 2017/04/14 21:53:47, Eugene But wrote:
> On 2017/04/14 21:05:16, kkhorimoto_ wrote:
> > If this condition passes the !GetPendingItem() check, then I think we can
> safely
> > use the last committed.  Technically, it could have been possible that there
> was
> > a transient item, but in practice this never occurs without there also being
a
> > pending item.
> I don't understand this code. Rohit, by any chance do you know what this this
> does?

I don't understand super well either, but since the condition has already
calculated that there is no pending item, then the |url| property would always
be returning the last committed URL anyway, so I think we're safe here.

[Eugene But (OOO till 7-30), 2017-05-31 23:42:54]

lgtm

[kkhorimoto, 2017-06-01 03:16:01]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-06-01 03:16:14]

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 03:26:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496286961596460,
"parent_rev": "0b9abe6d9be48f1f92f708d46afdac2811134102", "commit_rev":
"64c801e7a62896e59ab8337ae53dd10c8b4d7126"}

[commit-bot: I haz the power, 2017-06-01 03:26:15]

Description was changed from

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

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

to

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

BUG=546208

Review-Url: https://codereview.chromium.org/2824543002
Cr-Commit-Position: refs/heads/master@{#476168}
Committed:
https://chromium.googlesource.com/chromium/src/+/64c801e7a62896e59ab8337ae53d...
==========

[commit-bot: I haz the power, 2017-06-01 03:26:16]

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