Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
913033002:
Extend ShareHelper#share to share a screenshot as a stream (Closed)
|
Created:
5 years, 10 months ago by Jaekyun Seok (inactive) Modified:
5 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtend ShareHelper#share to share a screenshot as a stream
BUG=455996
Committed: https://crrev.com/5523a258ffe874a67e839559ae902d94b55fd69c
Cr-Commit-Position: refs/heads/master@{#316156}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Move file saving logic into ShareHelper #
Total comments: 2
Patch Set 3 : Move save file creation logic into ShareHelper #
Total comments: 8
Patch Set 4 : Run AsyncTask #
Total comments: 6
Patch Set 5 : Check application status #Patch Set 6 : Fix findbugs #
Messages
Total messages: 24 (7 generated)
jaekyun@chromium.org changed reviewers: + aurimas@chromium.org, tedchoc@chromium.org
Please review this CL.
https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:78: * @param screenshot Screenshot of the page to be shared. I made the same comment downstream, but do we have any callers that will still pass screenshot? Or screenshot and screenshot URIs? They seem mutually exclusive and we should try to remove all callers to screenshot. And now that I think about this more, why don't we have all the logic for converting to the URI in this class and keep the signature the same? i.e. all callers still just pass the bitmap and internally we convert this to the URI and send it out. Then everyone gets the new desired behavior for free. https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:79: * @param screenshotURI URI of screenshot data. This will be ignored when |screenshot| is set. https://source.android.com/source/code-style.html#treat-acronyms-as-words URI when used as a variable should be Uri (screenshotUri).
klobag@chromium.org changed reviewers: + klobag@chromium.org
https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:197: intent.putExtra(Intent.EXTRA_STREAM, screenshotURI); Normally setType() will define the minetype of the stream. But it is already used for EXTRA_TEXT above.
https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:78: * @param screenshot Screenshot of the page to be shared. On 2015/02/11 16:42:52, Ted C wrote: > I made the same comment downstream, but do we have any callers that will still > pass screenshot? Or screenshot and screenshot URIs? They seem mutually > exclusive and we should try to remove all callers to screenshot. > > And now that I think about this more, why don't we have all the logic for > converting to the URI in this class and keep the signature the same? i.e. all > callers still just pass the bitmap and internally we convert this to the URI and > send it out. > > Then everyone gets the new desired behavior for free. Actually, my first try was to implement the converting logic in this file. But I changed my mind because I thought that a saving destination and image compress format could be different per caller. Anyway, I will try to move more logics into here. https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:79: * @param screenshotURI URI of screenshot data. This will be ignored when |screenshot| is set. On 2015/02/11 16:42:52, Ted C wrote: > https://source.android.com/source/code-style.html#treat-acronyms-as-words > > URI when used as a variable should be Uri (screenshotUri). I will change names as you recommended. https://codereview.chromium.org/913033002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:197: intent.putExtra(Intent.EXTRA_STREAM, screenshotURI); On 2015/02/11 22:03:44, klobag.chromium wrote: > Normally setType() will define the minetype of the stream. But it is already > used for EXTRA_TEXT above. I searched cases which use Intent.EXTRA_TEXT and Intent.EXTRA_STREAM, and in most places the type has a value for Intent.EXTRA_STREAM. But I confirmed that codes worked well even though the type was "text/plain". So, I will keep the type as "text/plain".
PTAL. I moved the logic related to image path into UiUtils, and moved the screenshot saving logic into ShareHelper.
https://codereview.chromium.org/913033002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:86: * @param saveFile File which will save |screenshot| to share it as stream. If this is null, until there is a need to have the file location configurable, let's just keep this all internal to this class. I think all share actions should behave the same right now, so this just adds more burden to the callers. I don't think we should change this api signature from what it was before. And with that said, I think we should also remove the EXTRA_SHARE_SCREENSHOT intent entirely (the only usecase we're aware of that previously used it -- by code search -- already supports the stream behave as you tested).
PTAL. https://codereview.chromium.org/913033002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:86: * @param saveFile File which will save |screenshot| to share it as stream. If this is null, On 2015/02/12 20:44:43, Ted C wrote: > until there is a need to have the file location configurable, let's just keep > this all internal to this class. I think all share actions should behave the > same right now, so this just adds more burden to the callers. I don't think we > should change this api signature from what it was before. > > And with that said, I think we should also remove the EXTRA_SHARE_SCREENSHOT > intent entirely (the only usecase we're aware of that previously used it -- by > code search -- already supports the stream behave as you tested). Done.
This approach looks right to me. Some annoying bits about how we should make it async to avoid file / ui thread warnings, but that looks it to me. https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:56: private static final String SCREENSHOT_FILE_PATH = "screenshot"; should this be screenshot.jpg? s/PATH/NAME https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:76: File imagePath = UiUtils.getDirectoryForImageCapture(context); we should probably do this in an asynctask so it doesn't block the UI thread. https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:214: FileOutputStream fOut = new FileOutputStream(saveFile); even here this is probably a strict mode violation, so we should write the file in a background thread and send the intent once it is done. https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:223: if (Math.max(screenshot.getWidth(), screenshot.getHeight()) I don't think this is worth handling IMO. We are never going to be passing images this small and we can just resurrect this behavior if it is ever needed again. But for now, let's just always use only EXTRA_STREAM and call it the way of the future.
PTAL. https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:56: private static final String SCREENSHOT_FILE_PATH = "screenshot"; On 2015/02/12 23:10:54, Ted C wrote: > should this be screenshot.jpg? > > s/PATH/NAME No, this is a directory name for screenshot files. I will rename it to SCREENSHOT_DIRECTORY_NAME. https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:76: File imagePath = UiUtils.getDirectoryForImageCapture(context); On 2015/02/12 23:10:54, Ted C wrote: > we should probably do this in an asynctask so it doesn't block the UI thread. Done. https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:214: FileOutputStream fOut = new FileOutputStream(saveFile); On 2015/02/12 23:10:54, Ted C wrote: > even here this is probably a strict mode violation, so we should write the file > in a background thread and send the intent once it is done. Done. https://codereview.chromium.org/913033002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:223: if (Math.max(screenshot.getWidth(), screenshot.getHeight()) On 2015/02/12 23:10:54, Ted C wrote: > I don't think this is worth handling IMO. We are never going to be passing > images this small and we can just resurrect this behavior if it is ever needed > again. > > But for now, let's just always use only EXTRA_STREAM and call it the way of the > future. Done.
lgtm w/ a couple questions but you can submit when you're ready https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:64: private static void deleteFile(File file) { Maybe deleteScreenshotFiles(File path) might be clearer...that is what threw me off a bit with the directory above. https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:178: activity.startActivity(getDirectShareIntentForComponent( I'm actually surprised that startActivity can be called not on the UI thread. I was thinking that you'd have to create the intent in the background and do the starting on the run in UI thread bits of asynctask. Also, there is the ever so slight chance that the activity might have died so you'll want to make sure this will behave nicely in that case. I would test by doing Thread.sleep(30seconds) after building the intent, making sure you have don't keep activities turned on, then going to settings or something and seeing if it crashes. If so, you might want to look at ApplicationStatus to see if the activity is still alive. https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:232: } should we have an else fail case?
New patchsets have been uploaded after l-g-t-m from tedchoc@chromium.org
PTAL. https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:64: private static void deleteFile(File file) { On 2015/02/13 01:27:47, Ted C wrote: > Maybe deleteScreenshotFiles(File path) might be clearer...that is what threw me > off a bit with the directory above. Done. https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:178: activity.startActivity(getDirectShareIntentForComponent( On 2015/02/13 01:27:47, Ted C wrote: > I'm actually surprised that startActivity can be called not on the UI thread. I > was thinking that you'd have to create the intent in the background and do the > starting on the run in UI thread bits of asynctask. startActivity works even in the background. > > Also, there is the ever so slight chance that the activity might have died so > you'll want to make sure this will behave nicely in that case. > > I would test by doing Thread.sleep(30seconds) after building the intent, making > sure you have don't keep activities turned on, then going to settings or > something and seeing if it crashes. If so, you might want to look at > ApplicationStatus to see if the activity is still alive. I will check ApplicationStatus before sending an Intent because a screenshot file could be deleted when application state is HAS_DESTROYED_ACTIVITIES. https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:232: } On 2015/02/13 01:27:47, Ted C wrote: > should we have an else fail case? Done.
On 2015/02/13 01:58:35, Jaekyun Seok wrote: > PTAL. > > https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... > File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java > (right): > > https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... > chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:64: > private static void deleteFile(File file) { > On 2015/02/13 01:27:47, Ted C wrote: > > Maybe deleteScreenshotFiles(File path) might be clearer...that is what threw > me > > off a bit with the directory above. > > Done. > > https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... > chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:178: > activity.startActivity(getDirectShareIntentForComponent( > On 2015/02/13 01:27:47, Ted C wrote: > > I'm actually surprised that startActivity can be called not on the UI thread. > I > > was thinking that you'd have to create the intent in the background and do the > > starting on the run in UI thread bits of asynctask. > > startActivity works even in the background. > > > > > Also, there is the ever so slight chance that the activity might have died so > > you'll want to make sure this will behave nicely in that case. > > > > I would test by doing Thread.sleep(30seconds) after building the intent, > making > > sure you have don't keep activities turned on, then going to settings or > > something and seeing if it crashes. If so, you might want to look at > > ApplicationStatus to see if the activity is still alive. > > I will check ApplicationStatus before sending an Intent because a screenshot > file could be deleted when application state is HAS_DESTROYED_ACTIVITIES. > > https://codereview.chromium.org/913033002/diff/60001/chrome/android/java/src/... > chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:232: > } > On 2015/02/13 01:27:47, Ted C wrote: > > should we have an else fail case? > > Done. Lgtm
The CQ bit was checked by jaekyun@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/913033002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
New patchsets have been uploaded after l-g-t-m from tedchoc@chromium.org
The CQ bit was checked by jaekyun@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/913033002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5523a258ffe874a67e839559ae902d94b55fd69c Cr-Commit-Position: refs/heads/master@{#316156} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
