Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2364133002:
Download WebAPK into a WebAPK-specifc directory, and delete all cached ones. (Closed)
DescriptionDownload WebAPK into WebAPK-specifc directory, and delete all cached
APKs before downloading a new one.
BUG=634208
Committed: https://crrev.com/3f0eca90e1ae6168837a85f01a644b729d326f67
Cr-Commit-Position: refs/heads/master@{#421439}
Patch Set 1 #
Total comments: 4
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 14
Patch Set 3 : Nits. #
Total comments: 8
Patch Set 4 : dominickn@'s comments. #
Total comments: 2
Messages
Total messages: 38 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Download WebApk into WebApk-specifc directory, and delete all cached apks before downloading a new on. BUG=634208 ========== to ========== Download WebApk into WebApk-specifc directory, and delete all cached apks before downloading a new one. BUG=634208 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
As mentioned offline, it would be interesting to find out how much time does the deletion add to the WebAPK install time. Here are the cases which interest me: - Nothing to delete. There is still a penalty because the code checks whether the directory exists, deletes it, then recreates it - 1 downloaded file to delete - Many downloaded files to delete (e.g. 5)
On 2016/09/23 18:25:26, pkotwicz wrote: > As mentioned offline, it would be interesting to find out how much time does the > deletion add to the WebAPK install time. > Here are the cases which interest me: > - Nothing to delete. There is still a penalty because the code checks whether > the directory exists, deletes it, then recreates it I repeated every cases twice. It takes 240 / 246 micro seconds. > - 1 downloaded file to delete It takes 615 / 468 / 577 micro seconds, less than 1 milliseconds. > - Many downloaded files to delete (e.g. 5) It takes 2281 micro seconds, still less than 1 milliseconds.
On 2016/09/23 20:55:03, Xi Han wrote: > On 2016/09/23 18:25:26, pkotwicz wrote: > > As mentioned offline, it would be interesting to find out how much time does > the > > deletion add to the WebAPK install time. > > Here are the cases which interest me: > > - Nothing to delete. There is still a penalty because the code checks whether > > the directory exists, deletes it, then recreates it > I repeated every cases twice. It takes 240 / 246 micro seconds. > > > - 1 downloaded file to delete > It takes 615 / 468 / 577 micro seconds, less than 1 milliseconds. > > > > - Many downloaded files to delete (e.g. 5) > It takes 2281 micro seconds.
On 2016/09/23 20:55:03, Xi Han wrote: > On 2016/09/23 18:25:26, pkotwicz wrote: > > As mentioned offline, it would be interesting to find out how much time does > the > > deletion add to the WebAPK install time. > > Here are the cases which interest me: > > - Nothing to delete. There is still a penalty because the code checks whether > > the directory exists, deletes it, then recreates it > I repeated every cases twice. It takes 240 / 246 micro seconds. > > > - 1 downloaded file to delete > It takes 615 / 468 / 577 micro seconds, less than 1 milliseconds. > > > > - Many downloaded files to delete (e.g. 5) > It takes 2281 micro seconds.
Wow! The deletion is faster than I expected https://codereview.chromium.org/2364133002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2364133002/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:61: base::FILE_PERMISSION_WRITE_BY_OTHERS; I did some local testing. According to my testing, FILE_PERMISSION_WRITE_BY_GROUP and FILE_PERMISSION_WRITE_BY_OTHERS seem unnecessary https://codereview.chromium.org/2364133002/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:148: task_type == WebApkInstaller::INSTALL ? "install" : "update"); Can you explain why you use a different directory name for installs and updates?
Patchset #2 (id:80001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2364133002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2364133002/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:61: base::FILE_PERMISSION_WRITE_BY_OTHERS; On 2016/09/24 01:28:35, pkotwicz wrote: > I did some local testing. According to my testing, > FILE_PERMISSION_WRITE_BY_GROUP and FILE_PERMISSION_WRITE_BY_OTHERS seem > unnecessary Good catch, reverted it back. https://codereview.chromium.org/2364133002/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:148: task_type == WebApkInstaller::INSTALL ? "install" : "update"); On 2016/09/24 01:28:34, pkotwicz wrote: > Can you explain why you use a different directory name for installs and updates? As discussed offline, add a comment here.
Description was changed from ========== Download WebApk into WebApk-specifc directory, and delete all cached apks before downloading a new one. BUG=634208 ========== to ========== Download WebApk into WebApk-specifc directory, and delete all cached APKs before downloading a new one. BUG=634208 ==========
LGTM with nits https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:55: const int KBasicPosixPermissions = base::FILE_PERMISSION_READ_BY_USER | Nit: rename |KBasicPosixPermissions| to |kWorldReadableFilePermission|. I don't think that making a file world readable is the "default" setting for a file. Can you move FILE_PERMISSION_EXECUTE_BY_USER to line 153 and out of |kWorldReadableFilePermission| (Also lowercase "k". I have done this typo in the past and will probably do it again) https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:141: // which helps to clean up cached data. Creating different downloaded How about: "Previously downloaded APKs are deleted in order to clean up unused cached data." In my version I removed: "before downloading a new one" because this function does not know about when the download occurs https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:150: base::FilePath webapk_dir = output_root_dir.AppendASCII("webapks"); Move the part of the comment which starts with "Creating different downloaded directory" here because it is an implementation detail https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:387: base::FilePath output_dir; Nit: Move GetCacheDirectory() to CreateSubDirAndSetPermissionsInBackground() https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:400: const GURL& download_url, const base::FilePath& output_dir) { Nit: OnCreateSubDirAndSetPermissions() -> OnCreatedSubDirAndSetPermissions() "created" not "create" because the directory would have been created in the past when this function is called https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:153: // created with permissions set properly or failed to create. Nit: "or failed to create." -> "or if creation fails."
Description was changed from ========== Download WebApk into WebApk-specifc directory, and delete all cached APKs before downloading a new one. BUG=634208 ========== to ========== Download WebAPK into WebAPK-specifc directory, and delete all cached APKs before downloading a new one. BUG=634208 ==========
https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:152: // Called once the sub directory to store the downloaded WebApk was WebApk -> WebAPK
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dominick, could you please take a look? Thanks! https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:55: const int KBasicPosixPermissions = base::FILE_PERMISSION_READ_BY_USER | On 2016/09/26 21:04:25, pkotwicz wrote: > Nit: rename |KBasicPosixPermissions| to |kWorldReadableFilePermission|. > > I don't think that making a file world readable is the "default" setting for a > file. > > Can you move FILE_PERMISSION_EXECUTE_BY_USER to line 153 and out of > |kWorldReadableFilePermission| > > (Also lowercase "k". I have done this typo in the past and will probably do it > again) Done. https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:141: // which helps to clean up cached data. Creating different downloaded On 2016/09/26 21:04:25, pkotwicz wrote: > How about: "Previously downloaded APKs are deleted in order to clean up unused > cached data." > > In my version I removed: "before downloading a new one" because this function > does not know about when the download occurs Acknowledged. https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:150: base::FilePath webapk_dir = output_root_dir.AppendASCII("webapks"); On 2016/09/26 21:04:24, pkotwicz wrote: > Move the part of the comment which starts with "Creating different downloaded > directory" here because it is an implementation detail Done. https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:387: base::FilePath output_dir; On 2016/09/26 21:04:24, pkotwicz wrote: > Nit: Move GetCacheDirectory() to CreateSubDirAndSetPermissionsInBackground() Done. https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:400: const GURL& download_url, const base::FilePath& output_dir) { On 2016/09/26 21:04:25, pkotwicz wrote: > Nit: OnCreateSubDirAndSetPermissions() -> OnCreatedSubDirAndSetPermissions() > > "created" not "create" because the directory would have been created in the past > when this function is called Done. https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:152: // Called once the sub directory to store the downloaded WebApk was On 2016/09/26 21:05:49, pkotwicz wrote: > WebApk -> WebAPK Done. https://codereview.chromium.org/2364133002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:153: // created with permissions set properly or failed to create. On 2016/09/26 21:04:25, pkotwicz wrote: > Nit: "or failed to create." -> "or if creation fails." Change to "or if creation failed".
https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:56: base::FILE_PERMISSION_WRITE_BY_USER | Why is WRITE_BY_USER in the world-readable list here? https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:139: // The previously downloaded APKs are deleted in order to clean up unused Nit: fill comment to 80 chaarcters https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:41: enum TaskType { Nit: is this enum going to be used outside of the class?
Hi Dominick, PTAL, thanks! https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:56: base::FILE_PERMISSION_WRITE_BY_USER | On 2016/09/27 03:16:24, dominickn wrote: > Why is WRITE_BY_USER in the world-readable list here? Good point, move it out. https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:139: // The previously downloaded APKs are deleted in order to clean up unused On 2016/09/27 03:16:24, dominickn wrote: > Nit: fill comment to 80 chaarcters Done. https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:41: enum TaskType { On 2016/09/27 03:16:24, dominickn wrote: > Nit: is this enum going to be used outside of the class? This enum is used by CreateSubDirAndSetPermissionsInBackground() in the anonymous namespace. The function is called by PostTaskAndReplyWithResult() as the first function. Since we need a return value from CreateSubDirAndSetPermissionsInBackground(), so it doesn't allow to be bound with a weak pointer. I have to declare it as a function in anonymous namespace. Are you ok with it? Or do you prefer to post task back without using PostTaskAndReplyWithResult()?
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:41: enum TaskType { On 2016/09/27 12:55:13, Xi Han (OOO 9.28-10.20) wrote: > On 2016/09/27 03:16:24, dominickn wrote: > > Nit: is this enum going to be used outside of the class? > > This enum is used by CreateSubDirAndSetPermissionsInBackground() in the > anonymous namespace. The function is called by PostTaskAndReplyWithResult() as > the first function. Since we need a return value from > CreateSubDirAndSetPermissionsInBackground(), so it doesn't allow to be bound > with a weak pointer. I have to declare it as a function in anonymous namespace. > Are you ok with it? Or do you prefer to post task back without using > PostTaskAndReplyWithResult()? It looks like task_type is only used in CreateSubDirAndSetPermissionsInBackground to choose which suffix to add to the directory. Instead, you could make that method take a base::StringPiece argument which is the suffix, and just append it. Then the caller inside WebApkInstaller uses task_type_ to work out what suffix is necessary, saving you from exposing the internal TaskType detail. Would that work?
Patchset #5 (id:160001) has been deleted
Patchset #4 (id:140001) has been deleted
Hi Dominick, PTAL, thanks for the quick reply! https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2364133002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:41: enum TaskType { On 2016/09/28 00:56:28, dominickn wrote: > On 2016/09/27 12:55:13, Xi Han (OOO 9.28-10.20) wrote: > > On 2016/09/27 03:16:24, dominickn wrote: > > > Nit: is this enum going to be used outside of the class? > > > > This enum is used by CreateSubDirAndSetPermissionsInBackground() in the > > anonymous namespace. The function is called by PostTaskAndReplyWithResult() as > > the first function. Since we need a return value from > > CreateSubDirAndSetPermissionsInBackground(), so it doesn't allow to be bound > > with a weak pointer. I have to declare it as a function in anonymous > namespace. > > Are you ok with it? Or do you prefer to post task back without using > > PostTaskAndReplyWithResult()? > > It looks like task_type is only used in > CreateSubDirAndSetPermissionsInBackground to choose which suffix to add to the > directory. Instead, you could make that method take a base::StringPiece argument > which is the suffix, and just append it. Then the caller inside WebApkInstaller > uses task_type_ to work out what suffix is necessary, saving you from exposing > the internal TaskType detail. Would that work? Done.
lgtm % followup, thanks. :) https://codereview.chromium.org/2364133002/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2364133002/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:162: return base::FilePath(); Minor nit/question: is it appropriate to clean up output_dir and webapk_dir here if either of the permission setting calls fails? Not really sure if that's necessary/appropriate, so I'm not going to block on it. Might be worth following up though.
https://codereview.chromium.org/2364133002/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2364133002/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:162: return base::FilePath(); On 2016/09/28 02:15:03, dominickn wrote: > Minor nit/question: is it appropriate to clean up output_dir and webapk_dir here > if either of the permission setting calls fails? Not really sure if that's > necessary/appropriate, so I'm not going to block on it. Might be worth following > up though. Good point. I am not sure in which cases the setting permissions would fail, but it is not impossible. The good thing is these code is only temporary and will not be used once the installation goes through Play. Anyway, I will keep this in mind:)
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2364133002/#ps180001 (title: "dominickn@'s comments.")
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 ========== Download WebAPK into WebAPK-specifc directory, and delete all cached APKs before downloading a new one. BUG=634208 ========== to ========== Download WebAPK into WebAPK-specifc directory, and delete all cached APKs before downloading a new one. BUG=634208 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Download WebAPK into WebAPK-specifc directory, and delete all cached APKs before downloading a new one. BUG=634208 ========== to ========== Download WebAPK into WebAPK-specifc directory, and delete all cached APKs before downloading a new one. BUG=634208 Committed: https://crrev.com/3f0eca90e1ae6168837a85f01a644b729d326f67 Cr-Commit-Position: refs/heads/master@{#421439} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3f0eca90e1ae6168837a85f01a644b729d326f67 Cr-Commit-Position: refs/heads/master@{#421439} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
