Add support for virtual files to Clank.

Add support for virtual files to Clank.

If a virtual file is selected, then coerce it to the first streamable
MIME type. As a result, users will be able to pick Google Docs documents
from Drive as PDF files, as in the past.

TEST=Tested manually by picking a hosted document from Drive with
     enabled support for virtual files.
BUG=b/33185812

Committed: https://crrev.com/3be618619e8365afe55130c44da9a24941cc34c4
Cr-Commit-Position: refs/heads/master@{#439678}

[mtomasz, 2016-12-05 02:05:10]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 02:05:18]

Dry run: 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-05 02:53:11]

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

[commit-bot: I haz the power, 2016-12-05 02:53:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mtomasz, 2016-12-06 01:42:25]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 01:42:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtomasz, 2016-12-06 01:44:01]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 01:44:32]

Dry run: 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-06 02:14:09]

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

[commit-bot: I haz the power, 2016-12-06 02:14:10]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mtomasz, 2016-12-06 02:21:36]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 02:21:58]

Dry run: 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-06 03:06:05]

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

[commit-bot: I haz the power, 2016-12-06 03:06:06]

Dry run: This issue passed the CQ dry run.

[mtomasz, 2016-12-07 01:17:59]

Description was changed from

==========
Add support for virtual files to Clank.

If a virtual file is selected, then coerce it to the first streamable
MIME type. As a result, users will be able to pick Google Docs documents
from Drive as PDF files, as in the past.

TEST=Tested manually by picking a hosted document from Drive with
     enabled support for virtual files.
BUG=b/33185812
==========

to

==========
Add support for virtual files to Clank.

If a virtual file is selected, then coerce it to the first streamable
MIME type. As a result, users will be able to pick Google Docs documents
from Drive as PDF files, as in the past.

TEST=Tested manually by picking a hosted document from Drive with
     enabled support for virtual files.
BUG=b/33185812
==========

[mtomasz, 2016-12-07 01:17:59]

mtomasz@chromium.org changed reviewers:
+ agrieve@chromium.org, jam@chromium.org, miguelg@chromium.org,
rmcilroy@chromium.org

[mtomasz, 2016-12-07 01:18:32]

miguelg@, jam@, agrieve@, rmcilroy@: PTAL at:

miguelg:
 
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java
 
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
  ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
  ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java
  ui/android/java/src/org/chromium/ui/base/WindowAndroid.java
jam:
 
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
  base/android/java/src/org/chromium/base/ContentUriUtils.java
  build/android/AndroidManifest.xml
 
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
 
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
  ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
  ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java
  ui/android/java/src/org/chromium/ui/base/WindowAndroid.java
agrieve:
  base/android/java/src/org/chromium/base/ContentUriUtils.java
  build/android/AndroidManifest.xml
rmcilroy:
  base/android/java/src/org/chromium/base/ContentUriUtils.java

Thanks!

[mtomasz, 2016-12-07 01:23:01]

mtomasz@chromium.org changed reviewers:
- agrieve@chromium.org

[mtomasz, 2016-12-07 01:23:02]

git cl owners seems to have a bad day.

PTAL at the following files:

miguelg:
 
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java
 
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
  ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
  ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java
  ui/android/java/src/org/chromium/ui/base/WindowAndroid.java

jam:
 
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
 
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java

agrieve:
  base/android/java/src/org/chromium/base/ContentUriUtils.java
  build/android/AndroidManifest.xml

Thanks!

[mtomasz, 2016-12-07 01:24:58]

Now rietveld breaks formatting.

PTAL at:

miguelg:
  chrome/android/*
  ui/android/*

jam:
  android_webview/*
  content/public/android/*

agrieve:
  base/android/*
  build/android/*

[agrieve, 2016-12-07 16:44:53]

agrieve@chromium.org changed reviewers:
+ agrieve@chromium.org

[agrieve, 2016-12-07 16:44:54]

Did an initial review. After staring at ContentUriUtils for a while though, I
worry that its api will cause clients to make excessive ContentResolver calls.
E.g. Maybe it would be better to have it expose just a single function that
returns a struct?

Not saying I would block this CL on such a refactoring, but figured I'd share
the thought.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:96: if (asf !=
null) {
nit: Use StreamUtil.closeQuietly(asf)

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:139: if
(streamTypes != null) {
should we check for empty array here as well?

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:146: // Closing
quietly.
StreamUtil.closeQuietly()

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:205: if (mimeTypes
!= null) {
check non-empty?

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:236:
@SuppressLint("NewApi")
Do you need to add Build.VERSION check somewhere below?

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:237: public static
boolean isVirtualDocument(Uri uri, Context context) {
Doesn't look like this is used outside of this class. Let's make it private
until it's required.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:238: if (uri ==
null) return false;
nit: Make Uri @Nullable and state it can be null in the javadoc. But likely even
better would be to change this to an assert.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:259: if (cursor !=
null) cursor.close();
StreamUtil.closeQuietly()

https://codereview.chromium.org/2545343002/diff/80001/build/android/AndroidMa...
File build/android/AndroidManifest.xml (right):

https://codereview.chromium.org/2545343002/diff/80001/build/android/AndroidMa...
build/android/AndroidManifest.xml:18: <uses-sdk android:minSdkVersion="16"
android:targetSdkVersion="24" />
This could affect other things. Better to do it as a separate commit.

https://codereview.chromium.org/2545343002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/2545343002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:497: * @param
context Application context.
No need to ever pass around the application context. It's available as
ContextUtils.getApplicationContext().

[jam, 2016-12-07 20:20:44]

On 2016/12/07 01:18:32, mtomasz wrote:
> miguelg@, jam@, agrieve@, rmcilroy@: PTAL at:
> 
> miguelg:
>  
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java
>  
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
>   ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
>   ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java
>   ui/android/java/src/org/chromium/ui/base/WindowAndroid.java
> jam:
please get a more specific owner for these files
>  
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
>   base/android/java/src/org/chromium/base/ContentUriUtils.java
>   build/android/AndroidManifest.xml
>  
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
>  
>
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
>   ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
>   ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java
>   ui/android/java/src/org/chromium/ui/base/WindowAndroid.java
> agrieve:
>   base/android/java/src/org/chromium/base/ContentUriUtils.java
>   build/android/AndroidManifest.xml
> rmcilroy:
>   base/android/java/src/org/chromium/base/ContentUriUtils.java
> 
> Thanks!

[mtomasz, 2016-12-08 05:43:04]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

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

Dry run: 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-08 06:31:00]

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

[commit-bot: I haz the power, 2016-12-08 06:31:01]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[mtomasz, 2016-12-08 06:36:37]

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:96: if (asf !=
null) {
On 2016/12/07 16:44:54, agrieve (OOO Dec 8) wrote:
> nit: Use StreamUtil.closeQuietly(asf)

Done.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:139: if
(streamTypes != null) {
On 2016/12/07 16:44:53, agrieve (OOO Dec 8) wrote:
> should we check for empty array here as well?

Done.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:146: // Closing
quietly.
On 2016/12/07 16:44:54, agrieve (OOO Dec 8) wrote:
> StreamUtil.closeQuietly()

Done.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:205: if (mimeTypes
!= null) {
On 2016/12/07 16:44:54, agrieve (OOO Dec 8) wrote:
> check non-empty?

Done.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:236:
@SuppressLint("NewApi")
On 2016/12/07 16:44:54, agrieve (OOO Dec 8) wrote:
> Do you need to add Build.VERSION check somewhere below?

I don't think so. All we needed from the newer API is the constant which is
resolved in compile time. So, the entire code will execute correctly on lower
API levels.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:237: public static
boolean isVirtualDocument(Uri uri, Context context) {
On 2016/12/07 16:44:53, agrieve (OOO Dec 8) wrote:
> Doesn't look like this is used outside of this class. Let's make it private
> until it's required.

Done.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:238: if (uri ==
null) return false;
On 2016/12/07 16:44:53, agrieve (OOO Dec 8) wrote:
> nit: Make Uri @Nullable and state it can be null in the javadoc. But likely
even
> better would be to change this to an assert.

I tried to be consistent with the rest of the file. If you feel strong about it,
I'm happy to fix it everywhere in this file in a follow up CL. Let me know.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:259: if (cursor !=
null) cursor.close();
On 2016/12/07 16:44:53, agrieve (OOO Dec 8) wrote:
> StreamUtil.closeQuietly()

Done.

https://codereview.chromium.org/2545343002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/2545343002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:497: * @param
context Application context.
On 2016/12/07 16:44:54, agrieve (OOO Dec 8) wrote:
> No need to ever pass around the application context. It's available as
> ContextUtils.getApplicationContext().

Done.

[mtomasz, 2016-12-08 06:43:32]

On 2016/12/07 16:44:54, agrieve (OOO Dec 8) wrote:
> Did an initial review. After staring at ContentUriUtils for a while though, I
> worry that its api will cause clients to make excessive ContentResolver calls.
> E.g. Maybe it would be better to have it expose just a single function that
> returns a struct?
> 
> Not saying I would block this CL on such a refactoring, but figured I'd share
> the thought.
> 

It's a valid concern, but reducing calls would require major refactoring. Also,
I don't think these calls are performance critical. Do we call them millions of
times per second? Finally, I'm not sure how many calls we'd reduce by
refactoring.

1. getAssetFileDescriptor is now 3 calls, instead of 1.
2. getDisplayName is now 2 calls, instead of 1.
3. getMimeType is now 2 calls, instead of 1.

[mtomasz, 2016-12-08 06:43:32]

On 2016/12/07 16:44:54, agrieve (OOO Dec 8) wrote:
> Did an initial review. After staring at ContentUriUtils for a while though, I
> worry that its api will cause clients to make excessive ContentResolver calls.
> E.g. Maybe it would be better to have it expose just a single function that
> returns a struct?
> 
> Not saying I would block this CL on such a refactoring, but figured I'd share
> the thought.
> 

It's a valid concern, but reducing calls would require major refactoring. Also,
I don't think these calls are performance critical. Do we call them millions of
times per second? Finally, I'm not sure how many calls we'd reduce by
refactoring.

1. getAssetFileDescriptor is now 3 calls, instead of 1.
2. getDisplayName is now 2 calls, instead of 1.
3. getMimeType is now 2 calls, instead of 1.

[mtomasz, 2016-12-08 07:04:40]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 07:04:54]

Dry run: 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-08 08:25:17]

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

[commit-bot: I haz the power, 2016-12-08 08:25:18]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jam, 2016-12-08 17:03:10]

jam@chromium.org changed reviewers:
- jam@chromium.org

[agrieve, 2016-12-09 18:23:14]

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:236:
@SuppressLint("NewApi")
On 2016/12/08 06:36:37, mtomasz wrote:
> On 2016/12/07 16:44:54, agrieve (OOO Dec 8) wrote:
> > Do you need to add Build.VERSION check somewhere below?
> 
> I don't think so. All we needed from the newer API is the constant which is
> resolved in compile time. So, the entire code will execute correctly on lower
> API levels.

Depending on a compiler optimization for correctness is a bad idea I think. If
virtual documents don't exist before the API the constant is added in, it's
probably a good idea to short-circuit anyways.

https://codereview.chromium.org/2545343002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:238: if (uri ==
null) return false;
On 2016/12/08 06:36:37, mtomasz wrote:
> On 2016/12/07 16:44:53, agrieve (OOO Dec 8) wrote:
> > nit: Make Uri @Nullable and state it can be null in the javadoc. But likely
> even
> > better would be to change this to an assert.
> 
> I tried to be consistent with the rest of the file. If you feel strong about
it,
> I'm happy to fix it everywhere in this file in a follow up CL. Let me know.

Yes please. Only getDisplayName() seems to check it btw, I don't see any other
function check it.

https://codereview.chromium.org/2545343002/diff/120001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/2545343002/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContentUriUtils.java:112: return
streamTypes != null ? streamTypes[0] : null;
Need to check for empty array here as well?

https://codereview.chromium.org/2545343002/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContentUriUtils.java:124:
@SuppressLint("NewApi")
Please remove the need for this as well. Looks like it's maybe caused by
openTypedAssetFileDescriptor? Changing to the 3-arg version might make lint
happy. Otherwise, you should add Build.VERSION checks rather than suppress lint.

https://codereview.chromium.org/2545343002/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContentUriUtils.java:188: boolean
isVirtual =
This part is duplicated in isVirtualDocument(). If we don't care about
performance here, then I think just call isVirtualDocument(). Otherwise, maybe
add another helper that both isVirtualDocument() and  this function call.

[mtomasz, 2016-12-12 03:33:35]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 03:33:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtomasz, 2016-12-12 03:53:56]

https://codereview.chromium.org/2545343002/diff/120001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/2545343002/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContentUriUtils.java:112: return
streamTypes != null ? streamTypes[0] : null;
On 2016/12/09 18:23:14, agrieve wrote:
> Need to check for empty array here as well?

Done.

https://codereview.chromium.org/2545343002/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContentUriUtils.java:124:
@SuppressLint("NewApi")
On 2016/12/09 18:23:14, agrieve wrote:
> Please remove the need for this as well. Looks like it's maybe caused by
> openTypedAssetFileDescriptor? Changing to the 3-arg version might make lint
> happy. Otherwise, you should add Build.VERSION checks rather than suppress
lint.

Done.

https://codereview.chromium.org/2545343002/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContentUriUtils.java:188: boolean
isVirtual =
On 2016/12/09 18:23:14, agrieve wrote:
> This part is duplicated in isVirtualDocument(). If we don't care about
> performance here, then I think just call isVirtualDocument(). Otherwise, maybe
> add another helper that both isVirtualDocument() and  this function call.

Done.

[commit-bot: I haz the power, 2016-12-12 04:30:39]

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

[commit-bot: I haz the power, 2016-12-12 04:30:40]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-12-12 18:33:41]

lgtm /w one nit

https://codereview.chromium.org/2545343002/diff/140001/build/android/AndroidM...
File build/android/AndroidManifest.xml (right):

https://codereview.chromium.org/2545343002/diff/140001/build/android/AndroidM...
build/android/AndroidManifest.xml:18: <uses-sdk android:minSdkVersion="16"
android:targetSdkVersion="24" />
Please make a separate CL to bump this number.

[mtomasz, 2016-12-13 08:19:25]

https://codereview.chromium.org/2545343002/diff/140001/build/android/AndroidM...
File build/android/AndroidManifest.xml (right):

https://codereview.chromium.org/2545343002/diff/140001/build/android/AndroidM...
build/android/AndroidManifest.xml:18: <uses-sdk android:minSdkVersion="16"
android:targetSdkVersion="24" />
On 2016/12/12 18:33:41, agrieve wrote:
> Please make a separate CL to bump this number.

This CL needs a bump, and bump is not needed without this CL, so what's the
purpose of splitting this into a separate CL?

[agrieve, 2016-12-13 14:46:55]

https://codereview.chromium.org/2545343002/diff/140001/build/android/AndroidM...
File build/android/AndroidManifest.xml (right):

https://codereview.chromium.org/2545343002/diff/140001/build/android/AndroidM...
build/android/AndroidManifest.xml:18: <uses-sdk android:minSdkVersion="16"
android:targetSdkVersion="24" />
On 2016/12/13 08:19:25, mtomasz wrote:
> On 2016/12/12 18:33:41, agrieve wrote:
> > Please make a separate CL to bump this number.
> 
> This CL needs a bump, and bump is not needed without this CL, so what's the
> purpose of splitting this into a separate CL?

There's code in the Android runtime that looks like
if (app.targetSdk < ##) {
  // do one thing
} else {
  // do another.
}

So I think it's important to have the CL description be "Update targetSdk
version", so that if bumping the targetSdk breaks anything, it'll be obvious
when looking at git log --oneline.

[mtomasz, 2016-12-15 02:09:45]

mtomasz@chromium.org changed reviewers:
+ torne@chromium.org

[mtomasz, 2016-12-15 02:15:05]

mtomasz@chromium.org changed reviewers:
+ boliu@chromium.org

[mtomasz, 2016-12-15 02:16:19]

@Torne: PTAL at
	android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java

@boliu: PTAL at
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java

Thanks

[mtomasz, 2016-12-15 02:16:33]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 02:16:48]

Dry run: 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-15 02:22:06]

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

[commit-bot: I haz the power, 2016-12-15 02:22:07]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Torne, 2016-12-15 16:24:03]

android_webview LGTM, one nit in other code I happened to spot

https://codereview.chromium.org/2545343002/diff/160001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (left):

https://codereview.chromium.org/2545343002/diff/160001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContentUriUtils.java:135: * @param
columnField the column field to query.
nit: you probably didn't mean to remove the documentation for columnField?

[boliu, 2016-12-15 18:12:07]

content lgtm

[mtomasz, 2016-12-16 03:25:31]

https://codereview.chromium.org/2545343002/diff/160001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (left):

https://codereview.chromium.org/2545343002/diff/160001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContentUriUtils.java:135: * @param
columnField the column field to query.
On 2016/12/15 16:24:03, Torne wrote:
> nit: you probably didn't mean to remove the documentation for columnField?

Done.

[mtomasz, 2016-12-16 03:25:34]

The CQ bit was checked by mtomasz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 03:26:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtomasz, 2016-12-16 03:29:26]

mtomasz@chromium.org changed reviewers:
- rmcilroy@chromium.org

[mtomasz, 2016-12-16 03:30:02]

@miguelg: PTAL at:
  chrome/android/*
  ui/android/*

Thanks.

[commit-bot: I haz the power, 2016-12-16 04:38:52]

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

[commit-bot: I haz the power, 2016-12-16 04:38:53]

Dry run: This issue passed the CQ dry run.

[Miguel Garcia, 2016-12-16 10:24:52]

On 2016/12/16 03:30:02, mtomasz wrote:
> @miguelg: PTAL at:
>   chrome/android/*
>   ui/android/*
> 
> Thanks.

I am OOO till the new year. Please pick another reviewer (tedchoc@ or
dfalcantara@ come to mind)

[mtomasz, 2016-12-19 03:37:53]

mtomasz@chromium.org changed reviewers:
+ tedchoc@chromium.org
- miguelg@chromium.org

[mtomasz, 2016-12-19 03:37:53]

mtomasz@chromium.org changed reviewers:
+ tedchoc@chromium.org
- miguelg@chromium.org

[mtomasz, 2016-12-19 03:38:53]

mtomasz@chromium.org changed reviewers:
+ dfalcantara@chromium.org
- tedchoc@chromium.org

[mtomasz, 2016-12-19 03:40:08]

@dfalcantara: PTAL at chrome/android/*. Thanks!

[mtomasz, 2016-12-19 03:41:19]

mtomasz@chromium.org changed reviewers:
+ twellington@chromium.org

[mtomasz, 2016-12-19 03:41:34]

@twellington: PTAL at ui/android/*. Thanks!

[Theresa, 2016-12-19 17:02:34]

lgtm

[Theresa, 2016-12-19 17:05:49]

https://codereview.chromium.org/2545343002/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right):

https://codereview.chromium.org/2545343002/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:194:
getContentIntent.addCategory(Intent.CATEGORY_OPENABLE);
Can you please add a test for this to SelectFileDialogTest.java in a follow up
CL?

[gone, 2016-12-19 18:39:26]

lgtm for mechanical changes to those files

[mtomasz, 2016-12-20 01:01:55]

https://codereview.chromium.org/2545343002/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right):

https://codereview.chromium.org/2545343002/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:194:
getContentIntent.addCategory(Intent.CATEGORY_OPENABLE);
On 2016/12/19 17:05:49, Theresa Wellington wrote:
> Can you please add a test for this to SelectFileDialogTest.java in a follow up
> CL?

Sure!

[mtomasz, 2016-12-20 01:02:13]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2016-12-20 01:02:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, torne@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2545343002/#ps180001
(title: "Fixed comment.")

[commit-bot: I haz the power, 2016-12-20 01:03:01]

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-20 01:47:48]

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

[commit-bot: I haz the power, 2016-12-20 01:47:49]

Failed to apply patch for build/android/AndroidManifest.xml:
While running git apply --index -p1;
  error: patch failed: build/android/AndroidManifest.xml:15
  error: build/android/AndroidManifest.xml: patch does not apply

Patch:       build/android/AndroidManifest.xml
Index: build/android/AndroidManifest.xml
diff --git a/build/android/AndroidManifest.xml
b/build/android/AndroidManifest.xml
index
143de62e8e96732087dba508639946bf9e5decab..2c469aa01aac67ea06f747a5dc5d5f4ba206c883
100644
--- a/build/android/AndroidManifest.xml
+++ b/build/android/AndroidManifest.xml
@@ -15,6 +15,6 @@
 <manifest xmlns:android="http://schemas.android.com/apk/res/android"
     package="dummy.package">
 
-    <uses-sdk android:minSdkVersion="16" android:targetSdkVersion="23" />
+    <uses-sdk android:minSdkVersion="16" android:targetSdkVersion="24" />
 
 </manifest>

[mtomasz, 2016-12-20 01:54:49]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2016-12-20 01:54:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, boliu@chromium.org, dfalcantara@chromium.org,
torne@chromium.org, agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2545343002/#ps200001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-12-20 01:55:06]

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-20 02:41:35]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1482198889633420,
"parent_rev": "b23b8f04a5c9a306a2ee802ebfc64dc11e67c132", "commit_rev":
"87853db33b4cde9bea0d9feb593e7c2cc2a3035f"}

[commit-bot: I haz the power, 2016-12-20 02:42:07]

Description was changed from

==========
Add support for virtual files to Clank.

If a virtual file is selected, then coerce it to the first streamable
MIME type. As a result, users will be able to pick Google Docs documents
from Drive as PDF files, as in the past.

TEST=Tested manually by picking a hosted document from Drive with
     enabled support for virtual files.
BUG=b/33185812
==========

to

==========
Add support for virtual files to Clank.

If a virtual file is selected, then coerce it to the first streamable
MIME type. As a result, users will be able to pick Google Docs documents
from Drive as PDF files, as in the past.

TEST=Tested manually by picking a hosted document from Drive with
     enabled support for virtual files.
BUG=b/33185812

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

[commit-bot: I haz the power, 2016-12-20 02:42:08]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-12-20 02:44:51]

Description was changed from

==========
Add support for virtual files to Clank.

If a virtual file is selected, then coerce it to the first streamable
MIME type. As a result, users will be able to pick Google Docs documents
from Drive as PDF files, as in the past.

TEST=Tested manually by picking a hosted document from Drive with
     enabled support for virtual files.
BUG=b/33185812

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

to

==========
Add support for virtual files to Clank.

If a virtual file is selected, then coerce it to the first streamable
MIME type. As a result, users will be able to pick Google Docs documents
from Drive as PDF files, as in the past.

TEST=Tested manually by picking a hosted document from Drive with
     enabled support for virtual files.
BUG=b/33185812

Committed: https://crrev.com/3be618619e8365afe55130c44da9a24941cc34c4
Cr-Commit-Position: refs/heads/master@{#439678}
==========

[commit-bot: I haz the power, 2016-12-20 02:44:52]

Patchset 11 (id:??) landed as
https://crrev.com/3be618619e8365afe55130c44da9a24941cc34c4
Cr-Commit-Position: refs/heads/master@{#439678}