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

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

BUG=546208
Review-Url: https://codereview.chromium.org/2816213002
Cr-Commit-Position: refs/heads/master@{#476411}
Committed: https://chromium.googlesource.com/chromium/src/+/3e6bc59e946bac1b9829d21df6d03481718f12fd

[kkhorimoto, 2017-04-14 20:35:23]

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

[kkhorimoto, 2017-04-14 20:35:24]

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:34: return
[[ShareToData alloc] initWithURL:tab.lastCommittedURL
If a user is attempting to share a page, we can assume they're interested in
sharing the visible content, not a pending load.

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

lgtm

[sdefresne, 2017-04-18 15:56:49]

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:34: return
[[ShareToData alloc] initWithURL:tab.lastCommittedURL
On 2017/04/14 20:35:24, kkhorimoto_ wrote:
> If a user is attempting to share a page, we can assume they're interested in
> sharing the visible content, not a pending load.

I personally frequently share pages that are still loading (e.g. sending to
"Instapaper" pages that are slow to load because they are under heavy load
because they were featured on a high visibility site like reddit). I think we
should test pending load here.

[Eugene But (OOO till 7-30), 2017-04-18 16:16:47]

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:34: return
[[ShareToData alloc] initWithURL:tab.lastCommittedURL
On 2017/04/18 15:56:49, sdefresne wrote:
> On 2017/04/14 20:35:24, kkhorimoto_ wrote:
> > If a user is attempting to share a page, we can assume they're interested in
> > sharing the visible content, not a pending load.
> 
> I personally frequently share pages that are still loading (e.g. sending to
> "Instapaper" pages that are slow to load because they are under heavy load
> because they were featured on a high visibility site like reddit). I think we
> should test pending load here.
I think we should be consistent with other features and platforms. Bookmarks,
f.e. use lastCommitted URL on Desktop and this is something that I would expect
from iOS as well. Because different people may have different opinions, how
about we start a team discussion on this topic? This is product level decision,
not engineering decision.

[kkhorimoto, 2017-05-31 22:12:36]

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:34: return
[[ShareToData alloc] initWithURL:tab.lastCommittedURL
On 2017/04/18 16:16:47, Eugene But wrote:
> On 2017/04/18 15:56:49, sdefresne wrote:
> > On 2017/04/14 20:35:24, kkhorimoto_ wrote:
> > > If a user is attempting to share a page, we can assume they're interested
in
> > > sharing the visible content, not a pending load.
> > 
> > I personally frequently share pages that are still loading (e.g. sending to
> > "Instapaper" pages that are slow to load because they are under heavy load
> > because they were featured on a high visibility site like reddit). I think
we
> > should test pending load here.
> I think we should be consistent with other features and platforms. Bookmarks,
> f.e. use lastCommitted URL on Desktop and this is something that I would
expect
> from iOS as well. Because different people may have different opinions, how
> about we start a team discussion on this topic? This is product level
decision,
> not engineering decision.

I agree with Eugene that it should be last committed, but I've sent an email to
bling-team@ for more discussion since this is a product-level decision.

[kkhorimoto, 2017-06-01 19:52:23]

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2816213002/diff/1/ios/chrome/browser/ui/activ...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:34: return
[[ShareToData alloc] initWithURL:tab.lastCommittedURL
On 2017/05/31 22:12:36, kkhorimoto_ wrote:
> On 2017/04/18 16:16:47, Eugene But wrote:
> > On 2017/04/18 15:56:49, sdefresne wrote:
> > > On 2017/04/14 20:35:24, kkhorimoto_ wrote:
> > > > If a user is attempting to share a page, we can assume they're
interested
> in
> > > > sharing the visible content, not a pending load.
> > > 
> > > I personally frequently share pages that are still loading (e.g. sending
to
> > > "Instapaper" pages that are slow to load because they are under heavy load
> > > because they were featured on a high visibility site like reddit). I think
> we
> > > should test pending load here.
> > I think we should be consistent with other features and platforms.
Bookmarks,
> > f.e. use lastCommitted URL on Desktop and this is something that I would
> expect
> > from iOS as well. Because different people may have different opinions, how
> > about we start a team discussion on this topic? This is product level
> decision,
> > not engineering decision.
> 
> I agree with Eugene that it should be last committed, but I've sent an email
to
> bling-team@ for more discussion since this is a product-level decision.

Changed to visible per discussion on email thread.

[kkhorimoto, 2017-06-01 19:52:27]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-06-01 19:52:27]

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/2816213002/#ps20001
(title: "use visible URL")

[commit-bot: I haz the power, 2017-06-01 19:53:28]

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:27:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496346747345780,
"parent_rev": "4ffd63b5892e5a92f7fecb1a873883f406bb8033", "commit_rev":
"3e6bc59e946bac1b9829d21df6d03481718f12fd"}

[commit-bot: I haz the power, 2017-06-01 20:27:39]

Description was changed from

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

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

to

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

BUG=546208

Review-Url: https://codereview.chromium.org/2816213002
Cr-Commit-Position: refs/heads/master@{#476411}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e6bc59e946bac1b9829d21df6d0...
==========

[commit-bot: I haz the power, 2017-06-01 20:27:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3e6bc59e946bac1b9829d21df6d0...

[rohitrao (ping after 24h), 2017-08-15 15:24:39]

https://codereview.chromium.org/2816213002/diff/20001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2816213002/diff/20001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:34: return
[[ShareToData alloc] initWithURL:tab.visibleURL
I was looking at this code today and it seems weird that we use the URL from the
visible item, but the title from the lastCommittedItem.

This inconsistency is bad, right?

[Eugene But (OOO till 7-30), 2017-08-15 17:12:34]

https://codereview.chromium.org/2816213002/diff/20001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2816213002/diff/20001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:34: return
[[ShareToData alloc] initWithURL:tab.visibleURL
On 2017/08/15 15:24:39, rohitrao (ping after 24h) wrote:
> I was looking at this code today and it seems weird that we use the URL from
the
> visible item, but the title from the lastCommittedItem.
> 
> This inconsistency is bad, right?
Good catch. tab.title will always be the title of the currently rendered page.
tab.visibleURL can be a URL of the pending page.

[kkhorimoto (Do not use), 2017-08-15 17:53:15]

kkhorimoto@google.com changed reviewers:
+ kkhorimoto@google.com

[kkhorimoto (Do not use), 2017-08-15 17:53:16]

https://codereview.chromium.org/2816213002/diff/20001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2816213002/diff/20001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:34: return
[[ShareToData alloc] initWithURL:tab.visibleURL
On 2017/08/15 17:12:33, Eugene But wrote:
> On 2017/08/15 15:24:39, rohitrao (ping after 24h) wrote:
> > I was looking at this code today and it seems weird that we use the URL from
> the
> > visible item, but the title from the lastCommittedItem.
> > 
> > This inconsistency is bad, right?
> Good catch. tab.title will always be the title of the currently rendered page.
> tab.visibleURL can be a URL of the pending page.
> 
> 

Since we agreed that it's most likely that the user's intent is to share the
visible URL, does it make sense to use GetTitleForDisplay() on the visible
NavigationItem?

[rohitrao (ping after 24h), 2017-08-15 20:20:06]

> Since we agreed that it's most likely that the user's intent is to share the
> visible URL, does it make sense to use GetTitleForDisplay() on the visible
> NavigationItem?

VisibleItem and LastCommittedItem are only different between when we start a
renderer-initiated request and when the server responds.  In that time period,
the title for the pending item will always be empty, so we'll fall back to
either the URL or Untitled.

There's nothing stopping us from doing this, just that the title will almost
certainly be nonexistent in this scenario.  We need to be consistent though, one
way or another.

[Eugene But (OOO till 7-30), 2017-08-15 20:51:35]

On 2017/08/15 20:20:06, rohitrao (ping after 24h) wrote:
> > Since we agreed that it's most likely that the user's intent is to share the
> > visible URL, does it make sense to use GetTitleForDisplay() on the visible
> > NavigationItem?
> 
> VisibleItem and LastCommittedItem are only different between when we start a
> renderer-initiated request and when the server responds.  In that time period,
> the title for the pending item will always be empty, so we'll fall back to
> either the URL or Untitled.
> 
> There's nothing stopping us from doing this, just that the title will almost
> certainly be nonexistent in this scenario.  We need to be consistent though,
one
> way or another.
Small correction. These are the cases when VisibleItem and LastCommittedItem are
different:
 - SSL interstitial is displayed
 - User-initiated pending load is in progress