Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476783002:
Adding a destination platform histogram for UMA. (Closed)
|
Created:
4 years, 1 month ago by djacobo_ Modified:
4 years, 1 month ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, asvitkine+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding a destination platform histogram for UMA.
This enum helps to keep track of whether a navigation observed by
ArcNavigationThrottle is continued in Chrome or redirected to a ARC.
Likewise it helps determinating if a navigation is continued in Chrome
or redirected to ARC via the external protocol dialog.
BUG=661672
TEST=try
Committed: https://crrev.com/a0465d157795b6f988a795763ad4ef4faa6d994a
Cr-Commit-Position: refs/heads/master@{#430176}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Replacing 'selected' to 'destination', adding a note to the ArcIntentHandlerAction histogram #
Total comments: 14
Patch Set 3 : Adding unit tests, removing wrong UMA recording. #
Total comments: 9
Patch Set 4 : Style fixes, git cl lint/format #
Total comments: 4
Patch Set 5 : Promoting RecordUma as a static method to ArcNavigationThrottle #Patch Set 6 : Removing histograms include #
Messages
Total messages: 47 (29 generated)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Adding a selected platform histogram for UMA. This enum helps to keep track of whether a navigation observed by ArcNavigationThrottle is continued in Chrome or redirected to a ARC. Likewise it helps determinating if a navigation is continued in Chrome or redirected to ARC via the external dialog protocol. BUG=661672 TEST=try ========== to ========== Adding a selected platform histogram for UMA. This enum helps to keep track of whether a navigation observed by ArcNavigationThrottle is continued in Chrome or redirected to a ARC. Likewise it helps determinating if a navigation is continued in Chrome or redirected to ARC via the external dialog protocol. BUG=661672 TEST=try ==========
djacobo@google.com changed reviewers: + elijahtaylor@chromium.org, yusukes@chromium.org
PTAL, provided the discussion we have, I am adding another independent histogram with the purpose of only telling if the navigation is continued on Chrome or sent to ARC. https://codereview.chromium.org/2476783002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:336: RecordUma(ArcNavigationThrottle::CloseReason::PREFERRED_ACTIVITY_FOUND, I assumed we would like to track as PREFERRED also under the OPEN_URL_IN_CHROME option.
https://codereview.chromium.org/2476783002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:50: "Arc.IntentHandlerSelectedPlatform", static_cast<int>(platform), nit: calling it Selected (here and elsewhere) seems like there was a user action, which isn't always the case. This is why I called it Destination or something like that in the bug https://codereview.chromium.org/2476783002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:336: RecordUma(ArcNavigationThrottle::CloseReason::PREFERRED_ACTIVITY_FOUND, On 2016/11/03 22:11:54, djacobo wrote: > I assumed we would like to track as PREFERRED also under the OPEN_URL_IN_CHROME > option. Ah, this is interesting that we were only setting this on ARC navigation. I agree we probably want to have this recorded for both. You probably should put a note in the histograms.xml file for this histogram that we changed the nature of this in M56 so we know how to interpret the values in different milestones. https://codereview.chromium.org/2476783002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2476783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:73477: + Defines Arc intent handler platforms to continue the navigation. s/Arc/ARC
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2476783002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:50: "Arc.IntentHandlerSelectedPlatform", static_cast<int>(platform), On 2016/11/03 22:22:00, elijahtaylor1 wrote: > nit: calling it Selected (here and elsewhere) seems like there was a user > action, which isn't always the case. This is why I called it Destination or > something like that in the bug Makes sense. Done. https://codereview.chromium.org/2476783002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:336: RecordUma(ArcNavigationThrottle::CloseReason::PREFERRED_ACTIVITY_FOUND, On 2016/11/03 22:22:00, elijahtaylor1 wrote: > On 2016/11/03 22:11:54, djacobo wrote: > > I assumed we would like to track as PREFERRED also under the > OPEN_URL_IN_CHROME > > option. > > Ah, this is interesting that we were only setting this on ARC navigation. > > I agree we probably want to have this recorded for both. > > You probably should put a note in the histograms.xml file for this histogram > that we changed the nature of this in M56 so we know how to interpret the values > in different milestones. Alright, let me add a comment in the related histogram https://codereview.chromium.org/2476783002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2476783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:73477: + Defines Arc intent handler platforms to continue the navigation. On 2016/11/03 22:22:00, elijahtaylor1 wrote: > s/Arc/ARC Done.
Description was changed from ========== Adding a selected platform histogram for UMA. This enum helps to keep track of whether a navigation observed by ArcNavigationThrottle is continued in Chrome or redirected to a ARC. Likewise it helps determinating if a navigation is continued in Chrome or redirected to ARC via the external dialog protocol. BUG=661672 TEST=try ========== to ========== Adding a destination platform histogram for UMA. This enum helps to keep track of whether a navigation observed by ArcNavigationThrottle is continued in Chrome or redirected to a ARC. Likewise it helps determinating if a navigation is continued in Chrome or redirected to ARC via the external protocol dialog. BUG=661672 TEST=try ==========
https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:336: RecordUma(ArcNavigationThrottle::CloseReason::PREFERRED_ACTIVITY_FOUND, When there's no app that can handle the custom |url|, GetActionResult::SHOW_CHROME_OS_DIALOG is returned. In that case, we should not record any UMA because neither ARC nor Chrome has handled the URL. https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:337: result == GetActionResult::HANDLE_URL_IN_ARC Sorry that the enum does not have descriptions (please check the unit tests instead that are actually very extensive...), but OPEN_URL_IN_CHROME is probably not what you want to record UMA for. In short, HANDLE_URL_IN_ARC is returned when there are one or more apps that can handle the custom |url| _and_ one of them is a preferred one. OPEN_URL_IN_CHROME is returned when 1) |url|'s scheme is intent: (e.g. "intent://scan/#Intent;scheme=abc;package=com.google.abc;S.browser_fallback_url=http://zxing.org;end" ) and 2) the package specified in the intent: |url| is not found in ARC++, and 3) the intent: |url| has http or https S.browser_fallback_url. In this case, HandleUrl() silently opens S.browser_fallback_url without asking the user, but I'm not sure if we call it PREFERRED_ACTIVITY_FOUND. OPEN_URL_IN_CHROME can be returned even if the user hasn't pressed "Always" for Chrome before, for example. This is the reason why the current code only call RecordUma() for HANDLE_URL_IN_ARC. Please also note that |url|'s scheme cannot be http(s) as this file is for handling external protocols like intent: and app-specific-custom-uri:. https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:385: ArcNavigationThrottle::Platform ArcNavigationThrottle::GetDestinationPlatform( * Would you mind adding some unit tests for this? * Move to L405 to be consistent with the order in .h? https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h:46: enum class Platform : int { please add an enum description (like L33-35). https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h:88: static Platform GetDestinationPlatform( * Since this is a public function, it's better to add a function comment. It's unclear what'll be returned when ERROR or DIALOG_DEACTIVATED is passed. * L81-87 is for ForTesting functions. Please move this to L80.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:336: RecordUma(ArcNavigationThrottle::CloseReason::PREFERRED_ACTIVITY_FOUND, On 2016/11/04 00:16:14, Yusuke Sato wrote: > When there's no app that can handle the custom |url|, > GetActionResult::SHOW_CHROME_OS_DIALOG is returned. In that case, we should not > record any UMA because neither ARC nor Chrome has handled the URL. Makes sense, let me rollback the comments on histograms.xml, also putting back line 327. Thanks for the clarification!!! https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:337: result == GetActionResult::HANDLE_URL_IN_ARC On 2016/11/04 00:16:14, Yusuke Sato wrote: > Sorry that the enum does not have descriptions (please check the unit tests > instead that are actually very extensive...), but OPEN_URL_IN_CHROME is probably > not what you want to record UMA for. > > In short, HANDLE_URL_IN_ARC is returned when there are one or more apps that can > handle the custom |url| _and_ one of them is a preferred one. > > OPEN_URL_IN_CHROME is returned when 1) |url|'s scheme is intent: > > (e.g. > "intent://scan/#Intent;scheme=abc;package=com.google.abc;S.browser_fallback_url=http://zxing.org;end" > ) > > and 2) the package specified in the intent: |url| is not found in ARC++, and 3) > the intent: |url| has http or https S.browser_fallback_url. > > In this case, HandleUrl() silently opens S.browser_fallback_url without asking > the user, but I'm not sure if we call it PREFERRED_ACTIVITY_FOUND. > OPEN_URL_IN_CHROME can be returned even if the user hasn't pressed "Always" for > Chrome before, for example. > > This is the reason why the current code only call RecordUma() for > HANDLE_URL_IN_ARC. Please also note that |url|'s scheme cannot be http(s) as > this file is for handling external protocols like intent: and > app-specific-custom-uri:. Ok, so if I understand correctly I could Use the code as it was before in line 328, and I can still add ARC as the platform for those, but none of the CloseReason entries matches the case you describe (keeping the traffic in Chrome by using a fallback string). I think we can still record this for the new histogram, but we need another entry on CloseReason, maybe something like FALLBACK_STRING_USED (I thought about APP_NOT_INSTALLED, but naming things is difficult to me), WDYT? https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe... https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc:385: ArcNavigationThrottle::Platform ArcNavigationThrottle::GetDestinationPlatform( On 2016/11/04 00:16:14, Yusuke Sato wrote: > * Would you mind adding some unit tests for this? > * Move to L405 to be consistent with the order in .h? Done. https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h:46: enum class Platform : int { On 2016/11/04 00:16:14, Yusuke Sato wrote: > please add an enum description (like L33-35). Done. https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.h:88: static Platform GetDestinationPlatform( On 2016/11/04 00:16:14, Yusuke Sato wrote: > * Since this is a public function, it's better to add a function comment. It's > unclear what'll be returned when ERROR or DIALOG_DEACTIVATED is passed. > * L81-87 is for ForTesting functions. Please move this to L80. > Moved and added comments, let me know what you think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:337: result == GetActionResult::HANDLE_URL_IN_ARC On 2016/11/04 05:53:50, djacobo wrote: > On 2016/11/04 00:16:14, Yusuke Sato wrote: > > Sorry that the enum does not have descriptions (please check the unit tests > > instead that are actually very extensive...), but OPEN_URL_IN_CHROME is > probably > > not what you want to record UMA for. > > > > In short, HANDLE_URL_IN_ARC is returned when there are one or more apps that > can > > handle the custom |url| _and_ one of them is a preferred one. > > > > OPEN_URL_IN_CHROME is returned when 1) |url|'s scheme is intent: > > > > (e.g. > > > "intent://scan/#Intent;scheme=abc;package=com.google.abc;S.browser_fallback_url=http://zxing.org;end" > > ) > > > > and 2) the package specified in the intent: |url| is not found in ARC++, and > 3) > > the intent: |url| has http or https S.browser_fallback_url. > > > > In this case, HandleUrl() silently opens S.browser_fallback_url without asking > > the user, but I'm not sure if we call it PREFERRED_ACTIVITY_FOUND. > > OPEN_URL_IN_CHROME can be returned even if the user hasn't pressed "Always" > for > > Chrome before, for example. > > > > This is the reason why the current code only call RecordUma() for > > HANDLE_URL_IN_ARC. Please also note that |url|'s scheme cannot be http(s) as > > this file is for handling external protocols like intent: and > > app-specific-custom-uri:. > > Ok, so if I understand correctly I could Use the code as it was before in line > 328, and I can still add ARC as the platform for those, but none of the > CloseReason entries matches the case you describe (keeping the traffic in Chrome > by using a fallback string). > > I think we can still record this for the new histogram, but we need another > entry on CloseReason, maybe something like FALLBACK_STRING_USED (I thought about > APP_NOT_INSTALLED, but naming things is difficult to me), WDYT? > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe... Re FALLBACK_STRING_USED, I'd defer the decision to Elijah. Adding it in a separate CL later seems also fine because Chrome for Chrome OS rarely handles intent: anyway. https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. If you haven't, please run git cl format and git cl lint. https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:336: if (result == GetActionResult::HANDLE_URL_IN_ARC) Since the RecordUma() call is multi-line, please use {}. https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc:146: TEST(ArcNavigationThrottleTest, TestGetDestinationPlatform) { Thanks! https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc:159: EXPECT_EQ(ArcNavigationThrottle::Platform::CHROME, nit: could you swap L159 & L163? The order (chrome_app, non_chrome_app, non_chrome_app, chrome_app) looks a bit unusual to me.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:337: result == GetActionResult::HANDLE_URL_IN_ARC On 2016/11/04 16:18:06, Yusuke Sato wrote: > On 2016/11/04 05:53:50, djacobo wrote: > > On 2016/11/04 00:16:14, Yusuke Sato wrote: > > > Sorry that the enum does not have descriptions (please check the unit tests > > > instead that are actually very extensive...), but OPEN_URL_IN_CHROME is > > probably > > > not what you want to record UMA for. > > > > > > In short, HANDLE_URL_IN_ARC is returned when there are one or more apps that > > can > > > handle the custom |url| _and_ one of them is a preferred one. > > > > > > OPEN_URL_IN_CHROME is returned when 1) |url|'s scheme is intent: > > > > > > (e.g. > > > > > > "intent://scan/#Intent;scheme=abc;package=com.google.abc;S.browser_fallback_url=http://zxing.org;end" > > > ) > > > > > > and 2) the package specified in the intent: |url| is not found in ARC++, and > > 3) > > > the intent: |url| has http or https S.browser_fallback_url. > > > > > > In this case, HandleUrl() silently opens S.browser_fallback_url without > asking > > > the user, but I'm not sure if we call it PREFERRED_ACTIVITY_FOUND. > > > OPEN_URL_IN_CHROME can be returned even if the user hasn't pressed "Always" > > for > > > Chrome before, for example. > > > > > > This is the reason why the current code only call RecordUma() for > > > HANDLE_URL_IN_ARC. Please also note that |url|'s scheme cannot be http(s) as > > > this file is for handling external protocols like intent: and > > > app-specific-custom-uri:. > > > > Ok, so if I understand correctly I could Use the code as it was before in line > > 328, and I can still add ARC as the platform for those, but none of the > > CloseReason entries matches the case you describe (keeping the traffic in > Chrome > > by using a fallback string). > > > > I think we can still record this for the new histogram, but we need another > > entry on CloseReason, maybe something like FALLBACK_STRING_USED (I thought > about > > APP_NOT_INSTALLED, but naming things is difficult to me), WDYT? > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe... > > Re FALLBACK_STRING_USED, I'd defer the decision to Elijah. Adding it in a > separate CL later seems also fine because Chrome for Chrome OS rarely handles > intent: anyway. Acknowledged. https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/04 16:18:06, Yusuke Sato wrote: > If you haven't, please run git cl format and git cl lint. I (almost) always do, lint/format didn't suggest a style fix, is something in these patch sets kind of odd style-wise? https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:336: if (result == GetActionResult::HANDLE_URL_IN_ARC) On 2016/11/04 16:18:06, Yusuke Sato wrote: > Since the RecordUma() call is multi-line, please use {}. Done. https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc:146: TEST(ArcNavigationThrottleTest, TestGetDestinationPlatform) { On 2016/11/04 16:18:06, Yusuke Sato wrote: > Thanks! Not a problem :) https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle_unittest.cc:159: EXPECT_EQ(ArcNavigationThrottle::Platform::CHROME, On 2016/11/04 16:18:06, Yusuke Sato wrote: > nit: could you swap L159 & L163? The order (chrome_app, non_chrome_app, > non_chrome_app, chrome_app) looks a bit unusual to me. Done.
still lgtm https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/30001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/04 17:43:43, djacobo wrote: > On 2016/11/04 16:18:06, Yusuke Sato wrote: > > If you haven't, please run git cl format and git cl lint. > > I (almost) always do, lint/format didn't suggest a style fix, is something in > these patch sets kind of odd style-wise? no, not really. was just a reminder.
elijahtaylor@google.com changed reviewers: + elijahtaylor@google.com
I think l-gtm but would like to get clarification on what Preferred activity found really means when UMA is recorded https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:337: result == GetActionResult::HANDLE_URL_IN_ARC On 2016/11/04 17:43:43, djacobo wrote: > On 2016/11/04 16:18:06, Yusuke Sato wrote: > > On 2016/11/04 05:53:50, djacobo wrote: > > > On 2016/11/04 00:16:14, Yusuke Sato wrote: > > > > Sorry that the enum does not have descriptions (please check the unit > tests > > > > instead that are actually very extensive...), but OPEN_URL_IN_CHROME is > > > probably > > > > not what you want to record UMA for. > > > > > > > > In short, HANDLE_URL_IN_ARC is returned when there are one or more apps > that > > > can > > > > handle the custom |url| _and_ one of them is a preferred one. > > > > > > > > OPEN_URL_IN_CHROME is returned when 1) |url|'s scheme is intent: > > > > > > > > (e.g. > > > > > > > > > > "intent://scan/#Intent;scheme=abc;package=com.google.abc;S.browser_fallback_url=http://zxing.org;end" > > > > ) > > > > > > > > and 2) the package specified in the intent: |url| is not found in ARC++, > and > > > 3) > > > > the intent: |url| has http or https S.browser_fallback_url. > > > > > > > > In this case, HandleUrl() silently opens S.browser_fallback_url without > > asking > > > > the user, but I'm not sure if we call it PREFERRED_ACTIVITY_FOUND. > > > > OPEN_URL_IN_CHROME can be returned even if the user hasn't pressed > "Always" > > > for > > > > Chrome before, for example. > > > > > > > > This is the reason why the current code only call RecordUma() for > > > > HANDLE_URL_IN_ARC. Please also note that |url|'s scheme cannot be http(s) > as > > > > this file is for handling external protocols like intent: and > > > > app-specific-custom-uri:. > > > > > > Ok, so if I understand correctly I could Use the code as it was before in > line > > > 328, and I can still add ARC as the platform for those, but none of the > > > CloseReason entries matches the case you describe (keeping the traffic in > > Chrome > > > by using a fallback string). > > > > > > I think we can still record this for the new histogram, but we need another > > > entry on CloseReason, maybe something like FALLBACK_STRING_USED (I thought > > about > > > APP_NOT_INSTALLED, but naming things is difficult to me), WDYT? > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe... > > > > Re FALLBACK_STRING_USED, I'd defer the decision to Elijah. Adding it in a > > separate CL later seems also fine because Chrome for Chrome OS rarely handles > > intent: anyway. > > Acknowledged. fallback string in a different CL sgtm Just to clarify, we are also setting PREFERRED_ACTIVITY_FOUND when chrome is used as the default from an "always" choice?
https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/10001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:337: result == GetActionResult::HANDLE_URL_IN_ARC On 2016/11/04 20:26:19, elijahtaylor (use chromium) wrote: > On 2016/11/04 17:43:43, djacobo wrote: > > On 2016/11/04 16:18:06, Yusuke Sato wrote: > > > On 2016/11/04 05:53:50, djacobo wrote: > > > > On 2016/11/04 00:16:14, Yusuke Sato wrote: > > > > > Sorry that the enum does not have descriptions (please check the unit > > tests > > > > > instead that are actually very extensive...), but OPEN_URL_IN_CHROME is > > > > probably > > > > > not what you want to record UMA for. > > > > > > > > > > In short, HANDLE_URL_IN_ARC is returned when there are one or more apps > > that > > > > can > > > > > handle the custom |url| _and_ one of them is a preferred one. > > > > > > > > > > OPEN_URL_IN_CHROME is returned when 1) |url|'s scheme is intent: > > > > > > > > > > (e.g. > > > > > > > > > > > > > > > "intent://scan/#Intent;scheme=abc;package=com.google.abc;S.browser_fallback_url=http://zxing.org;end" > > > > > ) > > > > > > > > > > and 2) the package specified in the intent: |url| is not found in ARC++, > > and > > > > 3) > > > > > the intent: |url| has http or https S.browser_fallback_url. > > > > > > > > > > In this case, HandleUrl() silently opens S.browser_fallback_url without > > > asking > > > > > the user, but I'm not sure if we call it PREFERRED_ACTIVITY_FOUND. > > > > > OPEN_URL_IN_CHROME can be returned even if the user hasn't pressed > > "Always" > > > > for > > > > > Chrome before, for example. > > > > > > > > > > This is the reason why the current code only call RecordUma() for > > > > > HANDLE_URL_IN_ARC. Please also note that |url|'s scheme cannot be > http(s) > > as > > > > > this file is for handling external protocols like intent: and > > > > > app-specific-custom-uri:. > > > > > > > > Ok, so if I understand correctly I could Use the code as it was before in > > line > > > > 328, and I can still add ARC as the platform for those, but none of the > > > > CloseReason entries matches the case you describe (keeping the traffic in > > > Chrome > > > > by using a fallback string). > > > > > > > > I think we can still record this for the new histogram, but we need > another > > > > entry on CloseReason, maybe something like FALLBACK_STRING_USED (I thought > > > about > > > > APP_NOT_INSTALLED, but naming things is difficult to me), WDYT? > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe... > > > > > > Re FALLBACK_STRING_USED, I'd defer the decision to Elijah. Adding it in a > > > separate CL later seems also fine because Chrome for Chrome OS rarely > handles > > > intent: anyway. > > > > Acknowledged. > > fallback string in a different CL sgtm > > Just to clarify, we are also setting PREFERRED_ACTIVITY_FOUND when chrome is > used as the default from an "always" choice? If Chrome is used as the default (aka Always) when clicking an http(s) link, yes, ArcNavigationThrottle::OnIntentPickerClosed() records PREFERRED_ACTIVITY_FOUND with Platform::CHROME.
djacobo@google.com changed reviewers: + isherman@chromium.org
Yusuke is right, this is a bit more evident on ArcNavigationThrottle. isherman@ could you take a look at tools/metrics/histograms/histograms.xml ? thanks
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2476783002/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:51: static_cast<int>(ArcNavigationThrottle::Platform::SIZE)); nit: Are these static casts needed? I thought I remembered some template magic landing that made them no longer necessary. https://codereview.chromium.org/2476783002/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:51: static_cast<int>(ArcNavigationThrottle::Platform::SIZE)); It looks like you emit this same histogram both here and in arc_navigation_throttle.cc. Could you please refactor the code to be shared?
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the prompt review Ilya :) https://codereview.chromium.org/2476783002/diff/50001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc (right): https://codereview.chromium.org/2476783002/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:51: static_cast<int>(ArcNavigationThrottle::Platform::SIZE)); On 2016/11/05 01:39:22, Ilya Sherman wrote: > It looks like you emit this same histogram both here and in > arc_navigation_throttle.cc. Could you please refactor the code to be shared? True, very nice catch. Done. https://codereview.chromium.org/2476783002/diff/50001/chrome/browser/chromeos... chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:51: static_cast<int>(ArcNavigationThrottle::Platform::SIZE)); On 2016/11/05 01:39:22, Ilya Sherman wrote: > nit: Are these static casts needed? I thought I remembered some template magic > landing that made them no longer necessary. didnt work :'( could revisit later.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics LGTM, thanks!
The CQ bit was checked by djacobo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2476783002/#ps90001 (title: "Removing histograms include")
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 ========== Adding a destination platform histogram for UMA. This enum helps to keep track of whether a navigation observed by ArcNavigationThrottle is continued in Chrome or redirected to a ARC. Likewise it helps determinating if a navigation is continued in Chrome or redirected to ARC via the external protocol dialog. BUG=661672 TEST=try ========== to ========== Adding a destination platform histogram for UMA. This enum helps to keep track of whether a navigation observed by ArcNavigationThrottle is continued in Chrome or redirected to a ARC. Likewise it helps determinating if a navigation is continued in Chrome or redirected to ARC via the external protocol dialog. BUG=661672 TEST=try ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== Adding a destination platform histogram for UMA. This enum helps to keep track of whether a navigation observed by ArcNavigationThrottle is continued in Chrome or redirected to a ARC. Likewise it helps determinating if a navigation is continued in Chrome or redirected to ARC via the external protocol dialog. BUG=661672 TEST=try ========== to ========== Adding a destination platform histogram for UMA. This enum helps to keep track of whether a navigation observed by ArcNavigationThrottle is continued in Chrome or redirected to a ARC. Likewise it helps determinating if a navigation is continued in Chrome or redirected to ARC via the external protocol dialog. BUG=661672 TEST=try Committed: https://crrev.com/a0465d157795b6f988a795763ad4ef4faa6d994a Cr-Commit-Position: refs/heads/master@{#430176} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a0465d157795b6f988a795763ad4ef4faa6d994a Cr-Commit-Position: refs/heads/master@{#430176} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
