Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2604063002:
Log intents received differently if they are internal or external. (Closed)
DescriptionLog intents received differently if they are internal or external.
Currently, all intents received by ChromeTabbedActivity are logged
as MobileReceivedExternalIntent despite there being many codepaths
within Chrome that also trigger intent dispatch.
A code search for Browser.EXTRA_CREATE_NEW_TAB shows a bunch of
callers that trigger intents and we logged them all as external.
This conflates whether we are receiving intents from other applications
or ourself. The intent of this change (yuck, yuck, yuck) is to
split out intents that we dispatch ourselves and ones that an
actual external app did.
BUG=648386
Patch Set 1 #
Total comments: 5
Patch Set 2 : Centralize logging #
Total comments: 4
Patch Set 3 : Keep old metrics #
Total comments: 8
Patch Set 4 : Remove obsolete #
Total comments: 5
Patch Set 5 : mpearson@ comments #Patch Set 6 : Rebase #Patch Set 7 : Fix compile #
Messages
Total messages: 22 (8 generated)
tedchoc@chromium.org changed reviewers: + mariakhomenko@chromium.org
PTAL...let me know if you think this is crazy or not. I tried to do an audit of the codebase and it looks like we do a good job of specifying putExtra(Browser.EXTRA_APPLICATION_ID, getPackageName()); in places that send intents.
It's crazy :) https://codereview.chromium.org/2604063002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2604063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:859: RecordUserAction.record("MobileReceivedExternalIntent"); This will need rebase I guess -- I think this one is now removed. https://codereview.chromium.org/2604063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:904: RecordUserAction.record("MobileReceivedExternalIntent"); I think we need to deprecate this one and start logging a new user action, otherwise we'll get super conflated stats where newer clients record this differently from old clients, but it's all showing up together in the stats. https://codereview.chromium.org/2604063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:906: RecordUserAction.record("MobileReceivedInternalIntent"); This doesn't make much sense because the recording currently happens only on the paths that are handling external intents. internal-only branches don't call this, so the InternalIntent thing is kind of bogus. Either we need to call this at the top of the function and hope we've added externalAppId everywhere or make some claims about what exactly this action is.
https://codereview.chromium.org/2604063002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2604063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:904: RecordUserAction.record("MobileReceivedExternalIntent"); On 2017/01/03 21:33:47, Maria wrote: > I think we need to deprecate this one and start logging a new user action, > otherwise we'll get super conflated stats where newer clients record this > differently from old clients, but it's all showing up together in the stats. Fine with me. Preference for a new name? Should we leave the old one behind to allow some comparison in the transition? https://codereview.chromium.org/2604063002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:906: RecordUserAction.record("MobileReceivedInternalIntent"); On 2017/01/03 21:33:47, Maria wrote: > This doesn't make much sense because the recording currently happens only on the > paths that are handling external intents. internal-only branches don't call > this, so the InternalIntent thing is kind of bogus. Either we need to call this > at the top of the function and hope we've added externalAppId everywhere or make > some claims about what exactly this action is. Fair. My goal of the change was to only split out the cases where we were previously logging MobileReceivedExternalIntent (which I believe it did do). It did not cover the cases where it was not logging it previously, CLOBBER_CURRENT_TAB in particular (I also missed the incognito one due to my other inflight change). I think it's better and clearer to just log this at the top of the function anyway.
Updated a version (w/o new metric names) that has the logging at the top. It also removed previous logging places of MobileReceivedExternalIntent, which we may want to keep if pending the previous set of questions.
Okay, here's the suggestion: From the places we used to log MobileReceivedExternalIntent, log a new user action with breakdown for internal vs external. Add a TODO to remove this once we understand the numbers there. The two new actions look good (just rename them) and would be what we keep track of going forward. We just need to make sure that we are not missing Chrome tagging on some -- so if we find that there are more external intents logged at the top than from the old sites, there must be annotations missing from internal-only intents. https://codereview.chromium.org/2604063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2604063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:771: RecordUserAction.record("MobileReceivedExternalIntent"); Since these are new, how about MobileViewIntentFromApp and MobileViewIntentFromChrome? https://codereview.chromium.org/2604063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:794: int shortcutSource = intent.getIntExtra( Also add MobileReceivedExternalIntent.App and MobileReceivedExternalIntent.Chrome -- that would be logged in identical spots where MobileReceivedExternalIntent is logged now. This would let us compare old numbers to new numbers we are adding AND get a breakdown between those that have externalAppId and those that don't.
https://codereview.chromium.org/2604063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2604063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:771: RecordUserAction.record("MobileReceivedExternalIntent"); On 2017/01/03 23:14:11, Maria wrote: > Since these are new, how about MobileViewIntentFromApp and > MobileViewIntentFromChrome? I added TabbedMode to the name to make it even clearer (at least to me). https://codereview.chromium.org/2604063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:794: int shortcutSource = intent.getIntExtra( On 2017/01/03 23:14:11, Maria wrote: > Also add > > MobileReceivedExternalIntent.App and MobileReceivedExternalIntent.Chrome -- that > would be logged in identical spots where MobileReceivedExternalIntent is logged > now. > > This would let us compare old numbers to new numbers we are adding AND get a > breakdown between those that have externalAppId and those that don't. Done.
lgtm % comments https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9924: <obsolete> Don't mark it as obsolete yet -- it's going to disappear from UMA dropdown selector -- I learned that the hard way :) https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9936: <obsolete> same as above https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9944: A VIEW intent was received by the tabbed mode activity from Chrome. Mention that this is only recorded in places where external requests are received. https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9946: <obsolete> same as above
tedchoc@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson@ for actions.xml OWNERS https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9924: <obsolete> On 2017/01/05 01:01:20, Maria wrote: > Don't mark it as obsolete yet -- it's going to disappear from UMA dropdown > selector -- I learned that the hard way :) Done. https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9936: <obsolete> On 2017/01/05 01:01:20, Maria wrote: > same as above Done. https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9944: A VIEW intent was received by the tabbed mode activity from Chrome. On 2017/01/05 01:01:20, Maria wrote: > Mention that this is only recorded in places where external requests are > received. Done. https://codereview.chromium.org/2604063002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9946: <obsolete> On 2017/01/05 01:01:20, Maria wrote: > same as above Done.
https://codereview.chromium.org/2604063002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2604063002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:768: || TextUtils.equals(externalAppId, getPackageName())) { Why is the latter test necessary? (Is it obvious to Android folks? Otherwise a comment might be worthwhile. I'd think these two tests are equivalent...) Also, you repeat this combo test in two places. Perhaps it should be moved into a helper function (or into wasIntentSenderChrome), assuming that most people who want to do something like this need both tests. https://codereview.chromium.org/2604063002/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2604063002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:9919: <owner>mariakhomenko@chromium.org, tedchoc@chromium.org</owner> owner is a repeated field, one user per entry ditto below https://codereview.chromium.org/2604063002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:10035: <action name="MobileTabbedModeViewIntentFromApp"> Naively, if I read the description of these two actions here: A VIEW intent was received by the tabbed mode activity from an external application. and A VIEW intent was received by the tabbed mode activity from Chrome. I'd think I can add them up to yield: A VIEW intent was received by the tabbed mode activity. This will combine intents sent by external application as well as Chrome itself. This last bit is your description of MobileReceivedExternalIntent. Yet I know this isn't the case. The latter is only logged in some subset of those cases (switch (tabOpenType) ...). Can you revise the descriptions so that people like me don't make the inference I made above?
https://codereview.chromium.org/2604063002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2604063002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:768: || TextUtils.equals(externalAppId, getPackageName())) { On 2017/01/05 22:41:18, Mark P wrote: > Why is the latter test necessary? > (Is it obvious to Android folks? Otherwise a comment might be worthwhile. I'd > think these two tests are equivalent...) > > Also, you repeat this combo test in two places. Perhaps it should be moved into > a helper function (or into wasIntentSenderChrome), assuming that most people who > want to do something like this need both tests. Done. https://codereview.chromium.org/2604063002/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2604063002/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:10035: <action name="MobileTabbedModeViewIntentFromApp"> On 2017/01/05 22:41:19, Mark P wrote: > Naively, if I read the description of these two actions here: > A VIEW intent was received by the tabbed mode activity from an external > application. > and > A VIEW intent was received by the tabbed mode activity from Chrome. > I'd think I can add them up to yield: > A VIEW intent was received by the tabbed mode activity. This will combine > intents sent by external application as well as Chrome itself. > > This last bit is your description of MobileReceivedExternalIntent. > > Yet I know this isn't the case. The latter is only logged in some subset of > those cases (switch (tabOpenType) ...). > > Can you revise the descriptions so that people like me don't make the inference > I made above? Done. I initially marked MobileReceivedExternalIntent as obsolete to try and achieve this, but Maria pointed out this has implications in the UI that I don't want to trigger quite yet.
lgtm
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2604063002/#ps100001 (title: "Rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2604063002/#ps120001 (title: "Fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483725453719220,
"parent_rev": "ba85a156e1974f8efabe02594e2abf23726a9462", "commit_rev":
"b89df0eff63dbd798fbc33765c6f70a9b3d7f3bf"}
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
