Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2301263004:
Add WebAPK installation metrics. (Closed)
DescriptionAdd WebAPK installation metrics.
Records metrics when WebAPK is installed via banner or add-to-homescreen menu,
including: install events, installation start types, and user actions.
Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt-I/edit#heading=h.1ctovw0c1tx
BUG=626510
Committed: https://crrev.com/2f4bcfddf49def3cf2f81700922f43390e06a8a5
Cr-Commit-Position: refs/heads/master@{#420433}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Update the metrics. #
Total comments: 2
Patch Set 3 : Rebase. #
Total comments: 21
Patch Set 4 : domnickn@'s comments. #
Total comments: 27
Patch Set 5 : Another round of comments. #
Total comments: 16
Patch Set 6 : Renaming. #
Total comments: 2
Patch Set 7 : Nits. #
Total comments: 6
Patch Set 8 : pkotwicz@'s comments. #
Messages
Total messages: 42 (21 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add WebAPK installation metrics. Records metrics when WebAPK is installed via banner or add-to-homescreen menu, including: install events, dismiss events, and user actions. Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt... BUG=626510 ========== to ========== Add WebAPK installation metrics. Records metrics when WebAPK is installed via banner or add-to-homescreen menu, including: install events, dismiss events, and user actions. Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt... BUG=626510 ==========
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org, dominickn@chromium.org
Hi dominickn@ and dfalcantara@: I would like to know your opinions about whether I should split the mixed data between WebAPK and regular Webapps further. Please take a look, thanks!
One challenge we've found with the app banner metrics is that they're difficult to interpret, because the same histogram records multiple buckets in an additive fashion. Also, we never really look at the dismiss metrics. I left a comment on the doc to discuss. https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (left): https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:355: DVLOG(1) << "The WebAPK installation failed."; Not sure if you want to remove the DVLOG here - remember that metrics are only recorded for ~10% of Chrome's user base, so it may still be handy to have something in the log. Not sure what the right thing to do is. https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:32: enum InstallState { This enum should be in webapk_metrics.h https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_metrics.cc (right): https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_metrics.cc:7: #include "base/metrics/histogram_macros.h" If you're only using SPARSE_SLOWLY, then I think you can get rid of this include. https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_metrics.cc:16: void TrackDismissEvent(int event) { You could make these take the enum argument directly, and avoid the need for the DCHECKs? Again, the app banner code isn't an exemplar for this. :) https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_metrics.h:24: DISMISS_EVENT_MIN = 10, Start these enums from 0. The app banner ones don't for historical reasons; if I was reimplementing them (which I will be), I'm going to start them all from 0. Additionally, you should do: enum InstallEvent { INSTALL_EVENT_MIN = 0, INSTALL_EVENT_ADD_FROM_BANNER_STARTED = INSTALL_EVENT_MIN, //etc. } So that we don't waste the 0 bucket.
Description was changed from ========== Add WebAPK installation metrics. Records metrics when WebAPK is installed via banner or add-to-homescreen menu, including: install events, dismiss events, and user actions. Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt... BUG=626510 ========== to ========== Add WebAPK installation metrics. Records metrics when WebAPK is installed via banner or add-to-homescreen menu, including: install events, installation start types, and user actions. Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt... BUG=626510 ==========
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Hi Dominick, I replied your comments on the "webapk installation metrics" doc and updated this CL accordingly. PTAL, thanks! https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (left): https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:355: DVLOG(1) << "The WebAPK installation failed."; On 2016/09/05 07:34:31, dominickn wrote: > Not sure if you want to remove the DVLOG here - remember that metrics are only > recorded for ~10% of Chrome's user base, so it may still be handy to have > something in the log. Not sure what the right thing to do is. Good point, add the LOG back. https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:32: enum InstallState { On 2016/09/05 07:34:31, dominickn wrote: > This enum should be in webapk_metrics.h Done. https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_metrics.cc (right): https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_metrics.cc:7: #include "base/metrics/histogram_macros.h" On 2016/09/05 07:34:31, dominickn wrote: > If you're only using SPARSE_SLOWLY, then I think you can get rid of this > include. I change to use UMA_HISTOGRAM_ENUMERATION. https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_metrics.cc:16: void TrackDismissEvent(int event) { On 2016/09/05 07:34:31, dominickn wrote: > You could make these take the enum argument directly, and avoid the need for the > DCHECKs? Again, the app banner code isn't an exemplar for this. :) Done. https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_metrics.h:24: DISMISS_EVENT_MIN = 10, On 2016/09/05 07:34:31, dominickn wrote: > Start these enums from 0. The app banner ones don't for historical reasons; if I > was reimplementing them (which I will be), I'm going to start them all from 0. > > Additionally, you should do: > > enum InstallEvent { > INSTALL_EVENT_MIN = 0, > INSTALL_EVENT_ADD_FROM_BANNER_STARTED = INSTALL_EVENT_MIN, > //etc. > } > > So that we don't waste the 0 bucket. I see, thanks, updated. Also, since I changed to UMA_HISTOGRAM_ENUMRATION, the declaration of the event enum is more flexible.
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by hanxi@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: This issue passed the CQ dry run.
This is looking better, but it's still a bit confusing. I'm not sure if the metrics are actually properly exclusive; I think I need to sit down next week and draw out exactly where all these metrics calls are happening to work it out before I leave final comments. Just to check that my understanding matches yours, this is how I envision the WebAPK install flow with metrics EVENT, DISMISS, and INSTALL: 1. User presses add to homescreen in the app menu OR an ap banner is triggered a. record EVENT:WEBAPK_PROMPT_TRIGGERED for an overall number 2. User accepts or rejects the banner a. if the user dismisses, record RESPONSE:PROMPT_DISMISSED_BEFORE_INSTALL and INSTALL:FAILED_USER_DISMISSED_BEFORE_INSTALL and return 3. Begin installation a. if the user dismisses the prompt, record RESPONSE:PROMPT_DISMISSED_DURING_INSTALL and INSTALL:FAILED_USER_DISMISSED_DURING_INSTALL 4. Installation finishes or fails a. if fails, record INSTALL:FAILED_SYSTEM_ERROR b. if it succeeded, record INSTALL:SUCCEED 5. User opens the WebAPK or dismisses the banner a. record RESPONSE:OPEN_WEBAPK b. record RESPONSE:DISMISS_AFTER_INSTALL Is that understanding correct? The INSTALL histogram tells you whether the WebAPK ended up installed or not, while the RESPONSE histogram tells you what the user directly did? Would appreciate it if you could check my understanding! https://codereview.chromium.org/2301263004/diff/80001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/80001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:50: webapk::TrackInstallEvent(webapk::INSTALL_EVENT_ADDING_DISMISS); Why is this method recording metrics in two different histograms based on the install state? That seems like it could be a source of bugs. https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:333: webapk::TrackStartType(webapk::STARTED_FROM_BANNER); Won't this metric be hit at least twice when the user installs a WebAPK? Once in AppBannerInfoBarDelegateAndroid::Create, and again here? The way I understand it, when the A2HS menu item is selected for a WebAPK, it'll create an infobar delegate, show the infobar, and wait for the user to hit Add before proceeding right? https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:8: #include "base/macros.h" Nit: I don't think you use this header. https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:14: enum InstallState { Nit: histograms reviewer will probably ask you to add this comment to all of these enums. "This enum backs an UMA histogram, and new entries must be added at the end prior to INSTALL_EVENT_MAX" https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:14: enum InstallState { This is actually a DismissEvent enum - it's recording what state the WebAPK installation was in when the infobar was dismissed. Rename it, e.g. InstallStateOnDismiss. https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:25: INSTALL_EVENT_BANNER_IGNORED = 0, Nit: I don't think you need the explicit = 0 now. https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:27: // installation stared. The *_DISMISS events are the cases that user Nit: installation started Nit: that the user Nit: no more _DISMISS events https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65290: +<histogram name="WebApk.Install.InstallEvent" enum="WebApkInstallEvent"> Make sure you update this description when you rename the histogram to InstallStateOnDismiss. https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65295: + WebAPKs are installed via App banners or via user clicking Add-To-Homescreen Nit: change the first sentence for all of these to: "WebAPKs are PWAs wrapped in an Android apk, installed from an app banner or the add to homescreen menu item." https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65298: + user before or during the installation, as well as whether the installation Nit: by the user https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65299: + is sucessful or failed. Nit: whether the installation was successful. https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:97986: + <int value="0" label="Banner is ignored"/> WebAPK banner was ignored
On 2016/09/09 08:14:12, dominickn wrote:
> This is looking better, but it's still a bit confusing. I'm not sure if the
> metrics are actually properly exclusive; I think I need to sit down next week
> and draw out exactly where all these metrics calls are happening to work it
out
> before I leave final comments.
>
> Just to check that my understanding matches yours, this is how I envision the
> WebAPK install flow with metrics EVENT, DISMISS, and INSTALL:
>
> 1. User presses add to homescreen in the app menu OR an ap banner is triggered
> a. record EVENT:WEBAPK_PROMPT_TRIGGERED for an overall number
I only record EVENT: WEBAPK_PROMPT_TRIGGERED when the installation starts from
app banner. This is because for the case via menu, once user clicks the menu the
installation is starting immediately.
In order to know via which way the installation is really started (accepted by
user after the banner is triggered), record EVENT:
WEBAPK_PROMPT_ACCEPTED_FROM_BANNER & WEBAPK_PROMPT_ACCEPTED_FROM_MENU.
> 2. User accepts or rejects the banner
> a. if the user dismisses, record RESPONSE:PROMPT_DISMISSED_BEFORE_INSTALL
and
> INSTALL:FAILED_USER_DISMISSED_BEFORE_INSTALL and return
Correct. I use PROMPT_DISMISSED_ADD_TO_HOMESCREEN to represent
PROMPT_DISMISSED_BEFORE_INSTALL, since that is the text on the button when the
dismiss happens.
> 3. Begin installation
> a. if the user dismisses the prompt, record
> RESPONSE:PROMPT_DISMISSED_DURING_INSTALL and
> INSTALL:FAILED_USER_DISMISSED_DURING_INSTALL
Correct. I use PROMPT_DISMISSED_ADDING to represent
PROMPT_DISMISSED_DURING_INSTALL since the text on the button is "Adding".
> 4. Installation finishes or fails
> a. if fails, record INSTALL:FAILED_SYSTEM_ERROR
> b. if it succeeded, record INSTALL:SUCCEED
correct.
> 5. User opens the WebAPK or dismisses the banner
> a. record RESPONSE:OPEN_WEBAPK
> b. record RESPONSE:DISMISS_AFTER_INSTALL
correct.
This is a 6: The banner is ignored due to the facts that Chrome or the tab is
closed.
a. INSTALL: BANNER_IGNORED
Same as regular banner, this records the banner is ignored between the banner is
triggered and user has any response (e.g, click the infobar button or the "X"
button). Once user has any response, we actually cannot tell whether the banner
is dismissed or ignored in the destructor of the infobar delegate, unless adding
more installation states to track.
> Is that understanding correct? The INSTALL histogram tells you whether the
> WebAPK ended up installed or not, while the RESPONSE histogram tells you what
> the user directly did? Would appreciate it if you could check my
understanding!
Yes, your INSTALL histogram is exactly what I have designed, your EVENT
histogram is more like my START_TYPE histogram. Your RESPONSE histogram is
similar to my USER_ACTION, but has DISMISS_*_RESPONSES which are not in my
design. This is because they duplicate their INSTALL counterparts, so I tried to
avoid duplications in these two histogram. I could add them back if you suggest
to have all the responses in the RESPONSE histogram.
Additionally, the |InstallState| isn't a histogram, remember it was first
declared in the AppBannerInfoBarDelegateAndroid? Maybe I'd better to move it to
somewhere else to avoid confusion.
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Hi Dominick, PTAL, thanks! https://codereview.chromium.org/2301263004/diff/80001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/80001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:50: webapk::TrackInstallEvent(webapk::INSTALL_EVENT_ADDING_DISMISS); On 2016/09/09 08:14:11, dominickn wrote: > Why is this method recording metrics in two different histograms based on the > install state? That seems like it could be a source of bugs. Because the first two are recorded in the INSTALL_EVENT, which contains every possible results of the installation process. The third one belongs to the scenario of INSTALL_COMPELETE, but a user action whether the "open" button is clicked or not. https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:333: webapk::TrackStartType(webapk::STARTED_FROM_BANNER); On 2016/09/09 08:14:11, dominickn wrote: > Won't this metric be hit at least twice when the user installs a WebAPK? Once in > AppBannerInfoBarDelegateAndroid::Create, and again here? The way I understand > it, when the A2HS menu item is selected for a WebAPK, it'll create an infobar > delegate, show the infobar, and wait for the user to hit Add before proceeding > right? No, it is only hit here, not by AppBannerInfoBarDelegateAndroid::Create(). When the A2HS menu item is selected, the banner will skip the waiting step but directly jumping to the "Adding" step. https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:8: #include "base/macros.h" On 2016/09/09 08:14:11, dominickn wrote: > Nit: I don't think you use this header. Done. https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:14: enum InstallState { On 2016/09/09 08:14:12, dominickn wrote: > Nit: histograms reviewer will probably ask you to add this comment to all of > these enums. Updated. > "This enum backs an UMA histogram, and new entries must be added at the end > prior to INSTALL_EVENT_MAX" The InstallState isn't for a UMA histogram, so I think maybe it is better to move it back to AppBannerInfobarDelegateAndroid to avoid confusion? Besides, do we really need to rename it to InstallStateOnDismiss since these states are fairly generic? https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:25: INSTALL_EVENT_BANNER_IGNORED = 0, On 2016/09/09 08:14:12, dominickn wrote: > Nit: I don't think you need the explicit = 0 now. Done. https://codereview.chromium.org/2301263004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:27: // installation stared. The *_DISMISS events are the cases that user On 2016/09/09 08:14:12, dominickn wrote: > Nit: installation started > > Nit: that the user > > Nit: no more _DISMISS events Updated to: "Dismiss" means the user closes the banner by clicking the "X" button." Is this one better? https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65290: +<histogram name="WebApk.Install.InstallEvent" enum="WebApkInstallEvent"> On 2016/09/09 08:14:12, dominickn wrote: > Make sure you update this description when you rename the histogram to > InstallStateOnDismiss. This won't be renamed to InstallStateOnDismiss:( This is the histogram for all the possible results of WebAPK installation flow. https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65295: + WebAPKs are installed via App banners or via user clicking Add-To-Homescreen On 2016/09/09 08:14:12, dominickn wrote: > Nit: change the first sentence for all of these to: "WebAPKs are PWAs wrapped in > an Android apk, installed from an app banner or the add to homescreen menu > item." Done. https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65298: + user before or during the installation, as well as whether the installation On 2016/09/09 08:14:12, dominickn wrote: > Nit: by the user Done. https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65299: + is sucessful or failed. On 2016/09/09 08:14:12, dominickn wrote: > Nit: whether the installation was successful. Done. https://codereview.chromium.org/2301263004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:97986: + <int value="0" label="Banner is ignored"/> On 2016/09/09 08:14:12, dominickn wrote: > WebAPK banner was ignored Done.
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
(Apologies for the delay - I need to sit down and work through the callsites here, but perf + interviews have taken a bunch of my time this week. Will be on this soon.)
Hi Dominick, do you have a chance to look at this CL?
Sorry for the delay. Finally got to this and I think I understand what's going on now. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:47: void TrackWebApkInstallationDismissEvents(banners::InstallState install_state) { As per comment in the .h file, consider making this a private class member method, with the enum as a private class member? https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:49: webapk::TrackInstallEvent(webapk::INSTALL_EVENT_ADD_TO_HOME_SCREEN_DISMISS); Won't you be in the WAIT_FOR_START state if you trigger a banner, and then the user dismisses it? ADD_TO_HOME_SCREEN_DISMISS might not be the best name. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:87: webapk::TrackStartType(webapk::TRIGGERED_FROM_BANNER); See comment in webapk_metrics.h https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:200: return true; If a user already has the WebAPK installed, it seems to me that the following happens: InfoBar::Create (install_state_ = WAITING, has_user_interaction_ = false) InfoBar::AcceptWebAPK (install_state_ = WAITING, has_user_interaction_ = false) Then what happens? When the infobar is eventually destroyed, its destructor will call webapk::TrackInstallEvent(webapk::INSTALL_EVENT_BANNER_IGNORED) in this case, which doesn't seem right. Have you tested this flow to make sure the right thing happens? https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:32: enum InstallState { Nit: call this WebApkInstallState, and change the comment to: "The state of a WebAPK installation, where the infobar is displayed during the entire installation process. This state is used to correctly record UMA metrics". Second nit: consider making this enum a private member of AppBannerInfoBarDelegate, since it's an implementation detail. The method in the anonymous namespace of the cc file can also move to the private area of the class. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:33: WAIT_FOR_START, Nit: "INSTALL_NOT_STARTED"? https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:143: // Indicates the current state of a WebAPK installation. Nit: add a newline above the comment. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:21: enum InstallEvent { I think if you get rid of the INSTALL_EVENT prefixes here that'll make it easier to work out what is going on. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:22: // Banner is deleted due to the closing of Chrome or the associated tab. Nit: change comment to "The user did not interact with the banner". Banners can also be closed by the user navigating from the page. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:27: INSTALL_EVENT_ADD_TO_HOME_SCREEN_DISMISS, Call this BANNER_DISMISSED_BEFORE_INSTALLATION. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:29: INSTALL_EVENT_ADDING_DISMISS, Call this DISMISSED_DURING_INSTALLATION https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:35: // The ways of how WebAPK instllation is triggered / started. Nit: "The ways in which WebAPK installation can be started." https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:37: TRIGGERED_FROM_BANNER, TRIGGERED and STARTED are pretty difficult to tell apart from the names. From what I can tell, TRIGGERED means that the banner was triggered, while STARTED means that the user clicked INSTALL on the banner? These events seem like they begin in different histograms: TRIGGERED seems to record that a WebAPK banner was shown, and thus shouldn't be in this metric?
Patchset #5 (id:200001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:47: void TrackWebApkInstallationDismissEvents(banners::InstallState install_state) { On 2016/09/19 01:38:39, dominickn wrote: > As per comment in the .h file, consider making this a private class member > method, with the enum as a private class member? Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:49: webapk::TrackInstallEvent(webapk::INSTALL_EVENT_ADD_TO_HOME_SCREEN_DISMISS); On 2016/09/19 01:38:39, dominickn wrote: > Won't you be in the WAIT_FOR_START state if you trigger a banner, and then the > user dismisses it? ADD_TO_HOME_SCREEN_DISMISS might not be the best name. Rename it to DISMISS_BEFORE_INSTALLATION. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:87: webapk::TrackStartType(webapk::TRIGGERED_FROM_BANNER); On 2016/09/19 01:38:39, dominickn wrote: > See comment in webapk_metrics.h Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:200: return true; On 2016/09/19 01:38:39, dominickn wrote: > If a user already has the WebAPK installed, it seems to me that the following > happens: > > InfoBar::Create (install_state_ = WAITING, has_user_interaction_ = false) > InfoBar::AcceptWebAPK (install_state_ = WAITING, has_user_interaction_ = false) > Then what happens? > > When the infobar is eventually destroyed, its destructor will call > webapk::TrackInstallEvent(webapk::INSTALL_EVENT_BANNER_IGNORED) in this case, > which doesn't seem right. Have you tested this flow to make sure the right thing > happens? This is a good point. I add "has_user_interaction_ = true" in the |AcceptWebApk| to avoid the webapk::TrackInstallEvent(webapk::INSTALL_EVENT_BANNER_IGNORED) being called in the destructor. Additionally, if the WebAPK is already installed: 1) the app banner doesn't show. 2) we filed a bug to figure out what to do when user clicks A2HS menu if WebAPK is installed (crbug.com/638614). After fixing the bug of the case 2), I think another state "Open" should be introduced, and the banner simply shows the "open" button to point the user to the WebAPK installed. For example: InfoBar::Create (install_state_ = OPEN, has_user_interaction_ = true) InfoBar::AcceptWebAPK (install_state_ = OPEN, has_user_interaction_ = true) https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:32: enum InstallState { On 2016/09/19 01:38:39, dominickn wrote: > Nit: call this WebApkInstallState, and change the comment to: > > "The state of a WebAPK installation, where the infobar is displayed during the > entire installation process. This state is used to correctly record UMA > metrics". > > > Second nit: consider making this enum a private member of > AppBannerInfoBarDelegate, since it's an implementation detail. The method in the > anonymous namespace of the cc file can also move to the private area of the > class. Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:33: WAIT_FOR_START, On 2016/09/19 01:38:39, dominickn wrote: > Nit: "INSTALL_NOT_STARTED"? Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:143: // Indicates the current state of a WebAPK installation. On 2016/09/19 01:38:39, dominickn wrote: > Nit: add a newline above the comment. Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:21: enum InstallEvent { On 2016/09/19 01:38:39, dominickn wrote: > I think if you get rid of the INSTALL_EVENT prefixes here that'll make it easier > to work out what is going on. Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:22: // Banner is deleted due to the closing of Chrome or the associated tab. On 2016/09/19 01:38:39, dominickn wrote: > Nit: change comment to "The user did not interact with the banner". > > Banners can also be closed by the user navigating from the page. Acknowledged. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:27: INSTALL_EVENT_ADD_TO_HOME_SCREEN_DISMISS, On 2016/09/19 01:38:39, dominickn wrote: > Call this BANNER_DISMISSED_BEFORE_INSTALLATION. Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:29: INSTALL_EVENT_ADDING_DISMISS, On 2016/09/19 01:38:39, dominickn wrote: > Call this DISMISSED_DURING_INSTALLATION Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:35: // The ways of how WebAPK instllation is triggered / started. On 2016/09/19 01:38:39, dominickn wrote: > Nit: "The ways in which WebAPK installation can be started." Done. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:37: TRIGGERED_FROM_BANNER, On 2016/09/19 01:38:39, dominickn wrote: > TRIGGERED and STARTED are pretty difficult to tell apart from the names. From > what I can tell, TRIGGERED means that the banner was triggered, while STARTED > means that the user clicked INSTALL on the banner? These events seem like they > begin in different histograms: TRIGGERED seems to record that a WebAPK banner > was shown, and thus shouldn't be in this metric? Hmm, how about introduce a "InstallTriggeredType" if that makes the meaning of each metrics more clear? We do care about how many times that banner was shown and times of installation is actually started in both ways. It might be less confusing to have both TRIGGERED_FROM_BANNER & TRIGGERED_FROM_ADD_TO_HOME_SCREEN_MENU. The only concern is the TRIGGERED_FROM_ADD_TO_HOME_SCREEN_MENU is the same measurement as STARTED_FROM_ADD_TO_HOME_SCREEN_MENU.
Just about there now. I think I've finally figured out the right naming to use here - thanks for bearing with me. If you send another copy of this out and I'm still awake, I'll have another look. Btw, it would be good to have histogram tests here if that's possible, though that can be for another CL since I know you're pushing for M55 dev. https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:200: return true; This seems reasonable to me. https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:412: else if (install_state == INSTALLED) Minor nit: add an explicit comment saying that when install_state is INSTALLED, the InstallEvent will have been recorded in OnWebApkInstallFinished. That'll stop dumb questions from the likes of me. :) https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:27: // The ways in which WebAPK instllation can be started. Nit: "installation" https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:30: STARTED_FROM_ADD_TO_HOME_SCREEN_MENU, General nit: change ADD_TO_HOME_SCREEN_MENU to just MENU in these, that's much easier to parse. https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:34: // The ways in which WebApk installation can be triggered. Splitting these histograms is the right move, but I still think the distinction between "triggered" and "started" is very easy to get wrong. How about this: // The WebAPK infobar source, i.e. where the infobar was triggered from. enum InfoBarShown { WEBAPK_INFOBAR_SHOWN_FROM_MENU, WEBAPK_INFOBAR_SHOWN_FROM_BANNER, WEBAPK_INFOBAR_SHOWN_MAX } // The WebAPK installation source, i.e. where an installation was triggered from. Counts under INSTALL_SOURCE_MENU should be identical to WEBAPK_INFOBAR_SHOWN_FROM_MENU, as the add to homescreen menu item immediately shows the WebAPK infobar and starts the installation. Counts under INSTALL_SOURCE_BANNER will be lower than WEBAPK_INFOBAR_SHOWN_BANNER, as users must explicitly click accept in the infobar to begin the installation from a banner. enum InstallSource { INSTALL_SOURCE_BANNER, INSTALL_SOURCE_MENU, INSTALL_SOURCE_MAX } This is much clearer to me that one is about when a WebAPK infobar is shown, and the other is when WebAPK installation starts. If you try and remove usage of the "banner" term, and instead use "WebAPK infobar" in comments where you could have the A2HS menu item or app banners triggering the infobar, I think that will help too. Thoughts? Btw, sorry it's taken me this long to think about the naming. The metrics here are quite complicated, and I think that getting the names right is really important to make sure that we can interpret everything. https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:66030: + the ways that user accepets to install a WebAPK. Nit: "accepts". https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:66052: + the add to homescreen menu item. If user clicks add to homescreen menu, and Nit: "If the user" https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:66053: + a WebAPK has been installed before, an app banner with an open button will Nit: use "infobar" rather than "app banner" unless you're talking about the WebAPK infobar triggering as an app banner specifically. That will clarify things I think https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:66054: + show. Besides, the open button will also show on the app banner after a Nit: remove "Besides", use "infobar" instead of "app banner"
Thanks for all the suggestions about naming. It makes the metrics look much clean now. Sure, I will add metrics tests in a following up CL. PTAL, thanks! https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:412: else if (install_state == INSTALLED) On 2016/09/21 06:54:27, dominickn wrote: > Minor nit: add an explicit comment saying that when install_state is INSTALLED, > the InstallEvent will have been recorded in OnWebApkInstallFinished. That'll > stop dumb questions from the likes of me. :) Done. https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:27: // The ways in which WebAPK instllation can be started. On 2016/09/21 06:54:27, dominickn wrote: > Nit: "installation" Sorry for the typo. https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:30: STARTED_FROM_ADD_TO_HOME_SCREEN_MENU, On 2016/09/21 06:54:28, dominickn wrote: > General nit: change ADD_TO_HOME_SCREEN_MENU to just MENU in these, that's much > easier to parse. Done. https://codereview.chromium.org/2301263004/diff/220001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:34: // The ways in which WebApk installation can be triggered. On 2016/09/21 06:54:28, dominickn wrote: > Splitting these histograms is the right move, but I still think the distinction > between "triggered" and "started" is very easy to get wrong. How about this: > > // The WebAPK infobar source, i.e. where the infobar was triggered from. > enum InfoBarShown { > WEBAPK_INFOBAR_SHOWN_FROM_MENU, > WEBAPK_INFOBAR_SHOWN_FROM_BANNER, > WEBAPK_INFOBAR_SHOWN_MAX > } > > // The WebAPK installation source, i.e. where an installation was triggered > from. Counts under INSTALL_SOURCE_MENU should be identical to > WEBAPK_INFOBAR_SHOWN_FROM_MENU, as the add to homescreen menu item immediately > shows the WebAPK infobar and starts the installation. Counts under > INSTALL_SOURCE_BANNER will be lower than WEBAPK_INFOBAR_SHOWN_BANNER, as users > must explicitly click accept in the infobar to begin the installation from a > banner. That is correct. > enum InstallSource { > INSTALL_SOURCE_BANNER, > INSTALL_SOURCE_MENU, > INSTALL_SOURCE_MAX > } > > This is much clearer to me that one is about when a WebAPK infobar is shown, and > the other is when WebAPK installation starts. If you try and remove usage of the > "banner" term, and instead use "WebAPK infobar" in comments where you could have > the A2HS menu item or app banners triggering the infobar, I think that will help > too. Thoughts? Agree, infobar is preferred here, comparing with app banners. I will update comments. > Btw, sorry it's taken me this long to think about the naming. The metrics here > are quite complicated, and I think that getting the names right is really > important to make sure that we can interpret everything. Totally agree. https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:66030: + the ways that user accepets to install a WebAPK. On 2016/09/21 06:54:28, dominickn wrote: > Nit: "accepts". Done. https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:66052: + the add to homescreen menu item. If user clicks add to homescreen menu, and On 2016/09/21 06:54:28, dominickn wrote: > Nit: "If the user" Done. https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:66053: + a WebAPK has been installed before, an app banner with an open button will On 2016/09/21 06:54:28, dominickn wrote: > Nit: use "infobar" rather than "app banner" unless you're talking about the > WebAPK infobar triggering as an app banner specifically. That will clarify > things I think Done. https://codereview.chromium.org/2301263004/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:66054: + show. Besides, the open button will also show on the app banner after a On 2016/09/21 06:54:28, dominickn wrote: > Nit: remove "Besides", use "infobar" instead of "app banner" Done.
lgtm % spelling and assuming tests are coming (there's still time to catch and fix metrics bugs before M55). :) https://codereview.chromium.org/2301263004/diff/240001/chrome/browser/android... File chrome/browser/android/webapk/webapk_metrics.h (right): https://codereview.chromium.org/2301263004/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:11: // Events for WebAPKs installation flow. The sum of |InstalllEvent| histogram Nit: InstallEvent https://codereview.chromium.org/2301263004/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_metrics.h:34: // The ways in which WebApk installation can be triggered. Nit: The ways in which the WebAPK infobar can be shown.
rs lgtm
hanxi@chromium.org changed reviewers: + gayane@chromium.org
hanxi@chromium.org changed reviewers: + holte@chromium.org - gayane@chromium.org
+holte@: could you please review histograms.xml, thanks!
LGTM with a nit and a few comments for a follow up CL https://codereview.chromium.org/2301263004/diff/260001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:47: } // namespace Nit: "namespace" -> "anonymous namespace" https://codereview.chromium.org/2301263004/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:75: raw_delegate->AcceptWebApk(web_contents); For a follow up CL: - I think you should call Accept() instead of AcceptWebApk() here. That way you do not need to set |has_user_interaction_| in both Accept() and AcceptWebApk() - Determining the install source from |start_install_webapk| feels hacky to me. I would prefer passing webapk::InstallSource to the constructor and using the webapk::InstallSource to determine whether to start he install in the constructor https://codereview.chromium.org/2301263004/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:413: // |OnWebApkInstallFinished|. Nit: "|OnWebApkInstallFinished|" -> "OnWebApkInstallFinished()"
histograms lgtm
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, dfalcantara@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2301263004/#ps280001 (title: "pkotwicz@'s comments.")
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 WebAPK installation metrics. Records metrics when WebAPK is installed via banner or add-to-homescreen menu, including: install events, installation start types, and user actions. Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt... BUG=626510 ========== to ========== Add WebAPK installation metrics. Records metrics when WebAPK is installed via banner or add-to-homescreen menu, including: install events, installation start types, and user actions. Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt... BUG=626510 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add WebAPK installation metrics. Records metrics when WebAPK is installed via banner or add-to-homescreen menu, including: install events, installation start types, and user actions. Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt... BUG=626510 ========== to ========== Add WebAPK installation metrics. Records metrics when WebAPK is installed via banner or add-to-homescreen menu, including: install events, installation start types, and user actions. Design doc: https://docs.google.com/document/d/1pQPrHeI4L_jUZ8B8Sh4N0z_yiXwiFuoEhLl3AMXvt... BUG=626510 Committed: https://crrev.com/2f4bcfddf49def3cf2f81700922f43390e06a8a5 Cr-Commit-Position: refs/heads/master@{#420433} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2f4bcfddf49def3cf2f81700922f43390e06a8a5 Cr-Commit-Position: refs/heads/master@{#420433}
Message was sent while issue was closed.
https://codereview.chromium.org/2301263004/diff/260001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2301263004/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:47: } // namespace On 2016/09/22 14:35:25, pkotwicz wrote: > Nit: "namespace" -> "anonymous namespace" Done. https://codereview.chromium.org/2301263004/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:75: raw_delegate->AcceptWebApk(web_contents); On 2016/09/22 14:35:25, pkotwicz wrote: > For a follow up CL: > - I think you should call Accept() instead of AcceptWebApk() here. That way you > do not need to set |has_user_interaction_| in both Accept() and AcceptWebApk() I had the same thought, but I wasn't sure whether we prefer to make Accept() public or AcceptWebApk(), since the former has more functionality than the latter. > - Determining the install source from |start_install_webapk| feels hacky to me. > I would prefer passing webapk::InstallSource to the constructor and using the > webapk::InstallSource to determine whether to start he install in the > constructor Sure, we can do that. https://codereview.chromium.org/2301263004/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:413: // |OnWebApkInstallFinished|. On 2016/09/22 14:35:25, pkotwicz wrote: > Nit: "|OnWebApkInstallFinished|" -> "OnWebApkInstallFinished()" Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
