rlz: Change omnibox / homepage access points from C1/C2 to C5/C6 on mac

rlz: Change omnibox / homepage access points from C1/C2 to C5/C6 on mac

BUG=134939
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=145337

[Nico, 2012-06-27 23:10:18]

(order fixed on rietveld)


On Wed, Jun 27, 2012 at 4:08 PM, <thakis@chromium.org> wrote:

> Reviewers: Roger Tawa,
>
> Description:
> rlz: Change omnibox / homepage access points from C1/C2 to C5/C6 on mac
>
> BUG=134939
> TEST=none
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10698024/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M rlz/lib/lib_values.cc
>
>
> Index: rlz/lib/lib_values.cc
> diff --git a/rlz/lib/lib_values.cc b/rlz/lib/lib_values.cc
> index 467e45683468172a9d2f40425fde00**7c8648be0d..**
> 4781cb69d2f459f8b2ec0e21d3b560**864b76f61b 100644
> --- a/rlz/lib/lib_values.cc
> +++ b/rlz/lib/lib_values.cc
> @@ -67,8 +67,13 @@ const char* GetAccessPointName(AccessPoint point) {
>   case GD_SEARCH_GADGET:              return "D2";
>   case GD_WEB_SERVER:                 return "D3";
>   case GD_OUTLOOK:                    return "D4";
> +#if defined(OS_MACOSX)
>   case CHROME_OMNIBOX:                return "C1";
>   case CHROME_HOME_PAGE:              return "C2";
> +#else
> +  case CHROME_OMNIBOX:                return "C5";
> +  case CHROME_HOME_PAGE:              return "C6";
> +#endif
>   case FFTB2_BOX:                     return "B2";
>   case FFTB3_BOX:                     return "B3";
>   case PINYIN_IME_BHO:                return "N1";
>
>
>

[(NOT FOR CODE REVIEWS), 2012-06-28 00:26:31]

Hi Nico,

It should not be done at the rlzlib level.  lib_values.cc should contain all the
access points, for windows and mac.  Then in chrome\browser\rlz\rlz.cc, we
should have the #ifdef to pick the right value.  This is really a chrome thing,
not an rlzlib thing.

[Nico, 2012-06-29 01:42:45]

On 2012/06/28 00:26:31, rogerta1 wrote:
> Hi Nico,
> 
> It should not be done at the rlzlib level.  lib_values.cc should contain all
the
> access points, for windows and mac.  Then in chrome\browser\rlz\rlz.cc, we
> should have the #ifdef to pick the right value.  This is really a chrome
thing,
> not an rlzlib thing.

I started going down that road, but:

$ git gs CHROME_OMNIBOX | wc -l
      30

As far as I understand, chrome is the only product that uses the two CHROME_
access points -- are you sure the current patch isn't a simpler way to achieve
the same thing?

[Roger Tawa OOO till Jul 10th, 2012-06-29 14:10:39]

Hi Nico,

It may be a simpler change if you think of chrome as the only client of rlz_lib,
but that is not the case.  I would not want each app that uses rlz_lib to start
including specific #ifdefs and/or code for itself.

The fact that the PSO folks want different AP names is because they are
different access points.  So they should be represented that way in rlz_lib.

As for 29 line changes across 8 files, I don't think that is onerous.  In fact,
looking at these file, I wonder if we should go further and hide more of rlz_lib
from code outside of RLZTracker.  For example, RLZTracker::RecordProductEvent()
does need to accept rlz_lib::CHROME.  Also, it may be cleaner to define the
access points values in RLZTracker itself.  So calls like:

  RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_OMNIBOX, &rlz_string);

could become:

  RLZTracker::GetAccessPointRlz(RLZTracker::CHROME_OMNIBOX, &rlz_string);

where RLZTracker::CHROME_OMNIBOX is declared appropriately for each platform.

[Nico, 2012-06-30 00:57:05]

> It may be a simpler change if you think of chrome as the only client of
rlz_lib,
> but that is not the case.  I would not want each app that uses rlz_lib to
start
> including specific #ifdefs and/or code for itself.

As far as I understand, chrome is the only client of CHROME_OMNIBOX /
CHROME_HOME_PAGE -- and even if it weren't, other clients would want to use C5 /
C6 for these platforms on mac as well from what I hear, so every client would
have to duplicate these ifdefs.

So I feel there are arguments either way, and a 20x increase in number of lines
in the patch suggest "not worth it" to me.

But done.

[Roger Tawa OOO till Jul 10th, 2012-07-03 18:11:09]

lgtm

Thanks Nico.  More lines changed, but better overall.

http://codereview.chromium.org/10698024/diff/9001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/10698024/diff/9001/chrome/browser/rlz/rlz.cc#n...
chrome/browser/rlz/rlz.cc:289: GetAccessPointRlz(RLZTracker::CHROME_HOME_PAGE,
NULL);
I don't think you need the namespace here and below in this file.

http://codereview.chromium.org/10698024/diff/9001/rlz/lib/lib_values.cc
File rlz/lib/lib_values.cc (right):

http://codereview.chromium.org/10698024/diff/9001/rlz/lib/lib_values.cc#newco...
rlz/lib/lib_values.cc:73: case CHROME_MAC_HOME_PAGE:          return "C6";
Should keep these at lines 96 and 97, same order as enum.

[Nico, 2012-07-03 18:18:08]

Thanks!

http://codereview.chromium.org/10698024/diff/9001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/10698024/diff/9001/chrome/browser/rlz/rlz.cc#n...
chrome/browser/rlz/rlz.cc:289: GetAccessPointRlz(RLZTracker::CHROME_HOME_PAGE,
NULL);
On 2012/07/03 18:11:09, Roger Tawa wrote:
> I don't think you need the namespace here and below in this file.

I think it's a bit clearer though, since the same symbol is in two different
contexts here (rlz_lib:: and RLZTracker::). Do you mind if I keep it?

http://codereview.chromium.org/10698024/diff/9001/rlz/lib/lib_values.cc
File rlz/lib/lib_values.cc (right):

http://codereview.chromium.org/10698024/diff/9001/rlz/lib/lib_values.cc#newco...
rlz/lib/lib_values.cc:73: case CHROME_MAC_HOME_PAGE:          return "C6";
On 2012/07/03 18:11:09, Roger Tawa wrote:
> Should keep these at lines 96 and 97, same order as enum.

Done.

[(NOT FOR CODE REVIEWS), 2012-07-03 18:31:54]

OK Nico for the order.
Thanks,
Roger

-


On Tue, Jul 3, 2012 at 2:18 PM, <thakis@chromium.org> wrote:

> Thanks!
>
>
>
> http://codereview.chromium.**org/10698024/diff/9001/chrome/**
>
browser/rlz/rlz.cc<http://codereview.chromium.org/10698024/diff/9001/chrome/browser/rlz/rlz.cc>
> File chrome/browser/rlz/rlz.cc (right):
>
> http://codereview.chromium.**org/10698024/diff/9001/chrome/**
>
browser/rlz/rlz.cc#newcode289<http://codereview.chromium.org/10698024/diff/9001/chrome/browser/rlz/rlz.cc#newcode289>
> chrome/browser/rlz/rlz.cc:289:
> GetAccessPointRlz(RLZTracker::**CHROME_HOME_PAGE, NULL);
> On 2012/07/03 18:11:09, Roger Tawa wrote:
>
>> I don't think you need the namespace here and below in this file.
>>
>
> I think it's a bit clearer though, since the same symbol is in two
> different contexts here (rlz_lib:: and RLZTracker::). Do you mind if I
> keep it?
>
>
> http://codereview.chromium.**org/10698024/diff/9001/rlz/**
>
lib/lib_values.cc<http://codereview.chromium.org/10698024/diff/9001/rlz/lib/lib_values.cc>
> File rlz/lib/lib_values.cc (right):
>
> http://codereview.chromium.**org/10698024/diff/9001/rlz/**
>
lib/lib_values.cc#newcode73<http://codereview.chromium.org/10698024/diff/9001/rlz/lib/lib_values.cc#newcode73>
> rlz/lib/lib_values.cc:73: case CHROME_MAC_HOME_PAGE:          return
> "C6";
> On 2012/07/03 18:11:09, Roger Tawa wrote:
>
>> Should keep these at lines 96 and 97, same order as enum.
>>
>
> Done.
>
>
http://codereview.chromium.**org/10698024/<http://codereview.chromium.org/106...
>