Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2081153005:
[Offline Page] Offline page sharing implementation (Closed)
|
Created:
4 years, 6 months ago by Vivian Modified:
4 years, 4 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrimary implementation of offline page sharing
This is the primary implementation of offline page sharing feature.
If current tab is an offline page, clicking on share would result in:
the corresponding mhtml file copied to the external storage, and,
the the copied file in the external storage is passed in intent to be shared using third party application.
BUG=622139
Committed: https://crrev.com/be3aae7913a59268351546d2a17686fd5a79371a
Cr-Commit-Position: refs/heads/master@{#412661}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Merge CL #
Total comments: 18
Patch Set 3 : Refactor code and enable multiple files sharing. #Patch Set 4 : Fix typo #
Total comments: 16
Patch Set 5 : Record current working version #Patch Set 6 : Change dot in file path to dash #
Total comments: 17
Patch Set 7 : Add cache directory clean up #Patch Set 8 : Format code and add unit test #Patch Set 9 : Add file overwrite and share both screentshot and mhtml #
Total comments: 33
Patch Set 10 : Adjustments according to Filip's comments #
Total comments: 5
Patch Set 11 : Rebased and test update added #
Total comments: 55
Patch Set 12 : Address comments #
Total comments: 43
Patch Set 13 : Address comments and edit test #
Total comments: 6
Patch Set 14 : Remove unused line #
Total comments: 6
Patch Set 15 : Patch to land #
Total comments: 6
Patch Set 16 : Grammar fix #Dependent Patchsets: Messages
Total messages: 60 (26 generated)
weiran@google.com changed reviewers: + dewittj@chromium.org, fgorski@chromium.org
Hi Filip and Justin, This is my "primary draft" of the page sharing part. Thanks for all the helps! The code looks not so pretty right now since I was just focusing on making it work. Please let me know how I can improve it. Also, right now the files are copied to external storage once user click on the share button, I'm wondering if I should add some code to delete the file afterward, and if so, when. I'll keep thinking about it and add more stuff to it. Again, thank for helping me out. ptal
Thanks for sending this! I only took a cursory look so far, but think that you would do well to split out the new Feature/command-line flag into its own CL so that there were 2 smaller, more focused CLs. I just did something similar: https://chromium.googlesource.com/chromium/src/+/54fd4951328e78a93e0cb928e006... https://codereview.chromium.org/2081153005/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2081153005/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1889: IDS_FLAGS_ENABLE_AUTOPLAY_MUTED_VIDEOS_DESCRIPTION, kOsAndroid, please don't change these entries' whitespace, I think git-cl format does bad things in this file so may need to manually check that only your code is changed. https://codereview.chromium.org/2081153005/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_feature.h (right): https://codereview.chromium.org/2081153005/diff/1/components/offline_pages/of... components/offline_pages/offline_page_feature.h:32: bool IsOfflinePagesSharingEnabled(); I would do this in a first CL, without adding any code besides the new feature, and the test for that feature. Maybe should consider adding this feature to https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li... so that we don't need a new bridge api for whether this feature is enabled.
Great start, my comments are mostly oriented towards meshing better with the existing code for sharing screenshots. https://codereview.chromium.org/2081153005/diff/1/chrome/android/java/res/xml... File chrome/android/java/res/xml/file_paths.xml (right): https://codereview.chromium.org/2081153005/diff/1/chrome/android/java/res/xml... chrome/android/java/res/xml/file_paths.xml:12: <external-path name="offlinePages" path="DCIM/OfflinePages/"/> How was this path chosen? Why can't we have another files-path name="offline-pages" or something similar? https://codereview.chromium.org/2081153005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:200: public static void shareOfflinePage(final boolean shareDirectly, final Activity activity, There seems to be a lot of code here that is very similar to the bitmap sharing code. Possibly it would be best to encapsulate the Bitmap -> File process and the offlineUrl -> File process. Then you'd have a common thing that you could pass to ShareHelper#Share, which does the appropriate file copy etc at the right time. https://codereview.chromium.org/2081153005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:222: copyToExternal(offlinePageInternal, offlinePageExternal); I would prefer if we copied to internal if possible. Based on other code in ShareHelper, it seems that after a certain version of Android we are able to share internal files. Let's try to emulate that and see if there's any common code. https://codereview.chromium.org/2081153005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:268: public static void copyToExternal(File src, File dst) throws IOException { I'm surprised that this is the first time in Chromium that this has been done; is there a helper function somewhere that you could use?
Yes the files related to flags were supposed to be in another CL. I uploaded those changes before but it appears on this CL again because I didn't set up the branches correctly. Just fixed the issue and uploaded a new patch without unrelated files. https://codereview.chromium.org/2081153005/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2081153005/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1889: IDS_FLAGS_ENABLE_AUTOPLAY_MUTED_VIDEOS_DESCRIPTION, kOsAndroid, On 2016/06/23 00:55:46, dewittj wrote: > please don't change these entries' whitespace, I think git-cl format does bad > things in this file so may need to manually check that only your code is > changed. Acknowledged. https://codereview.chromium.org/2081153005/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_feature.h (right): https://codereview.chromium.org/2081153005/diff/1/components/offline_pages/of... components/offline_pages/offline_page_feature.h:32: bool IsOfflinePagesSharingEnabled(); On 2016/06/23 00:55:46, dewittj wrote: > I would do this in a first CL, without adding any code besides the new feature, > and the test for that feature. > > Maybe should consider adding this feature to > https://cs.chromium.org/chromium/src/chrome/browser/android/chrome_feature_li... > > so that we don't need a new bridge api for whether this feature is enabled. Acknowledged.
https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:965: @Override nit: this indentation looks weir to me. Since it is defining a new class it should probably be only 4 lines. (and that will make the rest of the code look less weird. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:976: // tab's nit: please remove NL from the end. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:988: // This is for experienmental feature Offline Page Sharing nit: End sentences with a period, please. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:990: // instead of url ditto Re: period. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:991: String offlineUrl = currentTab.getUrl(); how do you know the URL is offline here? https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:995: offlinePageBridge.getPageByOfflineUrl( Perhaps this whole code could be moved outside of ChromeActivity, e.g. to OfflinePageUtils or OfflinePageBridge and tested there. Let's try to avoid putting logic in here. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:95: if (!OfflinePageBridge.isPageSharingEnabled()) return; I don't think I understand that one. Please explain what it does. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:218: + File.separator + offlinePageInternal.getName()); Would this constructor work: File(File parent, String child) To avoid string concatenation. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:227: return offlinePageExternal; move the space above this line.
Made some changes according to the comments. In short, I moved the piece of code in ChromeActivity to OfflinePageUtils, and change the functions in ShareHelper so that offline page sharing is now an entry in previous code not separate one. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:965: @Override On 2016/06/23 17:41:10, fgorski wrote: > nit: this indentation looks weir to me. Since it is defining a new class it > should probably be only 4 lines. (and that will make the rest of the code look > less weird. Done. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:976: // tab's On 2016/06/23 17:41:10, fgorski wrote: > nit: please remove NL from the end. Done. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:988: // This is for experienmental feature Offline Page Sharing On 2016/06/23 17:41:11, fgorski wrote: > nit: End sentences with a period, please. Done. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:990: // instead of url On 2016/06/23 17:41:11, fgorski wrote: > ditto Re: period. Done. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:991: String offlineUrl = currentTab.getUrl(); On 2016/06/23 17:41:11, fgorski wrote: > how do you know the URL is offline here? It is checked in this line: boolean offlinePageSharing = (url != null && OfflinePageBridge.isPageSharingEnabled()); So my understanding is that if it goes to the else statement, then it must be an offline page, and the tab.getUrl() should return the offline url. Let me know if there's anything wrong with my understanding. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:995: offlinePageBridge.getPageByOfflineUrl( On 2016/06/23 17:41:10, fgorski wrote: > Perhaps this whole code could be moved outside of ChromeActivity, e.g. to > OfflinePageUtils or OfflinePageBridge and tested there. Let's try to avoid > putting logic in here. Done. This part is moved to OfflinePageUtils. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:95: if (!OfflinePageBridge.isPageSharingEnabled()) return; On 2016/06/23 17:41:11, fgorski wrote: > I don't think I understand that one. Please explain what it does. This line is unused. Removed it. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:218: + File.separator + offlinePageInternal.getName()); On 2016/06/23 17:41:11, fgorski wrote: > Would this constructor work: File(File parent, String child) > To avoid string concatenation. Done. https://codereview.chromium.org/2081153005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:227: return offlinePageExternal; On 2016/06/23 17:41:11, fgorski wrote: > move the space above this line. Done.
https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:985: // This is for experienmental feature Offline Page Sharing. This feature is embedded enough in sharing and has enough questions about implementation that I'd love to see you write up a document about what you're going to do, and how you're going to do it, before requesting a code review. Some things that would be useful to share in a document to gain consensus: * Where to put the copies of offline pages that are prepped for sharing, how they get cleaned up, how many copies you expect to have at one time. * How this will interact with the other 2 things we currently share (URL and screenshot) * How to surface sharing errors if file copy fails * How to organize the code better (today it is all spaghetti code, and that is partly the fault of the Bitmap implementation. We could do better and probably should clean it up as a part of your change.) Does that make sense? https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:263: OfflinePageBridge.getForProfile(currentTab.getProfile()); This needs a null check, because for certain tabs such as incognito tabs offlinePageBridge will be null. https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:269: // ShareHelper.shareOfflinePage(shareDirectly, nit: remove commented out code. https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:298: copyToExternal(offlinePageOriginal, offlinePageShareable); When does this new file get cleaned up? https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:312: if (offlinePageShareable == null) return; If this has an error, how does the user know what happened? https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:340: public static void copyToExternal(File src, File dst) throws IOException { This should be renamed, since we're not copying externally any more. Also, have you tried using symbolic links to avoid actual file copy? That'd be more ideal if it works with the Android FS and with the file URI permissions.
https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:187: <action android:name="android.intent.action.VIEW" /> nit: Do you need that extra space? https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:969: boolean offlinePageSharing = rename to isOfflinePageSharingEnabled, please
please remove the files that are already part of another CL (manifest) or that are not needed here (swp) ;) also, I think you can introduce file path in offline page item as a separate (and fairly small) CL, further cutting this one down. (It will pay off). https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:253: * Share saved offline page. nit: document parameters. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:258: final String offlineUrl = currentTab.getUrl(); Are we 100% that we are browsing an offline page? Is there a chance we are not? What happens if the feature is enabled and the user is trying to share an online page? https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:274: public static void preparingForShare(final boolean shareDirectly, final Activity activity, Please change the name to: prepareForSharing https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:274: public static void preparingForShare(final boolean shareDirectly, final Activity activity, does this method need to be public? does this method need to be static? (have you tried writing a test for it?) https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:282: // File shareableDir = new File(context.getFilesDir(), "offline-pages"); please choose what code you want use here, and remove what is not necessary. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:317: // Uri offlineUri = ditto: RE: deleting unused code. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:331: public static void copyToExternal(File src, File dst) throws IOException { Please consider name: copyToShareableLocation does it need to be public? does it need to be static? did you try to test this? https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:458: Intent intent = new Intent(Intent.ACTION_SEND); Extract common code out of the if/else. Put a single return statement at the end, please. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:815: Log.d(TAG, "loadType: " + loadType); do you need to change this file? If this is only for debugging, then I think it would be save to remove this change and minimize the CL
Just formatted the code and added some test. The last patch was submitted a while ago. Basically what the new patch does: 1. Remove code for getting file path to another CL. 2. Formatted code according to comments. 3. Added clearing files in cache. 4. Added some unit tests. ptal https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:187: <action android:name="android.intent.action.VIEW" /> On 2016/06/28 20:03:22, fgorski wrote: > nit: Do you need that extra space? Deprecated. Changes on this file is moved to a separate CL. https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:969: boolean offlinePageSharing = On 2016/06/28 20:03:22, fgorski wrote: > rename to isOfflinePageSharingEnabled, please Done. https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:985: // This is for experienmental feature Offline Page Sharing. On 2016/06/28 16:45:25, dewittj wrote: > This feature is embedded enough in sharing and has enough questions about > implementation that I'd love to see you write up a document about what you're > going to do, and how you're going to do it, before requesting a code review. > Some things that would be useful to share in a document to gain consensus: > * Where to put the copies of offline pages that are prepped for sharing, how > they get cleaned up, how many copies you expect to have at one time. > * How this will interact with the other 2 things we currently share (URL and > screenshot) > * How to surface sharing errors if file copy fails > * How to organize the code better (today it is all spaghetti code, and that is > partly the fault of the Bitmap implementation. We could do better and probably > should clean it up as a part of your change.) > > Does that make sense? Acknowledged. https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:263: OfflinePageBridge.getForProfile(currentTab.getProfile()); On 2016/06/28 16:45:25, dewittj wrote: > This needs a null check, because for certain tabs such as incognito tabs > offlinePageBridge will be null. Done. https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:269: // ShareHelper.shareOfflinePage(shareDirectly, On 2016/06/28 16:45:25, dewittj wrote: > nit: remove commented out code. Done. https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:298: copyToExternal(offlinePageOriginal, offlinePageShareable); On 2016/06/28 16:45:25, dewittj wrote: > When does this new file get cleaned up? Added code for file cleaning up. Files are copied to a folder in external cache directory. Once Chrome is closed, the folder is cleaned up. https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:312: if (offlinePageShareable == null) return; On 2016/06/28 16:45:25, dewittj wrote: > If this has an error, how does the user know what happened? Seems like the current sharing features (sharing screenshot and image) does not handle this problem. Is this something I should discuss with UI people? https://codereview.chromium.org/2081153005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:340: public static void copyToExternal(File src, File dst) throws IOException { On 2016/06/28 16:45:25, dewittj wrote: > This should be renamed, since we're not copying externally any more. > > Also, have you tried using symbolic links to avoid actual file copy? That'd be > more ideal if it works with the Android FS and with the file URI permissions. I didn't find many useful discussion on symbolic links from research. Seems like Android only allow symbolic link in external storage. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:253: * Share saved offline page. On 2016/07/26 17:00:54, fgorski wrote: > nit: document parameters. Done. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:258: final String offlineUrl = currentTab.getUrl(); On 2016/07/26 17:00:54, fgorski wrote: > Are we 100% that we are browsing an offline page? > Is there a chance we are not? > > What happens if the feature is enabled and the user is trying to share an online > page? This check is done in ChromeActivity.onShareMenuItemSelected. This method would only be called if the feature is enabled AND the page is offline. I reused Justin's earlier code for checking if page is offline page: if currentTab.getOfflinePageOriginalUrl() returns a not null url, the page is offline. Is there any necessity that I should do another check here as well? https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:274: public static void preparingForShare(final boolean shareDirectly, final Activity activity, On 2016/07/26 17:00:54, fgorski wrote: > Please change the name to: prepareForSharing Changed name and made it private. Seems like because the caller function in ChromeActivity is static, all methods need to be static. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:282: // File shareableDir = new File(context.getFilesDir(), "offline-pages"); On 2016/07/26 17:00:54, fgorski wrote: > please choose what code you want use here, and remove what is not necessary. Done. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:317: // Uri offlineUri = On 2016/07/26 17:00:54, fgorski wrote: > ditto: RE: deleting unused code. Done. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:331: public static void copyToExternal(File src, File dst) throws IOException { On 2016/07/26 17:00:54, fgorski wrote: > Please consider name: copyToShareableLocation > > does it need to be public? > does it need to be static? > did you try to test this? Changed name and added test. Need to be static as it's called by static method. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:458: Intent intent = new Intent(Intent.ACTION_SEND); On 2016/07/26 17:00:55, fgorski wrote: > Extract common code out of the if/else. > Put a single return statement at the end, please. Done. https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2081153005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:815: Log.d(TAG, "loadType: " + loadType); On 2016/07/26 17:00:55, fgorski wrote: > do you need to change this file? If this is only for debugging, then I think it > would be save to remove this change and minimize the CL Done. Removed the line.
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
Description was changed from ========== Primary implementation of offline page sharing This is the primary implementation of offline page sharing feature. If current tab is an offline page, clicking on share would result in: the corresponding mhtml file copied to the external storage, and, the the copied file in the external storage is passed in intent to be shared using third party application. BUG=622139 ========== to ========== Primary implementation of offline page sharing This is the primary implementation of offline page sharing feature. If current tab is an offline page, clicking on share would result in: the corresponding mhtml file copied to the external storage, and, the the copied file in the external storage is passed in intent to be shared using third party application. BUG=622139 ==========
looks very nice. Please update based on comments and we can send to someone in the wider team. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:970: boolean isOfflinePageSharingEnabled = nit: rename to canShareOfflinePage https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:271: * @param onlineUrl Online Url associated with the offline page that is used to access the nit: Online URL in Java code onlineUrl is correct, but in comments use URL https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:302: final String offlineUrl, final Context context) { offlineUrl could be renamed fileName or filePath, as that is what is actually carried there. WDYT? https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:312: if (offlinePageShareable.exists()) offlinePageShareable.delete(); can deleting a file throw an exception here? https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:332: } catch (IllegalArgumentException e) { Are both methods above throwing the exception or only one of them? https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:351: } finally { in 313 you have a try catch that only covers a single line. What if you actually put a catch in here and return boolean result indicating success? https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:361: * @return path to directory for the shared file to be stored. nit: when you decide to put a period at the end make sure to capitalize the first letter in the sentence. Bonus point if you actually have a sentence. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:362: * */ nit, extra * https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:373: * Rewrite file name so that it does not contain ".". how about rewording: contain periods except the one to separate the file extension. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:375: * As Android's pathPattern only match to the first dot appears in a file path. nit: replace pathPattern with path pattern only matches the first dot that appears in a file path. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:377: * */ nit: fix the * https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:386: * @param file Path to the directory of offline page files. I believe File object is not a file path. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:387: * */ nit: * https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:391: if (file.isDirectory()) { what about directories within directories? will this work? FileUtils.deleteDirectory(new File("directory")); following: http://stackoverflow.com/questions/779519/delete-directories-recursively-in-java https://codereview.chromium.org/2081153005/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:269: assertTrue("Unable to create subdirectory in shareable directory.", success); Please try to express this comment in a positive way, e.g. It should be able to create a subdirectory... That way this is a meaningful message when it passes and when it fails. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:274: OfflinePageUtils.copyToShareableLocation(offlinePageOriginal, offlinePageShareable); Does this work? I thought copying happens on a different thread and you don't seem to be synchronizing this. What's going on?
Comments addressed. Could you help me find a reviewer for files not owned by us? The two people I have them reviewed before are both OOO. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:970: boolean isOfflinePageSharingEnabled = On 2016/08/09 23:17:47, fgorski wrote: > nit: rename to canShareOfflinePage Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:271: * @param onlineUrl Online Url associated with the offline page that is used to access the On 2016/08/09 23:17:47, fgorski wrote: > nit: Online URL > > in Java code onlineUrl is correct, but in comments use URL Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:302: final String offlineUrl, final Context context) { On 2016/08/09 23:17:47, fgorski wrote: > offlineUrl could be renamed fileName or filePath, as that is what is actually > carried there. WDYT? Yes! Nice catch. I changed to filePath. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:312: if (offlinePageShareable.exists()) offlinePageShareable.delete(); On 2016/08/09 23:17:47, fgorski wrote: > can deleting a file throw an exception here? The only exception it throws is SecurityException. I added catch the exception, but not completely sure we need it. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:332: } catch (IllegalArgumentException e) { On 2016/08/09 23:17:47, fgorski wrote: > Are both methods above throwing the exception or only one of them? This line was used for previous "FileProvider" approach and I forget to delete it. Uri.fromFile throws nullPointerException and share does not throw exception. Deleted this part for now. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:351: } finally { On 2016/08/09 23:17:47, fgorski wrote: > in 313 you have a try catch that only covers a single line. What if you actually > put a catch in here and return boolean result indicating success? Done. Edited test for the method accordingly. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:361: * @return path to directory for the shared file to be stored. On 2016/08/09 23:17:47, fgorski wrote: > nit: when you decide to put a period at the end make sure to capitalize the > first letter in the sentence. Bonus point if you actually have a sentence. Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:362: * */ On 2016/08/09 23:17:48, fgorski wrote: > nit, extra * Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:373: * Rewrite file name so that it does not contain ".". On 2016/08/09 23:17:47, fgorski wrote: > how about rewording: contain periods except the one to separate the file > extension. Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:375: * As Android's pathPattern only match to the first dot appears in a file path. On 2016/08/09 23:17:47, fgorski wrote: > nit: replace pathPattern with path pattern > only matches the first dot that appears in a file path. Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:377: * */ On 2016/08/09 23:17:47, fgorski wrote: > nit: fix the * Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:386: * @param file Path to the directory of offline page files. On 2016/08/09 23:17:47, fgorski wrote: > I believe File object is not a file path. Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:387: * */ On 2016/08/09 23:17:47, fgorski wrote: > nit: * Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:391: if (file.isDirectory()) { On 2016/08/09 23:17:47, fgorski wrote: > what about directories within directories? > > will this work? > FileUtils.deleteDirectory(new File("directory")); following: > http://stackoverflow.com/questions/779519/delete-directories-recursively-in-java I believe the this works for nested directories (same approach with the second answer in the link). FileUtils is class from org.apache.commons.io, which requires download external package. I'm hesitate to do so. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:269: assertTrue("Unable to create subdirectory in shareable directory.", success); On 2016/08/09 23:17:48, fgorski wrote: > Please try to express this comment in a positive way, e.g. > > It should be able to create a subdirectory... > > That way this is a meaningful message when it passes and when it fails. Done. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:274: OfflinePageUtils.copyToShareableLocation(offlinePageOriginal, offlinePageShareable); On 2016/08/09 23:17:48, fgorski wrote: > Does this work? I thought copying happens on a different thread and you don't > seem to be synchronizing this. What's going on? Based on the fact that the test passes, I think it works.
lgtm mod making sure that tests work on try bots. https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:391: if (file.isDirectory()) { On 2016/08/10 19:57:39, Vivian wrote: > On 2016/08/09 23:17:47, fgorski wrote: > > what about directories within directories? > > > > will this work? > > FileUtils.deleteDirectory(new File("directory")); following: > > > http://stackoverflow.com/questions/779519/delete-directories-recursively-in-java > > I believe the this works for nested directories (same approach with the second > answer in the link). > FileUtils is class from org.apache.commons.io, which requires download external > package. I'm hesitate to do so. Acknowledged. https://codereview.chromium.org/2081153005/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:368: * */ nit: * still needs removing https://codereview.chromium.org/2081153005/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:264: boolean success = true; boolean success = offlineCacheDir.exists() || offlineCacheDir.mkdir(); and drop the if. https://codereview.chromium.org/2081153005/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:275: assertEquals("File copy result incorrect", offlinePageOriginal.length(), Let's see if that works on try bots.
Patchset #11 (id:240001) has been deleted
The CQ bit was checked by weiran@google.com 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...
weiran@google.com changed reviewers: + dfalcantara@chromium.org
dfalcantara@chromium.org: Hi Dan, Filip suggested me to have you review the code. Could you take a look or forward to someone else? Thanks, Vivian https://codereview.chromium.org/2081153005/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:368: * */ On 2016/08/11 16:57:53, fgorski wrote: > nit: * still needs removing Done. https://codereview.chromium.org/2081153005/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:264: boolean success = true; On 2016/08/11 16:57:53, fgorski wrote: > boolean success = offlineCacheDir.exists() || offlineCacheDir.mkdir(); > > and drop the if. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1001: String url = currentTab.getOfflinePageOriginalUrl(); Revert this line and use currentTab.getOriginalUrl(), which was added explicitly for this purpose. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1008: ShareHelper.TargetChosenCallback callback = null; nit: Doesn't seem to be a point to pulling these variables out. You don't change anything. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1026: // instead of url. 1) Don't need to say it's experimental. Just use this instead: // Share the offline page instead of the URL. 3) nit: Put the positive part of the conditional on top instead of the negative. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1027: Context mContext = getApplicationContext(); Variables starting with "m" indicate that they're member variables, which this isn't. You should just be able to use mainActivity for the context, anyway. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:345: OfflinePageUtils.clearSharedOfflineFiles(this); This is run every single time an Activity dies (which can be often). Is that absolutely necessary? You already do it once in onDeferredStartup, when Clank starts up cold, which seems Good Enough™ and better for performance. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:286: if (offlinePageBridge != null) { Avoid indentation by early exiting: if (offlinePageBridge == null) { Log.e(TAG, "Unable to perform sharing on current tab"); return; } https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:289: public void onResult(OfflinePageItem item) { nit: When indentation gets deep in simple functions, it's sometimes nicer to do the early return there, too: if (item == null) return; String offlineFilePath = item.getFilePath(); prepareForSharing(shareDirectly, saveLastUsed, mainActivity, title, text, onlineUrl, bitmap, callback, offlineFilePath, mContext); https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:320: "failed to delete the file: " + offlinePageOriginal.getName()); nit: Might be worth printing the exception: Log.e(TAG, "Failed to delete: " + offlinePageOriginal.getName(), e); https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:337: offlineUri, bitmap, callback); If sharing the offline page fails, you should fall back to just sharing the URL. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:356: if (outChannel != null) outChannel.close(); 1) Log.e takes an Exception as the third argument, so you don't need to + e it at the end. 2) Do what DocumentModeAssassin does for consistency: ============== FileInputStream inputStream = null; FileInputStream outputStream = null; try { inputStream = new FileInputStream(src); outStream = new FileOutputStream(dst); FileChannel inChannel = inputStream.getChannel(); FileChannel outChannel = outputStream.getChannel(); inChannel.transferTo(0, inChannel.size(), outChannel); } catch (FileNotFoundException e) { Log.e(TAG, "Failed to copy the file: " + src.getName(), e); return false; } catch (IOException e) { Log.e(TAG, "Failed to copy the file: " + src.getName(), e); return false; } finally { StreamUtil.closeQuietly(inChannel); StreamUtil.closeQuietly(outChannel); } https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:374: boolean success = true; Useless variable? https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:399: static void deleteSharedOfflineFiles(File file) { Just use FileUtils.recursivelyDeleteFile(File file) instead. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:421: }.execute(); AsyncTask#execute() does different things, depending on what version of Android you're on. Explicitly use executeOnExecutor instead. AsyncTask#executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR) https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:213: * @param url URL of the page to be shared. You need to add the offlineUri @param. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:375: shareIntent(saveLastUsed, activity, getDirectShareIntentForComponent(activity, title, Pull out the getDirectShareIntentForComponent call to get around this weird indentation. https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:522: // clipData because adding Intent.FLAG_GRANT_READ_URI_PERMISSION doesn't work for Revert the comment move https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:526: } Maybe: if (offlineUri == null) { intent.setType("text/plain"); } else { intent.setType("multipart/related"); intent.putExtra(Intent.EXTRA_STREAM, offlineUri); } https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:42: "/storage/emulated/0/Android/data/org.chromium.chrome/cache/offline-pages"; 1) This directory only applies to local builds, which use the org.chromium.chrome package. 2) Can't you pull the correct directory from the Context? https://chromiumcodereview.appspot.com/2081153005/diff/260001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:271: if (!offlinePageShareable.exists()) { What happens if the file doesn't exist? Should the test fail?
dfalcantara@chromium.org changed reviewers: + twellington@chromium.org
+Theresa because she's handling the sharing bits.
https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1013: // tab's URL directly. Once you download an offline page, does going to the original URL always navigate to the offline page? If not, could we encounter a scenario like. 1. http://somesite.com download 2. Navigate to http://somesite.com's live webpage (not the offline version) 3. Click share - shares offline page instead of the live page the user is looking at. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1025: // If this page is an offline page, and the share the offline page "... an offline page then share the page instead of the original url."? https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:313: File offlinePageShareable = new File(shareableDir, fileName); Do we really need to rewrite the offline page file name, delete the original then copy it to a "shareable location"? We should be able to share from the original directory by adding it to file_paths.xml so that we can provide a URI through our ContentProvider. https://cs.chromium.org/chromium/src/chrome/android/java/res/xml/file_paths.x... See my recent CL to do something similar for other files in Downloads/ https://codereview.chromium.org/2230803004/ https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:499: Uri offlineUri, Uri screenshotUri) { It seems a bit odd to pass in a url and an offlineUri. Can we use a boolean to indicate whether the String url (possibly renamed to uri) is an offline page URI? https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:528: intent.setType("multipart/related"); What sort of apps can handle this type mime type besides Chrome (also, do we show up on the intent picker for these share intents?)? Maybe upload to drive or email the file? Do we think this is what the user wants to do vs sharing the original page URL? https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:529: intent.putExtra(Intent.EXTRA_STREAM, offlineUri); Since this is just a single extra, I think we should be able to share multiple offline pages from the downloads UI unless there's something else specifically prohibiting that? To share multiple, we just have to do something like this instead of putting a single extra: shareIntent.putParcelableArrayListExtra(Intent.EXTRA_STREAM, ArrayList<Uri> uris);
Thanks for the detailed the review! Comments addressed. ptal https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1001: String url = currentTab.getOfflinePageOriginalUrl(); On 2016/08/12 01:26:12, dfalcantara wrote: > Revert this line and use currentTab.getOriginalUrl(), which was added explicitly > for this purpose. Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1008: ShareHelper.TargetChosenCallback callback = null; On 2016/08/12 01:26:12, dfalcantara wrote: > nit: Doesn't seem to be a point to pulling these variables out. You don't > change anything. Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1013: // tab's URL directly. On 2016/08/12 17:36:44, Theresa Wellington wrote: > Once you download an offline page, does going to the original URL always > navigate to the offline page? > > If not, could we encounter a scenario like. > 1. http://somesite.com download > 2. Navigate to http://somesite.com live webpage (not the offline version) > 3. Click share - shares offline page instead of the live page the user is > looking at. I have this feature in a separate patch. (https://codereview.chromium.org/2202423004/) This patch only enables share offline page if you're currently viewing one. The other patch would enable share offline page from online tab. Is my understanding of the question correct? https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1025: // If this page is an offline page, and the share the offline page On 2016/08/12 17:36:44, Theresa Wellington wrote: > "... an offline page then share the page instead of the original url."? I accidentally deleted a part of the comment it in the previous CL. Changed according to Dan's suggestion. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1026: // instead of url. On 2016/08/12 01:26:12, dfalcantara wrote: > 1) Don't need to say it's experimental. Just use this instead: > // Share the offline page instead of the URL. > > 3) nit: Put the positive part of the conditional on top instead of the negative. > Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1027: Context mContext = getApplicationContext(); On 2016/08/12 01:26:12, dfalcantara wrote: > Variables starting with "m" indicate that they're member variables, which this > isn't. You should just be able to use mainActivity for the context, anyway. Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:345: OfflinePageUtils.clearSharedOfflineFiles(this); On 2016/08/12 01:26:12, dfalcantara wrote: > This is run every single time an Activity dies (which can be often). Is that > absolutely necessary? You already do it once in onDeferredStartup, when Clank > starts up cold, which seems Good Enough™ and better for performance. I'm not an expert of this...just followed wherever "clearShareImages" is called. I defer to your judgement. Deleted the line. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:286: if (offlinePageBridge != null) { On 2016/08/12 01:26:12, dfalcantara wrote: > Avoid indentation by early exiting: > > if (offlinePageBridge == null) { > Log.e(TAG, "Unable to perform sharing on current tab"); > return; > } Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:289: public void onResult(OfflinePageItem item) { On 2016/08/12 01:26:13, dfalcantara wrote: > nit: When indentation gets deep in simple functions, it's sometimes nicer to do > the early return there, too: > > if (item == null) return; > String offlineFilePath = item.getFilePath(); > prepareForSharing(shareDirectly, saveLastUsed, mainActivity, title, text, > onlineUrl, bitmap, callback, offlineFilePath, mContext); Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:313: File offlinePageShareable = new File(shareableDir, fileName); On 2016/08/12 17:36:44, Theresa Wellington wrote: > Do we really need to rewrite the offline page file name, delete the original > then copy it to a "shareable location"? > > We should be able to share from the original directory by adding it to > file_paths.xml so that we can provide a URI through our ContentProvider. > > https://cs.chromium.org/chromium/src/chrome/android/java/res/xml/file_paths.x... > > > See my recent CL to do something similar for other files in Downloads/ > https://codereview.chromium.org/2230803004/ This was my initial approach. The problem is, we want to be able to use ShareIt and Xender to share the page. but they won't recognize a content URI generated this way. Instead, using file URI can make it work. Read permission of file stored in internal directory cannot be granted for file URI. That's the reason I'm copying the files to external cache directory. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:320: "failed to delete the file: " + offlinePageOriginal.getName()); On 2016/08/12 01:26:12, dfalcantara wrote: > nit: Might be worth printing the exception: > > Log.e(TAG, "Failed to delete: " + offlinePageOriginal.getName(), e); Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:337: offlineUri, bitmap, callback); On 2016/08/12 01:26:12, dfalcantara wrote: > If sharing the offline page fails, you should fall back to just sharing the URL. Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:356: if (outChannel != null) outChannel.close(); On 2016/08/12 01:26:12, dfalcantara wrote: > 1) Log.e takes an Exception as the third argument, so you don't > need to + e it at the end. > 2) Do what DocumentModeAssassin does for consistency: > ============== > FileInputStream inputStream = null; > FileInputStream outputStream = null; > > try { > inputStream = new FileInputStream(src); > outStream = new FileOutputStream(dst); > > FileChannel inChannel = inputStream.getChannel(); > FileChannel outChannel = outputStream.getChannel(); > inChannel.transferTo(0, inChannel.size(), outChannel); > } catch (FileNotFoundException e) { > Log.e(TAG, "Failed to copy the file: " + src.getName(), e); > return false; > } catch (IOException e) { > Log.e(TAG, "Failed to copy the file: " + src.getName(), e); > return false; > } finally { > StreamUtil.closeQuietly(inChannel); > StreamUtil.closeQuietly(outChannel); > } Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:374: boolean success = true; On 2016/08/12 01:26:13, dfalcantara wrote: > Useless variable? Yes. Nice catch! Deleted it. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:399: static void deleteSharedOfflineFiles(File file) { On 2016/08/12 01:26:13, dfalcantara wrote: > Just use FileUtils.recursivelyDeleteFile(File file) instead. Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:421: }.execute(); On 2016/08/12 01:26:12, dfalcantara wrote: > AsyncTask#execute() does different things, depending on what version of Android > you're on. Explicitly use executeOnExecutor instead. > > AsyncTask#executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR) Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:213: * @param url URL of the page to be shared. On 2016/08/12 01:26:13, dfalcantara wrote: > You need to add the offlineUri @param. Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:375: shareIntent(saveLastUsed, activity, getDirectShareIntentForComponent(activity, title, On 2016/08/12 01:26:13, dfalcantara wrote: > Pull out the getDirectShareIntentForComponent call to get around this weird > indentation. Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:499: Uri offlineUri, Uri screenshotUri) { On 2016/08/12 17:36:44, Theresa Wellington wrote: > It seems a bit odd to pass in a url and an offlineUri. Can we use a boolean to > indicate whether the String url (possibly renamed to uri) is an offline page > URI? The url and offlineUri here would be two different thing. URL is an http link to the original online URL of the page. OfflineUri is the file URI to the mhtml file to be shared. Say you chose Gmail in share dialog, url would be shown in the email context, while mhtml file would be added as attachment. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:522: // clipData because adding Intent.FLAG_GRANT_READ_URI_PERMISSION doesn't work for On 2016/08/12 01:26:13, dfalcantara wrote: > Revert the comment move Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:526: } On 2016/08/12 01:26:13, dfalcantara wrote: > Maybe: > > if (offlineUri == null) { > intent.setType("text/plain"); > } else { > intent.setType("multipart/related"); > intent.putExtra(Intent.EXTRA_STREAM, offlineUri); > } Done. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:528: intent.setType("multipart/related"); On 2016/08/12 17:36:44, Theresa Wellington wrote: > What sort of apps can handle this type mime type besides Chrome (also, do we > show up on the intent picker for these share intents?)? Maybe upload to drive or > email the file? > > Do we think this is what the user wants to do vs sharing the original page URL? List of apps that can handle this mime type: Gmail, Drive, SHAREit, Xender, Beam. I have a patch enabling Chrome handling incoming multipart/related intent. So the user who receives the file would be able to open it via Chrome. (See https://codereview.chromium.org/2098263002/) The feature is behind flag "OfflinePagesSharing". If user does not have the flag set to ON, original page URL is shared instead. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:529: intent.putExtra(Intent.EXTRA_STREAM, offlineUri); On 2016/08/12 17:36:44, Theresa Wellington wrote: > Since this is just a single extra, I think we should be able to share multiple > offline pages from the downloads UI unless there's something else specifically > prohibiting that? > > To share multiple, we just have to do something like this instead of putting a > single extra: shareIntent.putParcelableArrayListExtra(Intent.EXTRA_STREAM, > ArrayList<Uri> uris); I talked with Dmitry. I think we want to leave it this way in this patch. And maybe add this capacity later. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:42: "/storage/emulated/0/Android/data/org.chromium.chrome/cache/offline-pages"; On 2016/08/12 01:26:13, dfalcantara wrote: > 1) This directory only applies to local builds, which use the > org.chromium.chrome > package. > 2) Can't you pull the correct directory from the Context? Removed this line and used pull directory from the Context instead. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:271: if (!offlinePageShareable.exists()) { On 2016/08/12 01:26:13, dfalcantara wrote: > What happens if the file doesn't exist? Should the test fail? Added check for file existence.
Patchset #12 (id:280001) has been deleted
Patchset #12 (id:300001) has been deleted
Patchset #12 (id:320001) has been deleted
https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1013: // tab's URL directly. On 2016/08/12 23:07:12, Vivian wrote: > On 2016/08/12 17:36:44, Theresa Wellington wrote: > > Once you download an offline page, does going to the original URL always > > navigate to the offline page? > > > > If not, could we encounter a scenario like. > > 1. http://somesite.com download > > 2. Navigate to http://somesite.com live webpage (not the offline version) > > 3. Click share - shares offline page instead of the live page the user is > > looking at. > > I have this feature in a separate patch. > (https://codereview.chromium.org/2202423004/) > This patch only enables share offline page if you're currently viewing one. The > other patch would enable share offline page from online tab. > > Is my understanding of the question correct? It sounds like yes, you can still view the live tab if it's been downloaded offline. With the current patch if you're on the live tab it shares the URL rather than the offline page. In my opinion, that's the correct behavior and we shouldn't try to share the offline page if the user is on the live tab. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:313: File offlinePageShareable = new File(shareableDir, fileName); On 2016/08/12 23:07:12, Vivian wrote: > On 2016/08/12 17:36:44, Theresa Wellington wrote: > > Do we really need to rewrite the offline page file name, delete the original > > then copy it to a "shareable location"? > > > > We should be able to share from the original directory by adding it to > > file_paths.xml so that we can provide a URI through our ContentProvider. > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/res/xml/file_paths.x... > > > > > > See my recent CL to do something similar for other files in Downloads/ > > https://codereview.chromium.org/2230803004/ > > This was my initial approach. The problem is, we want to be able to use ShareIt > and Xender to share the page. but they won't recognize a content URI generated > this way. Instead, using file URI can make it work. Read permission of file > stored in internal directory cannot be granted for file URI. That's the reason > I'm copying the files to external cache directory. That really surprises me. The Android documentation talks explicitly about being able to grant read permission on an internal directory to enable sharing a content:// URI generated from an internal file: https://developer.android.com/reference/android/support/v4/content/FileProvid... When you try to share to Xender and ShareIt using the approach where a new path is added to file_paths.xml, what's the error in adb logcat? https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:499: Uri offlineUri, Uri screenshotUri) { On 2016/08/12 23:07:12, Vivian wrote: > On 2016/08/12 17:36:44, Theresa Wellington wrote: > > It seems a bit odd to pass in a url and an offlineUri. Can we use a boolean to > > indicate whether the String url (possibly renamed to uri) is an offline page > > URI? > > The url and offlineUri here would be two different thing. URL is an http link to > the original online URL of the page. OfflineUri is the file URI to the mhtml > file to be shared. Say you chose Gmail in share dialog, url would be shown in > the email context, while mhtml file would be added as attachment. Ah, I see now - I missed that we were using both when sharing an offline page. Thanks. https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:529: intent.putExtra(Intent.EXTRA_STREAM, offlineUri); On 2016/08/12 23:07:13, Vivian wrote: > On 2016/08/12 17:36:44, Theresa Wellington wrote: > > Since this is just a single extra, I think we should be able to share multiple > > offline pages from the downloads UI unless there's something else specifically > > prohibiting that? > > > > To share multiple, we just have to do something like this instead of putting a > > single extra: shareIntent.putParcelableArrayListExtra(Intent.EXTRA_STREAM, > > ArrayList<Uri> uris); > > I talked with Dmitry. I think we want to leave it this way in this patch. And > maybe add this capacity later. It doesn't need to get integrated here. I'm asking because we're implement sharing multiple downloads from the download manager ui. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:339: Uri offlineUri = null; Eventually we'll want to be able to share the mhtml for an offline page from the download manger UI as well as the "share" menu item. For the DownloadManagerUi, it would be helpful if there were a static method that took the offlineUrl (and possibly the title) and returned a content:// URI. Can that piece be extracted into a separate method that can be shared? https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:221: String title, String text, String url, Uri offlineUri, Bitmap screenshot, Use the @Nullable annotation for offlineUri
https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:313: File offlinePageShareable = new File(shareableDir, fileName); On 2016/08/13 15:58:48, Theresa Wellington wrote: > On 2016/08/12 23:07:12, Vivian wrote: > > On 2016/08/12 17:36:44, Theresa Wellington wrote: > > > Do we really need to rewrite the offline page file name, delete the original > > > then copy it to a "shareable location"? > > > > > > We should be able to share from the original directory by adding it to > > > file_paths.xml so that we can provide a URI through our ContentProvider. > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/res/xml/file_paths.x... > > > > > > > > > See my recent CL to do something similar for other files in Downloads/ > > > https://codereview.chromium.org/2230803004/ > > > > This was my initial approach. The problem is, we want to be able to use > ShareIt > > and Xender to share the page. but they won't recognize a content URI generated > > this way. Instead, using file URI can make it work. Read permission of file > > stored in internal directory cannot be granted for file URI. That's the reason > > I'm copying the files to external cache directory. > > That really surprises me. The Android documentation talks explicitly about being > able to grant read permission on an internal directory to enable sharing a > content:// URI generated from an internal file: > https://developer.android.com/reference/android/support/v4/content/FileProvid... > > When you try to share to Xender and ShareIt using the approach where a new path > is added to file_paths.xml, what's the error in adb logcat? To expand a little more on why I'm pushing on this - I think doing operations that require disk IO after the user requests to share a file is the wrong approach. It may be sufficiently fast for one file (but maybe not, have we tested this on a svelte device?), but it certainly doesn't scale to sharing multiple files because there may likely end up being a perceptible delay in between the user tapping a button to share and the share intent getting created.
https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:276: * @param mainActivity Activity that is used to access package manager nit: missing period at the end of this line "... access the package manager." https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:282: public static void shareOfflinePage(final boolean shareDirectly, final boolean saveLastUsed, The JavaDoc is missing some params - saveLastUsed, text, and callback https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:317: if (shareableDir != null) { early exit here too: if (shareableDir == null) { Log.e(TAG, ... return null; } String fileName = .... https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:321: if (offlinePageShareable.exists()) { Please add a comment explaining why the old offline page shareable file must be deleted before copying again. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:325: Log.e(TAG, "Failed to delete: " + offlinePageOriginal.getName(), e); If we fail to delete the old shareable file, what happens if we copy (will the bytes get appended to the existing shareable file or will they overwrite some or all of the existing bytes)? Should we skip copying? https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:339: Uri offlineUri = null; On 2016/08/13 15:58:48, Theresa Wellington wrote: > Eventually we'll want to be able to share the mhtml for an offline page from the > download manger UI as well as the "share" menu item. For the DownloadManagerUi, > it would be helpful if there were a static method that took the offlineUrl (and > possibly the title) and returned a content:// URI. Can that piece be extracted > into a separate method that can be shared? I think we can skip this for now. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:346: }.execute(); Add AsyncTask.SERIAL_EXECUTOR here. Not specifying leads to inconsistent behavior since different versions of Android have different default executors. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:351: * @param src file path of the original file to be copied nits: remove "This method" capitalize the first word of the param description ("Path") and end with a period. Also, since the parameter is a File and not a String (path) I think it'd make more sense to just say "The original file to be copied." and "The destination file." https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:369: } catch (IOException e) { This can be combined with the catch above: ... } catch (FileNotFound | IOException e) { ... } finally { .... https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:380: * Get a directory for offline page sharing operation. nit: "Gets the directory to use for sharing offline pages, creating it if necessary." https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:382: * @return path to directory for the shared file to be stored nit: capitalize Path https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:386: File path = new File(context.getExternalCacheDir(), EXTERNAL_MHTML_FILE_PATH); I think we should create a static variable that caches this file rather than creating it each time. This method will become something like: if (mOfflineSharingDirectory == null { mOfflineSharingDirectory = .... } return mOfflineSharingDirectory. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:396: * This step is used to ensure that file name can be recognized by intent filter (.*\\.mhtml"). nit: "... (.*\\.mhtml) as Android's path..." https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:418: }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); This should be done on the SERIAL_EXECUTOR so that clearing shared files can't get interleaved with creating files for sharing. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:308: final String title, final String text, final String url, final Uri offlineUri, nit: add JavaDoc for offlineUri here and below https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:518: intent.addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION); Since the offline page file is getting copied to an external directory, granting this flag may not be necessary. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:245: String testUrl = mTestServer.getURL(TEST_PAGE); It looks like the code to save an offline page is used in 3 places. I think it makes sense to pull it out into a helper method. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:266: // Use a random file name for test issue. What is the test issue that requires using a random file name? https://codereview.chromium.org/2081153005/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:269: assertFalse("Should not exist a file with same file name,", offlinePageShareable.exists()); "The shareable offline page file should lot already exist."
https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:386: File path = new File(context.getExternalCacheDir(), EXTERNAL_MHTML_FILE_PATH); On 2016/08/15 20:49:59, Theresa Wellington wrote: > I think we should create a static variable that caches this file rather than > creating it each time. This method will become something like: > > if (mOfflineSharingDirectory == null { > mOfflineSharingDirectory = .... > } > > return mOfflineSharingDirectory. *sOfflineSharingDirectory (since it'd be static)
ptal https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:276: * @param mainActivity Activity that is used to access package manager On 2016/08/15 20:50:00, Theresa Wellington wrote: > nit: missing period at the end of this line > > "... access the package manager." Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:282: public static void shareOfflinePage(final boolean shareDirectly, final boolean saveLastUsed, On 2016/08/15 20:50:00, Theresa Wellington wrote: > The JavaDoc is missing some params - saveLastUsed, text, and callback Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:317: if (shareableDir != null) { On 2016/08/15 20:50:00, Theresa Wellington wrote: > early exit here too: > > if (shareableDir == null) { > Log.e(TAG, ... > return null; > } > > String fileName = .... Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:321: if (offlinePageShareable.exists()) { On 2016/08/15 20:49:59, Theresa Wellington wrote: > Please add a comment explaining why the old offline page shareable file must be > deleted before copying again. Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:325: Log.e(TAG, "Failed to delete: " + offlinePageOriginal.getName(), e); On 2016/08/15 20:50:00, Theresa Wellington wrote: > If we fail to delete the old shareable file, what happens if we copy (will the > bytes get appended to the existing shareable file or will they overwrite some or > all of the existing bytes)? Should we skip copying? Good question! It will overwrite the some of the existing bytes (up to the number of bytes contained in the file copied from). So there's a chance that some bytes in the old file is not overwritten. I decide to keep the delete old page part and added a return. File copying would be skipped if delete failed. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:339: Uri offlineUri = null; On 2016/08/15 20:50:00, Theresa Wellington wrote: > On 2016/08/13 15:58:48, Theresa Wellington wrote: > > Eventually we'll want to be able to share the mhtml for an offline page from > the > > download manger UI as well as the "share" menu item. For the > DownloadManagerUi, > > it would be helpful if there were a static method that took the offlineUrl > (and > > possibly the title) and returned a content:// URI. Can that piece be > extracted > > into a separate method that can be shared? > > I think we can skip this for now. Acknowledged. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:346: }.execute(); On 2016/08/15 20:50:00, Theresa Wellington wrote: > Add AsyncTask.SERIAL_EXECUTOR here. Not specifying leads to inconsistent > behavior since different versions of Android have different default executors. Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:351: * @param src file path of the original file to be copied On 2016/08/15 20:50:00, Theresa Wellington wrote: > nits: > remove "This method" > > capitalize the first word of the param description ("Path") and end with a > period. > > Also, since the parameter is a File and not a String (path) I think it'd make > more sense to just say "The original file to be copied." and "The destination > file." Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:369: } catch (IOException e) { On 2016/08/15 20:50:00, Theresa Wellington wrote: > This can be combined with the catch above: > > ... > } catch (FileNotFound | IOException e) { > ... > } finally { > .... Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:380: * Get a directory for offline page sharing operation. On 2016/08/15 20:50:00, Theresa Wellington wrote: > nit: "Gets the directory to use for sharing offline pages, creating it if > necessary." Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:382: * @return path to directory for the shared file to be stored On 2016/08/15 20:50:00, Theresa Wellington wrote: > nit: capitalize Path Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:386: File path = new File(context.getExternalCacheDir(), EXTERNAL_MHTML_FILE_PATH); On 2016/08/16 03:00:10, Theresa Wellington wrote: > On 2016/08/15 20:49:59, Theresa Wellington wrote: > > I think we should create a static variable that caches this file rather than > > creating it each time. This method will become something like: > > > > if (mOfflineSharingDirectory == null { > > mOfflineSharingDirectory = .... > > } > > > > return mOfflineSharingDirectory. > > *sOfflineSharingDirectory (since it'd be static) Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:396: * This step is used to ensure that file name can be recognized by intent filter (.*\\.mhtml"). On 2016/08/15 20:50:00, Theresa Wellington wrote: > nit: "... (.*\\.mhtml) as Android's path..." Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:418: }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); On 2016/08/15 20:50:00, Theresa Wellington wrote: > This should be done on the SERIAL_EXECUTOR so that clearing shared files can't > get interleaved with creating files for sharing. Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:221: String title, String text, String url, Uri offlineUri, Bitmap screenshot, On 2016/08/13 15:58:48, Theresa Wellington wrote: > Use the @Nullable annotation for offlineUri Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:308: final String title, final String text, final String url, final Uri offlineUri, On 2016/08/15 20:50:00, Theresa Wellington wrote: > nit: add JavaDoc for offlineUri here and below Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:518: intent.addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION); On 2016/08/15 20:50:00, Theresa Wellington wrote: > Since the offline page file is getting copied to an external directory, granting > this flag may not be necessary. Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:245: String testUrl = mTestServer.getURL(TEST_PAGE); On 2016/08/15 20:50:00, Theresa Wellington wrote: > It looks like the code to save an offline page is used in 3 places. I think it > makes sense to pull it out into a helper method. Done. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:266: // Use a random file name for test issue. On 2016/08/15 20:50:00, Theresa Wellington wrote: > What is the test issue that requires using a random file name? This is deprecated after adding clearing directory before copying. Changed back to original file name. https://codereview.chromium.org/2081153005/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:269: assertFalse("Should not exist a file with same file name,", offlinePageShareable.exists()); On 2016/08/15 20:50:00, Theresa Wellington wrote: > "The shareable offline page file should lot already exist." Done.
https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:325: Log.e(TAG, "Failed to delete: " + offlinePageOriginal.getName(), e); On 2016/08/16 17:40:12, Vivian wrote: > On 2016/08/15 20:50:00, Theresa Wellington wrote: > > If we fail to delete the old shareable file, what happens if we copy (will the > > bytes get appended to the existing shareable file or will they overwrite some > or > > all of the existing bytes)? Should we skip copying? > > Good question! > It will overwrite the some of the existing bytes (up to the number of bytes > contained in the file copied from). So there's a chance that some bytes in the > old file is not overwritten. > I decide to keep the delete old page part and added a return. File copying would > be skipped if delete failed. Sounds good, thanks! https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:286: * older Android versions). nit: specify which versions of Android it doesn't work with so we can update this comment years in the future when we no longer have to worry about those "older" versions. https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:347: clearSharedOfflineFiles(activity); Don't we need to leave the offline page in the external directory while the intent is being sent?
https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:286: * older Android versions). On 2016/08/16 18:47:20, Theresa Wellington wrote: > nit: specify which versions of Android it doesn't work with so we can update > this comment years in the future when we no longer have to worry about those > "older" versions. I copied this parameter from ShareHelper because it is needed in ShareHelper.share. Not really sure which version they're referring to. https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:347: clearSharedOfflineFiles(activity); On 2016/08/16 18:47:20, Theresa Wellington wrote: > Don't we need to leave the offline page in the external directory while the > intent is being sent? Right. I put it there for debug and forget to delete it.
This is looking good overall. I'm going to do a final pass this evening or first thing tomorrow morning. https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:286: * older Android versions). On 2016/08/16 20:08:55, Vivian wrote: > On 2016/08/16 18:47:20, Theresa Wellington wrote: > > nit: specify which versions of Android it doesn't work with so we can update > > this comment years in the future when we no longer have to worry about those > > "older" versions. > > I copied this parameter from ShareHelper because it is needed in > ShareHelper.share. Not really sure which version they're referring to. It looks like it's only on Lollipop MR1+ (very new!): https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... For this comment, how we just reference that code? e.g. replace "(on older Android versions)" with "(see TargetChosenReceiver#isSupported())"
The CQ bit was checked by weiran@google.com 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.
lgtm % a few comments https://codereview.chromium.org/2081153005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:315: private static void prepareForSharing(final boolean shareDirectly, final boolean saveLastUsed, nit: this method also ultimately calls ShareHelper.share() so maybe it should be called "prepareFileAndShare" or something like that. https://codereview.chromium.org/2081153005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:395: * @return Path to directory for the shared file to be stored. nit: "Path to the directory where shared files are stored." https://codereview.chromium.org/2081153005/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:265: boolean success = (offlineCacheDir != null); nit: inline this check below since success is never reused. Same for testDeleteSharedOfflineFiles()
Comments addressed. Thanks for the reviews! https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:286: * older Android versions). On 2016/08/17 01:30:13, Theresa Wellington wrote: > On 2016/08/16 20:08:55, Vivian wrote: > > On 2016/08/16 18:47:20, Theresa Wellington wrote: > > > nit: specify which versions of Android it doesn't work with so we can update > > > this comment years in the future when we no longer have to worry about those > > > "older" versions. > > > > I copied this parameter from ShareHelper because it is needed in > > ShareHelper.share. Not really sure which version they're referring to. > > It looks like it's only on Lollipop MR1+ (very new!): > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > For this comment, how we just reference that code? e.g. replace "(on older > Android versions)" with "(see TargetChosenReceiver#isSupported())" Done. https://codereview.chromium.org/2081153005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2081153005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:315: private static void prepareForSharing(final boolean shareDirectly, final boolean saveLastUsed, On 2016/08/17 17:12:03, Theresa Wellington wrote: > nit: this method also ultimately calls ShareHelper.share() so maybe it should be > called "prepareFileAndShare" or something like that. Done. https://codereview.chromium.org/2081153005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:395: * @return Path to directory for the shared file to be stored. On 2016/08/17 17:12:03, Theresa Wellington wrote: > nit: "Path to the directory where shared files are stored." Done. https://codereview.chromium.org/2081153005/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/2081153005/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:265: boolean success = (offlineCacheDir != null); On 2016/08/17 17:12:03, Theresa Wellington wrote: > nit: inline this check below since success is never reused. Same for > testDeleteSharedOfflineFiles() Done.
lgtm, sorry for the long delay. Awesome to land this!
lgtm Just some comments about grammatical thingies. https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:335: // Old shareable files are stored in external directory, which may cause in an external? https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:338: // 2. Difference in file size which result in partial overwritten. Difference in file sizes that results in partial overwrite? https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:270: assertFalse("Should not exist a file with same file name,", offlinePageShareable.exists()); File with the same name should not exist.
Submitting for dry-run. https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:335: // Old shareable files are stored in external directory, which may cause On 2016/08/17 20:41:36, dfalcantara wrote: > in an external? Done. https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:338: // 2. Difference in file size which result in partial overwritten. On 2016/08/17 20:41:36, dfalcantara wrote: > Difference in file sizes that results in partial overwrite? Done. https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://chromiumcodereview.appspot.com/2081153005/diff/400001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:270: assertFalse("Should not exist a file with same file name,", offlinePageShareable.exists()); On 2016/08/17 20:41:36, dfalcantara wrote: > File with the same name should not exist. Done.
The CQ bit was checked by weiran@google.com 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.
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, twellington@chromium.org, dfalcantara@chromium.org, dewittj@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2081153005/#ps420001 (title: "Grammar fix")
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 ========== Primary implementation of offline page sharing This is the primary implementation of offline page sharing feature. If current tab is an offline page, clicking on share would result in: the corresponding mhtml file copied to the external storage, and, the the copied file in the external storage is passed in intent to be shared using third party application. BUG=622139 ========== to ========== Primary implementation of offline page sharing This is the primary implementation of offline page sharing feature. If current tab is an offline page, clicking on share would result in: the corresponding mhtml file copied to the external storage, and, the the copied file in the external storage is passed in intent to be shared using third party application. BUG=622139 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Primary implementation of offline page sharing This is the primary implementation of offline page sharing feature. If current tab is an offline page, clicking on share would result in: the corresponding mhtml file copied to the external storage, and, the the copied file in the external storage is passed in intent to be shared using third party application. BUG=622139 ========== to ========== Primary implementation of offline page sharing This is the primary implementation of offline page sharing feature. If current tab is an offline page, clicking on share would result in: the corresponding mhtml file copied to the external storage, and, the the copied file in the external storage is passed in intent to be shared using third party application. BUG=622139 Committed: https://crrev.com/be3aae7913a59268351546d2a17686fd5a79371a Cr-Commit-Position: refs/heads/master@{#412661} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/be3aae7913a59268351546d2a17686fd5a79371a Cr-Commit-Position: refs/heads/master@{#412661} |
