mac: Flush the autorelease pool after making a browser window in browser tests.

mac: Flush the autorelease pool after making a browser window in browser tests.

Failure to do so causes some Chrome NSObjects to not be destroyed until after
the rest of the test infrastructure has been torn down. This causes failures
when running browser tests on Yosemite.

This CL moves the method OpenURLOffTheRecord from the ui_test_utils namespace
into an instance method on InProcessBrowserTest. This allows the method to flush
the autoreleasepool on Macs.

This CL adds the method OpenDevToolsWindow to InProcessBrowserTest. This is a
wrapper around DevToolsWindow::OpenDevToolsWindow that flushes the
autoreleasepool on Macs.

BUG=445495
Committed: https://crrev.com/ff8b5c7ae0273be9b56d64930aaee1f5f13e1eba
Cr-Commit-Position: refs/heads/master@{#338593}

[erikchen, 2015-07-09 01:15:01]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[erikchen, 2015-07-09 01:19:09]

erikchen@chromium.org changed reviewers:
+ rsesek@chromium.org

[erikchen, 2015-07-09 01:19:10]

rsesek: Please review.

[Robert Sesek, 2015-07-09 15:11:46]

https://codereview.chromium.org/1225093003/diff/60001/chrome/test/base/in_pro...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/1225093003/diff/60001/chrome/test/base/in_pro...
chrome/test/base/in_process_browser_test.cc:434: return
ui_test_utils::OpenURLOffTheRecord(profile, url);
For this case, how will callers know to choose the IPBT version of this method
instead of the ui_test_utils one? What about other callers of the UTU version?

[erikchen, 2015-07-10 00:00:48]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-10 00:01:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225093003/80001

[commit-bot: I haz the power, 2015-07-10 00:06:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 00:06:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

(exceeded global retry quota)

[erikchen, 2015-07-10 00:14:41]

https://codereview.chromium.org/1225093003/diff/60001/chrome/test/base/in_pro...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/1225093003/diff/60001/chrome/test/base/in_pro...
chrome/test/base/in_process_browser_test.cc:434: return
ui_test_utils::OpenURLOffTheRecord(profile, url);
On 2015/07/09 15:11:46, Robert Sesek wrote:
> For this case, how will callers know to choose the IPBT version of this method
> instead of the ui_test_utils one? What about other callers of the UTU version?

I removed the UTU version and migrated all calls to that to calls to this
instead.

[Robert Sesek, 2015-07-10 19:15:36]

LGTM but not an OWNER

[erikchen, 2015-07-10 19:16:39]

erikchen@chromium.org changed reviewers:
+ jhawkins@chromium.org

[erikchen, 2015-07-10 19:16:39]

jhawkins: Looking for an OWNER review.

[James Hawkins, 2015-07-10 19:47:48]

On 2015/07/10 19:16:39, erikchen wrote:
> jhawkins: Looking for an OWNER review.

Ping back when the bots are green.

[erikchen, 2015-07-10 20:05:40]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-10 20:06:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225093003/80001

[commit-bot: I haz the power, 2015-07-10 20:19:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 20:19:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[erikchen, 2015-07-10 21:24:05]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[erikchen, 2015-07-10 21:24:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1225093003/#ps100001
(title: "Add missing include.")

[commit-bot: I haz the power, 2015-07-10 21:24:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225093003/100001

[commit-bot: I haz the power, 2015-07-10 22:07:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 22:07:35]

Dry run: This issue passed the CQ dry run.

[erikchen, 2015-07-10 22:12:29]

On 2015/07/10 22:07:35, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

jhawkins: PTAL

[James Hawkins, 2015-07-10 23:01:21]

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.h (right):

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.h:127: void
OpenDevToolsWindow(content::WebContents* web_contents);
The CL description doesn't mention OpenDevToolsWindow. Can you provide more
context?

[erikchen, 2015-07-10 23:12:54]

On 2015/07/10 23:01:21, James Hawkins wrote:
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> File chrome/test/base/in_process_browser_test.h (right):
> 
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> chrome/test/base/in_process_browser_test.h:127: void
> OpenDevToolsWindow(content::WebContents* web_contents);
> The CL description doesn't mention OpenDevToolsWindow. Can you provide more
> context?

jhawkins: I added more text to the CL description.

[James Hawkins, 2015-07-13 18:22:23]

https://codereview.chromium.org/1225093003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_incognito_apitest.cc (right):

https://codereview.chromium.org/1225093003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_incognito_apitest.cc:113:
"/extensions/test_file.html"));
Indentation is off.

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.cc:479: #if defined(OS_MACOSX)
Something doesn't seem right about this change.  I don't think this is the right
'level' to fix this issue.  I'm concerned that we have this same block repeated
in so many places.  I'm also concerned that we are adding so much mac-specific
code here.

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.h (right):

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.h:129: // Similar to
ui_test_utils::OpenURLOffTheRecord, but also flushes the
So we're now going to have two methods that do roughly the same thing just in
different locations?

[erikchen, 2015-07-13 19:39:22]

jhawkins: PTAL

https://codereview.chromium.org/1225093003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_incognito_apitest.cc (right):

https://codereview.chromium.org/1225093003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_incognito_apitest.cc:113:
"/extensions/test_file.html"));
On 2015/07/13 18:22:23, James Hawkins wrote:
> Indentation is off.

That was the output from clang format. I've manually fixed the formatting.

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.cc:479: #if defined(OS_MACOSX)
On 2015/07/13 18:22:23, James Hawkins wrote:
> Something doesn't seem right about this change.  I don't think this is the
right
> 'level' to fix this issue.  I'm concerned that we have this same block
repeated
> in so many places.  I'm also concerned that we are adding so much mac-specific
> code here.

I agree that this solution isn't ideal, although I think the alternatives are
not much better. For more details on the problem, see
https://codereview.chromium.org/1229473002/#msg12.

I will propose some alternative solutions. If you prefer one, let me know. If
you don't prefer any, and don't have any alternative proposals, then we should
have some synchronous communication.

1) Move this logic into Browser::Browser.
  Downside: This logic is only necessary for browser tests, and I dislike
polluting chrome with test-framework logic.

2) Make a file in_process_browser_test_mac.cc. Write Mac versions of these
functions in that file. These functions in this file only get defined on non-Mac
platforms.
  Downside: More files = more complexity. More duplicated code.

3) Make a new anonymous scoped class in this file, which does nothing on
non-Mac, and is equivalent to ScopedNSAutoreleasePool.
  Downside: This saves some code redundancy, but doesn't address your concern of
Mac logic in this class.

If you insist that the current code is not palatable, then I guess I'm learning
towards (1). Depending on semantics, we /could/ consider this problem to be that
Browser::Browser() has side effects on Mac. (Although if we go down this route,
really most code that interacts with AppKit contains possible side effects, and
we may want to systematically wrap all AppKit interactions in scoped autorelease
pools)

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.h (right):

https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.h:129: // Similar to
ui_test_utils::OpenURLOffTheRecord, but also flushes the
On 2015/07/13 18:22:23, James Hawkins wrote:
> So we're now going to have two methods that do roughly the same thing just in
> different locations?

I moved the method here, but forgot to update the comment.
ui_test_utils::OpenURLOffTheRecord no longer exists.

[James Hawkins, 2015-07-13 20:13:38]

On 2015/07/13 19:39:22, erikchen wrote:
> jhawkins: PTAL
> 
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/browser/extensi...
> File chrome/browser/extensions/extension_incognito_apitest.cc (right):
> 
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/browser/extensi...
> chrome/browser/extensions/extension_incognito_apitest.cc:113:
> "/extensions/test_file.html"));
> On 2015/07/13 18:22:23, James Hawkins wrote:
> > Indentation is off.
> 
> That was the output from clang format. I've manually fixed the formatting.
> 
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> File chrome/test/base/in_process_browser_test.cc (right):
> 
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> chrome/test/base/in_process_browser_test.cc:479: #if defined(OS_MACOSX)
> On 2015/07/13 18:22:23, James Hawkins wrote:
> > Something doesn't seem right about this change.  I don't think this is the
> right
> > 'level' to fix this issue.  I'm concerned that we have this same block
> repeated
> > in so many places.  I'm also concerned that we are adding so much
mac-specific
> > code here.
> 
> I agree that this solution isn't ideal, although I think the alternatives are
> not much better. For more details on the problem, see
> https://codereview.chromium.org/1229473002/#msg12.
> 
> I will propose some alternative solutions. If you prefer one, let me know. If
> you don't prefer any, and don't have any alternative proposals, then we should
> have some synchronous communication.
> 
> 1) Move this logic into Browser::Browser.
>   Downside: This logic is only necessary for browser tests, and I dislike
> polluting chrome with test-framework logic.
> 
> 2) Make a file in_process_browser_test_mac.cc. Write Mac versions of these
> functions in that file. These functions in this file only get defined on
non-Mac
> platforms.
>   Downside: More files = more complexity. More duplicated code.
> 
> 3) Make a new anonymous scoped class in this file, which does nothing on
> non-Mac, and is equivalent to ScopedNSAutoreleasePool.
>   Downside: This saves some code redundancy, but doesn't address your concern
of
> Mac logic in this class.
> 
> If you insist that the current code is not palatable, then I guess I'm
learning
> towards (1). Depending on semantics, we /could/ consider this problem to be
that
> Browser::Browser() has side effects on Mac. (Although if we go down this
route,
> really most code that interacts with AppKit contains possible side effects,
and
> we may want to systematically wrap all AppKit interactions in scoped
autorelease
> pools)
> 
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> File chrome/test/base/in_process_browser_test.h (right):
> 
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> chrome/test/base/in_process_browser_test.h:129: // Similar to
> ui_test_utils::OpenURLOffTheRecord, but also flushes the
> On 2015/07/13 18:22:23, James Hawkins wrote:
> > So we're now going to have two methods that do roughly the same thing just
in
> > different locations?
> 
> I moved the method here, but forgot to update the comment.
> ui_test_utils::OpenURLOffTheRecord no longer exists.

Thanks for all of the context.  I heavily prefer option 2), as it's a pretty
standard solution in Chrome for reducing platform-specific complexity in
cross-platform.

[erikchen, 2015-07-13 20:55:42]

Patchset #8 (id:140001) has been deleted

[erikchen, 2015-07-13 21:12:13]

Patchset #8 (id:160001) has been deleted

[erikchen, 2015-07-13 21:12:54]

On 2015/07/13 20:13:38, James Hawkins wrote:
> On 2015/07/13 19:39:22, erikchen wrote:
> > jhawkins: PTAL
> > 
> >
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/browser/extensi...
> > File chrome/browser/extensions/extension_incognito_apitest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/browser/extensi...
> > chrome/browser/extensions/extension_incognito_apitest.cc:113:
> > "/extensions/test_file.html"));
> > On 2015/07/13 18:22:23, James Hawkins wrote:
> > > Indentation is off.
> > 
> > That was the output from clang format. I've manually fixed the formatting.
> > 
> >
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> > File chrome/test/base/in_process_browser_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> > chrome/test/base/in_process_browser_test.cc:479: #if defined(OS_MACOSX)
> > On 2015/07/13 18:22:23, James Hawkins wrote:
> > > Something doesn't seem right about this change.  I don't think this is the
> > right
> > > 'level' to fix this issue.  I'm concerned that we have this same block
> > repeated
> > > in so many places.  I'm also concerned that we are adding so much
> mac-specific
> > > code here.
> > 
> > I agree that this solution isn't ideal, although I think the alternatives
are
> > not much better. For more details on the problem, see
> > https://codereview.chromium.org/1229473002/#msg12.
> > 
> > I will propose some alternative solutions. If you prefer one, let me know.
If
> > you don't prefer any, and don't have any alternative proposals, then we
should
> > have some synchronous communication.
> > 
> > 1) Move this logic into Browser::Browser.
> >   Downside: This logic is only necessary for browser tests, and I dislike
> > polluting chrome with test-framework logic.
> > 
> > 2) Make a file in_process_browser_test_mac.cc. Write Mac versions of these
> > functions in that file. These functions in this file only get defined on
> non-Mac
> > platforms.
> >   Downside: More files = more complexity. More duplicated code.
> > 
> > 3) Make a new anonymous scoped class in this file, which does nothing on
> > non-Mac, and is equivalent to ScopedNSAutoreleasePool.
> >   Downside: This saves some code redundancy, but doesn't address your
concern
> of
> > Mac logic in this class.
> > 
> > If you insist that the current code is not palatable, then I guess I'm
> learning
> > towards (1). Depending on semantics, we /could/ consider this problem to be
> that
> > Browser::Browser() has side effects on Mac. (Although if we go down this
> route,
> > really most code that interacts with AppKit contains possible side effects,
> and
> > we may want to systematically wrap all AppKit interactions in scoped
> autorelease
> > pools)
> > 
> >
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> > File chrome/test/base/in_process_browser_test.h (right):
> > 
> >
>
https://codereview.chromium.org/1225093003/diff/100001/chrome/test/base/in_pr...
> > chrome/test/base/in_process_browser_test.h:129: // Similar to
> > ui_test_utils::OpenURLOffTheRecord, but also flushes the
> > On 2015/07/13 18:22:23, James Hawkins wrote:
> > > So we're now going to have two methods that do roughly the same thing just
> in
> > > different locations?
> > 
> > I moved the method here, but forgot to update the comment.
> > ui_test_utils::OpenURLOffTheRecord no longer exists.
> 
> Thanks for all of the context.  I heavily prefer option 2), as it's a pretty
> standard solution in Chrome for reducing platform-specific complexity in
> cross-platform.

jhawkins: I made a new file in_process_browser_test_mac.cc and moved the
functions with the Mac specific logic into that file.

[James Hawkins, 2015-07-13 21:20:24]

lgtm

[erikchen, 2015-07-13 21:32:41]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-07-13 21:32:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1225093003/#ps180001
(title: "Comments from jhawkins, round two.")

[commit-bot: I haz the power, 2015-07-13 21:33:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225093003/180001

[commit-bot: I haz the power, 2015-07-13 23:41:41]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-07-13 23:43:38]

Patchset 8 (id:??) landed as
https://crrev.com/ff8b5c7ae0273be9b56d64930aaee1f5f13e1eba
Cr-Commit-Position: refs/heads/master@{#338593}