Add referrer through service connection in custom tabs

Add referrer through service connection in custom tabs

If there is a unique single package name that can be obtained
through custom tabs service and if the referrer has not been
explicitly set by the launching app, set it to a Uri that uses
the package name as authority(in line with Activity#getReferrer())

BUG=517528
Committed: https://crrev.com/55c0af75147dfb0bb434d7589288c856b64e9f5e
Cr-Commit-Position: refs/heads/master@{#344279}

[Yusuf, 2015-08-11 00:38:34]

yusufo@chromium.org changed reviewers:
+ lizeb@chromium.org, mariakhomenko@chromium.org

[Maria, 2015-08-11 01:02:57]

lgtm

Thanks for fixing my TODO

[Benoit L, 2015-08-11 08:46:06]

Thanks!

A few general comments:

- What about the prerendering case? In CustomTabsConnection#prerenderUrl(), a
referrer can be specified. It has to match the referrer we eventually use. So if
no referrer is passed to the mayLaunchUrl() call, we have to populate it, right?
- Does the test pass on devices before L MR1?
- Should we also have a test for the case where the application passes a
non-valid referrer?

https://codereview.chromium.org/1284743003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/1284743003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:382:
public String getPackageNameForSession(IBinder session) {
Javadoc. Especially since the behavior is not that trivial (with several
packages for a given UID, in particular).

[Yusuf, 2015-08-12 22:34:28]

Uploaded another patch with a few additions.



On 2015/08/11 08:46:06, Benoit L wrote:
> Thanks!
> 
> A few general comments:
> 
> - What about the prerendering case? In CustomTabsConnection#prerenderUrl(), a
> referrer can be specified. It has to match the referrer we eventually use. So
if
> no referrer is passed to the mayLaunchUrl() call, we have to populate it,
right?

Yes. Now I also create a constructed referrer for prerender and check against
that when necessary.


> - Does the test pass on devices before L MR1?

Yes, since we don't really use any L MR1 related APIs. (only
Activity#getReferrer) is L MR1 and these changes don't use that API.


> - Should we also have a test for the case where the application passes a
> non-valid referrer?

If they do pass a non-valid referrer within the extra, that is already covered
by IntentHandler tests.

I do feel a bit better if we had a test for adding the referrer to prerender as
well though. Thinking of how to build one. Not entirely sure it would be a part
of this CL. Suggestions there would be appreciated.


> 
>
https://codereview.chromium.org/1284743003/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
> (right):
> 
>
https://codereview.chromium.org/1284743003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:382:
> public String getPackageNameForSession(IBinder session) {
> Javadoc. Especially since the behavior is not that trivial (with several
> packages for a given UID, in particular).

[Benoit L, 2015-08-13 08:42:06]

On 2015/08/12 22:34:28, Yusuf wrote:
> Uploaded another patch with a few additions.
> 
> 
> 
> On 2015/08/11 08:46:06, Benoit L wrote:
> > Thanks!
> > 
> > A few general comments:
> > 
> > - What about the prerendering case? In CustomTabsConnection#prerenderUrl(),
a
> > referrer can be specified. It has to match the referrer we eventually use.
So
> if
> > no referrer is passed to the mayLaunchUrl() call, we have to populate it,
> right?
> 
> Yes. Now I also create a constructed referrer for prerender and check against
> that when necessary.
> 
> 
> > - Does the test pass on devices before L MR1?
> 
> Yes, since we don't really use any L MR1 related APIs. (only
> Activity#getReferrer) is L MR1 and these changes don't use that API.
> 
> 
> > - Should we also have a test for the case where the application passes a
> > non-valid referrer?
> 
> If they do pass a non-valid referrer within the extra, that is already covered
> by IntentHandler tests.
> 
> I do feel a bit better if we had a test for adding the referrer to prerender
as
> well though. Thinking of how to build one. Not entirely sure it would be a
part
> of this CL. Suggestions there would be appreciated.
> 
> 
> > 
> >
>
https://codereview.chromium.org/1284743003/diff/20001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1284743003/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:382:
> > public String getPackageNameForSession(IBinder session) {
> > Javadoc. Especially since the behavior is not that trivial (with several
> > packages for a given UID, in particular).

LGTM, thank you!

[Yusuf, 2015-08-13 22:24:03]

yusufo@chromium.org changed reviewers:
+ nyquist@chromium.org

[Yusuf, 2015-08-13 22:24:03]

nyquest for base/

[nyquist, 2015-08-18 21:57:31]

https://codereview.chromium.org/1284743003/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right):

https://codereview.chromium.org/1284743003/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:437: public
static Uri getReferrer(Activity activity) {
Is this even used?

[nyquist, 2015-08-19 09:35:56]

I guess you don't need my review anymore for this?

[Yusuf, 2015-08-19 17:11:21]

No.Sorry for the trouble

[Yusuf, 2015-08-19 17:11:25]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2015-08-19 17:11:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org, lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/1284743003/#ps60001
(title: "remvoe the ununsed api")

[commit-bot: I haz the power, 2015-08-19 17:12:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1284743003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1284743003/60001

[commit-bot: I haz the power, 2015-08-19 17:18:50]

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

[commit-bot: I haz the power, 2015-08-19 17:18:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yusuf, 2015-08-19 17:34:13]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2015-08-19 17:34:13]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/1284743003/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2015-08-19 17:34:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1284743003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1284743003/80001

[commit-bot: I haz the power, 2015-08-19 18:28:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-19 18:29:52]

Patchset 5 (id:??) landed as
https://crrev.com/55c0af75147dfb0bb434d7589288c856b64e9f5e
Cr-Commit-Position: refs/heads/master@{#344279}