Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2143133002:
Do screenshot capture async for share intents. (Closed)
DescriptionDo screenshot capture async for share intents.
The share intent is started with a delay because of screenshot capturing
and compressing. But, lot of times the other activity never uses the
screenshot. The screenshot capture, compress to jpeg and write to file
can be done in background when the user is selecting the app shared to.
This CL introduces a BlockingFileProvider that provides an empty file
uri without any contents and shares an intent. While sending the intent
it also triggers the screenshot capture. When the file is actually
requested by the other app, the file provider will wait till the capture
is done and the file is written. The blocking practically never happens
because the user takes some time to click on the app, before which the
capture is finished.
The BlockingFileProvider is made default file provider and the
CompatibilityFileProvider logic is added to it.
BUG=615229
Committed: https://crrev.com/ee36837ec9d6df950df4020f80ea702d599d666c
Cr-Commit-Position: refs/heads/master@{#417185}
Patch Set 1 : . #Patch Set 2 : fix. #Patch Set 3 : fix packagename #
Total comments: 30
Patch Set 4 : Fixes and add tests. #Patch Set 5 : fix. #Patch Set 6 : fixes and tests. #
Total comments: 2
Patch Set 7 : Remove AsyncTask. #
Total comments: 12
Patch Set 8 : Rebase and comments. #
Total comments: 2
Patch Set 9 : Call function on UI thread. #Patch Set 10 : Rebase. #
Total comments: 29
Patch Set 11 : Ted's comments. #Patch Set 12 : Fix StrictMode violation. #Patch Set 13 : rebase. #Patch Set 14 : Rebase + more comments addressed. #
Total comments: 24
Patch Set 15 : Comments and rebase. #
Total comments: 12
Patch Set 16 : Fixes. #
Total comments: 4
Patch Set 17 : move to util/ #Messages
Total messages: 90 (62 generated)
Patchset #1 (id:1) has been deleted
ssid@chromium.org changed reviewers: + nyquist@chromium.org
+nyquist ptal, thanks!
https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:20: * This class gives an unique identifier for the file to be shared and the user must write the file Could you clean up the usage of pronouns in this paragraph? In this line I believe 'user' refers to the caller, but later you refer to the user as the person that 'clicks' on something. "This class" or "The FileProvider" I think is OK for this class itself. Maybe the embedder or caller for the code in Chrome? And maybe the "client application" or something for the app that receives the screenshot? Anyway, being consistent is the important part. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:24: * can be shared at a given time. What happens if multiple files are shared? Is this a constraint from this class, from Chrome? Or from Android? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:26: public class BlockingFileProvider extends FileProvider { Could you add a test for this class? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:36: Uri uri = null; Nit: Could you just initialize this while building it? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:90: private static UriMatcher sUriMatcher = null; Where is this used? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:103: if (sUriMatcher != null) return; I don't think this is threadsafe, and it can be called from any thread. Maybe add thread checking or make it thread safe? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:118: while (!sIsFileReady && fileName.contains(sCurrentFileName)) { Should this be "endsWith" instead of "contains"? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:957: public void onShareMenuItemSelected(final boolean shareDirectly, boolean isIncognito) { Was there no test you needed to update for this? If so, could you please add one? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:979: final String fileName = String.valueOf(System.currentTimeMillis()); Will the user ever see this filename? If not, maybe we could use something that won't ever overlap? I was thinking about for example daylight savings time. It's a weird case though, but if it's not user-visible, maybe elapsedRealtime() or maybe even elapsedRealtimeNanos()? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:987: ShareHelper.onScreenshotReady(fileName, bitmap, mainActivity); Is mainActivity guaranteed to be alive here, or will ApplicationState.HAS_DESTROYED_ACTIVITIES catch that in the ShareHelper? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:993: currentTab.getWebContents().getContentBitmapAsync( Is it guaranteed that getWebContents() does not return null at this point? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:257: * The bitmap is compressed to jpeg before being written to the file. Nit: JPEG https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:266: BlockingFileProvider.notifyFileReady(fileName, null); Should this be posted as a task on the main thread, or is it OK to do it directly as a result of this method call? Below it's done as a task. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:300: if (ApplicationStatus.getStateForApplication() If the state is not correct, should notifyFileReady still be called, similarly to in the start of onScreenshotReady?
Patchset #5 (id:100001) has been deleted
Thanks, Made suggested changes. Ptal. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:20: * This class gives an unique identifier for the file to be shared and the user must write the file On 2016/07/14 17:36:32, nyquist wrote: > Could you clean up the usage of pronouns in this paragraph? In this line I > believe 'user' refers to the caller, but later you refer to the user as the > person that 'clicks' on something. > > "This class" or "The FileProvider" I think is OK for this class itself. > Maybe the embedder or caller for the code in Chrome? > And maybe the "client application" or something for the app that receives the > screenshot? > > Anyway, being consistent is the important part. Thanks, used these names. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:24: * can be shared at a given time. On 2016/07/14 17:36:31, nyquist wrote: > What happens if multiple files are shared? Is this a constraint from this class, > from Chrome? Or from Android? It is the constraint of this implementation and current use case only requires one file at a time. The other issue is that if I try to share multiple files, I need to keep track of all the files and have a file to ready flag map. This map can never be cleared and the system does not notify that the file is no longer needed. So, I made it use only one file. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:26: public class BlockingFileProvider extends FileProvider { On 2016/07/14 17:36:32, nyquist wrote: > Could you add a test for this class? Done. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:36: Uri uri = null; On 2016/07/14 17:36:32, nyquist wrote: > Nit: Could you just initialize this while building it? Done. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:90: private static UriMatcher sUriMatcher = null; On 2016/07/14 17:36:31, nyquist wrote: > Where is this used? I am not really sure. I I thought this was used by android to select content providers. I was wrong. Removed this. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:103: if (sUriMatcher != null) return; On 2016/07/14 17:36:32, nyquist wrote: > I don't think this is threadsafe, and it can be called from any thread. Maybe > add thread checking or make it thread safe? Acknowledged. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:118: while (!sIsFileReady && fileName.contains(sCurrentFileName)) { On 2016/07/14 17:36:32, nyquist wrote: > Should this be "endsWith" instead of "contains"? Done. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:957: public void onShareMenuItemSelected(final boolean shareDirectly, boolean isIncognito) { On 2016/07/14 17:36:32, nyquist wrote: > Was there no test you needed to update for this? > If so, could you please add one? Done. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:979: final String fileName = String.valueOf(System.currentTimeMillis()); On 2016/07/14 17:36:32, nyquist wrote: > Will the user ever see this filename? If not, maybe we could use something that > won't ever overlap? I was thinking about for example daylight savings time. It's > a weird case though, but if it's not user-visible, maybe elapsedRealtime() or > maybe even elapsedRealtimeNanos()? The uri wouldn't overlap with anything because the name would be something like "content://com.google.android.apps.chrome.BlockingFileProvider/1468532221978". So this will be unique across chromium since it has "BlockingFileProvider". The actual file itself has an uri like "content://com.android.chrome.FileProvider/images/screenshot/14642960919551746825363.jpg" This is exactly the same name and folder used previous to the change and I just kept it. Do you want to change this ? How about to "chrome-screenshot-%d"? https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:987: ShareHelper.onScreenshotReady(fileName, bitmap, mainActivity); On 2016/07/14 17:36:32, nyquist wrote: > Is mainActivity guaranteed to be alive here, or will > ApplicationState.HAS_DESTROYED_ACTIVITIES catch that in the ShareHelper? The check at the end of onScreenshotready should take care of activity being alive. I don't think that other code path that sends the intent needs this check since it is creating intent synchronously. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:993: currentTab.getWebContents().getContentBitmapAsync( On 2016/07/14 17:36:32, nyquist wrote: > Is it guaranteed that getWebContents() does not return null at this point? added a check https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:257: * The bitmap is compressed to jpeg before being written to the file. On 2016/07/14 17:36:32, nyquist wrote: > Nit: JPEG Done. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:266: BlockingFileProvider.notifyFileReady(fileName, null); On 2016/07/14 17:36:32, nyquist wrote: > Should this be posted as a task on the main thread, or is it OK to do it > directly as a result of this method call? > Below it's done as a task. Below it is on a task since it needs to compress a file and write it. Though we must already be on thread pool still posting since the above layer might change. Answer to your question is no since the blocking file provider already handles thread safety. https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:300: if (ApplicationStatus.getStateForApplication() On 2016/07/14 17:36:32, nyquist wrote: > If the state is not correct, should notifyFileReady still be called, similarly > to in the start of onScreenshotReady? Done.
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/2143133002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1002: protected Void doInBackground(Void... params) { drive by...why are you posting this to the background? WebContents isn't threadsafe, so I'm surprised this works in general. Isn't the api we are calling already async? Why the need to further delay it?
https://codereview.chromium.org/2143133002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1002: protected Void doInBackground(Void... params) { On 2016/07/19 21:08:43, Ted C wrote: > drive by...why are you posting this to the background? WebContents isn't > threadsafe, so I'm surprised this works in general. > > Isn't the api we are calling already async? Why the need to further delay it? Hm, weirdly only the function is called async. But a lot of processing happens in the same thread that it is called it. The other issue could be that if this is done in the main thread, the callback passed is also called in the main thread. But there could be a case (not sure if it's possible) where the BlockingFileProvider is accessed on the main thread and will start waiting on the notification which is supposed to come from the same thread. To be safer I am posting this on thread pool. I am not sure whats the right thing to do here.
On 2016/07/19 21:47:06, ssid wrote: > https://codereview.chromium.org/2143133002/diff/160001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (right): > > https://codereview.chromium.org/2143133002/diff/160001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1002: > protected Void doInBackground(Void... params) { > On 2016/07/19 21:08:43, Ted C wrote: > > drive by...why are you posting this to the background? WebContents isn't > > threadsafe, so I'm surprised this works in general. > > > > Isn't the api we are calling already async? Why the need to further delay it? > > Hm, weirdly only the function is called async. But a lot of processing happens > in the same thread that it is called it. The other issue could be that if this > is done in the main thread, the callback passed is also called in the main > thread. But there could be a case (not sure if it's possible) where the > BlockingFileProvider is accessed on the main thread and will start waiting on > the notification which is supposed to come from the same thread. To be safer I > am posting this on thread pool. > I am not sure whats the right thing to do here. If we need to delay starting the capture, we should just use a vanilla post message call instead of asynctask mHandler.postDelayed(new Runnable() { ... }, X_MILLIS); Might want to ping sievers@ to figure out what it is doing on the main thread that is causing it to block. Maybe it is doing the readback on the main thread and waiting or something. Ideally, we could fix that but I don't think we can run it on a background thread safely.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
On 2016/07/19 22:23:45, Ted C wrote: > On 2016/07/19 21:47:06, ssid wrote: > > > https://codereview.chromium.org/2143133002/diff/160001/chrome/android/java/sr... > > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > > (right): > > > > > https://codereview.chromium.org/2143133002/diff/160001/chrome/android/java/sr... > > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1002: > > protected Void doInBackground(Void... params) { > > On 2016/07/19 21:08:43, Ted C wrote: > > > drive by...why are you posting this to the background? WebContents isn't > > > threadsafe, so I'm surprised this works in general. > > > > > > Isn't the api we are calling already async? Why the need to further delay > it? > > > > Hm, weirdly only the function is called async. But a lot of processing happens > > in the same thread that it is called it. The other issue could be that if this > > is done in the main thread, the callback passed is also called in the main > > thread. But there could be a case (not sure if it's possible) where the > > BlockingFileProvider is accessed on the main thread and will start waiting on > > the notification which is supposed to come from the same thread. To be safer I > > am posting this on thread pool. > > I am not sure whats the right thing to do here. > > If we need to delay starting the capture, we should just use a vanilla post > message call instead of asynctask > > mHandler.postDelayed(new Runnable() { > ... > }, X_MILLIS); I guess we could just call the function directly here. Since the intent is already sent. At this point we have nothing better to do on main thread than taking screenshot. So post on same thread is not really required. > Might want to ping sievers@ to figure out what it is doing on the main thread > that > is causing it to block. Maybe it is doing the readback on the main thread and > waiting > or something. Ideally, we could fix that but I don't think we can run it on a > background thread safely. Ah, I was just paranoid that it might block other tasks in the main thread. And the content resolver does not seem to open the sent file from main thread. I realized that that my test was trying to wait on the same thread and that is not the way it happens in reality.
Please review this CL. Thanks
https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:979: final String fileName = String.valueOf(System.currentTimeMillis()); On 2016/07/19 18:32:57, ssid wrote: > On 2016/07/14 17:36:32, nyquist wrote: > > Will the user ever see this filename? If not, maybe we could use something > that > > won't ever overlap? I was thinking about for example daylight savings time. > It's > > a weird case though, but if it's not user-visible, maybe elapsedRealtime() or > > maybe even elapsedRealtimeNanos()? > > The uri wouldn't overlap with anything because the name would be something like > "content://com.google.android.apps.chrome.BlockingFileProvider/1468532221978". > So this will be unique across chromium since it has "BlockingFileProvider". > > The actual file itself has an uri like > "content://com.android.chrome.FileProvider/images/screenshot/14642960919551746825363.jpg" > This is exactly the same name and folder used previous to the change and I just > kept it. Do you want to change this ? How about to "chrome-screenshot-%d"? That'd be OK too. I was just thinking about if you take a screenshot just before daylight savings time, and then again exactly an hour (on the millisecond) later. Or immediately after each other. I don't think that'll happen in practice though. Feel free to keep as is too. https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:115: return sCurrentFileName != null && fileName.contains(sCurrentFileName); So null != null? Often the null-check is for the object you call .contains(...) on. Is it intentional to do the null-check on the member-field, but trust the input parameter? Also, should this be endsWith? https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:985: ShareHelper.share(shareDirectly, mainActivity, currentTab.getTitle(), url, null); The else-clause is big, and nothing happens after this if-else block. Can you just return; here, and inline the else-clause? https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:999: WebContents webContents = currentTab.getWebContents(); I guess now that this is not an async task, you can remove the null-check again? Otherwise I guess you'd already called ShareHelper.share() (line 988) Could you maybe just store the WebContents before that if-check and re-use it here? https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java (right): https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:41: assertNull(file); Could you comment as to why the file should be null? https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:59: }.execute(); Should this be executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR) just for consistensy? https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java (right): https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:81: private Object mLock = new Object(); I think these go before the constructor.
Patchset #8 (id:200001) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Made changes, PTAL thanks! https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:979: final String fileName = String.valueOf(System.currentTimeMillis()); On 2016/07/29 23:45:27, nyquist wrote: > On 2016/07/19 18:32:57, ssid wrote: > > On 2016/07/14 17:36:32, nyquist wrote: > > > Will the user ever see this filename? If not, maybe we could use something > > that > > > won't ever overlap? I was thinking about for example daylight savings time. > > It's > > > a weird case though, but if it's not user-visible, maybe elapsedRealtime() > or > > > maybe even elapsedRealtimeNanos()? > > > > The uri wouldn't overlap with anything because the name would be something > like > > "content://com.google.android.apps.chrome.BlockingFileProvider/1468532221978". > > So this will be unique across chromium since it has "BlockingFileProvider". > > > > The actual file itself has an uri like > > > "content://com.android.chrome.FileProvider/images/screenshot/14642960919551746825363.jpg" > > This is exactly the same name and folder used previous to the change and I > just > > kept it. Do you want to change this ? How about to "chrome-screenshot-%d"? > > That'd be OK too. I was just thinking about if you take a screenshot just before > daylight savings time, and then again exactly an hour (on the millisecond) > later. Or immediately after each other. I don't think that'll happen in practice > though. Feel free to keep as is too. The description says it returns the difference between current time and midnight, January 1, 1970 UTC. I think this will be unique? https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:115: return sCurrentFileName != null && fileName.contains(sCurrentFileName); On 2016/07/29 23:45:27, nyquist wrote: > So null != null? > Often the null-check is for the object you call .contains(...) on. Is it > intentional to do the null-check on the member-field, but trust the input > parameter? > > Also, should this be endsWith? It was necessary to do this check for sCurrentFileName != null since the .contains will crash if null. There should ideally be no case when the fileName is null, unless the other app sends an empty uri. Yes, it is better to check both. https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:985: ShareHelper.share(shareDirectly, mainActivity, currentTab.getTitle(), url, null); On 2016/07/29 23:45:27, nyquist wrote: > The else-clause is big, and nothing happens after this if-else block. Can you > just return; here, and inline the else-clause? Done. https://codereview.chromium.org/2143133002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:999: WebContents webContents = currentTab.getWebContents(); On 2016/07/29 23:45:27, nyquist wrote: > I guess now that this is not an async task, you can remove the null-check again? > Otherwise I guess you'd already called ShareHelper.share() (line 988) > > Could you maybe just store the WebContents before that if-check and re-use it > here? Done. https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java (right): https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:41: assertNull(file); On 2016/07/29 23:45:27, nyquist wrote: > Could you comment as to why the file should be null? Done. https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:59: }.execute(); On 2016/07/29 23:45:28, nyquist wrote: > Should this be executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR) just for > consistensy? Done. https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java (right): https://codereview.chromium.org/2143133002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:81: private Object mLock = new Object(); On 2016/07/29 23:45:28, nyquist wrote: > I think these go before the constructor. Done.
https://codereview.chromium.org/2143133002/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java (right): https://codereview.chromium.org/2143133002/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:112: mockActivity.onShareMenuItemSelected(true /* shareDirectly */, false /* isIncognito */); I think you have to make this be called on the main thread.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixed. Ptal thanks. https://codereview.chromium.org/2143133002/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java (right): https://codereview.chromium.org/2143133002/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:112: mockActivity.onShareMenuItemSelected(true /* shareDirectly */, false /* isIncognito */); On 2016/08/03 23:02:22, nyquist wrote: > I think you have to make this be called on the main thread. Thanks! Fixed.
Also, this build failed because of "Strict mode violation" but passed the second time. Should this be fixed? https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...
On 2016/08/04 22:43:09, ssid wrote: > Also, this build failed because of "Strict mode violation" but passed the second > time. Should this be fixed? > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... Did you figure this out? I couldn't seem to find the logcat as to what really failed.
https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:30: private static boolean sIsFileReady = false; false and null are defaults in java...so you don't need to specify them for variable declarations at the file level https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:35: * Returns an unique uri to identify the file to be shared. This doesn't mention that it will clobber any pending files being written (potentially causing the blockers to get partially written files etc...). So, I think we should either split this api 1.) get URI but do nothing else 2.) mark file as blocking. Or, just name this generateUriAndBlockAccess(String filename) or something that makes this clearer https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:44: sCurrentFileName = fileName; I apologize if this was discussed earlier in this patch, but why don't we just have a set of file urls that we are blocking on (something like): private static Set sPendingFiles = Collections.synchronizedSet(new HashSet()); Or potentially a synchronized map of Filename -> Lock While it is quite the edge case, in multiwindow, you can trigger two share actions at the same time. This will break that (again, not super critical), but allowing multiple doesn't seem like a huge issue to me (at first blush). https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:59: public static void notifyFileReady(String fileName, Uri fileUri) { why doesn't this class just deal with the URI that it passed back from getContentUriForFile above? Why do we need to store sCurrentFileName and sFileUri? Why wouldn't we just store the Uri that we give them above in sFileUri and do away with sCurrentFileName? My thinking is that we could just have sFileUri and a lock. If sFileUri == null, we could just treat it as loaded and let it pass through. Granted, that only makes sense if you do a map. I guess the current logic handles the case where you clobber other blocking file reads. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:241: private boolean mScreenshotCaptureSkippedForTesting = false; no need for = false here either https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:305: != ApplicationState.HAS_DESTROYED_ACTIVITIES this should be indented 8 from the start of the previous line https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:307: fileUri = UiUtils.getUriForImageCaptureFile(activity, saveFile); Hmm...this previously caused file access to go through CompatibilityFileProvider (whose authority was .FileProvider), and now it goes through BlockingFileProvider. How does that work? The authorities differ from the original URI generated vs this one. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:29: class MockChromeActivity extends ChromeTabbedActivity { two top level java class definitions is really really wonky. make this a private static inner class or make it a separate class file. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:124: assert false : "Test thread was interrupted while trying to wait."; use assertFalse since we're in a test https://codereview.chromium.org/2143133002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:132: startMainActivityOnBlankPage(); do we actually need this to start an activity? we don't seem to be using it at all. You're only dealing with your mock activity. I guess you need native library to be enabled. Could this just extend from NativeLibraryTestBase? Ahhh....mock activity wraps the underlying activity...gotcha...nevermind
Thanks, made changes, ptal. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:30: private static boolean sIsFileReady = false; On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > false and null are defaults in java...so you don't need to specify them for > variable declarations at the file level Done. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:35: * Returns an unique uri to identify the file to be shared. On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > This doesn't mention that it will clobber any pending files being written > (potentially causing the blockers to get partially written files etc...). So, I > think we should either split this api 1.) get URI but do nothing else 2.) mark > file as blocking. Or, just name this generateUriAndBlockAccess(String filename) > or something that makes this clearer Thanks, renamed and added better comment. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:44: sCurrentFileName = fileName; On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > I apologize if this was discussed earlier in this patch, but why don't we just > have a set of file urls that we are blocking on (something like): > > private static Set sPendingFiles = Collections.synchronizedSet(new HashSet()); > > Or potentially a synchronized map of Filename -> Lock > > While it is quite the edge case, in multiwindow, you can trigger two share > actions at the same time. This will break that (again, not super critical), but > allowing multiple doesn't seem like a huge issue to me (at first blush). Not sure if it was discussed. There is no way to clear the elements added to this set. there is no notification sent back to the provider that says the file has been used and now it can be deleted. There is a delete() call but is never called. It could cause ever growing map of file names. To keep it simple only one file is shared at a given time. I think the only thing that will break is the screenshot. If the user manages to synchronously press share on two windows and wants to share to "Keep" there is a chance of race condition. This will only result in one of the share not getting screenshot. But, it is not worth adding complexity here. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:59: public static void notifyFileReady(String fileName, Uri fileUri) { On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > why doesn't this class just deal with the URI that it passed back from > getContentUriForFile above? Why do we need to store sCurrentFileName and > sFileUri? Why wouldn't we just store the Uri that we give them above in > sFileUri and do away with sCurrentFileName? Yes I could store the uri instead of fileName. But it is confusing to read the code with 2 uri(s). I would have to store blockingFileProviderUri and fileUri, and do the comparison with currentBlockingFileProviderUri and the given uri. It is same as comparing with the fileName. If you prefer storing uri than fileName I can change it. But I do not see a big difference. > My thinking is that we could just have sFileUri and a lock. If sFileUri == > null, we could just treat it as loaded and let it pass through. Granted, that > only makes sense if you do a map. I guess the current logic handles the case > where you clobber other blocking file reads. Yess the current file name is stored because of the clobbering case here. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:241: private boolean mScreenshotCaptureSkippedForTesting = false; On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > no need for = false here either Done. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:305: != ApplicationState.HAS_DESTROYED_ACTIVITIES On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > this should be indented 8 from the start of the previous line Um, git cl format does this ! https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:307: fileUri = UiUtils.getUriForImageCaptureFile(activity, saveFile); On 2016/08/06 00:02:41, Ted C (OOO till 8.15) wrote: > Hmm...this previously caused file access to go through CompatibilityFileProvider > (whose authority was .FileProvider), and now it goes through > BlockingFileProvider. How does that work? The authorities differ from the > original URI generated vs this one. The BlockingFileProvider just wraps around the FileProvider like CompatibilityFileProvider. The openFile call to BlockingProvider is redirected to FileProvider::openFile with this uri that is stored in BlockingFileProvider::sFileUri. The fileUri created here is never sent to the applications. I do not have to go through CompatibilityFileProvider since that is a workaround for a particular media store case. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:29: class MockChromeActivity extends ChromeTabbedActivity { On 2016/08/06 00:02:41, Ted C (OOO till 8.15) wrote: > two top level java class definitions is really really wonky. make this a > private static inner class or make it a separate class file. Done. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:124: assert false : "Test thread was interrupted while trying to wait."; On 2016/08/06 00:02:41, Ted C (OOO till 8.15) wrote: > use assertFalse since we're in a test Are you suggesting to use assertFalse (false) and not add any messages? is that better? https://codereview.chromium.org/2143133002/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java:132: startMainActivityOnBlankPage(); On 2016/08/06 00:02:41, Ted C (OOO till 8.15) wrote: > do we actually need this to start an activity? we don't seem to be using it at > all. You're only dealing with your mock activity. I guess you need native > library to be enabled. Could this just extend from NativeLibraryTestBase? > > Ahhh....mock activity wraps the underlying activity...gotcha...nevermind Yeah I think it is easier to just wrap the activity than implement all the above functions and use NativeLibraryTestBase.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) 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...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping.
Sorry for the delay! I was definitely confused to begin with, but I "think" I've got a handle on this now. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:21: * till the file is ready. This lets us create a share intent immediately after the user clicks on We shouldn't mention share here. We should mention how to use this class not where this class is used. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:44: sCurrentFileName = fileName; On 2016/08/10 21:45:19, ssid wrote: > On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > > I apologize if this was discussed earlier in this patch, but why don't we just > > have a set of file urls that we are blocking on (something like): > > > > private static Set sPendingFiles = Collections.synchronizedSet(new HashSet()); > > > > Or potentially a synchronized map of Filename -> Lock > > > > While it is quite the edge case, in multiwindow, you can trigger two share > > actions at the same time. This will break that (again, not super critical), > but > > allowing multiple doesn't seem like a huge issue to me (at first blush). > > Not sure if it was discussed. > There is no way to clear the elements added to this set. > there is no notification sent back to the provider that says the file has been > used and now it can be deleted. There is a delete() call but is never called. > It could cause ever growing map of file names. > To keep it simple only one file is shared at a given time. > I think the only thing that will break is the screenshot. If the user manages to > synchronously press share on two windows and wants to share to "Keep" there is a > chance of race condition. This will only result in one of the share not getting > screenshot. But, it is not worth adding complexity here. Yeah, I believe this is more misunderstanding based on my comment below. I was under the assumption the URIs would/could be the same. I guess as written. We are only going to be holding onto one stale Uri in sFileUri if delete is never called. But yeah, a map would always grow if they never called delete. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:59: public static void notifyFileReady(String fileName, Uri fileUri) { On 2016/08/10 21:45:19, ssid wrote: > On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > > why doesn't this class just deal with the URI that it passed back from > > getContentUriForFile above? Why do we need to store sCurrentFileName and > > sFileUri? Why wouldn't we just store the Uri that we give them above in > > sFileUri and do away with sCurrentFileName? > > Yes I could store the uri instead of fileName. But it is confusing to read the > code with 2 uri(s). > I would have to store blockingFileProviderUri and fileUri, and do the comparison > with currentBlockingFileProviderUri and the given uri. It is same as comparing > with the fileName. > If you prefer storing uri than fileName I can change it. But I do not see a big > difference. Do you actually need a filename passed in the original function? Couldn't this just generate a random URI and pass it back. I suspect the reason I was so confused is that I assumed the filename passed in original was actually the file name, but it is only the prefix of the generated tmp filename in the ShareHelper. Personally, I'd drop file name from generateUriAndBlockAccess, and just store the blockingUri as separate fields fileUri. > > > My thinking is that we could just have sFileUri and a lock. If sFileUri == > > null, we could just treat it as loaded and let it pass through. Granted, that > > only makes sense if you do a map. I guess the current logic handles the case > > where you clobber other blocking file reads. > > Yess the current file name is stored because of the clobbering case here. I think the fundamental question is why there are two URIs? Looks like building the legitimate URI requires doing a bit of I/O to get UiUtils.getDirectoryForImageCapture and then generate the tmp file. While I find it unfortunate that we can't share the same URI, I don't want to say that any amount of blocking the share is ok since we are doing so much work to make it better. I think I was misunderstanding how this file was used. The URI generated from generateUriAndBlockAccess is shared with the client app but doesn't actually represent any files in reality. When the client requests that URI, we under the covers replace it with one that is actually handled via the CompatibilityFileProvider (UiUtils.getUriForImageCaptureFile). But this doesn't call CompatibilityFileProvider, and just relies on the super class to do the right thing with the new URI. Yet, CompatibilityFileProvider was explicitly added to handle cases where sharing screenshots crashed, so we need it to execute that code. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:305: != ApplicationState.HAS_DESTROYED_ACTIVITIES On 2016/08/10 21:45:19, ssid wrote: > On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > > this should be indented 8 from the start of the previous line > > Um, git cl format does this ! git cl format doesn't behave perfectly in java. it's more of a first start than the end solution https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:307: fileUri = UiUtils.getUriForImageCaptureFile(activity, saveFile); On 2016/08/10 21:45:19, ssid wrote: > On 2016/08/06 00:02:41, Ted C (OOO till 8.15) wrote: > > Hmm...this previously caused file access to go through > CompatibilityFileProvider > > (whose authority was .FileProvider), and now it goes through > > BlockingFileProvider. How does that work? The authorities differ from the > > original URI generated vs this one. > > The BlockingFileProvider just wraps around the FileProvider like > CompatibilityFileProvider. The openFile call to BlockingProvider is redirected > to FileProvider::openFile with this uri that is stored in > BlockingFileProvider::sFileUri. > The fileUri created here is never sent to the applications. > I do not have to go through CompatibilityFileProvider since that is a workaround > for a particular media store case. Per my comment in the other file, CompatibilityFileProvider was added to work around bugs in sharing code, so I think we need that logic. We "might" consider merging the two file providers to avoid any of the weirdness here. https://bugs.chromium.org/p/chromium/issues/detail?id=467423
Patchset #14 (id:340001) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think I have done what you suggested. PTAL, thanks. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:21: * till the file is ready. This lets us create a share intent immediately after the user clicks on On 2016/08/19 00:35:29, Ted C wrote: > We shouldn't mention share here. We should mention how to use this class not > where this class is used. Done. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:59: public static void notifyFileReady(String fileName, Uri fileUri) { > Do you actually need a filename passed in the original function? Couldn't this > just generate a random URI and pass it back. I suspect the reason I was so > confused > is that I assumed the filename passed in original was actually the file name, > but > it is only the prefix of the generated tmp filename in the ShareHelper. > > Personally, I'd drop file name from generateUriAndBlockAccess, and just store > the blockingUri as separate fields fileUri. > Storing two uris instead of fileName and fileUri. > > > > > My thinking is that we could just have sFileUri and a lock. If sFileUri == > > > null, we could just treat it as loaded and let it pass through. Granted, > that > > > only makes sense if you do a map. I guess the current logic handles the > case > > > where you clobber other blocking file reads. > > > > Yess the current file name is stored because of the clobbering case here. > > I think the fundamental question is why there are two URIs? > > Looks like building the legitimate URI requires doing a bit of I/O to get > UiUtils.getDirectoryForImageCapture and then generate the tmp file. > > While I find it unfortunate that we can't share the same URI, I don't want to > say that any amount of blocking the share is ok since we are doing so much work > to make it better. > > I think I was misunderstanding how this file was used. The URI generated from > generateUriAndBlockAccess is shared with the client app but doesn't actually > represent any files in reality. When the client requests that URI, we under the > covers replace it with one that is actually handled via the > CompatibilityFileProvider (UiUtils.getUriForImageCaptureFile). > > But this doesn't call CompatibilityFileProvider, and just relies on the super > class to do the right thing with the new URI. Yet, CompatibilityFileProvider > was explicitly added to handle cases where sharing screenshots crashed, so we > need it to execute that code. Removed the compatibility provider and added the logic here. Also removed "BlockingFileProvider" authority and using FileProvider. The generated uri contains special file names. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:305: != ApplicationState.HAS_DESTROYED_ACTIVITIES On 2016/08/19 00:35:29, Ted C wrote: > On 2016/08/10 21:45:19, ssid wrote: > > On 2016/08/06 00:02:40, Ted C (OOO till 8.15) wrote: > > > this should be indented 8 from the start of the previous line > > > > Um, git cl format does this ! > > git cl format doesn't behave perfectly in java. it's more of a first start than > the end solution Done. https://codereview.chromium.org/2143133002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:307: fileUri = UiUtils.getUriForImageCaptureFile(activity, saveFile); On 2016/08/19 00:35:29, Ted C wrote: > On 2016/08/10 21:45:19, ssid wrote: > > On 2016/08/06 00:02:41, Ted C (OOO till 8.15) wrote: > > > Hmm...this previously caused file access to go through > > CompatibilityFileProvider > > > (whose authority was .FileProvider), and now it goes through > > > BlockingFileProvider. How does that work? The authorities differ from the > > > original URI generated vs this one. > > > > The BlockingFileProvider just wraps around the FileProvider like > > CompatibilityFileProvider. The openFile call to BlockingProvider is redirected > > to FileProvider::openFile with this uri that is stored in > > BlockingFileProvider::sFileUri. > > The fileUri created here is never sent to the applications. > > I do not have to go through CompatibilityFileProvider since that is a > workaround > > for a particular media store case. > > Per my comment in the other file, CompatibilityFileProvider was added to work > around bugs in sharing code, so I think we need that logic. > > We "might" consider merging the two file providers to avoid any of the weirdness > here. > > https://bugs.chromium.org/p/chromium/issues/detail?id=467423 Done.
Description was changed from ========== Do screenshot capture async for share intents. The share intent is started with a delay because of screenshot capturing and compressing. But, lot of times the other activity never uses the screenshot. The screenshot capture, compress to jpeg and write to file can be done in background when the user is selecting the app shared to. This CL introduces a BlockingFileProvider that provides an empty file uri without any contents and shares an intent. While sending the intent it also triggers the screenshot capture. When the file is actually requested by the other app, the file provider will wait till the capture is done and the file is written. The blocking practically never happens because the user takes some time to click on the app, before which the capture is finished. BUG=615229 ========== to ========== Do screenshot capture async for share intents. The share intent is started with a delay because of screenshot capturing and compressing. But, lot of times the other activity never uses the screenshot. The screenshot capture, compress to jpeg and write to file can be done in background when the user is selecting the app shared to. This CL introduces a BlockingFileProvider that provides an empty file uri without any contents and shares an intent. While sending the intent it also triggers the screenshot capture. When the file is actually requested by the other app, the file provider will wait till the capture is done and the file is written. The blocking practically never happens because the user takes some time to click on the app, before which the capture is finished. The BlockingFileProvider is made default file provider and the CompatibilityFileProvider logic is added to it. BUG=615229 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
looking super close to me...some naming nits and few little things left but super close https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:28: public class BlockingFileProvider extends FileProvider { I'd probably just call this ChromeFileProvider or something as it does more than blocking (and more than compatibility, so that name doesn't really work now either). https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:46: public static Uri generateUriAndBlockAccess(final Context activity) { s/activity/context (and update javadoc) https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:49: Uri uuidUri = When I think of uuid, I think of this: https://en.wikipedia.org/wiki/Universally_unique_identifier Which has a pretty standard format, and this doesn't really comply with that. Instead of uuidUri (and sCurrentUuidUri), I think sBlockingUri is probably more descriptive. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:131: sFileUri = null; should we only do this if doesMatchCurrentUuid? https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:142: private Uri getFileUriWhenReady(Uri uri) { looking at the test, I'd make this protected (marked @VisibleForTesting) as that would give you the ability to test non-null Uris w/o having to write files to disk and deal with the content providers openFile logic. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:163: && sCurrentUuidUri.toString().equals(uri.toString()); Uri.equals does this internally, so I think you can just do .equals directly on the Uris https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1052: if (canShareOfflinePage) { we were previously running this code if "(isIncognito || webContents == null)", but now you are early returning above. I wonder if we should just do: Uri uri = (isIncognito || webContents == null) ? null : BlockingFileProvider... Then if the uri is null below just return before the callback creation. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:300: * @param uuidUri The unique id given by the BlockingFileProvider for the screenshot. you can use {@link BlockingFileProvider} or if you want to get fancy, you can do {@link BlockingFileProvider#<method signature> to make it even nicer. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:344: != ApplicationState.HAS_DESTROYED_ACTIVITIES decrease indent, should be 8 from the previous line https://codereview.chromium.org/2143133002/diff/360001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java (right): https://codereview.chromium.org/2143133002/diff/360001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:53: Thread.sleep(200); i think you can make this much shorter...we just want to make sure it works on async notifies so 10 millis seems reasonable to me. if this doesn't work then it would timeout and the test would fail, so the shorter the runtime the better https://codereview.chromium.org/2143133002/diff/360001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:75: Thread.sleep(200); same timing comment https://codereview.chromium.org/2143133002/diff/360001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:85: ParcelFileDescriptor file1 = openFileFromProvider(uri2); i find it odd that file1 == uri2, I'd recommend making them match to make this easier to read
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry for the late fixes. I got pulled into some work. Please bear with me, and take one more look. Thanks https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:28: public class BlockingFileProvider extends FileProvider { On 2016/08/27 00:22:12, Ted C wrote: > I'd probably just call this ChromeFileProvider or something as it does more than > blocking (and more than compatibility, so that name doesn't really work now > either). Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:46: public static Uri generateUriAndBlockAccess(final Context activity) { On 2016/08/27 00:22:12, Ted C wrote: > s/activity/context (and update javadoc) Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:49: Uri uuidUri = On 2016/08/27 00:22:12, Ted C wrote: > When I think of uuid, I think of this: > https://en.wikipedia.org/wiki/Universally_unique_identifier > > Which has a pretty standard format, and this doesn't really comply with that. > > Instead of uuidUri (and sCurrentUuidUri), I think sBlockingUri is probably more > descriptive. Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:131: sFileUri = null; On 2016/08/27 00:22:11, Ted C wrote: > should we only do this if doesMatchCurrentUuid? Yes, missed it. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:142: private Uri getFileUriWhenReady(Uri uri) { On 2016/08/27 00:22:11, Ted C wrote: > looking at the test, I'd make this protected (marked @VisibleForTesting) as that > would give you the ability to test non-null Uris w/o having to write files to > disk and deal with the content providers openFile logic. Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/BlockingFileProvider.java:163: && sCurrentUuidUri.toString().equals(uri.toString()); On 2016/08/27 00:22:12, Ted C wrote: > Uri.equals does this internally, so I think you can just do .equals directly on > the Uris Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1052: if (canShareOfflinePage) { On 2016/08/27 00:22:12, Ted C wrote: > we were previously running this code if "(isIncognito || webContents == null)", > but now you are early returning above. > > I wonder if we should just do: > > Uri uri = (isIncognito || webContents == null) ? null : BlockingFileProvider... > > Then if the uri is null below just return before the callback creation. Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:300: * @param uuidUri The unique id given by the BlockingFileProvider for the screenshot. On 2016/08/27 00:22:12, Ted C wrote: > you can use {@link BlockingFileProvider} or if you want to get fancy, you can do > {@link BlockingFileProvider#<method signature> to make it even nicer. Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:344: != ApplicationState.HAS_DESTROYED_ACTIVITIES On 2016/08/27 00:22:12, Ted C wrote: > decrease indent, should be 8 from the previous line Sorry, I fixed and git cl format changed back. Stopped using it. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java (right): https://codereview.chromium.org/2143133002/diff/360001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:53: Thread.sleep(200); On 2016/08/27 00:22:12, Ted C wrote: > i think you can make this much shorter...we just want to make sure it works on > async notifies so 10 millis seems reasonable to me. if this doesn't work then > it would timeout and the test would fail, so the shorter the runtime the better Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:75: Thread.sleep(200); On 2016/08/27 00:22:12, Ted C wrote: > same timing comment Done. https://codereview.chromium.org/2143133002/diff/360001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/BlockingFileProviderTest.java:85: ParcelFileDescriptor file1 = openFileFromProvider(uri2); On 2016/08/27 00:22:12, Ted C wrote: > i find it odd that file1 == uri2, I'd recommend making them match to make this > easier to read Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
last few nits and we are good to go https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1057: I think you want to do if (blockingUri == null) return; No reason to worry about the bitmap fetching in that case. https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java:131: if (!doesMatchCurrentBlockingUri(uri)) return 0; Hmm...won't this block other deletions? Should we wrap the whole sync block by something like this: if (uri.getPath().contains(BLOCKED_FILE_PREFIX)) { } https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java:163: return uri != null && sCurrentBlockingUri != null && sCurrentBlockingUri.equals(uri); we null check uri here, but not in getFileUriWhenReady above. Should we add it above? https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:301: * {@link ChromeFileProvider#generateUriAndBlockAccess()} for the screenshot. align with "The unique" above https://codereview.chromium.org/2143133002/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeFileProviderTest.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeFileProviderTest.java:60: }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); if you want to just do the doInBackground bits with no params. You can run on an executor with this: https://developer.android.com/reference/java/util/concurrent/Executor.html#ex...
Fixed, thanks https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1057: On 2016/09/07 00:30:07, Ted C wrote: > I think you want to do > > if (blockingUri == null) return; > > No reason to worry about the bitmap fetching in that case. Done. https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java:131: if (!doesMatchCurrentBlockingUri(uri)) return 0; On 2016/09/07 00:30:08, Ted C wrote: > Hmm...won't this block other deletions? > > Should we wrap the whole sync block by something like this: > if (uri.getPath().contains(BLOCKED_FILE_PREFIX)) { > } Done. https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java:163: return uri != null && sCurrentBlockingUri != null && sCurrentBlockingUri.equals(uri); On 2016/09/07 00:30:08, Ted C wrote: > we null check uri here, but not in getFileUriWhenReady above. Should we add it > above? Done. https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:301: * {@link ChromeFileProvider#generateUriAndBlockAccess()} for the screenshot. On 2016/09/07 00:30:08, Ted C wrote: > align with "The unique" above Done. https://codereview.chromium.org/2143133002/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeFileProviderTest.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeFileProviderTest.java:60: }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); On 2016/09/07 00:30:08, Ted C wrote: > if you want to just do the doInBackground bits with no params. You can run on > an executor with this: > > https://developer.android.com/reference/java/util/concurrent/Executor.html#ex... Tommy suggested that I should always call executeOnExecutor instead of execute.
lgtm w/ a few last comments. thanks for sticking with this! https://codereview.chromium.org/2143133002/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeFileProviderTest.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeFileProviderTest.java:60: }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); On 2016/09/07 00:51:23, ssid wrote: > On 2016/09/07 00:30:08, Ted C wrote: > > if you want to just do the doInBackground bits with no params. You can run on > > an executor with this: > > > > > https://developer.android.com/reference/java/util/concurrent/Executor.html#ex... > > Tommy suggested that I should always call executeOnExecutor instead of execute. I just chatted to Tommy and I think there was a bit of confusion. He was saying when using an AsyncTask you should prefer executeOnExecutor (https://developer.android.com/reference/android/os/AsyncTask.html#executeOnEx..., Params...)) over execute (https://developer.android.com/reference/android/os/AsyncTask.html#execute(Par...). That is because you'll get more predictable behavior, but that wasn't saying you shouldn't use execute directly on an executor. If you have an AsyncTask that is <Void, Void, Void>, then you can replace this (with the exact same behavior as): AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() { @Override public void run() { .... } }); This still uses a specific executor and just avoids the extra complication of the AsyncTask bits. https://codereview.chromium.org/2143133002/diff/400001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (left): https://codereview.chromium.org/2143133002/diff/400001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:605: <provider android:name="org.chromium.chrome.browser.util.CompatibilityFileProvider" I just noticed, but I think util is a fine place for this class to live. https://codereview.chromium.org/2143133002/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java:130: if (uri.getPath().contains(BLOCKED_FILE_PREFIX)) { null check uri here to be consistent
Patchset #17 (id:420001) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2143133002/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeFileProviderTest.java (right): https://codereview.chromium.org/2143133002/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeFileProviderTest.java:60: }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); On 2016/09/07 17:30:13, Ted C wrote: > On 2016/09/07 00:51:23, ssid wrote: > > On 2016/09/07 00:30:08, Ted C wrote: > > > if you want to just do the doInBackground bits with no params. You can run > on > > > an executor with this: > > > > > > > > > https://developer.android.com/reference/java/util/concurrent/Executor.html#ex... > > > > Tommy suggested that I should always call executeOnExecutor instead of > execute. > > I just chatted to Tommy and I think there was a bit of confusion. > > He was saying when using an AsyncTask you should prefer > > executeOnExecutor > (https://developer.android.com/reference/android/os/AsyncTask.html#executeOnEx..., > Params...)) > > over execute > (https://developer.android.com/reference/android/os/AsyncTask.html#execute(Par...). > > That is because you'll get more predictable behavior, but that wasn't saying you > shouldn't use execute directly on an executor. > > If you have an AsyncTask that is <Void, Void, Void>, then you can replace this > (with the exact same behavior as): > > AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() { > @Override > public void run() { > .... > } > }); > > This still uses a specific executor and just avoids the extra complication of > the AsyncTask bits. Thanks for explanation. Fixed. https://codereview.chromium.org/2143133002/diff/400001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (left): https://codereview.chromium.org/2143133002/diff/400001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:605: <provider android:name="org.chromium.chrome.browser.util.CompatibilityFileProvider" On 2016/09/07 17:30:13, Ted C wrote: > I just noticed, but I think util is a fine place for this class to live. Done. https://codereview.chromium.org/2143133002/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java (right): https://codereview.chromium.org/2143133002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFileProvider.java:130: if (uri.getPath().contains(BLOCKED_FILE_PREFIX)) { On 2016/09/07 17:30:13, Ted C wrote: > null check uri here to be consistent Done.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2143133002/#ps440001 (title: "move to util/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Do screenshot capture async for share intents. The share intent is started with a delay because of screenshot capturing and compressing. But, lot of times the other activity never uses the screenshot. The screenshot capture, compress to jpeg and write to file can be done in background when the user is selecting the app shared to. This CL introduces a BlockingFileProvider that provides an empty file uri without any contents and shares an intent. While sending the intent it also triggers the screenshot capture. When the file is actually requested by the other app, the file provider will wait till the capture is done and the file is written. The blocking practically never happens because the user takes some time to click on the app, before which the capture is finished. The BlockingFileProvider is made default file provider and the CompatibilityFileProvider logic is added to it. BUG=615229 ========== to ========== Do screenshot capture async for share intents. The share intent is started with a delay because of screenshot capturing and compressing. But, lot of times the other activity never uses the screenshot. The screenshot capture, compress to jpeg and write to file can be done in background when the user is selecting the app shared to. This CL introduces a BlockingFileProvider that provides an empty file uri without any contents and shares an intent. While sending the intent it also triggers the screenshot capture. When the file is actually requested by the other app, the file provider will wait till the capture is done and the file is written. The blocking practically never happens because the user takes some time to click on the app, before which the capture is finished. The BlockingFileProvider is made default file provider and the CompatibilityFileProvider logic is added to it. BUG=615229 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Do screenshot capture async for share intents. The share intent is started with a delay because of screenshot capturing and compressing. But, lot of times the other activity never uses the screenshot. The screenshot capture, compress to jpeg and write to file can be done in background when the user is selecting the app shared to. This CL introduces a BlockingFileProvider that provides an empty file uri without any contents and shares an intent. While sending the intent it also triggers the screenshot capture. When the file is actually requested by the other app, the file provider will wait till the capture is done and the file is written. The blocking practically never happens because the user takes some time to click on the app, before which the capture is finished. The BlockingFileProvider is made default file provider and the CompatibilityFileProvider logic is added to it. BUG=615229 ========== to ========== Do screenshot capture async for share intents. The share intent is started with a delay because of screenshot capturing and compressing. But, lot of times the other activity never uses the screenshot. The screenshot capture, compress to jpeg and write to file can be done in background when the user is selecting the app shared to. This CL introduces a BlockingFileProvider that provides an empty file uri without any contents and shares an intent. While sending the intent it also triggers the screenshot capture. When the file is actually requested by the other app, the file provider will wait till the capture is done and the file is written. The blocking practically never happens because the user takes some time to click on the app, before which the capture is finished. The BlockingFileProvider is made default file provider and the CompatibilityFileProvider logic is added to it. BUG=615229 Committed: https://crrev.com/ee36837ec9d6df950df4020f80ea702d599d666c Cr-Commit-Position: refs/heads/master@{#417185} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/ee36837ec9d6df950df4020f80ea702d599d666c Cr-Commit-Position: refs/heads/master@{#417185} |
