First cut of privacy work for GCF. Implements IDeleteBrowsing history and mov...

Implements IDeleteBrowsing history and moves the GCF profile into the IE TIF directory for non-priv mode users on IE < 8.

Implementation notes:

Earlier work enabled InPrivate browsing detection and mapped it to creation of an incognito profile instance.Privacy features and how they operate with this change:

"Delete Browsing History":

	IE 6 & 7: all history (including databases) is deleted if cache is cleared *WITHOUT* an active Chrome process holding references to the profile resources. If GCF is rendering a page when the cache is cleared, history *WILL NOT* be deleted on the GCF side, however GCF will continue to operate and IE will remove all other history artifacts as usual.

	IE 8: GCF cache is cleared in alignment with the options specified by the user. Clearing Temporary Internet Files may destroy the profile entirely, and so we need to consider not moving the GCF profile on IE 8.

"InPrivate Filtering":

	IE 8 (only): more testing required.

"InPrivate Browsing":

	IE 8 (only): pages rendered in GCF *after* entering InPrivate mode are not persisted to disk (use an incognito wrapper on the specified profile). Currently displayed pages are not effected by the switch, although refreshing them will invoke the new behavior. Generally speaking, BHO's are disabled by IE 8 while in InPrivate mode, so entering this state is wonky to begin with but we handle it as well as can be expected.

BUG=22846
TEST=On IE 8, clear the cache entirely, note GCF entries in DbgView (better tests coming)

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

[tommi (sloooow) - chröme, 2010-03-12 16:05:54]

http://codereview.chromium.org/858003/diff/34003/44005
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/858003/diff/34003/44005#newcode328
chrome/browser/automation/automation_provider.h:328: void RemoveBrowsingData(int
remove_mask);
can you add a comment pointing to the mask flags?

http://codereview.chromium.org/858003/diff/34003/44001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/34003/44001#newcode278
chrome_frame/chrome_frame_automation.cc:278: if (entry->profile_name ==
std::wstring(kIexploreProfileName) ||
instead of creating a new wstring, can't you just use the compare() method?

if (entry->profile_name.compare(kIexploreProfileName) == 0 ||

Also, if the code you're adding is Windows only, you need to do case insensitive
path comparisons.

http://codereview.chromium.org/858003/diff/34003/44001#newcode279
chrome_frame/chrome_frame_automation.cc:279: entry->profile_name ==
std::wstring(kRundllProfileName)) {
and here :)

http://codereview.chromium.org/858003/diff/34003/44001#newcode292
chrome_frame/chrome_frame_automation.cc:292: entry->profile_name =
std::wstring(kIexploreProfileName);
no need for the std::wstring()

http://codereview.chromium.org/858003/diff/34003/44001#newcode310
chrome_frame/chrome_frame_automation.cc:310: profile_path.value());
indent

http://codereview.chromium.org/858003/diff/34003/44001#newcode857
chrome_frame/chrome_frame_automation.cc:857: 
remove

http://codereview.chromium.org/858003/diff/34003/44001#newcode1173
chrome_frame/chrome_frame_automation.cc:1173: // DLOG(INFO) << __FUNCTION__;
remove?

http://codereview.chromium.org/858003/diff/34003/44012
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/34003/44012#newcode48
chrome_frame/delete_chrome_history.cc:48: if (GetHostProcessName(false) ==
std::wstring(kRundllProfileName)) {
use .compare()
did you perhaps mean to remove this block?

http://codereview.chromium.org/858003/diff/34003/44012#newcode73
chrome_frame/delete_chrome_history.cc:73: if (flags &
DELETE_BROWSING_HISTORY_PRESERVEFAVORITES) {
remove or todo?

http://codereview.chromium.org/858003/diff/34003/44010
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/34003/44010#newcode27
chrome_frame/delete_chrome_history.h:27: public CComCoClass<DeleteChromeHistory,
&CLSID_DeleteChromeHistory>,
does this need to be registered or could we create an instance directly (without
calling CoCreateInstance)?

http://codereview.chromium.org/858003/diff/34003/44010#newcode28
chrome_frame/delete_chrome_history.h:28: public
CWindowImpl<DeleteChromeHistory>,
it doesn't look like you're expecting any messages. do you need a window?

http://codereview.chromium.org/858003/diff/34003/44010#newcode48
chrome_frame/delete_chrome_history.h:48: STDMETHOD(DeleteBrowsingHistory)(DWORD
dwFlags);
dwFlags -> flags

http://codereview.chromium.org/858003/diff/34003/44010#newcode58
chrome_frame/delete_chrome_history.h:58: MessageLoopForUI loop_;
for components that run on IE's UI thread, we usually don't need a chrome
message loop.  is there a special case here?

http://codereview.chromium.org/858003/diff/34003/44010#newcode59
chrome_frame/delete_chrome_history.h:59: 
remove

[amit, 2010-03-12 17:07:17]

http://codereview.chromium.org/858003/diff/34003/44010
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/34003/44010#newcode28
chrome_frame/delete_chrome_history.h:28: public
CWindowImpl<DeleteChromeHistory>,
On 2010/03/12 16:05:55, tommi wrote:
> it doesn't look like you're expecting any messages. do you need a window?

The ChromeFramePlugin class expect the delegate to have a window.

http://codereview.chromium.org/858003/diff/34003/44010#newcode58
chrome_frame/delete_chrome_history.h:58: MessageLoopForUI loop_;
On 2010/03/12 16:05:55, tommi wrote:
> for components that run on IE's UI thread, we usually don't need a chrome
> message loop.  is there a special case here?
This component actually runs separate through the control panel applet vial
rundll32.exe. It never runs in IE!

[tommi (sloooow) - chröme, 2010-03-12 17:59:59]

thanks - I was a bit confused :)

On Fri, Mar 12, 2010 at 12:07 PM, <amit@chromium.org> wrote:

>
> http://codereview.chromium.org/858003/diff/34003/44010
> File chrome_frame/delete_chrome_history.h (right):
>
> http://codereview.chromium.org/858003/diff/34003/44010#newcode28
> chrome_frame/delete_chrome_history.h:28: public
> CWindowImpl<DeleteChromeHistory>,
> On 2010/03/12 16:05:55, tommi wrote:
>
>> it doesn't look like you're expecting any messages. do you need a
>>
> window?
>
> The ChromeFramePlugin class expect the delegate to have a window.
>
>
> http://codereview.chromium.org/858003/diff/34003/44010#newcode58
> chrome_frame/delete_chrome_history.h:58: MessageLoopForUI loop_;
> On 2010/03/12 16:05:55, tommi wrote:
>
>> for components that run on IE's UI thread, we usually don't need a
>>
> chrome
>
>> message loop.  is there a special case here?
>>
> This component actually runs separate through the control panel applet
> vial rundll32.exe. It never runs in IE!
>
>
> http://codereview.chromium.org/858003
>

[slightlyoff, 2010-03-22 22:28:17]

http://codereview.chromium.org/858003/diff/34003/44006
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/858003/diff/34003/44006#newcode2144
chrome/browser/automation/automation_provider.cc:2144:
static_cast<BrowsingDataRemover::TimePeriod>(4), // EVERYTHING
On 2010/03/12 13:25:50, amit wrote:
> nit: BrowsingDataRemover::EVERYTHING?

Done.

http://codereview.chromium.org/858003/diff/34003/44005
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/858003/diff/34003/44005#newcode328
chrome/browser/automation/automation_provider.h:328: void RemoveBrowsingData(int
remove_mask);
On 2010/03/12 16:05:55, tommi wrote:
> can you add a comment pointing to the mask flags?

Done.

http://codereview.chromium.org/858003/diff/34003/44003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/858003/diff/34003/44003#newcode44
chrome_frame/chrome_active_document.cc:44: static const wchar_t
kRundllProfileName[] = L"rundll32";
On 2010/03/12 13:25:50, amit wrote:
> nit: unused?

Done.

http://codereview.chromium.org/858003/diff/34003/44001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/34003/44001#newcode278
chrome_frame/chrome_frame_automation.cc:278: if (entry->profile_name ==
std::wstring(kIexploreProfileName) ||
On 2010/03/12 16:05:55, tommi wrote:
> instead of creating a new wstring, can't you just use the compare() method?
> 
> if (entry->profile_name.compare(kIexploreProfileName) == 0 ||
> 
> Also, if the code you're adding is Windows only, you need to do case
insensitive
> path comparisons.

Done.

http://codereview.chromium.org/858003/diff/34003/44001#newcode279
chrome_frame/chrome_frame_automation.cc:279: entry->profile_name ==
std::wstring(kRundllProfileName)) {
On 2010/03/12 16:05:55, tommi wrote:
> and here :)

Done.

http://codereview.chromium.org/858003/diff/34003/44001#newcode280
chrome_frame/chrome_frame_automation.cc:280: // FIXME(slightlyoff): avoid this
path if in privelidged mode
On 2010/03/12 13:25:50, amit wrote:
> Please fix this along with the above :)

Done.

http://codereview.chromium.org/858003/diff/34003/44001#newcode292
chrome_frame/chrome_frame_automation.cc:292: entry->profile_name =
std::wstring(kIexploreProfileName);
On 2010/03/12 16:05:55, tommi wrote:
> no need for the std::wstring()

Moved to regular string (not wstring), so the equivalent is handled here now but
still need the wstring ctor.

http://codereview.chromium.org/858003/diff/34003/44001#newcode310
chrome_frame/chrome_frame_automation.cc:310: profile_path.value());
On 2010/03/12 16:05:55, tommi wrote:
> indent

Done.

http://codereview.chromium.org/858003/diff/34003/44001#newcode857
chrome_frame/chrome_frame_automation.cc:857: 
On 2010/03/12 16:05:55, tommi wrote:
> remove

Done.

http://codereview.chromium.org/858003/diff/34003/44001#newcode1173
chrome_frame/chrome_frame_automation.cc:1173: // DLOG(INFO) << __FUNCTION__;
On 2010/03/12 16:05:55, tommi wrote:
> remove?

Done.

http://codereview.chromium.org/858003/diff/34003/44001#newcode1187
chrome_frame/chrome_frame_automation.cc:1187: DLOG(INFO) << __FUNCTION__ << " 
sending headers to Chrome: " << std::endl << std::endl << headers;
On 2010/03/12 13:25:50, amit wrote:
> nit: remove or reformat?

Done.

http://codereview.chromium.org/858003/diff/34003/44012
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/34003/44012#newcode48
chrome_frame/delete_chrome_history.cc:48: if (GetHostProcessName(false) ==
std::wstring(kRundllProfileName)) {
On 2010/03/12 16:05:55, tommi wrote:
> use .compare()
> did you perhaps mean to remove this block?

Done.

http://codereview.chromium.org/858003/diff/34003/44012#newcode73
chrome_frame/delete_chrome_history.cc:73: if (flags &
DELETE_BROWSING_HISTORY_PRESERVEFAVORITES) {
On 2010/03/12 16:05:55, tommi wrote:
> remove or todo?

Done.

http://codereview.chromium.org/858003/diff/34003/44010
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/34003/44010#newcode48
chrome_frame/delete_chrome_history.h:48: STDMETHOD(DeleteBrowsingHistory)(DWORD
dwFlags);
On 2010/03/12 16:05:55, tommi wrote:
> dwFlags -> flags

Done.

http://codereview.chromium.org/858003/diff/34003/44010#newcode59
chrome_frame/delete_chrome_history.h:59: 
On 2010/03/12 16:05:55, tommi wrote:
> remove

Done.

http://codereview.chromium.org/858003/diff/34003/44011
File chrome_frame/delete_chrome_history.rgs (right):

http://codereview.chromium.org/858003/diff/34003/44011#newcode1
chrome_frame/delete_chrome_history.rgs:1: HKLM {
On 2010/03/12 13:25:50, amit wrote:
> Better to implement this as a tear-off from active document and in that case
> please move this to the active document rgs.

Done.

[tommi (sloooow) - chröme, 2010-03-22 23:37:36]

http://codereview.chromium.org/858003/diff/111001/112003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/858003/diff/111001/112003#newcode209
chrome_frame/chrome_active_document.cc:209: return E_UNEXPECTED;
add a NOTREACHED() here as well?

http://codereview.chromium.org/858003/diff/111001/112003#newcode238
chrome_frame/chrome_active_document.cc:238: //  DCHECK(mgr);
remove?

http://codereview.chromium.org/858003/diff/111001/112003#newcode453
chrome_frame/chrome_active_document.cc:453: return S_OK;
do we get here under normal circumstances?  If not, maybe we should do a
NOTREACHED here as well.

http://codereview.chromium.org/858003/diff/111001/112004
File chrome_frame/chrome_active_document.h (right):

http://codereview.chromium.org/858003/diff/111001/112004#newcode372
chrome_frame/chrome_active_document.h:372: bool cache_clearing_host_;
add documentation for this since it's a pretty special case.

http://codereview.chromium.org/858003/diff/111001/112017
File chrome_frame/chrome_active_document.rgs (right):

http://codereview.chromium.org/858003/diff/111001/112017#newcode31
chrome_frame/chrome_active_document.rgs:31: 'Implemented Categories' {
should be with bho?

http://codereview.chromium.org/858003/diff/111001/112017#newcode54
chrome_frame/chrome_active_document.rgs:54: HKLM {
remove

http://codereview.chromium.org/858003/diff/111001/112001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/111001/112001#newcode293
chrome_frame/chrome_frame_automation.cc:293: entry->profile_name =
std::wstring(explorer.begin(), explorer.end());
instead of this two step way to create a wide string, you can use
ASCIIToWide(kIexploreProfileName)

http://codereview.chromium.org/858003/diff/111001/112001#newcode1262
chrome_frame/chrome_frame_automation.cc:1262: 
remove

[amit, 2010-03-23 00:04:30]

http://codereview.chromium.org/858003/diff/104002/53005
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/858003/diff/104002/53005#newcode65
chrome_frame/chrome_active_document.cc:65: if (cache_clearing_host_) {
We can get rid of this hack by:
 - allowing the creation of automation client to happen here.
 - using COM_INTERFACE_ENTRY_FUNC to create a tear off and initializing it with
automation_client_.

http://codereview.chromium.org/858003/diff/104002/53006
File chrome_frame/chrome_active_document.h (right):

http://codereview.chromium.org/858003/diff/104002/53006#newcode160
chrome_frame/chrome_active_document.h:160: DeleteChromeHistory,
nit: indent 2 places right

http://codereview.chromium.org/858003/diff/104002/53006#newcode384
chrome_frame/chrome_active_document.h:384: CComPtr<IUnknown>
delete_chrome_frame_;
delete_chrome_history_?

http://codereview.chromium.org/858003/diff/104002/53019
File chrome_frame/chrome_active_document.rgs (right):

http://codereview.chromium.org/858003/diff/104002/53019#newcode54
chrome_frame/chrome_active_document.rgs:54: HKLM {
no longer needed?

http://codereview.chromium.org/858003/diff/104002/53003
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/104002/53003#newcode43
chrome_frame/chrome_frame_automation.cc:43: static const char
kRundllProfileName[] = "rundll32";
hopefully not needed anymore

http://codereview.chromium.org/858003/diff/104002/53003#newcode280
chrome_frame/chrome_frame_automation.cc:280: 
As per our discussion: InitiaizeAutomation takes profile path instead of profile
name and please move this to where profile name is being specified.

http://codereview.chromium.org/858003/diff/104002/53003#newcode498
chrome_frame/chrome_frame_automation.cc:498: bool incognito, bool is_privileged)
{
hopefully won't be needed anymore

http://codereview.chromium.org/858003/diff/104002/53014
File chrome_frame/chrome_tab.idl (right):

http://codereview.chromium.org/858003/diff/104002/53014#newcode187
chrome_frame/chrome_tab.idl:187: [uuid(DA8B911E-6137-4292-BB56-D130620BC5EB)]
is not needed anymore since this is just an additional interface implemented by
BHO

[slightlyoff, 2010-03-24 18:18:13]

reworked the tearoff to be attached to the BHO and not ChromeActiveDocument

http://codereview.chromium.org/858003/diff/111001/112003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/858003/diff/111001/112003#newcode209
chrome_frame/chrome_active_document.cc:209: return E_UNEXPECTED;
On 2010/03/22 23:37:36, tommi wrote:
> add a NOTREACHED() here as well?

Moved.

http://codereview.chromium.org/858003/diff/111001/112003#newcode238
chrome_frame/chrome_active_document.cc:238: //  DCHECK(mgr);
On 2010/03/22 23:37:36, tommi wrote:
> remove?

Re-instated

http://codereview.chromium.org/858003/diff/111001/112003#newcode453
chrome_frame/chrome_active_document.cc:453: return S_OK;
On 2010/03/22 23:37:36, tommi wrote:
> do we get here under normal circumstances?  If not, maybe we should do a
> NOTREACHED here as well.

Removed

http://codereview.chromium.org/858003/diff/111001/112017
File chrome_frame/chrome_active_document.rgs (right):

http://codereview.chromium.org/858003/diff/111001/112017#newcode31
chrome_frame/chrome_active_document.rgs:31: 'Implemented Categories' {
On 2010/03/22 23:37:36, tommi wrote:
> should be with bho?

Done.

http://codereview.chromium.org/858003/diff/111001/112017#newcode54
chrome_frame/chrome_active_document.rgs:54: HKLM {
On 2010/03/22 23:37:36, tommi wrote:
> remove

Done.

[amit, 2010-03-24 18:50:32]

Looking pretty good now. Mostly nits.

http://codereview.chromium.org/858003/diff/122001/123016
File chrome_frame/bho.h (right):

http://codereview.chromium.org/858003/diff/122001/123016#newcode110
chrome_frame/bho.h:110: CComPtr<IUnknown> delete_chrome_frame_;
nit: delete_chrome_history?

http://codereview.chromium.org/858003/diff/122001/123003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/858003/diff/122001/123003#newcode226
chrome_frame/chrome_active_document.cc:226: //  DCHECK(mgr);
nit: remove?

http://codereview.chromium.org/858003/diff/122001/123003#newcode440
chrome_frame/chrome_active_document.cc:440: 
nit: remove empty line?

http://codereview.chromium.org/858003/diff/122001/123013
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/122001/123013#newcode356
chrome_frame/chrome_frame_activex_base.h:356: *profile_path =
GetIETemporaryFilesFolder().Append(L"Google Chrome Frame");
nit: 80, also better to do in 2 steps.

http://codereview.chromium.org/858003/diff/122001/123001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/122001/123001#newcode1211
chrome_frame/chrome_frame_automation.cc:1211: 
nit: remove empty line

http://codereview.chromium.org/858003/diff/122001/123002
File chrome_frame/chrome_frame_automation.h (right):

http://codereview.chromium.org/858003/diff/122001/123002#newcode20
chrome_frame/chrome_frame_automation.h:20: #include "base/string_util.h"
nit: needed now?

http://codereview.chromium.org/858003/diff/122001/123012
File chrome_frame/chrome_frame_plugin.h (right):

http://codereview.chromium.org/858003/diff/122001/123012#newcode67
chrome_frame/chrome_frame_plugin.h:67: extra_chrome_arguments,
nit: 'extra_chrome_arguments' should fit above as before?

http://codereview.chromium.org/858003/diff/122001/123012#newcode220
chrome_frame/chrome_frame_plugin.h:220: virtual void
GetProfilePath(std::wstring* profile_name,
why not a const& for the profile_name?

http://codereview.chromium.org/858003/diff/122001/123011
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/122001/123011#newcode42
chrome_frame/delete_chrome_history.cc:42: DLOG(INFO) << __FUNCTION__ << ":
????";
nit: ???? :)

[tommi (sloooow) - chröme, 2010-03-24 19:08:06]

mostly style nits

http://codereview.chromium.org/858003/diff/125002/124020
File chrome_frame/bho.h (right):

http://codereview.chromium.org/858003/diff/125002/124020#newcode17
chrome_frame/bho.h:17: #define INITGUID
INITGUID should only be in a cc file (and only a single cc file).  Otherwise
this can cause build problems.

http://codereview.chromium.org/858003/diff/125002/124020#newcode110
chrome_frame/bho.h:110: CComPtr<IUnknown> delete_chrome_history_;
ScopedComPtr?  if we can't use that, then maybe add a comment for why.

http://codereview.chromium.org/858003/diff/125002/124021
File chrome_frame/bho.rgs (right):

http://codereview.chromium.org/858003/diff/125002/124021#newcode19
chrome_frame/bho.rgs:19: 'Version' = s '1.0'
do we need the version?

http://codereview.chromium.org/858003/diff/125002/124007
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/858003/diff/125002/124007#newcode440
chrome_frame/chrome_active_document.cc:440: 
remove

http://codereview.chromium.org/858003/diff/125002/124007#newcode1010
chrome_frame/chrome_active_document.cc:1010: }
actually, maybe you could just revert this file since there's no code change?

http://codereview.chromium.org/858003/diff/125002/124017
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/125002/124017#newcode342
chrome_frame/chrome_frame_activex_base.h:342: FilePath* profile_path) {
DCHECK(profile_name);

http://codereview.chromium.org/858003/diff/125002/124017#newcode348
chrome_frame/chrome_frame_activex_base.h:348:
this->ChromeFramePlugin::GetProfilePath(profile_name, profile_path);
remove "this->"?

http://codereview.chromium.org/858003/diff/125002/124017#newcode356
chrome_frame/chrome_frame_activex_base.h:356: *profile_path =
GetIETemporaryFilesFolder().Append(L"Google Chrome Frame");
80

http://codereview.chromium.org/858003/diff/125002/124017#newcode358
chrome_frame/chrome_frame_activex_base.h:358:
this->ChromeFramePlugin::GetProfilePath(profile_name, profile_path);
remove this->

http://codereview.chromium.org/858003/diff/125002/124017#newcode359
chrome_frame/chrome_frame_activex_base.h:359: return;
no need for this return

http://codereview.chromium.org/858003/diff/125002/124005
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/125002/124005#newcode274
chrome_frame/chrome_frame_automation.cc:274: params.profile_path.value());
indent

http://codereview.chromium.org/858003/diff/125002/124005#newcode1211
chrome_frame/chrome_frame_automation.cc:1211: 
remove

http://codereview.chromium.org/858003/diff/125002/124016
File chrome_frame/chrome_frame_plugin.h (right):

http://codereview.chromium.org/858003/diff/125002/124016#newcode56
chrome_frame/chrome_frame_plugin.h:56: bool InitializeAutomation(std::wstring&
profile_name,
did you mean to remove the constness of profile_name?
if so, then change it to std::wstring*

http://codereview.chromium.org/858003/diff/125002/124015
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/125002/124015#newcode41
chrome_frame/delete_chrome_history.cc:41: FilePath* profile_path) {
fix indent

http://codereview.chromium.org/858003/diff/125002/124015#newcode42
chrome_frame/delete_chrome_history.cc:42: DLOG(INFO) << __FUNCTION__ << ":
????";
any reason for the ???'s?:)

http://codereview.chromium.org/858003/diff/125002/124014
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/125002/124014#newcode12
chrome_frame/delete_chrome_history.h:12: #define INITGUID
same INITGUID problem

http://codereview.chromium.org/858003/diff/125002/124019
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/858003/diff/125002/124019#newcode425
chrome_frame/utils.cc:425: if (GetBrowserType() != BROWSER_IE) {
since you don't return for non IE browsers, you should just move this into a
DCHECK as to avoid including this code in release.

DCHECK_EQ(BROWSER_IE, GetBrowserType());

[slightlyoff, 2010-03-24 21:48:00]

http://codereview.chromium.org/858003/diff/122001/123016
File chrome_frame/bho.h (right):

http://codereview.chromium.org/858003/diff/122001/123016#newcode110
chrome_frame/bho.h:110: CComPtr<IUnknown> delete_chrome_frame_;
On 2010/03/24 18:50:33, amit wrote:
> nit: delete_chrome_history?

Done.

http://codereview.chromium.org/858003/diff/122001/123003
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/858003/diff/122001/123003#newcode226
chrome_frame/chrome_active_document.cc:226: //  DCHECK(mgr);
On 2010/03/24 18:50:33, amit wrote:
> nit: remove? 

Done.

http://codereview.chromium.org/858003/diff/122001/123003#newcode440
chrome_frame/chrome_active_document.cc:440: 
On 2010/03/24 18:50:33, amit wrote:
> nit: remove empty line?

Done.

http://codereview.chromium.org/858003/diff/122001/123013
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/122001/123013#newcode356
chrome_frame/chrome_frame_activex_base.h:356: *profile_path =
GetIETemporaryFilesFolder().Append(L"Google Chrome Frame");
On 2010/03/24 18:50:33, amit wrote:
> nit: 80, also better to do in 2 steps.

Done.

http://codereview.chromium.org/858003/diff/122001/123001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/122001/123001#newcode1211
chrome_frame/chrome_frame_automation.cc:1211: 
On 2010/03/24 18:50:33, amit wrote:
> nit: remove empty line

Done.

http://codereview.chromium.org/858003/diff/122001/123002
File chrome_frame/chrome_frame_automation.h (right):

http://codereview.chromium.org/858003/diff/122001/123002#newcode20
chrome_frame/chrome_frame_automation.h:20: #include "base/string_util.h"
On 2010/03/24 18:50:33, amit wrote:
> nit: needed now?

Done.

http://codereview.chromium.org/858003/diff/122001/123012
File chrome_frame/chrome_frame_plugin.h (right):

http://codereview.chromium.org/858003/diff/122001/123012#newcode67
chrome_frame/chrome_frame_plugin.h:67: extra_chrome_arguments,
On 2010/03/24 18:50:33, amit wrote:
> nit: 'extra_chrome_arguments' should fit above as before?

Done.

http://codereview.chromium.org/858003/diff/122001/123012#newcode220
chrome_frame/chrome_frame_plugin.h:220: virtual void
GetProfilePath(std::wstring* profile_name,
On 2010/03/24 18:50:33, amit wrote:
> why not a const& for the profile_name?

Because some variants assign to it.

http://codereview.chromium.org/858003/diff/122001/123011
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/122001/123011#newcode42
chrome_frame/delete_chrome_history.cc:42: DLOG(INFO) << __FUNCTION__ << ":
????";
On 2010/03/24 18:50:33, amit wrote:
> nit: ???? :)

removed

[slightlyoff, 2010-03-24 22:09:29]

nits addressed

http://codereview.chromium.org/858003/diff/125002/124020
File chrome_frame/bho.h (right):

http://codereview.chromium.org/858003/diff/125002/124020#newcode17
chrome_frame/bho.h:17: #define INITGUID
On 2010/03/24 19:08:06, tommi wrote:
> INITGUID should only be in a cc file (and only a single cc file).  Otherwise
> this can cause build problems.

Done.

http://codereview.chromium.org/858003/diff/125002/124020#newcode110
chrome_frame/bho.h:110: CComPtr<IUnknown> delete_chrome_history_;
On 2010/03/24 19:08:06, tommi wrote:
> ScopedComPtr?  if we can't use that, then maybe add a comment for why.

Done.

http://codereview.chromium.org/858003/diff/125002/124021
File chrome_frame/bho.rgs (right):

http://codereview.chromium.org/858003/diff/125002/124021#newcode19
chrome_frame/bho.rgs:19: 'Version' = s '1.0'
On 2010/03/24 19:08:06, tommi wrote:
> do we need the version?

Probalby not. Removed.

http://codereview.chromium.org/858003/diff/125002/124007
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/858003/diff/125002/124007#newcode1010
chrome_frame/chrome_active_document.cc:1010: }
On 2010/03/24 19:08:06, tommi wrote:
> actually, maybe you could just revert this file since there's no code change?

Done.

http://codereview.chromium.org/858003/diff/125002/124017
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/125002/124017#newcode342
chrome_frame/chrome_frame_activex_base.h:342: FilePath* profile_path) {
On 2010/03/24 19:08:06, tommi wrote:
> DCHECK(profile_name);

Done.

http://codereview.chromium.org/858003/diff/125002/124017#newcode348
chrome_frame/chrome_frame_activex_base.h:348:
this->ChromeFramePlugin::GetProfilePath(profile_name, profile_path);
On 2010/03/24 19:08:06, tommi wrote:
> remove "this->"?

Done.

http://codereview.chromium.org/858003/diff/125002/124017#newcode356
chrome_frame/chrome_frame_activex_base.h:356: *profile_path =
GetIETemporaryFilesFolder().Append(L"Google Chrome Frame");
On 2010/03/24 19:08:06, tommi wrote:
> 80

Done.

http://codereview.chromium.org/858003/diff/125002/124017#newcode358
chrome_frame/chrome_frame_activex_base.h:358:
this->ChromeFramePlugin::GetProfilePath(profile_name, profile_path);
On 2010/03/24 19:08:06, tommi wrote:
> remove this->

Done.

http://codereview.chromium.org/858003/diff/125002/124017#newcode359
chrome_frame/chrome_frame_activex_base.h:359: return;
On 2010/03/24 19:08:06, tommi wrote:
> no need for this return

Done.

http://codereview.chromium.org/858003/diff/125002/124005
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/125002/124005#newcode274
chrome_frame/chrome_frame_automation.cc:274: params.profile_path.value());
On 2010/03/24 19:08:06, tommi wrote:
> indent

Done.

http://codereview.chromium.org/858003/diff/125002/124005#newcode1211
chrome_frame/chrome_frame_automation.cc:1211: 
On 2010/03/24 19:08:06, tommi wrote:
> remove

Done.

http://codereview.chromium.org/858003/diff/125002/124016
File chrome_frame/chrome_frame_plugin.h (right):

http://codereview.chromium.org/858003/diff/125002/124016#newcode56
chrome_frame/chrome_frame_plugin.h:56: bool InitializeAutomation(std::wstring&
profile_name,
On 2010/03/24 19:08:06, tommi wrote:
> did you mean to remove the constness of profile_name?
> if so, then change it to std::wstring*

Yes. Done.

http://codereview.chromium.org/858003/diff/125002/124015
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/125002/124015#newcode41
chrome_frame/delete_chrome_history.cc:41: FilePath* profile_path) {
On 2010/03/24 19:08:06, tommi wrote:
> fix indent

Done.

http://codereview.chromium.org/858003/diff/125002/124015#newcode42
chrome_frame/delete_chrome_history.cc:42: DLOG(INFO) << __FUNCTION__ << ":
????";
On 2010/03/24 19:08:06, tommi wrote:
> any reason for the ???'s?:)

removed.

http://codereview.chromium.org/858003/diff/125002/124014
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/125002/124014#newcode12
chrome_frame/delete_chrome_history.h:12: #define INITGUID
On 2010/03/24 19:08:06, tommi wrote:
> same INITGUID problem

Done.

http://codereview.chromium.org/858003/diff/125002/124019
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/858003/diff/125002/124019#newcode425
chrome_frame/utils.cc:425: if (GetBrowserType() != BROWSER_IE) {
On 2010/03/24 19:08:06, tommi wrote:
> since you don't return for non IE browsers, you should just move this into a
> DCHECK as to avoid including this code in release.
> 
> DCHECK_EQ(BROWSER_IE, GetBrowserType());

Done.

[tommi (sloooow) - chröme, 2010-03-24 22:21:23]

final set of nits and one initguid concern

http://codereview.chromium.org/858003/diff/145002/170001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/145002/170001#newcode20
chrome_frame/chrome_frame_automation.cc:20: #include "base/sys_info.h"
nit: i think this should be where it was

http://codereview.chromium.org/858003/diff/145002/170009
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/145002/170009#newcode9
chrome_frame/delete_chrome_history.cc:9: #include <deletebrowsinghistory.h>
to be safe I would keep the headers affected by INITGUID below all other headers
just to avoid polluting other headers and potentially including symbols
unintentionally in the current compilation unit.

http://codereview.chromium.org/858003/diff/145002/170009#newcode46
chrome_frame/delete_chrome_history.cc:46:
this->ChromeFramePlugin::GetProfilePath(profile_name, profile_path);
nit: remove this->

http://codereview.chromium.org/858003/diff/145002/170008
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/145002/170008#newcode22
chrome_frame/delete_chrome_history.h:22: class Bho;
don't need this since you're already including bho.h

[slightlyoff, 2010-03-24 22:53:29]

http://codereview.chromium.org/858003/diff/145002/170001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/145002/170001#newcode20
chrome_frame/chrome_frame_automation.cc:20: #include "base/sys_info.h"
On 2010/03/24 22:21:23, tommi wrote:
> nit: i think this should be where it was

Done.

http://codereview.chromium.org/858003/diff/145002/170009
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/145002/170009#newcode9
chrome_frame/delete_chrome_history.cc:9: #include <deletebrowsinghistory.h>
On 2010/03/24 22:21:23, tommi wrote:
> to be safe I would keep the headers affected by INITGUID below all other
headers
> just to avoid polluting other headers and potentially including symbols
> unintentionally in the current compilation unit.

Done.

http://codereview.chromium.org/858003/diff/145002/170009#newcode46
chrome_frame/delete_chrome_history.cc:46:
this->ChromeFramePlugin::GetProfilePath(profile_name, profile_path);
On 2010/03/24 22:21:23, tommi wrote:
> nit: remove this->

Done.

[amit, 2010-03-24 23:21:33]

http://codereview.chromium.org/858003/diff/145002/170011
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/145002/170011#newcode352
chrome_frame/chrome_frame_activex_base.h:352:
profile_name->assign(ASCIIToWide(kIexploreProfileName));
nit: define kIexploreProfileName in unicode to avoid this?

http://codereview.chromium.org/858003/diff/145002/170011#newcode358
chrome_frame/chrome_frame_activex_base.h:358: *profile_path =
profile_path->Append(L"Google Chrome Frame");
Either use += or Append. This assignment is redundant. :)

http://codereview.chromium.org/858003/diff/145002/170010
File chrome_frame/chrome_frame_plugin.h (right):

http://codereview.chromium.org/858003/diff/145002/170010#newcode223
chrome_frame/chrome_frame_plugin.h:223: *profile_path =
profile_path->Append(*profile_name);
nit: redundant assignment.

http://codereview.chromium.org/858003/diff/186001/182011#newcode59
chrome_frame/chrome_frame_plugin.h:59: bool InitializeAutomation(std::wstring*
profile_name,
please change this back to const&

http://codereview.chromium.org/858003/diff/186001/182011#newcode220
chrome_frame/chrome_frame_plugin.h:220: virtual void
GetProfilePath(std::wstring* profile_name,
nit: please use const& for profile_name

http://codereview.chromium.org/858003/diff/186001/182011#newcode223
chrome_frame/chrome_frame_plugin.h:223: *profile_path =
profile_path->Append(*profile_name);
redundant assignment

http://codereview.chromium.org/858003/diff/186001/182010
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/186001/182010#newcode46
chrome_frame/delete_chrome_history.cc:46:
profile_name->assign(ASCIIToWide(kIexploreProfileName));
nit: you don't have to use the formal argument, just use
ChromeFramePlugin::GetProfilePath directly with kIexploreProfileName

http://codereview.chromium.org/858003/diff/186001/182009
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/186001/182009#newcode45
chrome_frame/delete_chrome_history.h:45: // CHAIN_MSG_MAP(Base)
CHAIN_MSG_MAP(ChromeFramePlugin<DeleteChromeHistory>)

http://codereview.chromium.org/858003/diff/186001/182018
File chrome_frame/test/automation_client_mock.cc (right):

http://codereview.chromium.org/858003/diff/186001/182018#newcode99
chrome_frame/test/automation_client_mock.cc:99: profile_path =
profile_path.Append(profile);
nit: remove redundant assignment. How about profile_path += L"Adam.N.Epilinter".
Also please make sure that the path has trailing '\'

http://codereview.chromium.org/858003/diff/186001/182018#newcode122
chrome_frame/test/automation_client_mock.cc:122: FilePath profile_path;
since this is being done in so many places now, it'll be nicer if there's a
single function that can work like:
profile_path = GetDefaultChromeFramePfofilePath(profile_name)

http://codereview.chromium.org/858003/diff/186001/182019
File chrome_frame/test/automation_client_mock.h (right):

http://codereview.chromium.org/858003/diff/186001/182019#newcode164
chrome_frame/test/automation_client_mock.h:164: profile_path_ =
profile_path_.Append(L"Adam.N.Epilinter");
nit: remove redundant assignment

http://codereview.chromium.org/858003/diff/186001/182013
File chrome_frame/utils.h (right):

http://codereview.chromium.org/858003/diff/186001/182013#newcode175
chrome_frame/utils.h:175: FilePath GetIETemporaryFilesFolder();
nit: comment above?

[slightlyoff, 2010-03-25 00:40:55]

http://codereview.chromium.org/858003/diff/145002/170011
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/145002/170011#newcode352
chrome_frame/chrome_frame_activex_base.h:352:
profile_name->assign(ASCIIToWide(kIexploreProfileName));
On 2010/03/24 23:21:33, amit wrote:
> nit: define kIexploreProfileName in unicode to avoid this?

Done.

http://codereview.chromium.org/858003/diff/145002/170011#newcode358
chrome_frame/chrome_frame_activex_base.h:358: *profile_path =
profile_path->Append(L"Google Chrome Frame");
On 2010/03/24 23:21:33, amit wrote:
> Either use += or Append. This assignment is redundant. :)

Append won't do it. The FilePath API returns new instances from Append.

http://codereview.chromium.org/858003/diff/145002/170010
File chrome_frame/chrome_frame_plugin.h (right):

http://codereview.chromium.org/858003/diff/145002/170010#newcode223
chrome_frame/chrome_frame_plugin.h:223: *profile_path =
profile_path->Append(*profile_name);
On 2010/03/24 23:21:33, amit wrote:
> nit: redundant assignment.

wontfix

http://codereview.chromium.org/858003/diff/145002/170008
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/145002/170008#newcode22
chrome_frame/delete_chrome_history.h:22: class Bho;
On 2010/03/24 22:21:23, tommi wrote:
> don't need this since you're already including bho.h

doesn't work = (

[amit, 2010-03-25 01:39:08]

http://codereview.chromium.org/858003/diff/133003/174015
File chrome_frame/bho.h (right):

http://codereview.chromium.org/858003/diff/133003/174015#newcode17
chrome_frame/bho.h:17: #include <deletebrowsinghistory.h>
nit: this should go with ATL/system headers above

http://codereview.chromium.org/858003/diff/133003/174012
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/133003/174012#newcode37
chrome_frame/chrome_frame_activex_base.h:37: static const char
kRundllProfileName[] = "rundll32";
nit: make both wchar_t

http://codereview.chromium.org/858003/diff/133003/174012#newcode350
chrome_frame/chrome_frame_activex_base.h:350: //
profile_name.assign(ASCIIToWide(kIexploreProfileName));
you can remove this and call
ChromeFramePlugin::GetProfilePath(kIexploreProfileName, profile_path) in the
else {} below.

http://codereview.chromium.org/858003/diff/133003/174002
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/133003/174002#newcode8
chrome_frame/chrome_frame_automation.cc:8: #include <shlobj.h>
nit: not needed anymore?

http://codereview.chromium.org/858003/diff/133003/174009
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/133003/174009#newcode45
chrome_frame/delete_chrome_history.h:45: // CHAIN_MSG_MAP(Base)
link message map to base

[slightlyoff, 2010-03-25 07:24:30]

http://codereview.chromium.org/858003/diff/133003/174012
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/133003/174012#newcode37
chrome_frame/chrome_frame_activex_base.h:37: static const char
kRundllProfileName[] = "rundll32";
On 2010/03/25 01:39:08, amit wrote:
> nit: make both wchar_t

Done.

http://codereview.chromium.org/858003/diff/133003/174012#newcode350
chrome_frame/chrome_frame_activex_base.h:350: //
profile_name.assign(ASCIIToWide(kIexploreProfileName));
On 2010/03/25 01:39:08, amit wrote:
> you can remove this and call
> ChromeFramePlugin::GetProfilePath(kIexploreProfileName, profile_path) in the
> else {} below.

Done.

http://codereview.chromium.org/858003/diff/133003/174002
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/133003/174002#newcode8
chrome_frame/chrome_frame_automation.cc:8: #include <shlobj.h>
On 2010/03/25 01:39:08, amit wrote:
> nit: not needed anymore?

Done.

http://codereview.chromium.org/858003/diff/133003/174009
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/133003/174009#newcode45
chrome_frame/delete_chrome_history.h:45: // CHAIN_MSG_MAP(Base)
On 2010/03/25 01:39:08, amit wrote:
> link message map to base

Done.

[amit, 2010-03-25 18:05:43]

LGTM! Thanks for the patience and keeping up with the nits so far :)

http://codereview.chromium.org/858003/diff/136003/149017
File chrome_frame/bho.h (right):

http://codereview.chromium.org/858003/diff/136003/149017#newcode17
chrome_frame/bho.h:17: #include <deletebrowsinghistory.h>
nit: move above with the other system headers.

http://codereview.chromium.org/858003/diff/136003/149019
File chrome_frame/test/chrome_frame_automation_mock.h (right):

http://codereview.chromium.org/858003/diff/136003/149019#newcode29
chrome_frame/test/chrome_frame_automation_mock.h:29: FilePath profile_path;
nit: please factor out.

[amit, 2010-03-25 18:06:08]

LGTM! Thanks for the patience and keeping up with the nits so far :)

Unit test coming up in the next CL?

[slightlyoff, 2010-03-25 19:00:44]

http://codereview.chromium.org/858003/diff/136003/149019
File chrome_frame/test/chrome_frame_automation_mock.h (right):

http://codereview.chromium.org/858003/diff/136003/149019#newcode29
chrome_frame/test/chrome_frame_automation_mock.h:29: FilePath profile_path;
On 2010/03/25 18:05:43, amit wrote:
> nit: please factor out.

Done.

[amit, 2010-03-25 19:07:50]

LGTM++

http://codereview.chromium.org/858003/diff/208001/209019
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/858003/diff/208001/209019#newcode367
chrome_frame/test/chrome_frame_test_utils.cc:367: FilePath GetProfilePath(const
std::wstring& suffix) {
nit: suffix -> really a profile_name

[tommi (sloooow) - chröme, 2010-03-25 19:32:35]

lgtm with at least the memory leak fixed.

http://codereview.chromium.org/858003/diff/208001/209014
File chrome_frame/bho.h (right):

http://codereview.chromium.org/858003/diff/208001/209014#newcode17
chrome_frame/bho.h:17: #include <deletebrowsinghistory.h>
move this up to the other windows includes

http://codereview.chromium.org/858003/diff/208001/209009
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/208001/209009#newcode8
chrome_frame/delete_chrome_history.cc:8: #include "chrome_frame/bho.h"
already included in the header.
maybe you don't need it in the header?

http://codereview.chromium.org/858003/diff/208001/209009#newcode19
chrome_frame/delete_chrome_history.cc:19: DLOG(INFO) << __FUNCTION__;
indent

http://codereview.chromium.org/858003/diff/208001/209008
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/208001/209008#newcode22
chrome_frame/delete_chrome_history.h:22: class Bho;
I'm curious what errors you're seeing if you remove this.
You've already included the file that declares the whole class so forward
declarations shouldn't be necessary.

http://codereview.chromium.org/858003/diff/208001/209013
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/858003/diff/208001/209013#newcode421
chrome_frame/utils.cc:421: DCHECK(SUCCEEDED(hr) && tif_pidl);
After using tif_pidl (at the end of the function), you need to free it by
calling
ILFree(tif_pidl);

http://codereview.chromium.org/858003/diff/208001/209013#newcode434
chrome_frame/utils.cc:434: DCHECK(SUCCEEDED(hr) && path.pOleStr);
no need to DCHECK path.pOleStr.

[Sigurður Ásgeirsson, 2010-03-25 19:50:38]

lgtm with a couple of nits.

http://codereview.chromium.org/858003/diff/208001/209011
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/208001/209011#newcode343
chrome_frame/chrome_frame_activex_base.h:343: bool is_IE =
(lstrcmpi(profile_name.c_str(), kIexploreProfileName) == 0) ||
nit: use string_util::LowerCaseEqualsASCII here?

http://codereview.chromium.org/858003/diff/208001/209001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/208001/209001#newcode267
chrome_frame/chrome_frame_automation.cc:267: DLOG(INFO) << " profile path: " <<
params.profile_path.value();
nit: why start DLOG message with a space, and lower case char?

http://codereview.chromium.org/858003/diff/208001/209013
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/858003/diff/208001/209013#newcode418
chrome_frame/utils.cc:418: LPITEMIDLIST tif_pidl = NULL;
Is there a special reason why SHGetFolderPath won't work here?

[amit, 2010-03-25 20:48:56]

http://codereview.chromium.org/858003/diff/208001/209011
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/208001/209011#newcode343
chrome_frame/chrome_frame_activex_base.h:343: bool is_IE =
(lstrcmpi(profile_name.c_str(), kIexploreProfileName) == 0) ||
On 2010/03/25 19:50:38, Ruðrugis wrote:
> nit: use string_util::LowerCaseEqualsASCII here?

Is there a unicode counterpart for comparison in string_util?

http://codereview.chromium.org/858003/diff/208001/209013
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/858003/diff/208001/209013#newcode418
chrome_frame/utils.cc:418: LPITEMIDLIST tif_pidl = NULL;
On 2010/03/25 19:50:38, Ruðrugis wrote:
> Is there a special reason why SHGetFolderPath won't work here? 
Due to MAX_PATH limit for SHGetFolderPath.

[slightlyoff, 2010-03-25 20:57:38]

nits fixed.

http://codereview.chromium.org/858003/diff/208001/209014
File chrome_frame/bho.h (right):

http://codereview.chromium.org/858003/diff/208001/209014#newcode17
chrome_frame/bho.h:17: #include <deletebrowsinghistory.h>
On 2010/03/25 19:32:35, tommi wrote:
> move this up to the other windows includes

Done.

http://codereview.chromium.org/858003/diff/208001/209011
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/858003/diff/208001/209011#newcode343
chrome_frame/chrome_frame_activex_base.h:343: bool is_IE =
(lstrcmpi(profile_name.c_str(), kIexploreProfileName) == 0) ||
On 2010/03/25 19:50:38, Ruðrugis wrote:
> nit: use string_util::LowerCaseEqualsASCII here?

Was using it before, Amit suggested I move to the windows API. Leaving it.

http://codereview.chromium.org/858003/diff/208001/209001
File chrome_frame/chrome_frame_automation.cc (right):

http://codereview.chromium.org/858003/diff/208001/209001#newcode267
chrome_frame/chrome_frame_automation.cc:267: DLOG(INFO) << " profile path: " <<
params.profile_path.value();
On 2010/03/25 19:50:38, Ruðrugis wrote:
> nit: why start DLOG message with a space, and lower case char?

Fixed.

http://codereview.chromium.org/858003/diff/208001/209009
File chrome_frame/delete_chrome_history.cc (right):

http://codereview.chromium.org/858003/diff/208001/209009#newcode8
chrome_frame/delete_chrome_history.cc:8: #include "chrome_frame/bho.h"
On 2010/03/25 19:32:35, tommi wrote:
> already included in the header.
> maybe you don't need it in the header?

Done.

http://codereview.chromium.org/858003/diff/208001/209009#newcode19
chrome_frame/delete_chrome_history.cc:19: DLOG(INFO) << __FUNCTION__;
On 2010/03/25 19:32:35, tommi wrote:
> indent

Done.

http://codereview.chromium.org/858003/diff/208001/209008
File chrome_frame/delete_chrome_history.h (right):

http://codereview.chromium.org/858003/diff/208001/209008#newcode22
chrome_frame/delete_chrome_history.h:22: class Bho;
On 2010/03/25 19:32:35, tommi wrote:
> I'm curious what errors you're seeing if you remove this.
> You've already included the file that declares the whole class so forward
> declarations shouldn't be necessary.

I agree, it doesn't make sense. Can investigate after this lands.

http://codereview.chromium.org/858003/diff/208001/209019
File chrome_frame/test/chrome_frame_test_utils.cc (right):

http://codereview.chromium.org/858003/diff/208001/209019#newcode367
chrome_frame/test/chrome_frame_test_utils.cc:367: FilePath GetProfilePath(const
std::wstring& suffix) {
On 2010/03/25 19:07:50, amit wrote:
> nit: suffix -> really a profile_name

Done.

http://codereview.chromium.org/858003/diff/208001/209013
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/858003/diff/208001/209013#newcode418
chrome_frame/utils.cc:418: LPITEMIDLIST tif_pidl = NULL;
On 2010/03/25 19:50:38, Ruðrugis wrote:
> Is there a special reason why SHGetFolderPath won't work here? 

It's limited to MAXPATH. An earlier version of this patch used it but Amit
objected. Gymnastics ensued.

http://codereview.chromium.org/858003/diff/208001/209013#newcode421
chrome_frame/utils.cc:421: DCHECK(SUCCEEDED(hr) && tif_pidl);
On 2010/03/25 19:32:35, tommi wrote:
> After using tif_pidl (at the end of the function), you need to free it by
> calling
> ILFree(tif_pidl);

Done.

http://codereview.chromium.org/858003/diff/208001/209013#newcode434
chrome_frame/utils.cc:434: DCHECK(SUCCEEDED(hr) && path.pOleStr);
On 2010/03/25 19:32:35, tommi wrote:
> no need to DCHECK path.pOleStr.

Done.

[tommi (sloooow) - chröme, 2010-03-25 21:08:58]

lgtm++

On Thu, Mar 25, 2010 at 1:57 PM, <slightlyoff@chromium.org> wrote:

> nits fixed.
>
>
>
> http://codereview.chromium.org/858003/diff/208001/209014
> File chrome_frame/bho.h (right):
>
> http://codereview.chromium.org/858003/diff/208001/209014#newcode17
> chrome_frame/bho.h:17: #include <deletebrowsinghistory.h>
> On 2010/03/25 19:32:35, tommi wrote:
>
>> move this up to the other windows includes
>>
>
> Done.
>
>
> http://codereview.chromium.org/858003/diff/208001/209011
> File chrome_frame/chrome_frame_activex_base.h (right):
>
> http://codereview.chromium.org/858003/diff/208001/209011#newcode343
> chrome_frame/chrome_frame_activex_base.h:343: bool is_IE =
> (lstrcmpi(profile_name.c_str(), kIexploreProfileName) == 0) ||
> On 2010/03/25 19:50:38, Ruðrugis wrote:
>
>> nit: use string_util::LowerCaseEqualsASCII here?
>>
>
> Was using it before, Amit suggested I move to the windows API. Leaving
> it.
>
>
> http://codereview.chromium.org/858003/diff/208001/209001
> File chrome_frame/chrome_frame_automation.cc (right):
>
> http://codereview.chromium.org/858003/diff/208001/209001#newcode267
> chrome_frame/chrome_frame_automation.cc:267: DLOG(INFO) << " profile
> path: " << params.profile_path.value();
> On 2010/03/25 19:50:38, Ruðrugis wrote:
>
>> nit: why start DLOG message with a space, and lower case char?
>>
>
> Fixed.
>
>
> http://codereview.chromium.org/858003/diff/208001/209009
> File chrome_frame/delete_chrome_history.cc (right):
>
> http://codereview.chromium.org/858003/diff/208001/209009#newcode8
> chrome_frame/delete_chrome_history.cc:8: #include "chrome_frame/bho.h"
> On 2010/03/25 19:32:35, tommi wrote:
>
>> already included in the header.
>> maybe you don't need it in the header?
>>
>
> Done.
>
>
> http://codereview.chromium.org/858003/diff/208001/209009#newcode19
> chrome_frame/delete_chrome_history.cc:19: DLOG(INFO) << __FUNCTION__;
> On 2010/03/25 19:32:35, tommi wrote:
>
>> indent
>>
>
> Done.
>
>
> http://codereview.chromium.org/858003/diff/208001/209008
> File chrome_frame/delete_chrome_history.h (right):
>
> http://codereview.chromium.org/858003/diff/208001/209008#newcode22
> chrome_frame/delete_chrome_history.h:22: class Bho;
> On 2010/03/25 19:32:35, tommi wrote:
>
>> I'm curious what errors you're seeing if you remove this.
>> You've already included the file that declares the whole class so
>>
> forward
>
>> declarations shouldn't be necessary.
>>
>
> I agree, it doesn't make sense. Can investigate after this lands.
>
>
> http://codereview.chromium.org/858003/diff/208001/209019
> File chrome_frame/test/chrome_frame_test_utils.cc (right):
>
> http://codereview.chromium.org/858003/diff/208001/209019#newcode367
> chrome_frame/test/chrome_frame_test_utils.cc:367: FilePath
> GetProfilePath(const std::wstring& suffix) {
> On 2010/03/25 19:07:50, amit wrote:
>
>> nit: suffix -> really a profile_name
>>
>
> Done.
>
>
> http://codereview.chromium.org/858003/diff/208001/209013
> File chrome_frame/utils.cc (right):
>
> http://codereview.chromium.org/858003/diff/208001/209013#newcode418
> chrome_frame/utils.cc:418: LPITEMIDLIST tif_pidl = NULL;
> On 2010/03/25 19:50:38, Ruðrugis wrote:
>
>> Is there a special reason why SHGetFolderPath won't work here?
>>
>
> It's limited to MAXPATH. An earlier version of this patch used it but
> Amit objected. Gymnastics ensued.
>
>
> http://codereview.chromium.org/858003/diff/208001/209013#newcode421
> chrome_frame/utils.cc:421: DCHECK(SUCCEEDED(hr) && tif_pidl);
> On 2010/03/25 19:32:35, tommi wrote:
>
>> After using tif_pidl (at the end of the function), you need to free it
>>
> by
>
>> calling
>> ILFree(tif_pidl);
>>
>
> Done.
>
>
> http://codereview.chromium.org/858003/diff/208001/209013#newcode434
> chrome_frame/utils.cc:434: DCHECK(SUCCEEDED(hr) && path.pOleStr);
> On 2010/03/25 19:32:35, tommi wrote:
>
>> no need to DCHECK path.pOleStr.
>>
>
> Done.
>
>
> http://codereview.chromium.org/858003
>