Test suite that monitors the disposition of JS-created windows based on what

Test suite that monitors the disposition of JS-created windows based on what
modifiers were involved in the user gesture that triggered them.

See https://bugs.webkit.org/show_bug.cgi?id=99202 for a patch that needs to land
before this test will fully pass.

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

[ericu, 2012-10-23 00:11:24]

Scott:

	I'd like to get this ready to land, but won't submit it until I get the webkit
side approved and landed.  Could you take a look?

	Thanks,

		Eric

[sky, 2012-10-23 00:53:38]

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
File chrome/browser/click_modifier_browsertest.cc (right):

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
chrome/browser/click_modifier_browsertest.cc:5: #include "base/file_path.h"
At a minimum this file needs to be in chrome/browser/ui (since it's testing the
ui). Additionally since I believe you're testing browser you want this in
browser_browsertest.

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
chrome/browser/click_modifier_browsertest.cc:35: void RunTest(Browser* browser,
int modifiers,
If you can't fit all params on their own line, wrap each to their own line. And
add a description.

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
chrome/browser/click_modifier_browsertest.cc:69: };
 private:
  DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/11235048/diff/7/content/public/test/browser_t...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/11235048/diff/7/content/public/test/browser_t...
content/public/test/browser_test_utils.h:66: void
SimulateModifiedMouseClick(WebContents* web_contents, int modifiers,
each param on its own line.
The name of this should be SimulateMouseClick and convert all callers to pass in
the button.

[ericu, 2012-10-23 18:05:33]

I also removed the popup-blocker suppression; it turned out not to be necessary.

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
File chrome/browser/click_modifier_browsertest.cc (right):

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
chrome/browser/click_modifier_browsertest.cc:5: #include "base/file_path.h"
On 2012/10/23 00:53:38, sky wrote:
> At a minimum this file needs to be in chrome/browser/ui (since it's testing
the
> ui). Additionally since I believe you're testing browser you want this in
> browser_browsertest.

I'd been modeling it after popup_blocker_browsertest.cc, which is in
chrome/browser; moving it into chrome/browser/ui makes sense.

I expect to add more tests for regular link clicks [as opposed to window.open
clicks], so browser_browsertest feels a bit too miscellaneous for this specific
subcategory.  Mind if I leave it separate?

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
chrome/browser/click_modifier_browsertest.cc:35: void RunTest(Browser* browser,
int modifiers,
On 2012/10/23 00:53:38, sky wrote:
> If you can't fit all params on their own line, wrap each to their own line.
And
> add a description.

While writing the description, I realised this could be made clearer and
simpler, so I changed the parameter list as well.

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
chrome/browser/click_modifier_browsertest.cc:69: };
On 2012/10/23 00:53:38, sky wrote:
>  private:
>   DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11235048/diff/7/content/public/test/browser_t...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/11235048/diff/7/content/public/test/browser_t...
content/public/test/browser_test_utils.h:66: void
SimulateModifiedMouseClick(WebContents* web_contents, int modifiers,
On 2012/10/23 00:53:38, sky wrote:
> each param on its own line.
> The name of this should be SimulateMouseClick and convert all callers to pass
in
> the button.

Done.

[sky, 2012-10-23 19:45:01]

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
File chrome/browser/click_modifier_browsertest.cc (right):

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
chrome/browser/click_modifier_browsertest.cc:5: #include "base/file_path.h"
On 2012/10/23 18:05:33, ericu wrote:
> On 2012/10/23 00:53:38, sky wrote:
> > At a minimum this file needs to be in chrome/browser/ui (since it's testing
> the
> > ui). Additionally since I believe you're testing browser you want this in
> > browser_browsertest.
> 
> I'd been modeling it after popup_blocker_browsertest.cc, which is in
> chrome/browser; moving it into chrome/browser/ui makes sense.
> 
> I expect to add more tests for regular link clicks [as opposed to window.open
> clicks], so browser_browsertest feels a bit too miscellaneous for this
specific
> subcategory.  Mind if I leave it separate?

I'm not a fan of test file that do not have a corresponding non test field
associated with them. While I see your point, I think putting this in
browser_browsertest is better.

https://codereview.chromium.org/11235048/diff/6001/chrome/browser/ui/click_mo...
File chrome/browser/ui/click_modifier_browsertest.cc (right):

https://codereview.chromium.org/11235048/diff/6001/chrome/browser/ui/click_mo...
chrome/browser/ui/click_modifier_browsertest.cc:86: private:
newline between 85/86.

[ericu, 2012-10-23 20:09:09]

I decided to disable the shift-click test while I was in there; there's no
reason all the other tests have to wait on my webkit-side patch.

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
File chrome/browser/click_modifier_browsertest.cc (right):

https://codereview.chromium.org/11235048/diff/7/chrome/browser/click_modifier...
chrome/browser/click_modifier_browsertest.cc:5: #include "base/file_path.h"
On 2012/10/23 19:45:01, sky wrote:
> On 2012/10/23 18:05:33, ericu wrote:
> > On 2012/10/23 00:53:38, sky wrote:
> > > At a minimum this file needs to be in chrome/browser/ui (since it's
testing
> > the
> > > ui). Additionally since I believe you're testing browser you want this in
> > > browser_browsertest.
> > 
> > I'd been modeling it after popup_blocker_browsertest.cc, which is in
> > chrome/browser; moving it into chrome/browser/ui makes sense.
> > 
> > I expect to add more tests for regular link clicks [as opposed to
window.open
> > clicks], so browser_browsertest feels a bit too miscellaneous for this
> specific
> > subcategory.  Mind if I leave it separate?
> 
> I'm not a fan of test file that do not have a corresponding non test field
> associated with them. While I see your point, I think putting this in
> browser_browsertest is better.

OK; done.

https://codereview.chromium.org/11235048/diff/6001/chrome/browser/ui/click_mo...
File chrome/browser/ui/click_modifier_browsertest.cc (right):

https://codereview.chromium.org/11235048/diff/6001/chrome/browser/ui/click_mo...
chrome/browser/ui/click_modifier_browsertest.cc:86: private:
On 2012/10/23 19:45:01, sky wrote:
> newline between 85/86.

Done.

[sky, 2012-10-23 23:09:28]

LGTM

https://codereview.chromium.org/11235048/diff/6009/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/11235048/diff/6009/chrome/browser/ui/browser_...
chrome/browser/ui/browser_browsertest.cc:189: static const FilePath::CharType*
kTestDir = FILE_PATH_LITERAL("click_modifier");
Move this close to your tests.

https://codereview.chromium.org/11235048/diff/6009/chrome/browser/ui/browser_...
chrome/browser/ui/browser_browsertest.cc:217: void RunTest(
nit: wrap at the ( and move the first arg up to this line.

https://codereview.chromium.org/11235048/diff/6009/chrome/browser/ui/browser_...
chrome/browser/ui/browser_browsertest.cc:223: 
nit: remove newline.

https://codereview.chromium.org/11235048/diff/6009/content/public/test/browse...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/11235048/diff/6009/content/public/test/browse...
content/public/test/browser_test_utils.cc:226: void
SimulateMouseClick(WebContents* web_contents, int modifiers,
nit: wrap each param to a new line.

[ericu, 2012-10-23 23:15:44]

Thanks; I'll submit once I'm sure I've fixed OSX properly.

https://codereview.chromium.org/11235048/diff/6009/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/11235048/diff/6009/chrome/browser/ui/browser_...
chrome/browser/ui/browser_browsertest.cc:189: static const FilePath::CharType*
kTestDir = FILE_PATH_LITERAL("click_modifier");
On 2012/10/23 23:09:28, sky wrote:
> Move this close to your tests.

Done.

https://codereview.chromium.org/11235048/diff/6009/chrome/browser/ui/browser_...
chrome/browser/ui/browser_browsertest.cc:217: void RunTest(
On 2012/10/23 23:09:28, sky wrote:
> nit: wrap at the ( and move the first arg up to this line.

Done.

https://codereview.chromium.org/11235048/diff/6009/chrome/browser/ui/browser_...
chrome/browser/ui/browser_browsertest.cc:223: 
On 2012/10/23 23:09:28, sky wrote:
> nit: remove newline.

Done.

https://codereview.chromium.org/11235048/diff/6009/content/public/test/browse...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/11235048/diff/6009/content/public/test/browse...
content/public/test/browser_test_utils.cc:226: void
SimulateMouseClick(WebContents* web_contents, int modifiers,
On 2012/10/23 23:09:28, sky wrote:
> nit: wrap each param to a new line.

Done.