Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
643803003:
Add UMA for testing whether device supports memory mapping APK files with executable permissions. (Closed)
|
Created:
6 years, 2 months ago by petrcermak Modified:
6 years, 2 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, asvitkine+watch_chromium.org, simonb (inactive), Sami Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA for testing whether device supports memory mapping APK files with executable permissions.
The following histogram is added:
ChromiumAndroidLinker.LoadFromApkSucceeded
Whether memory mapping the Chromium APK file with executable permissions
succeeded.
BUG=390618
Committed: https://crrev.com/a0b9d8cd4fb52e849aeb8174657518f6404c4008
Cr-Commit-Position: refs/heads/master@{#299461}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Update for review feedback #
Total comments: 18
Patch Set 3 : Update for review feedback #
Total comments: 4
Patch Set 4 : UMA update for review feedback #
Total comments: 8
Patch Set 5 : Update for review feedback #Patch Set 6 : Rebase after 611393002 landed #
Messages
Total messages: 24 (8 generated)
petrcermak@chromium.org changed reviewers: + rmcilroy@chromium.org
rmcilroy@chromium.org changed reviewers: + simonb@chromium.org
Looks good overall but a couple of suggestions. Adding SimonB incase he has some suggestions. Thanks! https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:300: Linker.loadFromApkSucceeded()); I think we could just put as an argument in nativeRecordChromiumAndroidLinkerHistogram rather than adding a new method. https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:869: return sLoadFromApkSucceeded; Simon's CL moves from storing the result as a static in Linker.java to storing the result in LibraryLoader.java - I would prefer you follow the same way for this change. https://codereview.chromium.org/643803003/diff/1/third_party/android_crazy_li... File third_party/android_crazy_linker/src/include/crazy_linker.h (right): https://codereview.chromium.org/643803003/diff/1/third_party/android_crazy_li... third_party/android_crazy_linker/src/include/crazy_linker.h:352: _CRAZY_PUBLIC; I don't think we want to extend the crazy linker API for this - could you just do the test in linker_jni.cc instead?
picksi@chromium.org changed reviewers: + picksi@chromium.org
Some nit-picky thoughts on naming and stuff. Mostly discussion points rather than requests! https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:159: "testLoadFromApk" makes this look like a unit-test and doesn't imply the UMA stuff (assuming this is the top level call). Can we rename this to (something like) generateDirectAPKLoadingUMAstats? https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:163: zipfile = context.getApplicationInfo().sourceDir; We called context.getApplicationInfo().sourceDir earlier (as a parameter to the new function), should we call it & cache it earlier? The way that zipfile is used here looks odd to me anyway, as its defined outside the scope it gets used in, and needlessly initialised. https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:301: } On first reading this code I didn't know if loadFromApkSucceeded() was returning a true/false value, or implementing a behaviour that needs to happen when loading was successful (like a callback or something). Can we rename it to make it clear if it's a Getter or a callback ... Or could we return an enum that this code can check directly e.g. loadFromApkStatus()==SUCCESS? Is loadFromAPK really a binary value? Can it fail for different reasons? https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:227: Is this really a bool? Should this be an enum that defines the possible states? e.g. LOAD_NOT_ATTEMPTED, LOAD_FAILED_DUE_TO_MISSING_FILE, LOAD_FAILED_DUE_TO_PERMISSIONS, LOAD_SUCCESSFUL? https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:870: } return sAPKStatus == LOADING_SUCCESSFUL ? Or (my favourite pattern!) replace this with a check function that you call with an enum, so you can say: if (IsLoadFromAPKStatus(LOADING_SUCCESSFUL))? https://codereview.chromium.org/643803003/diff/1/base/android/library_loader/... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/643803003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:52: JNIEnv* env, Is this the naming convention? If so fine, otherwise it might be more readable as RecordHistogramForChromiumAndroidLinkerLoadFromAPKSucceeded? Although a shorter name wouldn't hurt! https://codereview.chromium.org/643803003/diff/1/third_party/android_crazy_li... File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right): https://codereview.chromium.org/643803003/diff/1/third_party/android_crazy_li... third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:402: } I think this would be clearer is we swapped the if/else round and remove the default status assignment, e.g. crazy_status_t status; if (address == MAP_FAILED) { status = CRAZY_STATUS_FAILURE } else { status = CRAZY_STATUS_SUCCESS; munmap(address, PAGE_SIZE); }
https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:159: On 2014/10/10 10:52:17, picksi1 wrote: > "testLoadFromApk" makes this look like a unit-test and doesn't imply the UMA > stuff (assuming this is the top level call). Can we rename this to (something > like) generateDirectAPKLoadingUMAstats? I would agree that test is perhaps wrong, but would rather we kept UMA out of the name since we might eventually use the same function to check whether we want to use the fallback or not. Maybe call it checkLoadLibraryFromAPK or similar?
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:270001) has been deleted
Patchset #2 (id:340001) has been deleted
petrcermak@chromium.org changed reviewers: + isherman@chromium.org
Thanks for the review. I modified the patch according to your comments. PTAL. https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:159: On 2014/10/10 11:02:31, rmcilroy wrote: > On 2014/10/10 10:52:17, picksi1 wrote: > > "testLoadFromApk" makes this look like a unit-test and doesn't imply the UMA > > stuff (assuming this is the top level call). Can we rename this to (something > > like) generateDirectAPKLoadingUMAstats? > > I would agree that test is perhaps wrong, but would rather we kept UMA out of > the name since we might eventually use the same function to check whether we > want to use the fallback or not. > > Maybe call it checkLoadLibraryFromAPK or similar? Done. Renamed to "checkApkMemoryMappingSupport". https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:300: Linker.loadFromApkSucceeded()); On 2014/10/10 10:27:05, rmcilroy wrote: > I think we could just put as an argument in > nativeRecordChromiumAndroidLinkerHistogram rather than adding a new method. Done. https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:301: } On 2014/10/10 10:52:17, picksi1 wrote: > On first reading this code I didn't know if loadFromApkSucceeded() was returning > a true/false value, or implementing a behaviour that needs to happen when > loading was successful (like a callback or something). Can we rename it to make > it clear if it's a Getter or a callback ... Or could we return an enum that this > code can check directly e.g. loadFromApkStatus()==SUCCESS? Is loadFromAPK really > a binary value? Can it fail for different reasons? Removed method. https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:227: On 2014/10/10 10:52:17, picksi1 wrote: > Is this really a bool? Should this be an enum that defines the possible states? > e.g. LOAD_NOT_ATTEMPTED, LOAD_FAILED_DUE_TO_MISSING_FILE, > LOAD_FAILED_DUE_TO_PERMISSIONS, LOAD_SUCCESSFUL? As discussed, we will keep the boolean because we consider only two possible states (supported, not supported) and it simplifies code (if we used an enum, we would essentially need to define it twice because of the native call). https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:869: return sLoadFromApkSucceeded; On 2014/10/10 10:27:05, rmcilroy wrote: > Simon's CL moves from storing the result as a static in Linker.java to storing > the result in LibraryLoader.java - I would prefer you follow the same way for > this change. Done. https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/Linker.java:870: } On 2014/10/10 10:52:17, picksi1 wrote: > return sAPKStatus == LOADING_SUCCESSFUL ? Or (my favourite pattern!) replace > this with a check function that you call with an enum, so you can say: > > if (IsLoadFromAPKStatus(LOADING_SUCCESSFUL))? As explained above, we stick with the boolean type. https://codereview.chromium.org/643803003/diff/1/base/android/library_loader/... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/643803003/diff/1/base/android/library_loader/... base/android/library_loader/library_loader_hooks.cc:52: JNIEnv* env, On 2014/10/10 10:52:17, picksi1 wrote: > Is this the naming convention? If so fine, otherwise it might be more readable > as RecordHistogramForChromiumAndroidLinkerLoadFromAPKSucceeded? Although a > shorter name wouldn't hurt! I moved the code to the existing RecordChromiumAndroidLinkerBrowserHistogram method. https://codereview.chromium.org/643803003/diff/1/third_party/android_crazy_li... File third_party/android_crazy_linker/src/include/crazy_linker.h (right): https://codereview.chromium.org/643803003/diff/1/third_party/android_crazy_li... third_party/android_crazy_linker/src/include/crazy_linker.h:352: _CRAZY_PUBLIC; On 2014/10/10 10:27:05, rmcilroy wrote: > I don't think we want to extend the crazy linker API for this - could you just > do the test in linker_jni.cc instead? Done. https://codereview.chromium.org/643803003/diff/1/third_party/android_crazy_li... File third_party/android_crazy_linker/src/src/crazy_linker_api.cpp (right): https://codereview.chromium.org/643803003/diff/1/third_party/android_crazy_li... third_party/android_crazy_linker/src/src/crazy_linker_api.cpp:402: } On 2014/10/10 10:52:18, picksi1 wrote: > I think this would be clearer is we swapped the if/else round and remove the > default status assignment, e.g. > > crazy_status_t status; > if (address == MAP_FAILED) { > status = CRAZY_STATUS_FAILURE > } > else { > status = CRAZY_STATUS_SUCCESS; > munmap(address, PAGE_SIZE); > } Done.
A suggestion on naming and a couple of small comments. https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/643803003/diff/1/base/android/java/src/org/ch... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:159: On 2014/10/10 14:20:13, petrcermak wrote: > On 2014/10/10 11:02:31, rmcilroy wrote: > > On 2014/10/10 10:52:17, picksi1 wrote: > > > "testLoadFromApk" makes this look like a unit-test and doesn't imply the UMA > > > stuff (assuming this is the top level call). Can we rename this to > (something > > > like) generateDirectAPKLoadingUMAstats? > > > > I would agree that test is perhaps wrong, but would rather we kept UMA out of > > the name since we might eventually use the same function to check whether we > > want to use the fallback or not. > > > > Maybe call it checkLoadLibraryFromAPK or similar? > > Done. Renamed to "checkApkMemoryMappingSupport". Sorry, I still think this is the wrong name since it's checking for mapping as executable. I would still call this checkLibraryLoadFromApkSupport. https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:54: // APK files with executable permissions. add: Only used in the browser. https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:55: private static boolean sApkMemoryMappingSupported = false; Can we make this a bit more generic (in case we add other checks here) - e.g., sLibraryLoadFromApkSupported. Also change the histogram.xml as such. https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:168: // permissions. nit - "Check if the device supports loading a library directly from the APK file." https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:171: context.getApplicationInfo().sourceDir); nit - pull out "context.getApplicationInfo().sourceDir" to a variable set above and used both here and L177 https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... File base/android/linker/linker_jni.cc (right): https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:583: // |env| is the current JNI environment handle and is ignored here. not ignored - used for string conversion https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:585: // and is ignored here. nit - I would just write "|class| is the static class handle which is not used here." https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:593: int fd = TEMP_FAILURE_RETRY(open(apkfile_name_c_str, O_RDONLY)); I would avoid the TEMP_FAILURE_RETRY - it's not super critical that this succeeds and I'm worried that we might end up in a retry loop here if glibc decides it needs to keep retrying. https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:605: success = 0; /s/0/false (and true for '1' below) https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:611: TEMP_FAILURE_RETRY(close(fd)); ditto
Thanks for your comments. PTAL. https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java (right): https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:54: // APK files with executable permissions. On 2014/10/10 16:26:08, rmcilroy wrote: > add: Only used in the browser. Done. https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:55: private static boolean sApkMemoryMappingSupported = false; On 2014/10/10 16:26:08, rmcilroy wrote: > Can we make this a bit more generic (in case we add other checks here) - e.g., > sLibraryLoadFromApkSupported. Also change the histogram.xml as such. Done. https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:168: // permissions. On 2014/10/10 16:26:08, rmcilroy wrote: > nit - "Check if the device supports loading a library directly from the APK > file." Done. https://codereview.chromium.org/643803003/diff/410001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:171: context.getApplicationInfo().sourceDir); On 2014/10/10 16:26:07, rmcilroy wrote: > nit - pull out "context.getApplicationInfo().sourceDir" to a variable set above > and used both here and L177 Done. https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... File base/android/linker/linker_jni.cc (right): https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:583: // |env| is the current JNI environment handle and is ignored here. On 2014/10/10 16:26:08, rmcilroy wrote: > not ignored - used for string conversion Done. https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:585: // and is ignored here. On 2014/10/10 16:26:08, rmcilroy wrote: > nit - I would just write "|class| is the static class handle which is not used > here." Done. https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:593: int fd = TEMP_FAILURE_RETRY(open(apkfile_name_c_str, O_RDONLY)); On 2014/10/10 16:26:08, rmcilroy wrote: > I would avoid the TEMP_FAILURE_RETRY - it's not super critical that this > succeeds and I'm worried that we might end up in a retry loop here if glibc > decides it needs to keep retrying. Done. https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:605: success = 0; On 2014/10/10 16:26:08, rmcilroy wrote: > /s/0/false (and true for '1' below) Done. https://codereview.chromium.org/643803003/diff/410001/base/android/linker/lin... base/android/linker/linker_jni.cc:611: TEMP_FAILURE_RETRY(close(fd)); On 2014/10/10 16:26:08, rmcilroy wrote: > ditto Done.
https://codereview.chromium.org/643803003/diff/530001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/643803003/diff/530001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2711: + enum="Boolean"> nit: Please use a more custom-tailored boolean, e.g. "BooleanSupported", with labels like "Supported" and "Not supported". https://codereview.chromium.org/643803003/diff/530001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2714: + Whether the device supports loading a library directly from the APK file. Please document when this histogram is recorded.
great! lgtm.
Thanks for the comments. I modified the new histogram as you suggested. https://codereview.chromium.org/643803003/diff/530001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/643803003/diff/530001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2711: + enum="Boolean"> On 2014/10/10 22:26:05, Ilya Sherman wrote: > nit: Please use a more custom-tailored boolean, e.g. "BooleanSupported", with > labels like "Supported" and "Not supported". Done. https://codereview.chromium.org/643803003/diff/530001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2714: + Whether the device supports loading a library directly from the APK file. On 2014/10/10 22:26:05, Ilya Sherman wrote: > Please document when this histogram is recorded. Done.
lgtm, thanks. https://codereview.chromium.org/643803003/diff/630001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/643803003/diff/630001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:560: sInBrowserProcess = false; Note for future refactor - according to Simon this function could be called from the browser, so sInBrowserProcess doesn't necessarily mean we are in the browser process.... We really need to clean this up, but I think we should do that in a separate CL. https://codereview.chromium.org/643803003/diff/630001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:847: if (DEBUG) Log.i(TAG, "Memory mapping APK files with executable permissions " + nit - update logging https://codereview.chromium.org/643803003/diff/630001/base/android/library_lo... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/643803003/diff/630001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:101: nit - remove newline https://codereview.chromium.org/643803003/diff/630001/base/android/linker/lin... File base/android/linker/linker_jni.cc (right): https://codereview.chromium.org/643803003/diff/630001/base/android/linker/lin... base/android/linker/linker_jni.cc:597: LOG_INFO("%s: Memory mapping the first page of %s\n", __FUNCTION__, nit - mention "with executable permissions".
Thanks for the comments. https://codereview.chromium.org/643803003/diff/630001/base/android/java/src/o... File base/android/java/src/org/chromium/base/library_loader/Linker.java (right): https://codereview.chromium.org/643803003/diff/630001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:560: sInBrowserProcess = false; On 2014/10/13 11:08:19, rmcilroy wrote: > Note for future refactor - according to Simon this function could be called from > the browser, so sInBrowserProcess doesn't necessarily mean we are in the browser > process.... We really need to clean this up, but I think we should do that in a > separate CL. Acknowledged. https://codereview.chromium.org/643803003/diff/630001/base/android/java/src/o... base/android/java/src/org/chromium/base/library_loader/Linker.java:847: if (DEBUG) Log.i(TAG, "Memory mapping APK files with executable permissions " + On 2014/10/13 11:08:19, rmcilroy wrote: > nit - update logging Done. https://codereview.chromium.org/643803003/diff/630001/base/android/library_lo... File base/android/library_loader/library_loader_hooks.cc (right): https://codereview.chromium.org/643803003/diff/630001/base/android/library_lo... base/android/library_loader/library_loader_hooks.cc:101: On 2014/10/13 11:08:19, rmcilroy wrote: > nit - remove newline I would prefer to keep it there to make it clear that the two histograms are independent. Instead, I added a comment describing the new UMA call. https://codereview.chromium.org/643803003/diff/630001/base/android/linker/lin... File base/android/linker/linker_jni.cc (right): https://codereview.chromium.org/643803003/diff/630001/base/android/linker/lin... base/android/linker/linker_jni.cc:597: LOG_INFO("%s: Memory mapping the first page of %s\n", __FUNCTION__, On 2014/10/13 11:08:19, rmcilroy wrote: > nit - mention "with executable permissions". Done.
histograms lgtm, thanks
The CQ bit was checked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/643803003/790001
Message was sent while issue was closed.
Committed patchset #6 (id:790001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a0b9d8cd4fb52e849aeb8174657518f6404c4008 Cr-Commit-Position: refs/heads/master@{#299461} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
