mac: Swizzle out +[NSBundle mainBundle] for in process browser tests.

mac: Swizzle out +[NSBundle mainBundle] for in process browser tests.

The replacement is a partial mock for the real +[NSBundle mainBundle], but with
-[NSBundle bundleIdentifier] stubbed to return a non-nil string. This fixes many
browser_tests and interactive_ui_tests on OSX 10.10.

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

[erikchen, 2015-06-26 18:31:01]

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

[erikchen, 2015-06-26 18:31:02]

rsesek: Please review.

[Robert Sesek, 2015-06-26 22:36:54]

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:168: # deps += [ '../third_party/ocmock/ocmock.gyp:ocmock'
] TODO(GYP)
LOL :(

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

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/in_pro...
chrome/test/base/in_process_browser_test.cc:382: bundle_swizzler_.reset();
Does this need to be done between each test, or can this be done in the
ctor/dtor?

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
File chrome/test/base/scoped_bundle_swizzler_mac.h (right):

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
chrome/test/base/scoped_bundle_swizzler_mac.h:22: class ScopedBundleSwizzlerMac
{
How does this interact with the bundle faking out in base/mac/foundation_util.h?

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
File chrome/test/base/scoped_bundle_swizzler_mac.mm (right):

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
chrome/test/base/scoped_bundle_swizzler_mac.mm:36:
andReturn:@"swizzled.bundle.id"] bundleIdentifier];
Can this use bae::mac::BaseBundleID()?

[erikchen, 2015-06-26 23:14:53]

rsesek: PTAL

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

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/in_pro...
chrome/test/base/in_process_browser_test.cc:382: bundle_swizzler_.reset();
On 2015/06/26 22:36:54, Robert Sesek wrote:
> Does this need to be done between each test, or can this be done in the
> ctor/dtor?

I'm generally prefer to keep all light-weight configuration in test
setup/teardown. That being said, I don't care enough to debate this point.

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
File chrome/test/base/scoped_bundle_swizzler_mac.h (right):

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
chrome/test/base/scoped_bundle_swizzler_mac.h:22: class ScopedBundleSwizzlerMac
{
On 2015/06/26 22:36:54, Robert Sesek wrote:
> How does this interact with the bundle faking out in
base/mac/foundation_util.h?

Bundle faking works like this:
1) All Chrome code calls a Chrome-specific getter to access a bundle.
2) The getter is generally something like:
"""
  if (g_override_framework_bundle)
    return g_override_framework_bundle;
  return [NSBundle mainBundle];
"""

So this causes a behavior change if [NSBundle mainBundle] would have returned a
different bundle ID. Given that this is only used by tests (ensured by the
include of OCMock and DEPs files), this shouldn't be a problem.

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
File chrome/test/base/scoped_bundle_swizzler_mac.mm (right):

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
chrome/test/base/scoped_bundle_swizzler_mac.mm:36:
andReturn:@"swizzled.bundle.id"] bundleIdentifier];
On 2015/06/26 22:36:54, Robert Sesek wrote:
> Can this use bae::mac::BaseBundleID()?

Yup, done.

[Robert Sesek, 2015-06-30 22:21:07]

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
File chrome/test/base/scoped_bundle_swizzler_mac.h (right):

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
chrome/test/base/scoped_bundle_swizzler_mac.h:22: class ScopedBundleSwizzlerMac
{
On 2015/06/26 23:14:53, erikchen wrote:
> On 2015/06/26 22:36:54, Robert Sesek wrote:
> > How does this interact with the bundle faking out in
> base/mac/foundation_util.h?
> 
> Bundle faking works like this:
> 1) All Chrome code calls a Chrome-specific getter to access a bundle.
> 2) The getter is generally something like:
> """
>   if (g_override_framework_bundle)
>     return g_override_framework_bundle;
>   return [NSBundle mainBundle];
> """
> 
> So this causes a behavior change if [NSBundle mainBundle] would have returned
a
> different bundle ID. Given that this is only used by tests (ensured by the
> include of OCMock and DEPs files), this shouldn't be a problem.

OK. Can you add some commentary as to when one would use that versus (or in
conjection with) this?

https://codereview.chromium.org/1211213003/diff/80001/chrome/test/base/scoped...
File chrome/test/base/scoped_bundle_swizzler_mac.mm (right):

https://codereview.chromium.org/1211213003/diff/80001/chrome/test/base/scoped...
chrome/test/base/scoped_bundle_swizzler_mac.mm:37: [NSString
stringWithUTF8String:base::mac::BaseBundleID()];
sys_string_conversions.h is preferred.

[erikchen, 2015-07-06 22:00:06]

rsesek: PTAL

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
File chrome/test/base/scoped_bundle_swizzler_mac.h (right):

https://codereview.chromium.org/1211213003/diff/60001/chrome/test/base/scoped...
chrome/test/base/scoped_bundle_swizzler_mac.h:22: class ScopedBundleSwizzlerMac
{
On 2015/06/30 22:21:07, Robert Sesek wrote:
> On 2015/06/26 23:14:53, erikchen wrote:
> > On 2015/06/26 22:36:54, Robert Sesek wrote:
> > > How does this interact with the bundle faking out in
> > base/mac/foundation_util.h?
> > 
> > Bundle faking works like this:
> > 1) All Chrome code calls a Chrome-specific getter to access a bundle.
> > 2) The getter is generally something like:
> > """
> >   if (g_override_framework_bundle)
> >     return g_override_framework_bundle;
> >   return [NSBundle mainBundle];
> > """
> > 
> > So this causes a behavior change if [NSBundle mainBundle] would have
returned
> a
> > different bundle ID. Given that this is only used by tests (ensured by the
> > include of OCMock and DEPs files), this shouldn't be a problem.
> 
> OK. Can you add some commentary as to when one would use that versus (or in
> conjection with) this?

Done.

https://codereview.chromium.org/1211213003/diff/80001/chrome/test/base/scoped...
File chrome/test/base/scoped_bundle_swizzler_mac.mm (right):

https://codereview.chromium.org/1211213003/diff/80001/chrome/test/base/scoped...
chrome/test/base/scoped_bundle_swizzler_mac.mm:37: [NSString
stringWithUTF8String:base::mac::BaseBundleID()];
On 2015/06/30 22:21:07, Robert Sesek wrote:
> sys_string_conversions.h is preferred.

Done.

[Robert Sesek, 2015-07-07 15:45:07]

LGTM

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

https://codereview.chromium.org/1211213003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.cc:382: 
nit: remove extra blank line now?

[erikchen, 2015-07-07 18:28:14]

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

https://codereview.chromium.org/1211213003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.cc:382: 
On 2015/07/07 15:45:07, Robert Sesek wrote:
> nit: remove extra blank line now?

Done.

[erikchen, 2015-07-07 18:28:21]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-07-07 18:28:22]

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/1211213003/#ps120001
(title: "Remove blank line.")

[commit-bot: I haz the power, 2015-07-07 18:29:50]

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

[commit-bot: I haz the power, 2015-07-07 18:38:16]

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

[commit-bot: I haz the power, 2015-07-07 18:38:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2015-07-07 19:43:46]

erikchen@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[erikchen, 2015-07-07 19:43:46]

phajdan.jr: Looking for an OWNER review.

[Paweł Hajdan Jr., 2015-07-08 10:01:13]

Thanks for the patch!

https://codereview.chromium.org/1211213003/diff/120001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1211213003/diff/120001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:168: # deps += [ '../third_party/ocmock/ocmock.gyp:ocmock'
] TODO(GYP)
Why does this line say TODO(GYP)? - and why is it commented out?

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

https://codereview.chromium.org/1211213003/diff/120001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.h:230:
scoped_ptr<ScopedBundleSwizzlerMac> bundle_swizzler_;
Wouldn't the same apply to say content_browsertests?

I wonder why not make the change in content::BrowserTestBase instead.

[erikchen, 2015-07-08 17:48:38]

phajdan: PTAL

https://codereview.chromium.org/1211213003/diff/120001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1211213003/diff/120001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:168: # deps += [ '../third_party/ocmock/ocmock.gyp:ocmock'
] TODO(GYP)
On 2015/07/08 10:01:13, Paweł Hajdan Jr. wrote:
> Why does this line say TODO(GYP)? - and why is it commented out?

Mac doesn't build with GN yet. This copies line 904, which shows a couple of
other dependencies which also aren't GN-ified yet.

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

https://codereview.chromium.org/1211213003/diff/120001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.h:230:
scoped_ptr<ScopedBundleSwizzlerMac> bundle_swizzler_;
On 2015/07/08 10:01:13, Paweł Hajdan Jr. wrote:
> Wouldn't the same apply to say content_browsertests?
> 
> I wonder why not make the change in content::BrowserTestBase instead.

I don't believe so.

If you look at content_browsertests on the FYI waterfall, there's only 1
test-suite that fails, and it appears to be related to AppKit. 
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.10/buil...

I'm guessing that content_browsertests has sufficiently few dependencies on
AppKit that the swizzler is not necessary.

[erikchen, 2015-07-08 22:39:26]

On 2015/07/08 17:48:38, erikchen wrote:
> phajdan: PTAL
> 
> https://codereview.chromium.org/1211213003/diff/120001/chrome/test/BUILD.gn
> File chrome/test/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1211213003/diff/120001/chrome/test/BUILD.gn#n...
> chrome/test/BUILD.gn:168: # deps += [
'../third_party/ocmock/ocmock.gyp:ocmock'
> ] TODO(GYP)
> On 2015/07/08 10:01:13, Paweł Hajdan Jr. wrote:
> > Why does this line say TODO(GYP)? - and why is it commented out?
> 
> Mac doesn't build with GN yet. This copies line 904, which shows a couple of
> other dependencies which also aren't GN-ified yet.
> 
>
https://codereview.chromium.org/1211213003/diff/120001/chrome/test/base/in_pr...
> File chrome/test/base/in_process_browser_test.h (right):
> 
>
https://codereview.chromium.org/1211213003/diff/120001/chrome/test/base/in_pr...
> chrome/test/base/in_process_browser_test.h:230:
> scoped_ptr<ScopedBundleSwizzlerMac> bundle_swizzler_;
> On 2015/07/08 10:01:13, Paweł Hajdan Jr. wrote:
> > Wouldn't the same apply to say content_browsertests?
> > 
> > I wonder why not make the change in content::BrowserTestBase instead.
> 
> I don't believe so.
> 
> If you look at content_browsertests on the FYI waterfall, there's only 1
> test-suite that fails, and it appears to be related to AppKit. 
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.10/buil...
> 
> I'm guessing that content_browsertests has sufficiently few dependencies on
> AppKit that the swizzler is not necessary.

typo*: and it appears to be related to Accessibility.

[Paweł Hajdan Jr., 2015-07-09 08:33:36]

LGTM

[erikchen, 2015-07-09 17:54:00]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-07-09 17:55:49]

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

[commit-bot: I haz the power, 2015-07-09 18:58:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-09 19:00:08]

Patchset 7 (id:??) landed as
https://crrev.com/67995392313280c583e6936f267898cb13ab0db2
Cr-Commit-Position: refs/heads/master@{#338105}