Adds the 6-month flag functionality back into the criteria checker,...

Adds the 6-month flag functionality back into the criteria checker,
along with adding support for a boolean parameter that indicates whether
the flag should be set or not.

BUG=6802
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9278

[kuchhal, 2009-01-27 23:41:47]

lgtm.

Some nits below.

http://codereview.chromium.org/18490/diff/1/3
File chrome/installer/gcapi/gcapi.cc (right):

http://codereview.chromium.org/18490/diff/1/3#newcode31
Line 31: 0, KEY_ALL_ACCESS, &key) != ERROR_SUCCESS)
align with (HKEY.. in the line above.

http://codereview.chromium.org/18490/diff/1/3#newcode43
Line 43: value_len = 0;
I know this is just cut paste from original code but lines 42 and 43 should be
deleted.

http://codereview.chromium.org/18490/diff/1/3#newcode148
Line 148: // Cannot re-offer, if the timer already exists and is not expired yet
. at the end of comment.

http://codereview.chromium.org/18490/diff/1/3#newcode155
Line 155: // represented as a YYYYMMDD numeric value
. at the end of comment.

http://codereview.chromium.org/18490/diff/1/3#newcode164
Line 164: ::RegSetValueEx(key, company, 0, REG_DWORD, (LPBYTE)&value,
sizeof(DWORD));
more than 80 chars?

http://codereview.chromium.org/18490/diff/1/3#newcode343
Line 343: // Then only check whether we can re-offer, if everything else is OK.
Are there whitespaces at the end of this line? This shows up strange in
rietveld.

http://codereview.chromium.org/18490/diff/1/4
File chrome/installer/gcapi/gcapi.h (right):

http://codereview.chromium.org/18490/diff/1/4#newcode24
Line 24: // This function will only set the time lapse flag if set_flag is
passed TRUE
May be we should explain what is time lapse flag in more detail since this
header file goes to our partners. We can describe that we put a six months limit
between offers and this flag sets the flag for that.

http://codereview.chromium.org/18490/diff/1/4#newcode25
Line 25: DLLEXPORT BOOL __stdcall GoogleChromeCompatibilityCheck(BOOL set_flag,
DWORD *reasons);
is this line more than 80 chars?

[Glenn Wilson, 2009-01-30 23:48:54]

On 2009/01/27 23:41:47, kuchhal wrote:
> lgtm.
> 
> Some nits below.
> 
> http://codereview.chromium.org/18490/diff/1/3
> File chrome/installer/gcapi/gcapi.cc (right):
> 
> http://codereview.chromium.org/18490/diff/1/3#newcode31
> Line 31: 0, KEY_ALL_ACCESS, &key) != ERROR_SUCCESS)
> align with (HKEY.. in the line above.
> 

Done.  I also fixed a couple of the while() statements below this one to align
correctly.


> http://codereview.chromium.org/18490/diff/1/3#newcode43
> Line 43: value_len = 0;
> I know this is just cut paste from original code but lines 42 and 43 should be
> deleted.
> 

Done.

> http://codereview.chromium.org/18490/diff/1/3#newcode148
> Line 148: // Cannot re-offer, if the timer already exists and is not expired
yet
> . at the end of comment.
> 
> http://codereview.chromium.org/18490/diff/1/3#newcode155
> Line 155: // represented as a YYYYMMDD numeric value
> . at the end of comment.
>

Done and done.
 
> http://codereview.chromium.org/18490/diff/1/3#newcode164
> Line 164: ::RegSetValueEx(key, company, 0, REG_DWORD, (LPBYTE)&value,
> sizeof(DWORD));
> more than 80 chars?
> 

Yep, 82 chars.  Fixed.

> http://codereview.chromium.org/18490/diff/1/3#newcode343
> Line 343: // Then only check whether we can re-offer, if everything else is
OK.
> Are there whitespaces at the end of this line? This shows up strange in
> rietveld.

Fixed.  There were some tabs at the end of these lines.

> 
> http://codereview.chromium.org/18490/diff/1/4
> File chrome/installer/gcapi/gcapi.h (right):
> 
> http://codereview.chromium.org/18490/diff/1/4#newcode24
> Line 24: // This function will only set the time lapse flag if set_flag is
> passed TRUE
> May be we should explain what is time lapse flag in more detail since this
> header file goes to our partners. We can describe that we put a six months
limit
> between offers and this flag sets the flag for that.
> 

I tried writing a more descriptive comment for this.  Let me know if it needs
improvement.

> http://codereview.chromium.org/18490/diff/1/4#newcode25
> Line 25: DLLEXPORT BOOL __stdcall GoogleChromeCompatibilityCheck(BOOL
set_flag,
> DWORD *reasons);
> is this line more than 80 chars?

Yep, fixed.

[kuchhal, 2009-01-31 00:43:32]

lgtm.

One nit below and there are some comments that do not end with period. You can
fix those before submit, if you want.

http://codereview.chromium.org/18490/diff/606/413
File chrome/installer/gcapi/gcapi.h (right):

http://codereview.chromium.org/18490/diff/606/413#newcode26
Line 26: // set the flag even if Chrome is offered.  If passed TRUE, this method
will
"Chrome can be offered" might be better since the method is not actually
offering Chrome.

[Glenn Wilson, 2009-01-31 01:27:59]

On 2009/01/31 00:43:32, kuchhal wrote:
> lgtm.
> 
> One nit below and there are some comments that do not end with period. You can
> fix those before submit, if you want.
> 

I added periods to the rest of the comments in the file that did not have them,
just for consistency.

> http://codereview.chromium.org/18490/diff/606/413
> File chrome/installer/gcapi/gcapi.h (right):
> 
> http://codereview.chromium.org/18490/diff/606/413#newcode26
> Line 26: // set the flag even if Chrome is offered.  If passed TRUE, this
method
> will
> "Chrome can be offered" might be better since the method is not actually
> offering Chrome.

Fixed, now says "can be offered" instead.

Thanks for the review!