Add a command line flag to disable infobars.

Allow TabContentsDelegate classes to specify whether InfoBars are enabled.
Allow ChromeFrame to pass infobar enabled parameter to ExternalTabContainer.
BUG=2444936
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42800

[Jói, 2010-03-12 19:15:08]

Looks good, a couple of minor comments.

http://codereview.chromium.org/890005/diff/1/2
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/890005/diff/1/2#newcode263
chrome/browser/tab_contents/tab_contents.cc:263:
infobars_disabled_(CommandLine::ForCurrentProcess()->HasSwitch(
Seeing as this will not change for the lifetime of the process, maybe a trinary
static flag NOTCHECKED/YES/NO and a function AreInfobarsDisabled() which would
initialize it if needed would be more efficient and less storage?

http://codereview.chromium.org/890005/diff/1/6
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/890005/diff/1/6#newcode427
chrome_frame/chrome_frame_activex.cc:427: chrome_extra_arguments.append(L"
--disable-infobars");
Just to verify:  You're sure IOleObject_SetClientSite only gets called in widget
mode?

[amit, 2010-03-12 19:26:34]

Drive by

http://codereview.chromium.org/890005/diff/1/2
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/890005/diff/1/2#newcode933
chrome/browser/tab_contents/tab_contents.cc:933: if (infobars_disabled_) {
if this flag is maintained in the delegate, checks like this will no longer be
needed :)

http://codereview.chromium.org/890005/diff/1/3
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/890005/diff/1/3#newcode1091
chrome/browser/tab_contents/tab_contents.h:1091: bool infobars_disabled_;
Wouldn't it be better to ask the delegate if infobars are disabled instead of
keeping a flag here? The default return is enabled. In case of chrome frame the
delegate is ExternalTabContainer and it can override it based on if it is in
widget mode or not. 
Also, it will further simplify things if the flag about chrome frame widget mode
can be passed as one of the launch arguments.

[Eric, 2010-03-12 21:19:18]

http://codereview.chromium.org/890005/diff/1/2
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/890005/diff/1/2#newcode263
chrome/browser/tab_contents/tab_contents.cc:263:
infobars_disabled_(CommandLine::ForCurrentProcess()->HasSwitch(
On 2010/03/12 19:15:08, Jói wrote:
> Seeing as this will not change for the lifetime of the process, maybe a
trinary
> static flag NOTCHECKED/YES/NO and a function AreInfobarsDisabled() which would
> initialize it if needed would be more efficient and less storage?

This has been moved to external_tab_container.

http://codereview.chromium.org/890005/diff/1/2#newcode933
chrome/browser/tab_contents/tab_contents.cc:933: if (infobars_disabled_) {
On 2010/03/12 19:26:34, amit wrote:
> if this flag is maintained in the delegate, checks like this will no longer be
> needed :)

The flag is not maintained in the delegate, and these checks have been replaced
with calls to the delegate.

http://codereview.chromium.org/890005/diff/1/3
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/890005/diff/1/3#newcode1091
chrome/browser/tab_contents/tab_contents.h:1091: bool infobars_disabled_;
On 2010/03/12 19:26:34, amit wrote:
> Wouldn't it be better to ask the delegate if infobars are disabled instead of
> keeping a flag here? The default return is enabled. In case of chrome frame
the
> delegate is ExternalTabContainer and it can override it based on if it is in
> widget mode or not. 
> Also, it will further simplify things if the flag about chrome frame widget
mode
> can be passed as one of the launch arguments.

Done.

http://codereview.chromium.org/890005/diff/1/6
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/890005/diff/1/6#newcode427
chrome_frame/chrome_frame_activex.cc:427: chrome_extra_arguments.append(L"
--disable-infobars");
On 2010/03/12 19:15:08, Jói wrote:
> Just to verify:  You're sure IOleObject_SetClientSite only gets called in
widget
> mode?

To my understanding, this class is specifically for activex/widget mode and
chrome_active_document (which also has a IOleObject_SetClientSite) is for non
active document/non-widget mode.

[Jói, 2010-03-12 21:32:02]

http://codereview.chromium.org/890005/diff/18001/2008
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/890005/diff/18001/2008#newcode428
chrome_frame/chrome_frame_activex.cc:428:
chrome_extra_arguments.append(CA2W(switches::kChromeFrameWidgetMode));
this also needs to get added when launching Chrome for the NPAPI version of CF

[Eric, 2010-03-15 15:36:18]

http://codereview.chromium.org/890005/diff/18001/2008
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/890005/diff/18001/2008#newcode428
chrome_frame/chrome_frame_activex.cc:428:
chrome_extra_arguments.append(CA2W(switches::kChromeFrameWidgetMode));
On 2010/03/12 21:32:03, Jói wrote:
> this also needs to get added when launching Chrome for the NPAPI version of CF

Amit, what is the appropriate way to distinguish between widget/non-widget mode
in Firefox?

[Eric, 2010-03-16 13:08:36]

Ping, Amit?

On Mon, Mar 15, 2010 at 11:36 AM, <ericdingle@google.com> wrote:

>
> http://codereview.chromium.org/890005/diff/18001/2008
> File chrome_frame/chrome_frame_activex.cc (right):
>
> http://codereview.chromium.org/890005/diff/18001/2008#newcode428
> chrome_frame/chrome_frame_activex.cc:428:
> chrome_extra_arguments.append(CA2W(switches::kChromeFrameWidgetMode));
> On 2010/03/12 21:32:03, Jói wrote:
>
>> this also needs to get added when launching Chrome for the NPAPI
>>
> version of CF
>
> Amit, what is the appropriate way to distinguish between
> widget/non-widget mode in Firefox?
>
>
> http://codereview.chromium.org/890005
>

[Jói, 2010-03-16 13:09:49]

FYI, there is no full-tab mode in Firefox so perhaps just always
assume widget mode.

On Tue, Mar 16, 2010 at 9:08 AM, Eric Dingle <ericdingle@google.com> wrote:
> Ping, Amit?
>
> On Mon, Mar 15, 2010 at 11:36 AM, <ericdingle@google.com> wrote:
>>
>> http://codereview.chromium.org/890005/diff/18001/2008
>> File chrome_frame/chrome_frame_activex.cc (right):
>>
>> http://codereview.chromium.org/890005/diff/18001/2008#newcode428
>> chrome_frame/chrome_frame_activex.cc:428:
>> chrome_extra_arguments.append(CA2W(switches::kChromeFrameWidgetMode));
>> On 2010/03/12 21:32:03, Jói wrote:
>>>
>>> this also needs to get added when launching Chrome for the NPAPI
>>
>> version of CF
>>
>> Amit, what is the appropriate way to distinguish between
>> widget/non-widget mode in Firefox?
>>
>> http://codereview.chromium.org/890005
>
>

[amit, 2010-03-16 13:16:41]

Oops I thought I had replied to that... Sorry.

On Tue, Mar 16, 2010 at 6:09 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> FYI, there is no full-tab mode in Firefox so perhaps just always
> assume widget mode.
>

Yeah, Joi is right. There's no full tab mode in FF so for now, it's ok to
assume ChromeFrameNPAPI will always be in widget mode.


>
> On Tue, Mar 16, 2010 at 9:08 AM, Eric Dingle <ericdingle@google.com>
> wrote:
> > Ping, Amit?
> >
> > On Mon, Mar 15, 2010 at 11:36 AM, <ericdingle@google.com> wrote:
> >>
> >> http://codereview.chromium.org/890005/diff/18001/2008
> >> File chrome_frame/chrome_frame_activex.cc (right):
> >>
> >> http://codereview.chromium.org/890005/diff/18001/2008#newcode428
> >> chrome_frame/chrome_frame_activex.cc:428:
> >> chrome_extra_arguments.append(CA2W(switches::kChromeFrameWidgetMode));
> >> On 2010/03/12 21:32:03, Jói wrote:
> >>>
> >>> this also needs to get added when launching Chrome for the NPAPI
> >>
> >> version of CF
> >>
> >> Amit, what is the appropriate way to distinguish between
> >> widget/non-widget mode in Firefox?
> >>
> >> http://codereview.chromium.org/890005
> >
> >
>

[amit, 2010-03-16 13:18:23]

http://codereview.chromium.org/890005/diff/18001/2008
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/890005/diff/18001/2008#newcode428
chrome_frame/chrome_frame_activex.cc:428:
chrome_extra_arguments.append(CA2W(switches::kChromeFrameWidgetMode));
On 2010/03/15 15:36:18, ericdingle wrote:
> On 2010/03/12 21:32:03, Jói wrote:
> > this also needs to get added when launching Chrome for the NPAPI version of
CF
> 
> Amit, what is the appropriate way to distinguish between widget/non-widget
mode
> in Firefox?

Currently the only difference is the what gets instantiated ActiveX vs Active
Doc vs Plugin. So this check also needs to be in the NPAPI version. 

Now, ActiveDoc and ActiveX uses the same profile to launch. So doing this with
command like is incorrect. If widget mode is launched first and then Active doc
is launched, then it will inherit the same process and the infobars will be
turned off for Active doc. Similarly the other way round. 

This flag must be passed as an initialization argument to the
ExternalTabContainer.

[Eric, 2010-03-17 15:48:52]

I've updated the implementation as per Amit's comments. Please have another
look.

http://codereview.chromium.org/890005/diff/18001/2008
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/890005/diff/18001/2008#newcode428
chrome_frame/chrome_frame_activex.cc:428:
chrome_extra_arguments.append(CA2W(switches::kChromeFrameWidgetMode));
On 2010/03/16 13:18:23, amit wrote:
> On 2010/03/15 15:36:18, ericdingle wrote:
> > On 2010/03/12 21:32:03, Jói wrote:
> > > this also needs to get added when launching Chrome for the NPAPI version
of
> CF
> > 
> > Amit, what is the appropriate way to distinguish between widget/non-widget
> mode
> > in Firefox?
> 
> Currently the only difference is the what gets instantiated ActiveX vs Active
> Doc vs Plugin. So this check also needs to be in the NPAPI version. 
> 
> Now, ActiveDoc and ActiveX uses the same profile to launch. So doing this with
> command like is incorrect. If widget mode is launched first and then Active
doc
> is launched, then it will inherit the same process and the infobars will be
> turned off for Active doc. Similarly the other way round. 
> 
> This flag must be passed as an initialization argument to the
> ExternalTabContainer.

Done.

[joi, 2010-03-17 19:03:25]

LGTM

[amit, 2010-03-17 19:36:23]

Looks much better, Thanks.
LGTM with a few nits.

http://codereview.chromium.org/890005/diff/27002/28002
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/890005/diff/27002/28002#newcode393
chrome/browser/external_tab_container.cc:393: bool
ExternalTabContainer::AreInfoBarsEnabled() {
nit: convention is to name the getter after the member as infobars_enabled()

http://codereview.chromium.org/890005/diff/27002/28010
File chrome_frame/chrome_frame_automation.h (right):

http://codereview.chromium.org/890005/diff/27002/28010#newcode167
chrome_frame/chrome_frame_automation.h:167: bool is_widget_mode);
Since there are 7 arguments here, its better to take a struct as an argument. A
majority of these are common to ChromeFrameLaunchParams so it could be reused
with little refactoring.

[amit, 2010-03-17 19:37:12]

BTW, are tests for this coming soon?

On 2010/03/17 19:36:23, amit wrote:
> Looks much better, Thanks.
> LGTM with a few nits.
> 
> http://codereview.chromium.org/890005/diff/27002/28002
> File chrome/browser/external_tab_container.cc (right):
> 
> http://codereview.chromium.org/890005/diff/27002/28002#newcode393
> chrome/browser/external_tab_container.cc:393: bool
> ExternalTabContainer::AreInfoBarsEnabled() {
> nit: convention is to name the getter after the member as infobars_enabled()
> 
> http://codereview.chromium.org/890005/diff/27002/28010
> File chrome_frame/chrome_frame_automation.h (right):
> 
> http://codereview.chromium.org/890005/diff/27002/28010#newcode167
> chrome_frame/chrome_frame_automation.h:167: bool is_widget_mode);
> Since there are 7 arguments here, its better to take a struct as an argument.
A
> majority of these are common to ChromeFrameLaunchParams so it could be reused
> with little refactoring.

[Eric, 2010-03-22 19:21:58]

I have addressed all comments. Amit, I have updated the CF tests for the
changes, and have made sure tests are passing on the Chrome side. Do you have
any more suggestions?

http://codereview.chromium.org/890005/diff/27002/28002
File chrome/browser/external_tab_container.cc (right):

http://codereview.chromium.org/890005/diff/27002/28002#newcode393
chrome/browser/external_tab_container.cc:393: bool
ExternalTabContainer::AreInfoBarsEnabled() {
On 2010/03/17 19:36:23, amit wrote:
> nit: convention is to name the getter after the member as infobars_enabled()

Done.

http://codereview.chromium.org/890005/diff/27002/28010
File chrome_frame/chrome_frame_automation.h (right):

http://codereview.chromium.org/890005/diff/27002/28010#newcode167
chrome_frame/chrome_frame_automation.h:167: bool is_widget_mode);
On 2010/03/17 19:36:23, amit wrote:
> Since there are 7 arguments here, its better to take a struct as an argument.
A
> majority of these are common to ChromeFrameLaunchParams so it could be reused
> with little refactoring.

Done.

[amit, 2010-03-22 20:21:34]

LGTM++

http://codereview.chromium.org/890005/diff/42001/43010
File chrome_frame/chrome_frame_automation.h (right):

http://codereview.chromium.org/890005/diff/42001/43010#newcode162
chrome_frame/chrome_frame_automation.h:162: ChromeFrameLaunchParams
chrome_launch_params);
Nice, thanks!
nit: const &?

[Eric, 2010-03-23 17:34:26]

Joi, can you submit this for me? Thanks.

http://codereview.chromium.org/890005/diff/42001/43010
File chrome_frame/chrome_frame_automation.h (right):

http://codereview.chromium.org/890005/diff/42001/43010#newcode162
chrome_frame/chrome_frame_automation.h:162: ChromeFrameLaunchParams
chrome_launch_params);
On 2010/03/22 20:21:34, amit wrote:
> Nice, thanks!
> nit: const &?

Done.

[joi, 2010-03-23 19:11:32]

Committed as rev 42366.

As discussed, I saw a failure of UI test SelfDeletePluginInNPNEvaluate
with the same patch as your patch #6, and you did not get the same
failure when trying the patch.  Since this UI test has been failing on
other try jobs, it seems unrelated and probably flaky.

Cheers,
Jói


On Tue, Mar 23, 2010 at 1:34 PM,  <ericdingle@google.com> wrote:
> Joi, can you submit this for me? Thanks.
>
>
> http://codereview.chromium.org/890005/diff/42001/43010
> File chrome_frame/chrome_frame_automation.h (right):
>
> http://codereview.chromium.org/890005/diff/42001/43010#newcode162
> chrome_frame/chrome_frame_automation.h:162: ChromeFrameLaunchParams
> chrome_launch_params);
> On 2010/03/22 20:21:34, amit wrote:
>>
>> Nice, thanks!
>> nit: const &?
>
> Done.
>
> http://codereview.chromium.org/890005
>

[Eric, 2010-03-25 18:36:42]

On 2010/03/23 19:11:32, joi wrote:
> Committed as rev 42366.
> 
> As discussed, I saw a failure of UI test SelfDeletePluginInNPNEvaluate
> with the same patch as your patch #6, and you did not get the same
> failure when trying the patch.  Since this UI test has been failing on
> other try jobs, it seems unrelated and probably flaky.
> 
> Cheers,
> Jói
> 
> 
> On Tue, Mar 23, 2010 at 1:34 PM,  <mailto:ericdingle@google.com> wrote:
> > Joi, can you submit this for me? Thanks.
> >
> >
> > http://codereview.chromium.org/890005/diff/42001/43010
> > File chrome_frame/chrome_frame_automation.h (right):
> >
> > http://codereview.chromium.org/890005/diff/42001/43010#newcode162
> > chrome_frame/chrome_frame_automation.h:162: ChromeFrameLaunchParams
> > chrome_launch_params);
> > On 2010/03/22 20:21:34, amit wrote:
> >>
> >> Nice, thanks!
> >> nit: const &?
> >
> > Done.
> >
> > http://codereview.chromium.org/890005
> >
> 

I have resolved the issues seen when running chrome_frame_unittests and
chrome_frame_tests on the previous attempted check-in. The issues are resolved
in chrome_frame_automation.cc and chrome_frame_npapi_unittest.cc in patch 7.
Please have one more quick look.

[Jói, 2010-03-25 18:47:29]

LGTM

[stoyan, 2010-03-26 16:37:16]

http://codereview.chromium.org/890005/diff/93001/94006
File chrome/test/automation/automation_messages.h (right):

http://codereview.chromium.org/890005/diff/93001/94006#newcode359
chrome/test/automation/automation_messages.h:359: bool infobars_enabled;
You have to update Read/Write/Log methods of ParamTraits<ExternalTabSettings>
(below).