[Android] Record keyboard accessory bar metrics up to once

Before this CL, the accessory bar wouldn't add impressions to delayed
appearing actions. This CL ensures this is done once (per new set of
actions) while keeping the constraint that every bar impression happens
exactly once.

Bug: 873143
Change-Id: I4de6c8f8e95321176d9bca19459d6e9d2cdae746
Reviewed-on: https://chromium-review.googlesource.com/1171227
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582881}

chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java

@@ -56,8 +56,7 @@ public class AutofillKeyboardAccessoryBridge
 
     @Override
     public void suggestionSelected(int listIndex) {
-        KeyboardAccessoryMetricsRecorder.recordActionSelected(
-                AccessoryAction.GENERATE_PASSWORD_AUTOMATIC);
+        KeyboardAccessoryMetricsRecorder.recordActionSelected(AccessoryAction.AUTOFILL_SUGGESTION);
         if (mManualFillingCoordinator != null) mManualFillingCoordinator.dismiss();
         if (mNativeAutofillKeyboardAccessory == 0) return;
         nativeSuggestionSelected(mNativeAutofillKeyboardAccessory, listIndex);

chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetCoordinator.java

@@ -40,7 +40,7 @@ public class AccessorySheetCoordinator {
         model.addObserver(new PropertyModelChangeProcessor<>(model, stubHolder,
                 new LazyViewBinderAdapter<>(
                         new AccessorySheetViewBinder(), this::onViewInflated)));
-        KeyboardAccessoryMetricsRecorder.recordModelChanges(model);
+        KeyboardAccessoryMetricsRecorder.registerMetricsObserver(model);
         mMediator = new AccessorySheetMediator(model);
     }
 

chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryCoordinator.java

@@ -72,7 +72,7 @@ public class KeyboardAccessoryCoordinator {
         model.addObserver(new PropertyModelChangeProcessor<>(model, mViewHolder,
                 new LazyViewBinderAdapter<>(
                         new KeyboardAccessoryViewBinder(), this::onViewInflated)));
-        KeyboardAccessoryMetricsRecorder.recordModelChanges(model);
+        KeyboardAccessoryMetricsRecorder.registerMetricsObserver(model);
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryMetricsRecorder.java

@@ -6,10 +6,13 @@ package org.chromium.chrome.browser.autofill.keyboard_accessory;
 
 import static org.chromium.chrome.browser.autofill.keyboard_accessory.AccessorySheetTrigger.MANUAL_OPEN;
 
+import android.support.annotation.Nullable;
+
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.autofill.keyboard_accessory.KeyboardAccessoryData.Item;
 import org.chromium.chrome.browser.modelutil.ListObservable;
+import org.chromium.chrome.browser.modelutil.PropertyObservable;
 import org.chromium.chrome.browser.modelutil.SimpleListObservable;
 
 import java.util.HashSet;
@@ -39,12 +42,33 @@ public class KeyboardAccessoryMetricsRecorder {
      */
     private KeyboardAccessoryMetricsRecorder() {}
 
-    static void recordModelChanges(KeyboardAccessoryModel keyboardAccessoryModel) {
-        keyboardAccessoryModel.addObserver((source, propertyKey) -> {
+    /**
+     * This observer will react to changes of the {@link KeyboardAccessoryModel} and store each
+     * impression once per visibility change.
+     */
+    private static class AccessoryBarObserver
+            implements ListObservable.ListObserver<Void>,
+                       PropertyObservable.PropertyObserver<KeyboardAccessoryModel.PropertyKey> {
+        private final Set<Integer> mRecordedBarBuckets = new HashSet<>();
+        private final Set<Integer> mRecordedActionImpressions = new HashSet<>();
+        private final KeyboardAccessoryModel mModel;
+
+        AccessoryBarObserver(KeyboardAccessoryModel keyboardAccessoryModel) {
+            mModel = keyboardAccessoryModel;
+        }
+
+        @Override
+        public void onPropertyChanged(PropertyObservable<KeyboardAccessoryModel.PropertyKey> source,
+                @Nullable KeyboardAccessoryModel.PropertyKey propertyKey) {
             if (propertyKey == KeyboardAccessoryModel.PropertyKey.VISIBLE) {
-                if (keyboardAccessoryModel.isVisible()) {
-                    recordAccessoryBarImpression(keyboardAccessoryModel);
-                    recordAccessoryActionImpressions(keyboardAccessoryModel);
+                if (mModel.isVisible()) {
+                    recordFirstImpression();
+                    maybeRecordBarBucket(AccessoryBarContents.WITH_AUTOFILL_SUGGESTIONS);
+                    recordUnrecordedList(mModel.getTabList(), 0, mModel.getTabList().size());
+                    recordUnrecordedList(mModel.getActionList(), 0, mModel.getActionList().size());
+                } else {
+                    mRecordedBarBuckets.clear();
+                    mRecordedActionImpressions.clear();
                 }
                 return;
             }
@@ -53,10 +77,130 @@ public class KeyboardAccessoryMetricsRecorder {
                 return;
             }
             assert false : "Every property update needs to be handled explicitly!";
-        });
+        }
+
+        /**
+         * If not done yet, this records an impression for the general type of list that was added.
+         * In addition, it records impressions for each new action type that changed in the list.
+         * @param list A generic list with {@link KeyboardAccessoryData.Tab}s or
+         *             {@link KeyboardAccessoryData.Action}s.
+         * @param first Index of the first element that changed.
+         * @param count Number of elements starting with |first| that were added or changed.
+         */
+        private void recordUnrecordedList(ListObservable list, int first, int count) {
+            if (!mModel.isVisible()) return;
+            if (list == mModel.getTabList()) {
+                maybeRecordBarBucket(AccessoryBarContents.WITH_TABS);
+                return;
+            }
+            if (list == mModel.getActionList()) {
+                // Remove all actions that were changed, so changes are treated as new recordings.
+                for (int index = first; index < first + count; ++index) {
+                    KeyboardAccessoryData.Action action = mModel.getActionList().get(index);
+                    mRecordedActionImpressions.remove(action.getActionType());
+                }
+                // Record any unrecorded type, but not more than once (i.e. one set of suggestion).
+                for (int index = first; index < first + count; ++index) {
+                    KeyboardAccessoryData.Action action = mModel.getActionList().get(index);
+                    maybeRecordBarBucket(
+                            action.getActionType() == AccessoryAction.AUTOFILL_SUGGESTION
+                                    ? AccessoryBarContents.WITH_AUTOFILL_SUGGESTIONS
+                                    : AccessoryBarContents.WITH_ACTIONS);
+                    if (mRecordedActionImpressions.add(action.getActionType())) {
+                        recordActionImpression(action.getActionType());
+                    }
+                }
+                return;
+            }
+            assert false : "Tried to record metrics for unknown list " + list;
+        }
+
+        /**
+         * Records whether the first impression of the bar contained any contents (which it should).
+         */
+        private void recordFirstImpression() {
+            if (!mRecordedBarBuckets.isEmpty()) return;
+            @AccessoryBarContents
+            int bucketToRecord = AccessoryBarContents.NO_CONTENTS;
+            for (@AccessoryBarContents int bucket = 0; bucket < AccessoryBarContents.COUNT;
+                    ++bucket) {
+                if (shouldRecordAccessoryBarImpression(bucket)) {
+                    bucketToRecord = AccessoryBarContents.ANY_CONTENTS;
+                    break;
+                }
+            }
+            maybeRecordBarBucket(bucketToRecord);
+        }
+
+        @Override
+        public void onItemRangeInserted(ListObservable source, int index, int count) {
+            recordUnrecordedList(source, index, count);
+        }
+
+        @Override
+        public void onItemRangeRemoved(ListObservable source, int index, int count) {}
+
+        @Override
+        public void onItemRangeChanged(
+                ListObservable<Void> source, int index, int count, @Nullable Void payload) {
+            recordUnrecordedList(source, index, count);
+        }
+
+        /**
+         * Returns an impression for the accessory bar if it hasn't occurred yet.
+         * @param bucket The bucket to record.
+         */
+        private void maybeRecordBarBucket(@AccessoryBarContents int bucket) {
+            if (!shouldRecordAccessoryBarImpression(bucket)) return;
+            mRecordedBarBuckets.add(bucket);
+            RecordHistogram.recordEnumeratedHistogram(
+                    UMA_KEYBOARD_ACCESSORY_BAR_SHOWN, bucket, AccessoryBarContents.COUNT);
+        }
+
+        /**
+         * If a checks whether the given bucket should be recorded (i.e. the property it observes is
+         * not empty, the accessory is visible and it wasn't recorded yet).
+         * @param bucket
+         * @return
+         */
+        private boolean shouldRecordAccessoryBarImpression(int bucket) {
+            if (!mModel.isVisible()) return false;
+            if (mRecordedBarBuckets.contains(bucket)) return false;
+            switch (bucket) {
+                case AccessoryBarContents.WITH_ACTIONS:
+                    return hasAtLeastOneActionOfType(mModel.getActionList(),
+                            AccessoryAction.MANAGE_PASSWORDS,
+                            AccessoryAction.GENERATE_PASSWORD_AUTOMATIC);
+                case AccessoryBarContents.WITH_AUTOFILL_SUGGESTIONS:
+                    return hasAtLeastOneActionOfType(
+                            mModel.getActionList(), AccessoryAction.AUTOFILL_SUGGESTION);
+                case AccessoryBarContents.WITH_TABS:
+                    return mModel.getTabList().size() > 0;
+                case AccessoryBarContents.ANY_CONTENTS: // Intentional fallthrough.
+                case AccessoryBarContents.NO_CONTENTS:
+                    return true; // Logged on first impression.
+            }
+            assert false : "Did not check whether to record an impression bucket " + bucket + ".";
+            return false;
+        }
+    }
+
+    /**
+     * Registers an observer to the given model that records changes for all properties.
+     * @param keyboardAccessoryModel The observable {@link KeyboardAccessoryModel}.
+     */
+    static void registerMetricsObserver(KeyboardAccessoryModel keyboardAccessoryModel) {
+        AccessoryBarObserver observer = new AccessoryBarObserver(keyboardAccessoryModel);
+        keyboardAccessoryModel.addObserver(observer);
+        keyboardAccessoryModel.addTabListObserver(observer);
+        keyboardAccessoryModel.addActionListObserver(observer);
     }
 
-    static void recordModelChanges(AccessorySheetModel accessorySheetModel) {
+    /**
+     * Registers an observer to the given model that records changes for all properties.
+     * @param accessorySheetModel The observable {@link AccessorySheetModel}.
+     */
+    static void registerMetricsObserver(AccessorySheetModel accessorySheetModel) {
         accessorySheetModel.addObserver((source, propertyKey) -> {
             if (propertyKey == AccessorySheetModel.PropertyKey.VISIBLE) {
                 if (accessorySheetModel.isVisible()) {
@@ -79,6 +223,12 @@ public class KeyboardAccessoryMetricsRecorder {
         });
     }
 
+    /**
+     * Gets the complete name of a histogram for the given tab type.
+     * @param baseHistogram the base histogram.
+     * @param tabType The tab type that determines the histogram's suffix.
+     * @return The complete name of the histogram.
+     */
     @VisibleForTesting
     static String getHistogramForType(String baseHistogram, @AccessoryTabType int tabType) {
         switch (tabType) {
@@ -91,6 +241,11 @@ public class KeyboardAccessoryMetricsRecorder {
         return "";
     }
 
+    /**
+     * Records why an accessory sheet was toggled.
+     * @param tabType The tab that was selected to trigger the sheet.
+     * @param bucket The {@link AccessorySheetTrigger} to record..
+     */
     static void recordSheetTrigger(
             @AccessoryTabType int tabType, @AccessorySheetTrigger int bucket) {
         RecordHistogram.recordEnumeratedHistogram(
@@ -119,6 +274,11 @@ public class KeyboardAccessoryMetricsRecorder {
                 bucket, AccessorySuggestionType.COUNT);
     }
 
+    /**
+     * Records the number of interactive suggestions in the given list.
+     * @param tabType The tab that contained the list.
+     * @param suggestionList The list containing all suggestions.
+     */
     static void recordSheetSuggestions(
             @AccessoryTabType int tabType, SimpleListObservable<Item> suggestionList) {
         int interactiveSuggestions = 0;
@@ -136,49 +296,6 @@ public class KeyboardAccessoryMetricsRecorder {
         }
     }
 
-    private static void recordAccessoryActionImpressions(
-            KeyboardAccessoryModel keyboardAccessoryModel) {
-        for (KeyboardAccessoryData.Action action : keyboardAccessoryModel.getActionList()) {
-            recordActionImpression(action.getActionType());
-        }
-    }
-
-    private static void recordAccessoryBarImpression(
-            KeyboardAccessoryModel keyboardAccessoryModel) {
-        boolean barImpressionRecorded = false;
-        for (@AccessoryBarContents int bucket = 0; bucket < AccessoryBarContents.COUNT; ++bucket) {
-            if (shouldRecordAccessoryImpression(bucket, keyboardAccessoryModel)) {
-                RecordHistogram.recordEnumeratedHistogram(
-                        UMA_KEYBOARD_ACCESSORY_BAR_SHOWN, bucket, AccessoryBarContents.COUNT);
-                barImpressionRecorded = true;
-            }
-        }
-        RecordHistogram.recordEnumeratedHistogram(UMA_KEYBOARD_ACCESSORY_BAR_SHOWN,
-                barImpressionRecorded ? AccessoryBarContents.ANY_CONTENTS
-                                      : AccessoryBarContents.NO_CONTENTS,
-                AccessoryBarContents.COUNT);
-    }
-
-    private static boolean shouldRecordAccessoryImpression(
-            int bucket, KeyboardAccessoryModel keyboardAccessoryModel) {
-        switch (bucket) {
-            case AccessoryBarContents.WITH_TABS:
-                return keyboardAccessoryModel.getTabList().size() > 0;
-            case AccessoryBarContents.WITH_ACTIONS:
-                return hasAtLeastOneActionOfType(keyboardAccessoryModel.getActionList(),
-                        AccessoryAction.MANAGE_PASSWORDS,
-                        AccessoryAction.GENERATE_PASSWORD_AUTOMATIC);
-            case AccessoryBarContents.WITH_AUTOFILL_SUGGESTIONS:
-                return hasAtLeastOneActionOfType(keyboardAccessoryModel.getActionList(),
-                        AccessoryAction.AUTOFILL_SUGGESTION);
-            case AccessoryBarContents.ANY_CONTENTS: // Intentional fallthrough.
-            case AccessoryBarContents.NO_CONTENTS:
-                return false; // General impression is logged last.
-        }
-        assert false : "Did not check whether to record an impression bucket " + bucket + ".";
-        return false;
-    }
-
     private static boolean hasAtLeastOneActionOfType(
             SimpleListObservable<KeyboardAccessoryData.Action> actionList,
             @AccessoryAction int... types) {

chrome/android/junit/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryControllerTest.java

@@ -368,30 +368,64 @@ public class KeyboardAccessoryControllerTest {
     }
 
     @Test
-    public void testRecordsContentImpressionsOnlyOnInitialShowing() {
+    public void testRecordsContentBarImpressionOnceAndContentsUpToOnce() {
         assertThat(RecordHistogram.getHistogramTotalCountForTesting(
                            KeyboardAccessoryMetricsRecorder.UMA_KEYBOARD_ACCESSORY_BAR_SHOWN),
                 is(0));
 
         // First showing contains actions only.
-        mModel.getActionList().add(new Action(null, 0, null));
+        mCoordinator.addTab(mTestTab);
         mMediator.keyboardVisibilityChanged(true);
 
+        assertThat(getShownMetricsCount(AccessoryBarContents.WITH_TABS), is(1));
+        assertThat(getShownMetricsCount(AccessoryBarContents.ANY_CONTENTS), is(1));
+
+        // Adding a tabs doesn't change the total impression count but the specific bucket.
+        mModel.getActionList().add(new Action(null, 0, null));
         assertThat(getShownMetricsCount(AccessoryBarContents.WITH_ACTIONS), is(1));
         assertThat(getShownMetricsCount(AccessoryBarContents.ANY_CONTENTS), is(1));
 
-        // Adding a tabs or suggestions now doesn't change the impression count
         KeyboardAccessoryData.PropertyProvider<Action> autofillSuggestionProvider =
                 new KeyboardAccessoryData.PropertyProvider<>(AUTOFILL_SUGGESTION);
         Action suggestion = new Action("Suggestion", AUTOFILL_SUGGESTION, (a) -> {});
         mCoordinator.registerActionListProvider(autofillSuggestionProvider);
         autofillSuggestionProvider.notifyObservers(new Action[] {suggestion});
+        assertThat(getShownMetricsCount(AccessoryBarContents.WITH_AUTOFILL_SUGGESTIONS), is(1));
+
+        // The other changes were not recorded again - just the changes.
+        assertThat(getShownMetricsCount(AccessoryBarContents.WITH_ACTIONS), is(1));
+        assertThat(getShownMetricsCount(AccessoryBarContents.WITH_TABS), is(1));
+        assertThat(getShownMetricsCount(AccessoryBarContents.NO_CONTENTS), is(0));
+    }
+
+    @Test
+    public void testRecordsAgainIfExistingItemsChange() {
+        assertThat(RecordHistogram.getHistogramTotalCountForTesting(
+                           KeyboardAccessoryMetricsRecorder.UMA_KEYBOARD_ACCESSORY_BAR_SHOWN),
+                is(0));
+
+        // Add a tab and show, so the accessory is permanently visible.
         mCoordinator.addTab(mTestTab);
         mMediator.keyboardVisibilityChanged(true);
 
-        assertThat(getShownMetricsCount(AccessoryBarContents.WITH_TABS), is(0));
+        // Adding an action fills the bar impression bucket and the actions set once.
+        mModel.getActionList().set(
+                new Action[] {new Action("One", AccessoryAction.GENERATE_PASSWORD_AUTOMATIC, null),
+                        new Action("Two", AccessoryAction.GENERATE_PASSWORD_AUTOMATIC, null)});
+        assertThat(getActionImpressionCount(AccessoryAction.GENERATE_PASSWORD_AUTOMATIC), is(1));
         assertThat(getShownMetricsCount(AccessoryBarContents.WITH_ACTIONS), is(1));
-        assertThat(getShownMetricsCount(AccessoryBarContents.ANY_CONTENTS), is(1));
+
+        // Adding another action leaves bar impressions unchanged but affects the actions bucket.
+        mModel.getActionList().set(
+                new Action[] {new Action("Uno", AccessoryAction.GENERATE_PASSWORD_AUTOMATIC, null),
+                        new Action("Dos", AccessoryAction.GENERATE_PASSWORD_AUTOMATIC, null)});
+        assertThat(getShownMetricsCount(AccessoryBarContents.WITH_ACTIONS), is(1));
+        assertThat(getActionImpressionCount(AccessoryAction.GENERATE_PASSWORD_AUTOMATIC), is(2));
+    }
+
+    private int getActionImpressionCount(@AccessoryAction int bucket) {
+        return RecordHistogram.getHistogramValueCountForTesting(
+                KeyboardAccessoryMetricsRecorder.UMA_KEYBOARD_ACCESSORY_ACTION_IMPRESSION, bucket);
     }
 
     private int getShownMetricsCount(@AccessoryBarContents int bucket) {