Added more tests for IncognitoModeAvailability.

Added more tests for IncognitoModeAvailability.
The user should not be able to open Incognito windows from link and launcher context menus when the Incognito mode is disabled (e.g. by a policy).

This CL should help automate http://google-testcasemanager.appspot.com/home#!view=test&ti=16954266

BUG=

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

[sky, 2012-12-13 22:55:33]

Why do these tests need to be browser tests and not unit tests?

[rustema, 2012-12-14 03:03:45]

On 2012/12/13 22:55:33, sky wrote:
> Why do these tests need to be browser tests and not unit tests?

These tests use the pref service which needs a browser process IIRC. I haven't
been working on chrome for a long time though so if it changed please let me
know how I can make them unit tests.

[sky, 2012-12-14 17:11:41]

You can still use prefs in unit tests.

  -Scott

On Thu, Dec 13, 2012 at 7:03 PM,  <rustema@google.com> wrote:
> Reviewers: brettw, sky,
>
> Message:
>
> On 2012/12/13 22:55:33, sky wrote:
>>
>> Why do these tests need to be browser tests and not unit tests?
>
>
> These tests use the pref service which needs a browser process IIRC. I
> haven't
> been working on chrome for a long time though so if it changed please let me
> know how I can make them unit tests.
>
> Description:
> Added more tests for IncognitoModeAvailability.
> The user should not be able to open Incognito windows from link and launcher
> context menus when the Incognito mode is disabled (e.g. by a policy).
>
> This CL should help automate
> http://google-testcasemanager.appspot.com/home#!view=test&ti=16954266
>
> BUG=
>
> Please review this at https://codereview.chromium.org/11471027/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
>   M chrome/browser/ui/ash/launcher/launcher_context_menu.h
>   A chrome/browser/ui/ash/launcher/launcher_context_menu_browsertest.cc
>   M chrome/chrome_tests.gypi
>
>

[xot, 2012-12-14 18:09:07]

Will unit test be able to verify the behavior of the right-click (or
Ctrl+click) pop-up menu on Apps icons sitting in the Launch Tray of Chrome
OS?  That's one of the 'New Incognito Window' cases that I think should be
covered.  And could be covered by integration tests.

-Xot

On Fri, Dec 14, 2012 at 9:11 AM, Scott Violet <sky@chromium.org> wrote:

> You can still use prefs in unit tests.
>
>   -Scott
>
> On Thu, Dec 13, 2012 at 7:03 PM,  <rustema@google.com> wrote:
> > Reviewers: brettw, sky,
> >
> > Message:
> >
> > On 2012/12/13 22:55:33, sky wrote:
> >>
> >> Why do these tests need to be browser tests and not unit tests?
> >
> >
> > These tests use the pref service which needs a browser process IIRC. I
> > haven't
> > been working on chrome for a long time though so if it changed please
> let me
> > know how I can make them unit tests.
> >
> > Description:
> > Added more tests for IncognitoModeAvailability.
> > The user should not be able to open Incognito windows from link and
> launcher
> > context menus when the Incognito mode is disabled (e.g. by a policy).
> >
> > This CL should help automate
> > http://google-testcasemanager.appspot.com/home#!view=test&ti=16954266
> >
> > BUG=
> >
> > Please review this at https://codereview.chromium.org/11471027/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files:
> >   M chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
> >   M chrome/browser/ui/ash/launcher/launcher_context_menu.h
> >   A chrome/browser/ui/ash/launcher/launcher_context_menu_browsertest.cc
> >   M chrome/chrome_tests.gypi
> >
> >
>

[rustema, 2012-12-17 03:58:49]

Originally these tests were supposed to be integration tests. So we made
them browser tests. Are there serious arguments against browser tests?


On Fri, Dec 14, 2012 at 10:08 AM, Scott Cunningham
<scunningham@google.com>wrote:

> Will unit test be able to verify the behavior of the right-click (or
> Ctrl+click) pop-up menu on Apps icons sitting in the Launch Tray of Chrome
> OS?  That's one of the 'New Incognito Window' cases that I think should be
> covered.  And could be covered by integration tests.
>
> -Xot
>
>
> On Fri, Dec 14, 2012 at 9:11 AM, Scott Violet <sky@chromium.org> wrote:
>
>> You can still use prefs in unit tests.
>>
>>   -Scott
>>
>> On Thu, Dec 13, 2012 at 7:03 PM,  <rustema@google.com> wrote:
>> > Reviewers: brettw, sky,
>> >
>> > Message:
>> >
>> > On 2012/12/13 22:55:33, sky wrote:
>> >>
>> >> Why do these tests need to be browser tests and not unit tests?
>> >
>> >
>> > These tests use the pref service which needs a browser process IIRC. I
>> > haven't
>> > been working on chrome for a long time though so if it changed please
>> let me
>> > know how I can make them unit tests.
>> >
>> > Description:
>> > Added more tests for IncognitoModeAvailability.
>> > The user should not be able to open Incognito windows from link and
>> launcher
>> > context menus when the Incognito mode is disabled (e.g. by a policy).
>> >
>> > This CL should help automate
>> > http://google-testcasemanager.appspot.com/home#!view=test&ti=16954266
>> >
>> > BUG=
>> >
>> > Please review this at https://codereview.chromium.org/11471027/
>> >
>> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
>> >
>> > Affected files:
>> >   M chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
>> >   M chrome/browser/ui/ash/launcher/launcher_context_menu.h
>> >   A chrome/browser/ui/ash/launcher/launcher_context_menu_browsertest.cc
>> >   M chrome/chrome_tests.gypi
>> >
>> >
>>
>
>

[sky, 2012-12-17 17:10:18]

On Fri, Dec 14, 2012 at 10:08 AM, Scott Cunningham
<scunningham@google.com> wrote:
> Will unit test be able to verify the behavior of the right-click (or
> Ctrl+click) pop-up menu on Apps icons sitting in the Launch Tray of Chrome
> OS?  That's one of the 'New Incognito Window' cases that I think should be
> covered.  And could be covered by integration tests.

No, but that probably fits in a different place anyway.

  -Scott

>
> -Xot
>
>
> On Fri, Dec 14, 2012 at 9:11 AM, Scott Violet <sky@chromium.org> wrote:
>>
>> You can still use prefs in unit tests.
>>
>>   -Scott
>>
>> On Thu, Dec 13, 2012 at 7:03 PM,  <rustema@google.com> wrote:
>> > Reviewers: brettw, sky,
>> >
>> > Message:
>> >
>> > On 2012/12/13 22:55:33, sky wrote:
>> >>
>> >> Why do these tests need to be browser tests and not unit tests?
>> >
>> >
>> > These tests use the pref service which needs a browser process IIRC. I
>> > haven't
>> > been working on chrome for a long time though so if it changed please
>> > let me
>> > know how I can make them unit tests.
>> >
>> > Description:
>> > Added more tests for IncognitoModeAvailability.
>> > The user should not be able to open Incognito windows from link and
>> > launcher
>> > context menus when the Incognito mode is disabled (e.g. by a policy).
>> >
>> > This CL should help automate
>> > http://google-testcasemanager.appspot.com/home#!view=test&ti=16954266
>> >
>> > BUG=
>> >
>> > Please review this at https://codereview.chromium.org/11471027/
>> >
>> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
>> >
>> > Affected files:
>> >   M chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
>> >   M chrome/browser/ui/ash/launcher/launcher_context_menu.h
>> >   A chrome/browser/ui/ash/launcher/launcher_context_menu_browsertest.cc
>> >   M chrome/chrome_tests.gypi
>> >
>> >
>
>

[sky, 2012-12-17 17:13:42]

Unit tests are preferable as they are focused tests, orders of
magnitude faster that browser_tests and generally more reliable (a
browser_test can be effected by any random bug in chrome). Most tests
end up as browser tests because not enough of the surrounding code can
be mocked out. This test seemed to be rather narrow and thus capable
of being a unit test. If your test requires all of chrome running,
then by all means make it a browser test.

  -Scott

On Sun, Dec 16, 2012 at 7:58 PM, Rustem Arzymbetov <rustema@google.com> wrote:
> Originally these tests were supposed to be integration tests. So we made
> them browser tests. Are there serious arguments against browser tests?
>
>
> On Fri, Dec 14, 2012 at 10:08 AM, Scott Cunningham <scunningham@google.com>
> wrote:
>>
>> Will unit test be able to verify the behavior of the right-click (or
>> Ctrl+click) pop-up menu on Apps icons sitting in the Launch Tray of Chrome
>> OS?  That's one of the 'New Incognito Window' cases that I think should be
>> covered.  And could be covered by integration tests.
>>
>> -Xot
>>
>>
>> On Fri, Dec 14, 2012 at 9:11 AM, Scott Violet <sky@chromium.org> wrote:
>>>
>>> You can still use prefs in unit tests.
>>>
>>>   -Scott
>>>
>>> On Thu, Dec 13, 2012 at 7:03 PM,  <rustema@google.com> wrote:
>>> > Reviewers: brettw, sky,
>>> >
>>> > Message:
>>> >
>>> > On 2012/12/13 22:55:33, sky wrote:
>>> >>
>>> >> Why do these tests need to be browser tests and not unit tests?
>>> >
>>> >
>>> > These tests use the pref service which needs a browser process IIRC. I
>>> > haven't
>>> > been working on chrome for a long time though so if it changed please
>>> > let me
>>> > know how I can make them unit tests.
>>> >
>>> > Description:
>>> > Added more tests for IncognitoModeAvailability.
>>> > The user should not be able to open Incognito windows from link and
>>> > launcher
>>> > context menus when the Incognito mode is disabled (e.g. by a policy).
>>> >
>>> > This CL should help automate
>>> > http://google-testcasemanager.appspot.com/home#!view=test&ti=16954266
>>> >
>>> > BUG=
>>> >
>>> > Please review this at https://codereview.chromium.org/11471027/
>>> >
>>> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>> >
>>> > Affected files:
>>> >   M chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
>>> >   M chrome/browser/ui/ash/launcher/launcher_context_menu.h
>>> >   A chrome/browser/ui/ash/launcher/launcher_context_menu_browsertest.cc
>>> >   M chrome/chrome_tests.gypi
>>> >
>>> >
>>
>>
>

[rustema, 2013-01-13 21:51:54]

Hi Scott,

Sorry for not replying earlier: just came back from vacation.
I see your point about unit tests. So I've modified the CL appropriately.
Please, take another look.

Thanks,
Rustem


On Mon, Dec 17, 2012 at 9:13 AM, Scott Violet <sky@chromium.org> wrote:

> Unit tests are preferable as they are focused tests, orders of
> magnitude faster that browser_tests and generally more reliable (a
> browser_test can be effected by any random bug in chrome). Most tests
> end up as browser tests because not enough of the surrounding code can
> be mocked out. This test seemed to be rather narrow and thus capable
> of being a unit test. If your test requires all of chrome running,
> then by all means make it a browser test.
>
>   -Scott
>
> On Sun, Dec 16, 2012 at 7:58 PM, Rustem Arzymbetov <rustema@google.com>
> wrote:
> > Originally these tests were supposed to be integration tests. So we made
> > them browser tests. Are there serious arguments against browser tests?
> >
> >
> > On Fri, Dec 14, 2012 at 10:08 AM, Scott Cunningham <
> scunningham@google.com>
> > wrote:
> >>
> >> Will unit test be able to verify the behavior of the right-click (or
> >> Ctrl+click) pop-up menu on Apps icons sitting in the Launch Tray of
> Chrome
> >> OS?  That's one of the 'New Incognito Window' cases that I think should
> be
> >> covered.  And could be covered by integration tests.
> >>
> >> -Xot
> >>
> >>
> >> On Fri, Dec 14, 2012 at 9:11 AM, Scott Violet <sky@chromium.org> wrote:
> >>>
> >>> You can still use prefs in unit tests.
> >>>
> >>>   -Scott
> >>>
> >>> On Thu, Dec 13, 2012 at 7:03 PM,  <rustema@google.com> wrote:
> >>> > Reviewers: brettw, sky,
> >>> >
> >>> > Message:
> >>> >
> >>> > On 2012/12/13 22:55:33, sky wrote:
> >>> >>
> >>> >> Why do these tests need to be browser tests and not unit tests?
> >>> >
> >>> >
> >>> > These tests use the pref service which needs a browser process IIRC.
> I
> >>> > haven't
> >>> > been working on chrome for a long time though so if it changed please
> >>> > let me
> >>> > know how I can make them unit tests.
> >>> >
> >>> > Description:
> >>> > Added more tests for IncognitoModeAvailability.
> >>> > The user should not be able to open Incognito windows from link and
> >>> > launcher
> >>> > context menus when the Incognito mode is disabled (e.g. by a policy).
> >>> >
> >>> > This CL should help automate
> >>> >
> http://google-testcasemanager.appspot.com/home#!view=test&ti=16954266
> >>> >
> >>> > BUG=
> >>> >
> >>> > Please review this at https://codereview.chromium.org/11471027/
> >>> >
> >>> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >>> >
> >>> > Affected files:
> >>> >   M
> chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
> >>> >   M chrome/browser/ui/ash/launcher/launcher_context_menu.h
> >>> >   A
> chrome/browser/ui/ash/launcher/launcher_context_menu_browsertest.cc
> >>> >   M chrome/chrome_tests.gypi
> >>> >
> >>> >
> >>
> >>
> >
>

[sky, 2013-01-14 16:03:36]

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_test_util.cc (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_test_util.cc:8:
content::WebContents* web_contents, content::ContextMenuParams params)
same comment agbout each param on its own line.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_test_util.cc:18: int
command_id, ui::Accelerator* accelerator) {
each param on its own line here too.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_test_util.h (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_test_util.h:20: int
command_id, ui::Accelerator* accelerator) OVERRIDE;
when you wrap, each param on its own line.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_test_util.h:22: bool
IsItemPresent(int command_id);
add description.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_unittest.cc (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_unittest.cc:269: private:
nit: newline bewteen 268/269.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_unittest.cc:272: };
DISALLOW_...

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu.h:28: enum MenuItem {
I don't like making these public just for a test, use a friend instead.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:23: public
ChromeLauncherControllerPerBrowser {
indent two more

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:28: virtual
bool IsLoggedInAsGuest() {
OVERRIDE

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:31: };
private: DISALLOW...

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:33: class
LauncherContextMenuBrowserTest : public ChromeRenderViewHostTestHarness {
BrowserTest -> Test

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:44: virtual
void SetUp() {
OVERRIDE where appropriate

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:63: private:
newline between 62/63.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:67: };
DISALLOW...

[rustema, 2013-01-15 07:37:40]

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_test_util.cc (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_test_util.cc:8:
content::WebContents* web_contents, content::ContextMenuParams params)
On 2013/01/14 16:03:37, sky wrote:
> same comment agbout each param on its own line.

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_test_util.cc:18: int
command_id, ui::Accelerator* accelerator) {
On 2013/01/14 16:03:37, sky wrote:
> each param on its own line here too.

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_test_util.h (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_test_util.h:20: int
command_id, ui::Accelerator* accelerator) OVERRIDE;
On 2013/01/14 16:03:37, sky wrote:
> when you wrap, each param on its own line.

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_test_util.h:22: bool
IsItemPresent(int command_id);
On 2013/01/14 16:03:37, sky wrote:
> add description.

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_unittest.cc (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_unittest.cc:269: private:
On 2013/01/14 16:03:37, sky wrote:
> nit: newline bewteen 268/269.

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_unittest.cc:272: };
On 2013/01/14 16:03:37, sky wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu.h:28: enum MenuItem {
On 2013/01/14 16:03:37, sky wrote:
> I don't like making these public just for a test, use a friend instead.

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc (right):

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:23: public
ChromeLauncherControllerPerBrowser {
On 2013/01/14 16:03:37, sky wrote:
> indent two more

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:28: virtual
bool IsLoggedInAsGuest() {
On 2013/01/14 16:03:37, sky wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:31: };
On 2013/01/14 16:03:37, sky wrote:
> private: DISALLOW...

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:33: class
LauncherContextMenuBrowserTest : public ChromeRenderViewHostTestHarness {
On 2013/01/14 16:03:37, sky wrote:
> BrowserTest -> Test

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:44: virtual
void SetUp() {
On 2013/01/14 16:03:37, sky wrote:
> OVERRIDE where appropriate

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:63: private:
On 2013/01/14 16:03:37, sky wrote:
> newline between 62/63.

Done.

https://codereview.chromium.org/11471027/diff/36003/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:67: };
On 2013/01/14 16:03:37, sky wrote:
> DISALLOW...

Done.

[sky, 2013-01-15 16:49:32]

LGTM

[commit-bot: I haz the power, 2013-01-15 20:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rustema@google.com/11471027/32007

[commit-bot: I haz the power, 2013-01-15 22:32:32]

Change committed as 176991