Implement a ReadyPromptContent that displays a prompt to accept, temporarily ...

Implement a ReadyPromptContent that displays a prompt to accept, temporarily decline, or permanently decline ready mode. Implement a RegistryReadyModeState that manages the state (based on user's decision) in the registry. Interfaces to integrate those with the installer.

BUG=None
TEST=chrome_frame_unittests --gtest_filter='Ready*'

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

[amit, 2010-12-09 22:41:33]

Pretty good and nice tests...

http://codereview.chromium.org/5747002/diff/1/chrome/app/cf_resources.rc
File chrome/app/cf_resources.rc (right):

http://codereview.chromium.org/5747002/diff/1/chrome/app/cf_resources.rc#newc...
chrome/app/cf_resources.rc:43: LTEXT           "This site recommends the Google
Chrome Frame add-on for Internet Explorer. Would you like to enable it now? No
download or browser restart is required.",IDC_PROMPT_MESSAGE,34,3,254,16
Do we have to move this messages and the button captions in the grd file so that
they will get translated? or will they create a copy for each language based on
this dialog template?

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_content.cc (right):

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:18:
window_->DestroyWindow();
nit: window_ == NULL?

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:38: dimensions.left ==
dimensions.right ?
IsRectEmpty(&dimentions) ?

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:43: size_t
ReadyPromptContent::GetDesiredSize(size_t width, size_t height) {
nit: GetDesiredHeight?

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:47: return 39;  //
TODO(erikwright): Get the dialog template's natural height
will this be addressed before check in?

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:59: bool expired =
_abs64(seconds_since_declined) >
nit: bool expired = (GetNow() - when_declined) >
TimeDelta::FromSeconds(temporary_decline_length_seconds_)?

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:75: if
(!config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(), KEY_ALL_ACCESS)) {
nit: KEY_QUERY_VALUE

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:105: if
(config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(), KEY_ALL_ACCESS) &&
nit: just KEY_SET_VALUE is needed so either use just that or  KEY_WRITE if you
want to go for generic permissions?

[erikwright (departed), 2010-12-10 02:41:33]

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_content.cc (right):

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:47: return 39;  //
TODO(erikwright): Get the dialog template's natural height
On 2010/12/09 22:41:33, amit wrote:
> will this be addressed before check in?

Ideally. Do you know how, off the top of your head? Otherwise I'll dig around..
didn't seem obvious.

[erikwright (departed), 2010-12-10 20:29:58]

Incorporates Amit's comments.

http://codereview.chromium.org/5747002/diff/1/chrome/app/cf_resources.rc
File chrome/app/cf_resources.rc (right):

http://codereview.chromium.org/5747002/diff/1/chrome/app/cf_resources.rc#newc...
chrome/app/cf_resources.rc:43: LTEXT           "This site recommends the Google
Chrome Frame add-on for Internet Explorer. Would you like to enable it now? No
download or browser restart is required.",IDC_PROMPT_MESSAGE,34,3,254,16
On 2010/12/09 22:41:33, amit wrote:
> Do we have to move this messages and the button captions in the grd file so
that
> they will get translated? or will they create a copy for each language based
on
> this dialog template?

A duplicate automatically gets copied into each locale. According to Robert
"they" will replace these with manually localized versions.

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_content.cc (right):

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:18:
window_->DestroyWindow();
On 2010/12/09 22:41:33, amit wrote:
> nit: window_ == NULL?

Done.

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:38: dimensions.left ==
dimensions.right ?
On 2010/12/09 22:41:33, amit wrote:
> IsRectEmpty(&dimentions) ?

Done.

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:43: size_t
ReadyPromptContent::GetDesiredSize(size_t width, size_t height) {
On 2010/12/09 22:41:33, amit wrote:
> nit: GetDesiredHeight?

It's part of the infobar API, which could be used for vertical or horizontal. I
could modify it to have two functions (GetDesiredHeight, GetDesiredWidth), only
one of which would be called. Whichever way you prefer..

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:47: return 39;  //
TODO(erikwright): Get the dialog template's natural height
On 2010/12/10 02:41:33, erikwright wrote:
> On 2010/12/09 22:41:33, amit wrote:
> > will this be addressed before check in?
> 
> Ideally. Do you know how, off the top of your head? Otherwise I'll dig
around..
> didn't seem obvious.

Done.

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:59: bool expired =
_abs64(seconds_since_declined) >
On 2010/12/09 22:41:33, amit wrote:
> nit: bool expired = (GetNow() - when_declined) >
> TimeDelta::FromSeconds(temporary_decline_length_seconds_)?

Done.

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:75: if
(!config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(), KEY_ALL_ACCESS)) {
On 2010/12/09 22:41:33, amit wrote:
> nit: KEY_QUERY_VALUE

Done.

http://codereview.chromium.org/5747002/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:105: if
(config_key.Open(HKEY_CURRENT_USER, key_name_.c_str(), KEY_ALL_ACCESS) &&
On 2010/12/09 22:41:33, amit wrote:
> nit: just KEY_SET_VALUE is needed so either use just that or  KEY_WRITE if you
> want to go for generic permissions?

Done.

[erikwright (departed), 2010-12-10 20:43:01]

Realized that the ready_mode_manager.h file, which I had planned to commit
separately, was included from this CL.

I'm submitting a subset of the file's contents (public interface defined here
will be added separately).

[grt (UTC plus 2), 2010-12-11 03:12:06]

LGTM with nits below.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/installation_state.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/installation_state.h:6: #define
CHROME_FRAME_READY_MODE_INTERNAL_INSTALLATION_STATE_H_
#pragma once

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/installation_state.h:28: };  //
InstallationState
};  // class InstallationState

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_mode_state.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_mode_state.h:7: 
#pragma once

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_prompt_content.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_content.h:7: 
#pragma once

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_content.h:34: };  //
ReadyPromptContent
};  // class ReadyPromptContent

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_prompt_window.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.h:7: 
#pragma once

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:76: DPLOG(ERROR)
<< "Failed to open registry key " << key_name_;
RegKey::Open doesn't set the last-error code, so just use DLOG here (or change
Open so that it sets the LEC on error).

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:87: DPLOG(ERROR)
<< "Failed to open registry key " << key_name_;
DLOG

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:92: DPLOG(ERROR)
<< "Unexpected state found under registry key " << key_name_
DLOG always here, as LEC won't be set in any case.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:111: DPLOG(ERROR)
<< "Failed to open or write to registry key " << key_name_
DLOG

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.h:7: 
#pragma once

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.h:21: // instance of
InstallationState to interact with the isntaller. Notifies a
isntaller -> installer

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.h:70: };
};  // class RegistryReadyModeState

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/rea...
File chrome_frame/ready_mode/ready_mode_manager.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/rea...
chrome_frame/ready_mode/ready_mode_manager.h:7: 
#pragma once

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/test/ready_mod...
File chrome_frame/test/ready_mode_unittest.cc (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/test/ready_mod...
chrome_frame/test/ready_mode_unittest.cc:38: DCHECK_EQ(res_dll_,
ATL::_AtlBaseModule.SetResourceInstance(old_res_dll_));
Should EXPECT_EQ be used here (and above) so that the test doesn't crash if the
world is out of whack?

[erikwright (departed), 2010-12-14 16:15:17]

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/installation_state.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/installation_state.h:6: #define
CHROME_FRAME_READY_MODE_INTERNAL_INSTALLATION_STATE_H_
On 2010/12/11 03:12:06, grt wrote:
> #pragma once

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/installation_state.h:28: };  //
InstallationState
On 2010/12/11 03:12:06, grt wrote:
> };  // class InstallationState

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_mode_state.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_mode_state.h:7: 
On 2010/12/11 03:12:06, grt wrote:
> #pragma once

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_prompt_content.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_content.h:7: 
On 2010/12/11 03:12:06, grt wrote:
> #pragma once

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_content.h:34: };  //
ReadyPromptContent
On 2010/12/11 03:12:06, grt wrote:
> };  // class ReadyPromptContent

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_prompt_window.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.h:7: 
On 2010/12/11 03:12:06, grt wrote:
> #pragma once

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:76: DPLOG(ERROR)
<< "Failed to open registry key " << key_name_;
On 2010/12/11 03:12:06, grt wrote:
> RegKey::Open doesn't set the last-error code, so just use DLOG here (or change
> Open so that it sets the LEC on error).

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:87: DPLOG(ERROR)
<< "Failed to open registry key " << key_name_;
On 2010/12/11 03:12:06, grt wrote:
> DLOG

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:92: DPLOG(ERROR)
<< "Unexpected state found under registry key " << key_name_
On 2010/12/11 03:12:06, grt wrote:
> DLOG always here, as LEC won't be set in any case.

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:111: DPLOG(ERROR)
<< "Failed to open or write to registry key " << key_name_
On 2010/12/11 03:12:06, grt wrote:
> DLOG

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.h:7: 
On 2010/12/11 03:12:06, grt wrote:
> #pragma once

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.h:21: // instance of
InstallationState to interact with the isntaller. Notifies a
On 2010/12/11 03:12:06, grt wrote:
> isntaller -> installer

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/registry_ready_mode_state.h:70: };
On 2010/12/11 03:12:06, grt wrote:
> };  // class RegistryReadyModeState

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/rea...
File chrome_frame/ready_mode/ready_mode_manager.h (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/ready_mode/rea...
chrome_frame/ready_mode/ready_mode_manager.h:7: 
On 2010/12/11 03:12:06, grt wrote:
> #pragma once

Done.

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/test/ready_mod...
File chrome_frame/test/ready_mode_unittest.cc (right):

http://codereview.chromium.org/5747002/diff/29001/chrome_frame/test/ready_mod...
chrome_frame/test/ready_mode_unittest.cc:38: DCHECK_EQ(res_dll_,
ATL::_AtlBaseModule.SetResourceInstance(old_res_dll_));
On 2010/12/11 03:12:06, grt wrote:
> Should EXPECT_EQ be used here (and above) so that the test doesn't crash if
the
> world is out of whack?

If this fails, all later tests would potentially be out of whack: better to fail
hard. But the one above could fail without affecting other tests. Also, DCHECK
was a poor choice since it would've been a no-op in release.