|
|
DescriptionAdd WebVR histograms for requestPresent failures/success and headset types.
Add usage counters for gamepad extensions.
BUG=389343
Committed: https://crrev.com/6805d8ab4296d9033602bad708da31e67e9fff6d
Cr-Commit-Position: refs/heads/master@{#424522}
Patch Set 1 #
Total comments: 40
Patch Set 2 : Add gamepad extension metrics, and address most code review feedback before rebase. #Patch Set 3 : Sync/rebase, histograms.xml update, and fix build errors #
Total comments: 2
Patch Set 4 : Update for remaining CR feedback, along with enum renames #
Total comments: 25
Patch Set 5 : sync/rebase #Patch Set 6 : CR feedback #
Total comments: 12
Patch Set 7 : CR feedback #
Total comments: 9
Patch Set 8 : CR feedback #
Total comments: 2
Patch Set 9 : sync/rebase #
Total comments: 6
Patch Set 10 : fix typo #Patch Set 11 : minor cr feedback #Messages
Total messages: 48 (28 generated)
https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:156: "ViewerType enum has changed - update histogram to avoid corrupt data"); This assert feels like it would fit better in a test rather than the init of production code. https://codereview.chromium.org/2388163005/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388163005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:92721: + <int value="5" label="InvalidNumberOfLayers"/> Do we need all of these 'layer' statuses? Can they be combined into a generic 'layer' problem or does each one provide useful data? (I'm not familiar with the underlying context so I really have no idea, but they seem rather specific).
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:122: enum class PresentationStatus { Can we avoid adding local UMAs like this? I'd prefer centralizing all UMA counters in UseCounter.h.
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:156: "ViewerType enum has changed - update histogram to avoid corrupt data"); On 2016/10/04 22:39:25, amp wrote: > This assert feels like it would fit better in a test rather than the init of > production code. It's a static assert, so it only "runs" at compile time. I assume this is to ensure that the meaning of the UMA data is not changed. Usually, we comment that in the definition of the enum, but I assume this is third-party code so that's not possible. I don't understand the second half of the message. What does it mean to update the histogram? I think we'd have to convert the data. Another alternative here would be to do that from the beginning - use a switch statement instead. We already have if statement conditions. What if we get a value we didn't expect? Currently, we just ignore it, which seems bad. https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:157: UMA_HISTOGRAM_ENUMERATION("VR.GVRViewerType", viewer_type, 2); This is Android-specific, but so is this file. We may later want to replace this with a more generic UMA, but this is fine for now. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRController.cpp:66: ReportPresentationStatus(PresentationStatus::ServiceShutDown); Do we know that it is shut down? Maybe Inactive? https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:166: ("VR.ReportPresentationStatus", Status or Result? https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:168: vrPresentationStatusHistogram.count((int)status); Use C++ cast (unless this is a common style for this). https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:197: m_isPresenting = false; // bug? every other call will be a first present Seems weird. Maybe file a crbug, but don't leave this comment. :) https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:224: ReportPresentationStatus(PresentationStatus::InvalidLayerSource); Is this intentional? https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:292: ReportPresentationStatus(PresentationStatus::Success); There are two success paths. That seems a bit odd. Is one "already presenting" or something? Also, the other was after resolve() was called. Probably do that here for consistency too. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:298: UseCounter::count(*document, UseCounter::VRPresent); Note that this path was previously considered the relevant one. I'm not sure what line 254 is. Also, this seems redundant and we should probably remove. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:127: MustBeInitiatedByUserGesture = 4, s/MustBe/Not/ ? That more accurately describes the reason for the failure, right? https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:134: PresentationStatusMax = 11, Should this have a fixed value?
Postponing to get deprecated APIs taken care of first. I'll incorporate feedback and and update this review after deprecated parts of webvr are taken care of. https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:156: "ViewerType enum has changed - update histogram to avoid corrupt data"); On 2016/10/05 17:48:20, ddorwin wrote: > On 2016/10/04 22:39:25, amp wrote: > > This assert feels like it would fit better in a test rather than the init of > > production code. > > It's a static assert, so it only "runs" at compile time. > > I assume this is to ensure that the meaning of the UMA data is not changed. > Usually, we comment that in the definition of the enum, but I assume this is > third-party code so that's not possible. > > I don't understand the second half of the message. What does it mean to update > the histogram? I think we'd have to convert the data. Another alternative here > would be to do that from the beginning - use a switch statement instead. We > already have if statement conditions. > > What if we get a value we didn't expect? Currently, we just ignore it, which > seems bad. I think the switch statement, and translating to our own enum could be more forward looking if we expect this to change. https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:157: UMA_HISTOGRAM_ENUMERATION("VR.GVRViewerType", viewer_type, 2); On 2016/10/05 17:48:20, ddorwin wrote: > This is Android-specific, but so is this file. We may later want to replace this > with a more generic UMA, but this is fine for now. I think using another enum and translating between the two would make this forwards compatible with other types in the future. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRController.cpp:66: ReportPresentationStatus(PresentationStatus::ServiceShutDown); On 2016/10/05 17:48:21, ddorwin wrote: > Do we know that it is shut down? Maybe Inactive? sure https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:166: ("VR.ReportPresentationStatus", On 2016/10/05 17:48:21, ddorwin wrote: > Status or Result? Result https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:168: vrPresentationStatusHistogram.count((int)status); On 2016/10/05 17:48:21, ddorwin wrote: > Use C++ cast (unless this is a common style for this). Acknowledged. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:197: m_isPresenting = false; // bug? every other call will be a first present On 2016/10/05 17:48:21, ddorwin wrote: > Seems weird. Maybe file a crbug, but don't leave this comment. :) I didn't intend to leave this comment in - will be removed, and appropriate bug filed (or I can fix it in this checkin, simply by setting m_isPresenting=true in the case below where we update bounds and resolve the resolver). https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:224: ReportPresentationStatus(PresentationStatus::InvalidLayerSource); On 2016/10/05 17:48:21, ddorwin wrote: > Is this intentional? no - fixing https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:292: ReportPresentationStatus(PresentationStatus::Success); On 2016/10/05 17:48:21, ddorwin wrote: > There are two success paths. That seems a bit odd. Is one "already presenting" > or something? > > Also, the other was after resolve() was called. Probably do that here for > consistency too. Yes, one is "already presenting" https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:298: UseCounter::count(*document, UseCounter::VRPresent); On 2016/10/05 17:48:21, ddorwin wrote: > Note that this path was previously considered the relevant one. I'm not sure > what line 254 is. > > Also, this seems redundant and we should probably remove. Acknowledged. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:122: enum class PresentationStatus { On 2016/10/05 01:18:42, haraken wrote: > > Can we avoid adding local UMAs like this? I'd prefer centralizing all UMA > counters in UseCounter.h. > We could, but then we only get per-page stats, not relative number of failures. We'd also need to add 10 new counters for each of the enum values. I'll think it over and talk to my team to decide what is appropriate. Note that this change is currently postponed, so there could be some delay before coming back to it. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:127: MustBeInitiatedByUserGesture = 4, On 2016/10/05 17:48:21, ddorwin wrote: > s/MustBe/Not/ ? > > That more accurately describes the reason for the failure, right? Acknowledged. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:134: PresentationStatusMax = 11, On 2016/10/05 17:48:21, ddorwin wrote: > Should this have a fixed value? We can add more in the future, growing the enum. https://codereview.chromium.org/2388163005/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388163005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:92721: + <int value="5" label="InvalidNumberOfLayers"/> On 2016/10/04 22:39:25, amp wrote: > Do we need all of these 'layer' statuses? Can they be combined into a generic > 'layer' problem or does each one provide useful data? (I'm not familiar with > the underlying context so I really have no idea, but they seem rather specific). They are distinct errors - some usage errors, and some less expected. They could be combined, but there is little penalty to having them, and the request was to measure the different types of failures.
Description was changed from ========== Add WebVR histograms for requestPresent failures/success and headset types. BUG=389343 ========== to ========== Add WebVR histograms for requestPresent failures/success and headset types. Add usage counters for gamepad extensions. BUG=389343 ==========
addressed all outstanding comments. I prefer to keep the enum for PresentationResult rather than flattening it to several useCounter values, as it makes processing the results easier. https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:156: "ViewerType enum has changed - update histogram to avoid corrupt data"); On 2016/10/05 17:48:20, ddorwin wrote: > On 2016/10/04 22:39:25, amp wrote: > > This assert feels like it would fit better in a test rather than the init of > > production code. > > It's a static assert, so it only "runs" at compile time. > > I assume this is to ensure that the meaning of the UMA data is not changed. > Usually, we comment that in the definition of the enum, but I assume this is > third-party code so that's not possible. > > I don't understand the second half of the message. What does it mean to update > the histogram? I think we'd have to convert the data. Another alternative here > would be to do that from the beginning - use a switch statement instead. We > already have if statement conditions. > > What if we get a value we didn't expect? Currently, we just ignore it, which > seems bad. I now have a catch-all "unknown" type for types we didn't expect. https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:156: "ViewerType enum has changed - update histogram to avoid corrupt data"); On 2016/10/05 17:48:20, ddorwin wrote: > On 2016/10/04 22:39:25, amp wrote: > > This assert feels like it would fit better in a test rather than the init of > > production code. > > It's a static assert, so it only "runs" at compile time. > > I assume this is to ensure that the meaning of the UMA data is not changed. > Usually, we comment that in the definition of the enum, but I assume this is > third-party code so that's not possible. > > I don't understand the second half of the message. What does it mean to update > the histogram? I think we'd have to convert the data. Another alternative here > would be to do that from the beginning - use a switch statement instead. We > already have if statement conditions. > > What if we get a value we didn't expect? Currently, we just ignore it, which > seems bad. Done. https://codereview.chromium.org/2388163005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:157: UMA_HISTOGRAM_ENUMERATION("VR.GVRViewerType", viewer_type, 2); On 2016/10/05 20:45:54, billorr wrote: > On 2016/10/05 17:48:20, ddorwin wrote: > > This is Android-specific, but so is this file. We may later want to replace > this > > with a more generic UMA, but this is fine for now. > > I think using another enum and translating between the two would make this > forwards compatible with other types in the future. Done. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRController.cpp:66: ReportPresentationStatus(PresentationStatus::ServiceShutDown); On 2016/10/05 17:48:21, ddorwin wrote: > Do we know that it is shut down? Maybe Inactive? Done. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:166: ("VR.ReportPresentationStatus", On 2016/10/05 20:45:54, billorr wrote: > On 2016/10/05 17:48:21, ddorwin wrote: > > Status or Result? > > Result Done. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:168: vrPresentationStatusHistogram.count((int)status); On 2016/10/05 20:45:54, billorr wrote: > On 2016/10/05 17:48:21, ddorwin wrote: > > Use C++ cast (unless this is a common style for this). > > Acknowledged. Done. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:197: m_isPresenting = false; // bug? every other call will be a first present On 2016/10/05 20:45:54, billorr wrote: > On 2016/10/05 17:48:21, ddorwin wrote: > > Seems weird. Maybe file a crbug, but don't leave this comment. :) > > I didn't intend to leave this comment in - will be removed, and appropriate bug > filed (or I can fix it in this checkin, simply by setting m_isPresenting=true in > the case below where we update bounds and resolve the resolver). Done. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:224: ReportPresentationStatus(PresentationStatus::InvalidLayerSource); On 2016/10/05 20:45:54, billorr wrote: > On 2016/10/05 17:48:21, ddorwin wrote: > > Is this intentional? > > no - fixing Done. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:292: ReportPresentationStatus(PresentationStatus::Success); On 2016/10/05 20:45:54, billorr wrote: > On 2016/10/05 17:48:21, ddorwin wrote: > > There are two success paths. That seems a bit odd. Is one "already presenting" > > or something? > > > > Also, the other was after resolve() was called. Probably do that here for > > consistency too. > > Yes, one is "already presenting" explicitly calling out "already presenting" https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:298: UseCounter::count(*document, UseCounter::VRPresent); On 2016/10/05 17:48:21, ddorwin wrote: > Note that this path was previously considered the relevant one. I'm not sure > what line 254 is. > > Also, this seems redundant and we should probably remove. removed https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:122: enum class PresentationStatus { On 2016/10/05 20:45:54, billorr wrote: > On 2016/10/05 01:18:42, haraken wrote: > > > > Can we avoid adding local UMAs like this? I'd prefer centralizing all UMA > > counters in UseCounter.h. > > > > We could, but then we only get per-page stats, not relative number of failures. > We'd also need to add 10 new counters for each of the enum values. > > I'll think it over and talk to my team to decide what is appropriate. > > Note that this change is currently postponed, so there could be some delay > before coming back to it. We really want the histogram capabilities rather than a use counter - we want to know the relative frequency of errors that this call can have, which doesn't make sense as a use counter. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:127: MustBeInitiatedByUserGesture = 4, On 2016/10/05 17:48:21, ddorwin wrote: > s/MustBe/Not/ ? > > That more accurately describes the reason for the failure, right? Done. https://codereview.chromium.org/2388163005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:134: PresentationStatusMax = 11, On 2016/10/05 20:45:54, billorr wrote: > On 2016/10/05 17:48:21, ddorwin wrote: > > Should this have a fixed value? > > We can add more in the future, growing the enum. Done. https://codereview.chromium.org/2388163005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388163005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66976: +<histogram name="VR.GVRViewerType" enum="gvr::ViewerType"> out of sync with code https://codereview.chromium.org/2388163005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:93882: +<enum name="PresentationStatus" type="int"> enum is out of sync with code
billorr@chromium.org changed reviewers: + bshe@chromium.org
bshe@chromium.org: Please OWNER review changes in vr_shell.cc. haraken, please OWNER review the files other than vr_shell.cc. Thanks!
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by billorr@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...
https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:198: TypeUnknown = 0, Chromium style for enums is MACRO_STYLE: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... Also, this directory appears to follow this style. https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:198: TypeUnknown = 0, nit: UnknownType reads better https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:199: GvrTypeCardboard = 1, We can probably drop GvrType since it doesn't matter how we figured out it is Cardboard, right? Same below. https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:227: viewerType = ViewerType::TypeUnknown; NOTREACHED(); before this line https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:230: UMA_HISTOGRAM_ENUMERATION("VRViewerType", static_cast<int>(viewerType), Does this only apply to VR shell or does WebVR go through this path? If the former, we should change the name. https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:160: ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState); to Do we need to worry about calls to requestPresent() that do not have a result? While we have a UseCounter for all calls, that is per-page and thus not measuring the same thing. I'm not sure what the paths might be (it would most likely happen in the service), but I would assume things can go wrong before beginPresent() is called asynchronously. Also, the page can probably be torn down during that time. We have two options: 1. Record all calls and compare the difference when looking at the data. - This would allow us to detect blind spots if all the results do not add up to this value. 2. Try to account for that by ensuring we always report a result. https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:218: ReportPresentationResult(PresentationResult::LayerSourceMissingWebGL); ...WebGLContext? https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:250: m_isPresenting = true; It would probably be better to fix this in a separate CL independent of the usage stats changes. Also, I think some comments would be appropriate to explain why that was set to false above (to handle all the failure cases with early returns). Still, maybe some refactoring would be better and have avoided this bug. https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:287: ReportPresentationResult(PresentationResult::UnsupportedDisplay); PresentationNotSupported or NotSupportedByDisplay? UnsupportedDisplay infers that we don't support the display rather than the display does not support presentation. https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66979: + Measures the frequency of different types of errors or success that may be "Measures the frequency of" seems unnecessary and redundant. This can simply be "The result of calls to VRDisplay::requestPresent()." https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66987: + Measures the frequency for different headset types to use VR with Chrome ditto. Also, as in the .cpp file, does this cover all VR or just VR shell? https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:100834: + <int value="0" label="TypeUnknown"/> Ditto on all three names
https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:198: TypeUnknown = 0, On 2016/10/06 22:31:28, ddorwin wrote: > nit: UnknownType reads better Done. https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:198: TypeUnknown = 0, On 2016/10/06 22:31:28, ddorwin wrote: > Chromium style for enums is MACRO_STYLE: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... > Also, this directory appears to follow this style. Done. https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:199: GvrTypeCardboard = 1, On 2016/10/06 22:31:28, ddorwin wrote: > We can probably drop GvrType since it doesn't matter how we figured out it is > Cardboard, right? Same below. Done. https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:227: viewerType = ViewerType::TypeUnknown; On 2016/10/06 22:31:28, ddorwin wrote: > NOTREACHED(); before this line Done. https://codereview.chromium.org/2388163005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:230: UMA_HISTOGRAM_ENUMERATION("VRViewerType", static_cast<int>(viewerType), On 2016/10/06 22:31:28, ddorwin wrote: > Does this only apply to VR shell or does WebVR go through this path? If the > former, we should change the name. Both go through this path. https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:160: ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState); On 2016/10/06 22:31:28, ddorwin wrote: > to Do we need to worry about calls to requestPresent() that do not have a > result? While we have a UseCounter for all calls, that is per-page and thus not > measuring the same thing. > > I'm not sure what the paths might be (it would most likely happen in the > service), but I would assume things can go wrong before beginPresent() is called > asynchronously. Also, the page can probably be torn down during that time. > > We have two options: > 1. Record all calls and compare the difference when looking at the data. > - This would allow us to detect blind spots if all the results do not add up > to this value. > 2. Try to account for that by ensuring we always report a result. I can add a "StartPresent" result to the enum to try to catch this. I was a bit worried about this as well, but convinced myself that it couldn't happen in our code. https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:218: ReportPresentationResult(PresentationResult::LayerSourceMissingWebGL); On 2016/10/06 22:31:28, ddorwin wrote: > ...WebGLContext? Done. https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:250: m_isPresenting = true; On 2016/10/06 22:31:28, ddorwin wrote: > It would probably be better to fix this in a separate CL independent of the > usage stats changes. Also, I think some comments would be appropriate to explain > why that was set to false above (to handle all the failure cases with early > returns). Still, maybe some refactoring would be better and have avoided this > bug. I'll remove the from this change. I wasn't sure of the culture here - "fix while touching the code" vs. "file a bug so each issue fixes one thing". https://codereview.chromium.org/2388163005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:287: ReportPresentationResult(PresentationResult::UnsupportedDisplay); On 2016/10/06 22:31:28, ddorwin wrote: > PresentationNotSupported or NotSupportedByDisplay? > > UnsupportedDisplay infers that we don't support the display rather than the > display does not support presentation. Done. https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66979: + Measures the frequency of different types of errors or success that may be On 2016/10/06 22:31:28, ddorwin wrote: > "Measures the frequency of" seems unnecessary and redundant. > > This can simply be "The result of calls to VRDisplay::requestPresent()." Done. https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66987: + Measures the frequency for different headset types to use VR with Chrome On 2016/10/06 22:31:28, ddorwin wrote: > ditto. Also, as in the .cpp file, does this cover all VR or just VR shell? WebVR and vr shell both go through this codepath https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66987: + Measures the frequency for different headset types to use VR with Chrome On 2016/10/06 22:31:28, ddorwin wrote: > ditto. Also, as in the .cpp file, does this cover all VR or just VR shell? Done. https://codereview.chromium.org/2388163005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:100834: + <int value="0" label="TypeUnknown"/> On 2016/10/06 22:31:28, ddorwin wrote: > Ditto on all three names Done.
The CQ bit was checked by billorr@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...
https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("VRReportPresentationResult", It's interesting that "Web" is dropped from "WebVR" in the path, filenames, and UMA/UseCounter names. This could become ambiguous in the future. https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("VRReportPresentationResult", VRPresentResult seems better. We shouldn't use the function name. However, this could be confused with VRPresent, which is a per-page counter. Maybe VRDisplayPresentResult. Alternatively, maybe the UseCounter IDL item should contain the actual API name, leaving us more options here. https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:161: ReportPresentationResult(PresentationResult::BeginPresent); This seems odd since we now get two _results_ per call. I think we need a separate metric. https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:303: VRPresent is no longer used? This should be in VRDisplay.idl, right? https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.h:136: PresentationResultMax, // Must be last member of enum nit: Always end with a period.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("VRReportPresentationResult", On 2016/10/07 00:41:03, ddorwin wrote: > VRPresentResult seems better. We shouldn't use the function name. > > However, this could be confused with VRPresent, which is a per-page counter. > Maybe VRDisplayPresentResult. Alternatively, maybe the UseCounter IDL item > should contain the actual API name, leaving us more options here. Switching to WebVRPresentResult, which is accurate conceptually, and disambiguates other vr presents we may have. https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:161: ReportPresentationResult(PresentationResult::BeginPresent); On 2016/10/07 00:41:03, ddorwin wrote: > This seems odd since we now get two _results_ per call. I think we need a > separate metric. I agree it's a bit odd, but it seems beneficial to have all the correlated data in one place. Also the alternative is we want something like a use counter, and it would be a bit awkward using an EnumerationHistogram or other type of histogram like a use counter. I'm pushing back a bit, but if you feel strongly we can discuss. https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:303: On 2016/10/07 00:41:03, ddorwin wrote: > VRPresent is no longer used? This should be in VRDisplay.idl, right? I removed the UseCounter for VRPresent. In the other change I have out, we have a usecounter on requestPresent. https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.h:136: PresentationResultMax, // Must be last member of enum On 2016/10/07 00:41:03, ddorwin wrote: > nit: Always end with a period. Done.
https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:303: On 2016/10/07 16:41:54, billorr wrote: > On 2016/10/07 00:41:03, ddorwin wrote: > > VRPresent is no longer used? This should be in VRDisplay.idl, right? > > I removed the UseCounter for VRPresent. In the other change I have out, we have > a usecounter on requestPresent. For the record, this is now tracked as UseCounter::VRRequestPresent. https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("WebVRPresentResult", In theory, this class and interface could be used for other things. How about VRDisplayPresentResult? FWIW, of the two models we looked at, the one that reported query in addition to the result did NOT include "Result" in the name. https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:161: ReportPresentationResult(PresentationResult::RequestPresent); Maybe "Requested" https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.h:123: RequestPresent = 0, You might comment that this is call calls and will be reported for every call (in addition to one of the others).
vr_shell lgtm with nits https://codereview.chromium.org/2388163005/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:220: case gvr::ViewerType::GVR_VIEWER_TYPE_DAYDREAM: nit: 2 more spaces indention for line 220 to line 229
Addressed all CR feedback https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("VRReportPresentationResult", On 2016/10/07 16:41:55, billorr wrote: > On 2016/10/07 00:41:03, ddorwin wrote: > > VRPresentResult seems better. We shouldn't use the function name. > > > > However, this could be confused with VRPresent, which is a per-page counter. > > Maybe VRDisplayPresentResult. Alternatively, maybe the UseCounter IDL item > > should contain the actual API name, leaving us more options here. > > Switching to WebVRPresentResult, which is accurate conceptually, and > disambiguates other vr presents we may have. Done. https://codereview.chromium.org/2388163005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:303: On 2016/10/10 21:06:58, ddorwin wrote: > On 2016/10/07 16:41:54, billorr wrote: > > On 2016/10/07 00:41:03, ddorwin wrote: > > > VRPresent is no longer used? This should be in VRDisplay.idl, right? > > > > I removed the UseCounter for VRPresent. In the other change I have out, we > have > > a usecounter on requestPresent. > > For the record, this is now tracked as UseCounter::VRRequestPresent. Done. https://codereview.chromium.org/2388163005/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2388163005/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:220: case gvr::ViewerType::GVR_VIEWER_TYPE_DAYDREAM: On 2016/10/11 15:57:46, bshe wrote: > nit: 2 more spaces indention for line 220 to line 229 Done. https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("WebVRPresentResult", On 2016/10/10 21:06:58, ddorwin wrote: > In theory, this class and interface could be used for other things. How about > VRDisplayPresentResult? > > FWIW, of the two models we looked at, the one that reported query in addition to > the result did NOT include "Result" in the name. using VRDisplayPresentResult. Leaving result to disambiguate vs. the UseCounter. https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:154: ("WebVRPresentResult", On 2016/10/10 21:06:58, ddorwin wrote: > In theory, this class and interface could be used for other things. How about > VRDisplayPresentResult? > > FWIW, of the two models we looked at, the one that reported query in addition to > the result did NOT include "Result" in the name. Done. https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:161: ReportPresentationResult(PresentationResult::RequestPresent); On 2016/10/10 21:06:58, ddorwin wrote: > Maybe "Requested" Done. https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2388163005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.h:123: RequestPresent = 0, On 2016/10/10 21:06:58, ddorwin wrote: > You might comment that this is call calls and will be reported for every call > (in addition to one of the others). Done.
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by billorr@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...
A few minor things. https://codereview.chromium.org/2388163005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:164: ReportPresentationResult(PresentationResult::RequestedPreset); Present is misspelled. Also, thoughts on just "Requested" without "Present"? https://codereview.chromium.org/2388163005/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388163005/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67432: + RequestedPresent is reported each time requestPresent() is called, along This second sentence is not clear since "along with" could mean together/at the same time. The code comment that made it clear that it would be reported twice was good. For example: "Reported twice per requestPresent() call, once to record the call and once to record the result." https://codereview.chromium.org/2388163005/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67439: + <summary>Different headset types to use VR with Chrome</summary> This could be more concise. How about this? "The type of headset being used for VR." https://codereview.chromium.org/2388163005/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:94408: + <int value="0" label="RequestedPreset"/> Same misspelling.
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
https://codereview.chromium.org/2388163005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2388163005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:164: ReportPresentationResult(PresentationResult::RequestedPreset); On 2016/10/11 17:46:33, ddorwin wrote: > Present is misspelled. > > Also, thoughts on just "Requested" without "Present"? Done. https://codereview.chromium.org/2388163005/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388163005/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67432: + RequestedPresent is reported each time requestPresent() is called, along On 2016/10/11 17:46:33, ddorwin wrote: > This second sentence is not clear since "along with" could mean together/at the > same time. The code comment that made it clear that it would be reported twice > was good. > > For example: "Reported twice per requestPresent() call, once to record the call > and once to record the result." Done. https://codereview.chromium.org/2388163005/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67439: + <summary>Different headset types to use VR with Chrome</summary> On 2016/10/11 17:46:33, ddorwin wrote: > This could be more concise. How about this? > "The type of headset being used for VR." Done. https://codereview.chromium.org/2388163005/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:94408: + <int value="0" label="RequestedPreset"/> On 2016/10/11 17:46:33, ddorwin wrote: > Same misspelling. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by billorr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2388163005/#ps200001 (title: "minor cr feedback")
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 ========== Add WebVR histograms for requestPresent failures/success and headset types. Add usage counters for gamepad extensions. BUG=389343 ========== to ========== Add WebVR histograms for requestPresent failures/success and headset types. Add usage counters for gamepad extensions. BUG=389343 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add WebVR histograms for requestPresent failures/success and headset types. Add usage counters for gamepad extensions. BUG=389343 ========== to ========== Add WebVR histograms for requestPresent failures/success and headset types. Add usage counters for gamepad extensions. BUG=389343 Committed: https://crrev.com/6805d8ab4296d9033602bad708da31e67e9fff6d Cr-Commit-Position: refs/heads/master@{#424522} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6805d8ab4296d9033602bad708da31e67e9fff6d Cr-Commit-Position: refs/heads/master@{#424522} |