Clean up straggler classes to use embedded signin in page in the new profiles world.

Clean up straggler classes to use embedded signin in page in the new profiles world.

(namely the Bookmarks/Extension promos, and the Apps page signin link)

Unfortunately this lead to discovering that the old Signin histograms that tracked
the source of the signin page were broken, since the signin source is now always the
avatar menu. So I cleaned up that.

BUG=378792
Committed: https://crrev.com/5fb66fe67f0c862346257d5e419423f5a1a0f496
Cr-Commit-Position: refs/heads/master@{#308871}

[noms (inactive), 2014-12-05 15:35:19]

Patchset #1 (id:1) has been deleted

[noms (inactive), 2014-12-05 15:37:32]

noms@chromium.org changed reviewers:
+ rogerta@chromium.org

[noms (inactive), 2014-12-05 15:37:32]

Hi Roger,

Would you mind taking a look before I send it out to owners for review? This is
the CL that cleans up the currently broken source histogram logging. Thanks!

[Roger Tawa OOO till Jul 10th, 2014-12-08 15:33:55]

lgtm, but you may need to add some uma logging for reauth at line 108 in
signin_error_notifier_ash.cc

[noms (inactive), 2014-12-08 16:48:49]

On 2014/12/08 15:33:55, Roger Tawa wrote:
> lgtm, but you may need to add some uma logging for reauth at line 108 in
> signin_error_notifier_ash.cc

I think that's covered by sync_setup_handlercc (signin_error_notifier_ash just
opens chrome://settings/syncSetup, and then syncSetup opens the reauth page,
which already logs it)

[noms (inactive), 2014-12-12 15:44:09]

noms@chromium.org changed reviewers:
+ asvitkine@chromium.org, pkasting@chromium.org, xiyuan@chromium.org

[noms (inactive), 2014-12-12 15:44:10]

+ owners for review.

- asvitkine: histograms.xml and c/b/ui/cocoa
- xiyuan: c/b/ui/webui
- pkasting: c/b//ui/chrome_pages.cc and c/b/ui/views

Thanks!

[Alexei Svitkine (slow), 2014-12-12 15:48:56]

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (left):

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:704:
UMA_HISTOGRAM_ENUMERATION("Signin.StartPageActions", action,
If you're removing histograms, can you mark them as obsolete in the xml file?

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/s...
File chrome/browser/ui/sync/signin_histogram.h (right):

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/s...
chrome/browser/ui/sync/signin_histogram.h:15: // The user given a reauth login
screen.
Nit: Grammar.

https://codereview.chromium.org/777143003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/777143003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:54904: +  <int value="1" label="Shown"/>
I don't understand what this enum entry means in the context of the histogram
its associated with. Is it a normalizing factor to count all times its shown? If
so, histogram dashboard can already tell you the total of all groups, so it's
not really necessary. Unless there are cases when its shown that don't fall into
the other buckets - in which case maybe an Other bucket be here instead?

[noms (inactive), 2014-12-12 15:59:29]

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (left):

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:704:
UMA_HISTOGRAM_ENUMERATION("Signin.StartPageActions", action,
On 2014/12/12 15:48:55, asvitkine (OOO Dec13 - Jan4) wrote:
> If you're removing histograms, can you mark them as obsolete in the xml file?

These histograms are described with a suffix, and the affected histogram is just
"Signin", which has other histograms that aren't these ones. I wasn't sure how
exactly to do that. (see:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...)

https://codereview.chromium.org/777143003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/777143003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:54904: +  <int value="1" label="Shown"/>
On 2014/12/12 15:48:55, asvitkine (OOO Dec13 - Jan4) wrote:
> I don't understand what this enum entry means in the context of the histogram
> its associated with. Is it a normalizing factor to count all times its shown?
If
> so, histogram dashboard can already tell you the total of all groups, so it's
> not really necessary. Unless there are cases when its shown that don't fall
into
> the other buckets - in which case maybe an Other bucket be here instead?

I think so? I just added a histogram for it, since the code was logging it but
wasn't adding the histogram to the dashboard. Do you think I should remove this
value? It seems to be the only value that's being logged too.

[xiyuan, 2014-12-12 17:41:58]

c/b/ui/webui LGTM

[Alexei Svitkine (slow), 2014-12-12 17:53:05]

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (left):

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:704:
UMA_HISTOGRAM_ENUMERATION("Signin.StartPageActions", action,
On 2014/12/12 15:59:29, Monica Dinculescu wrote:
> On 2014/12/12 15:48:55, asvitkine (OOO Dec13 - Jan4) wrote:
> > If you're removing histograms, can you mark them as obsolete in the xml
file?
> 
> These histograms are described with a suffix, and the affected histogram is
just
> "Signin", which has other histograms that aren't these ones. I wasn't sure how
> exactly to do that. (see:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...)

In that case, I suggest the following:

Split the histogram_suffixes section into two sections. The first will contain
the entries that are still being logged (e.g. AllAccessPointActions and maybe
AndroidAccountConsistencyFirstRunActions?) while the second section will contain
the ones that have been removed. In the second section, add an <obsolete> tag at
the top.

https://codereview.chromium.org/777143003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/777143003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:54904: +  <int value="1" label="Shown"/>
On 2014/12/12 15:59:29, Monica Dinculescu wrote:
> On 2014/12/12 15:48:55, asvitkine (OOO Dec13 - Jan4) wrote:
> > I don't understand what this enum entry means in the context of the
histogram
> > its associated with. Is it a normalizing factor to count all times its
shown?
> If
> > so, histogram dashboard can already tell you the total of all groups, so
it's
> > not really necessary. Unless there are cases when its shown that don't fall
> into
> > the other buckets - in which case maybe an Other bucket be here instead?
> 
> I think so? I just added a histogram for it, since the code was logging it but
> wasn't adding the histogram to the dashboard. Do you think I should remove
this
> value? It seems to be the only value that's being logged too.

I guess you're trying to re-purpose the histogram to track more things than just
"Reauth Shown". This is OK - but then change the description of the histogram to
be clear. e.g. you can say "Tracks events related to the reauthentication Gaia
page." - that will make the "Shown" entry more clear (though maybe also rename
it to "Reauth Shown"). This would make more sense than "Shown" being the "reason
for showing the reauth page".

[Peter Kasting, 2014-12-12 21:43:05]

LGTM

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:91:
InstalledBubbleContent(Browser* browser,
Nit: While here, consider separating the implementations of these functions from
the declarations.  (See "What about code outside of headers?" in
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts .)

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:249:
browser_, GURL(configure_url.c_str())));
Remove .c_str() here, since GURL's constructor actually takes a std::string.

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:251: }
else if (source == sign_in_link_) {
Nit: While here, I might just do:

  if (source == sign_in_link_) {
    chrome::ShowBrowserSignin(...);
    return;
  }

  DCHECK_EQ(manage_shortcut_, source);
  ...

To get rid of the NOTREACHED(); return;.

[noms (inactive), 2014-12-16 16:55:49]

noms@chromium.org changed reviewers:
+ jwd@chromium.org

[noms (inactive), 2014-12-16 16:55:50]

+ subbing in Jesse for Alexei for histograms.xml, as he's on vacation

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (left):

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:704:
UMA_HISTOGRAM_ENUMERATION("Signin.StartPageActions", action,
On 2014/12/12 17:53:05, asvitkine (OOO until Jan4) wrote:
> On 2014/12/12 15:59:29, Monica Dinculescu wrote:
> > On 2014/12/12 15:48:55, asvitkine (OOO Dec13 - Jan4) wrote:
> > > If you're removing histograms, can you mark them as obsolete in the xml
> file?
> > 
> > These histograms are described with a suffix, and the affected histogram is
> just
> > "Signin", which has other histograms that aren't these ones. I wasn't sure
how
> > exactly to do that. (see:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...)
> 
> In that case, I suggest the following:
> 
> Split the histogram_suffixes section into two sections. The first will contain
> the entries that are still being logged (e.g. AllAccessPointActions and maybe
> AndroidAccountConsistencyFirstRunActions?) while the second section will
contain
> the ones that have been removed. In the second section, add an <obsolete> tag
at
> the top.

Done.

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/s...
File chrome/browser/ui/sync/signin_histogram.h (right):

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/sync/s...
chrome/browser/ui/sync/signin_histogram.h:15: // The user given a reauth login
screen.
On 2014/12/12 15:48:55, asvitkine (OOO until Jan4) wrote:
> Nit: Grammar.

Done.

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:91:
InstalledBubbleContent(Browser* browser,
On 2014/12/12 21:43:05, Peter Kasting wrote:
> Nit: While here, consider separating the implementations of these functions
from
> the declarations.  (See "What about code outside of headers?" in
> http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts .)

I will do it in a followup CL and send it over for review.

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:249:
browser_, GURL(configure_url.c_str())));
On 2014/12/12 21:43:05, Peter Kasting wrote:
> Remove .c_str() here, since GURL's constructor actually takes a std::string.

Done.

https://codereview.chromium.org/777143003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:251: }
else if (source == sign_in_link_) {
On 2014/12/12 21:43:05, Peter Kasting wrote:
> Nit: While here, I might just do:
> 
>   if (source == sign_in_link_) {
>     chrome::ShowBrowserSignin(...);
>     return;
>   }
> 
>   DCHECK_EQ(manage_shortcut_, source);
>   ...
> 
> To get rid of the NOTREACHED(); return;.

Done.

https://codereview.chromium.org/777143003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/777143003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:54904: +  <int value="1" label="Shown"/>
On 2014/12/12 17:53:05, asvitkine (OOO until Jan4) wrote:
> On 2014/12/12 15:59:29, Monica Dinculescu wrote:
> > On 2014/12/12 15:48:55, asvitkine (OOO Dec13 - Jan4) wrote:
> > > I don't understand what this enum entry means in the context of the
> histogram
> > > its associated with. Is it a normalizing factor to count all times its
> shown?
> > If
> > > so, histogram dashboard can already tell you the total of all groups, so
> it's
> > > not really necessary. Unless there are cases when its shown that don't
fall
> > into
> > > the other buckets - in which case maybe an Other bucket be here instead?
> > 
> > I think so? I just added a histogram for it, since the code was logging it
but
> > wasn't adding the histogram to the dashboard. Do you think I should remove
> this
> > value? It seems to be the only value that's being logged too.
> 
> I guess you're trying to re-purpose the histogram to track more things than
just
> "Reauth Shown". This is OK - but then change the description of the histogram
to
> be clear. e.g. you can say "Tracks events related to the reauthentication Gaia
> page." - that will make the "Shown" entry more clear (though maybe also rename
> it to "Reauth Shown"). This would make more sense than "Shown" being the
"reason
> for showing the reauth page".

Done.

[jwd, 2014-12-16 22:14:39]

https://codereview.chromium.org/777143003/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/777143003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:61401: <histogram_suffixes
name="Signin.Actions" separator=".">
Yeah, I think you can just create a new histogram_suffixes, with a different
name.

https://codereview.chromium.org/777143003/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/777143003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:32336: +    Signin Flow shown on android
after Account Consistency flag was enabled.
This appears to be on the wrong histogram.

[noms (inactive), 2014-12-17 15:10:44]

Hi Roger,

I moved all of the signin related histograms to components/signin, which means
I've had to touch a lot of files. Would you mind taking a look again, please?
Thanks.

[noms (inactive), 2014-12-17 15:34:01]

Jesse, I (hope) I fixed histograms.xml. Please take a look again. Thanks!

https://codereview.chromium.org/777143003/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/777143003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:61401: <histogram_suffixes
name="Signin.Actions" separator=".">
On 2014/12/16 22:14:38, Jesse Doherty wrote:
> Yeah, I think you can just create a new histogram_suffixes, with a different
> name.

Done.

https://codereview.chromium.org/777143003/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/777143003/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:32336: +    Signin Flow shown on android
after Account Consistency flag was enabled.
On 2014/12/16 22:14:38, Jesse Doherty wrote:
> This appears to be on the wrong histogram.

Acknowledged.

[Roger Tawa OOO till Jul 10th, 2014-12-17 15:52:40]

still lgtm

Great cleanup!

[noms (inactive), 2014-12-17 16:24:43]

noms@chromium.org changed reviewers:
+ rsesek@chromium.org

[noms (inactive), 2014-12-17 16:24:44]

Hi Robert,

Would you mind taking a look at chrome/browser/app_controller_mac.mm, please? I
did a bit of an enum cleanup and this file got caught in the crossfire. Thanks!

[jwd, 2014-12-17 16:54:18]

LGTM with comment

https://codereview.chromium.org/777143003/diff/50012/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/777143003/diff/50012/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:32482: +    or reauth Gaia page.
Please explicitly mention it is logged when the avatar is shown. (Assuming that
is true...)

[Robert Sesek, 2014-12-17 17:45:21]

Mac bits LGTM

[noms (inactive), 2014-12-17 18:43:51]

Woohoo, thanks everyone!

https://codereview.chromium.org/777143003/diff/50012/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/777143003/diff/50012/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:32482: +    or reauth Gaia page.
On 2014/12/17 16:54:18, Jesse Doherty wrote:
> Please explicitly mention it is logged when the avatar is shown. (Assuming
that
> is true...)

Done.

[noms (inactive), 2014-12-17 18:45:11]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-12-17 18:46:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777143003/190001

[commit-bot: I haz the power, 2014-12-17 18:51:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-17 18:51:12]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[noms (inactive), 2014-12-17 19:49:18]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-12-17 19:50:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777143003/230001

[commit-bot: I haz the power, 2014-12-17 20:07:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-17 20:07:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[noms (inactive), 2014-12-17 20:11:12]

Patchset #11 (id:210001) has been deleted

[noms (inactive), 2014-12-17 20:38:34]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-12-17 20:39:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777143003/250001

[commit-bot: I haz the power, 2014-12-17 23:01:47]

Committed patchset #12 (id:250001)

[commit-bot: I haz the power, 2014-12-17 23:02:38]

Patchset 12 (id:??) landed as
https://crrev.com/5fb66fe67f0c862346257d5e419423f5a1a0f496
Cr-Commit-Position: refs/heads/master@{#308871}