[Mac] Move ScopedSendingEvent from content/common/mac to base/mac.

[Mac] Move ScopedSendingEvent from content/common/mac to base/mac.

Also merge content/ MockCrControlApp into base/ MockCrApp.
Also use MockCrApp in test_shell_tests, and slight tweak to autorelease pool in test_shell's initialization.

BUG=102224
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112578

[Scott Hess - ex-Googler, 2011-11-29 20:42:59]

jam@ as the implicated content/common/OWNERS rep.

rsesek@ for all that the offline thread isn't going anywhere, this feels pretty
reasonable to me.  The prior separation of function between base/ and content/
(previously chrome/) didn't really make any sense, either, at least this way the
bound-together warts are in more-or-less the same place.

[Robert Sesek, 2011-11-29 22:41:55]

http://codereview.chromium.org/8724004/diff/1/base/test/mock_chrome_applicati...
File base/test/mock_chrome_application_mac.h (right):

http://codereview.chromium.org/8724004/diff/1/base/test/mock_chrome_applicati...
base/test/mock_chrome_application_mac.h:20: @interface MockCrApp :
NSApplication<CrAppProtocol,CrAppControlProtocol> {
nit: break after ',' and align the 'C's.

http://codereview.chromium.org/8724004/diff/1/chrome/browser/chrome_browser_a...
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/8724004/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:372:
base::mac::ScopedSendingEvent sendingEventScoper;
IWYU?

http://codereview.chromium.org/8724004/diff/1/content/test/content_test_suite.cc
File content/test/content_test_suite.cc (left):

http://codereview.chromium.org/8724004/diff/1/content/test/content_test_suite...
content/test/content_test_suite.cc:71: mock_cr_app::RegisterMockCrControlApp();
This test suite doesn't require this? What about unit_tests for chrome/?

[jam, 2011-11-29 23:20:19]

lgtm, thanks for doing this

so will chrome_application_mac.h be moved elsewhere in a separate patch?

[Scott Hess - ex-Googler, 2011-11-29 23:22:58]

http://codereview.chromium.org/8724004/diff/1/base/test/mock_chrome_applicati...
File base/test/mock_chrome_application_mac.h (right):

http://codereview.chromium.org/8724004/diff/1/base/test/mock_chrome_applicati...
base/test/mock_chrome_application_mac.h:20: @interface MockCrApp :
NSApplication<CrAppProtocol,CrAppControlProtocol> {
On 2011/11/29 22:41:56, rsesek wrote:
> nit: break after ',' and align the 'C's.

Done.

Actually ... CrAppControlProtocol includes CrAppProcotol.  So I technically
don't need both, but I figured it was better to be explicit.  So if you have
firm opinions on that, I would be easily swayed.

http://codereview.chromium.org/8724004/diff/1/chrome/browser/chrome_browser_a...
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/8724004/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:372:
base::mac::ScopedSendingEvent sendingEventScoper;
On 2011/11/29 22:41:56, rsesek wrote:
> IWYU?

This class inherits from a class which implements the protocol which
ScopedSendingEvent depends on.  Still IWYU?  It would otherwise be reasonable to
use AutoReset to accomplish things (ScopedSendingEvent was originally
implemented co-resident with the superclass, so abstraction-wise it made sense
to be indirect).

http://codereview.chromium.org/8724004/diff/1/content/test/content_test_suite.cc
File content/test/content_test_suite.cc (left):

http://codereview.chromium.org/8724004/diff/1/content/test/content_test_suite...
content/test/content_test_suite.cc:71: mock_cr_app::RegisterMockCrControlApp();
On 2011/11/29 22:41:56, rsesek wrote:
> This test suite doesn't require this? What about unit_tests for chrome/?

TestSuite::Initialize() calls RegisterMockCrApp().  ChromeTestSuite registers
BrowserCrApplication before calling to this initializer (which is why
+[MockCrApp sharedApplication] checks protocol adherence not class inheritance).

I'm not entirely convinced that the autorelease pool is still necessary, but I'm
also not entirely convinced I can think of a way to convince myself it isn't
necessary.

[Scott Hess - ex-Googler, 2011-11-29 23:25:12]

On Tue, Nov 29, 2011 at 3:20 PM,  <jam@chromium.org> wrote:
> lgtm, thanks for doing this
>
> so will chrome_application_mac.h be moved elsewhere in a separate patch?

Yeah, I'm an incrementalist with my CLs.  Also, that CL materially
touches message_pump_mac* initialization, whereas this one is really
just about where to put things.

> http://codereview.chromium.org/8724004/

[jam, 2011-11-29 23:29:01]

On 2011/11/29 23:25:12, shess wrote:
> On Tue, Nov 29, 2011 at 3:20 PM,  <mailto:jam@chromium.org> wrote:
> > lgtm, thanks for doing this
> >
> > so will chrome_application_mac.h be moved elsewhere in a separate patch?
> 
> Yeah, I'm an incrementalist with my CLs.  Also, that CL materially
> touches message_pump_mac* initialization, whereas this one is really
> just about where to put things.
> 
> > http://codereview.chromium.org/8724004/

sgtm. please update chrome/browser/DEPS to remove the scoped_sending_event.h
entry

[Scott Hess - ex-Googler, 2011-11-30 00:32:58]

On 2011/11/29 23:29:01, John Abd-El-Malek wrote:
> sgtm. please update chrome/browser/DEPS to remove the scoped_sending_event.h
> entry

done.

[Robert Sesek, 2011-11-30 15:29:44]

lgtm

http://codereview.chromium.org/8724004/diff/1/base/test/mock_chrome_applicati...
File base/test/mock_chrome_application_mac.h (right):

http://codereview.chromium.org/8724004/diff/1/base/test/mock_chrome_applicati...
base/test/mock_chrome_application_mac.h:20: @interface MockCrApp :
NSApplication<CrAppProtocol,CrAppControlProtocol> {
On 2011/11/29 23:22:58, shess wrote:
> On 2011/11/29 22:41:56, rsesek wrote:
> > nit: break after ',' and align the 'C's.
> 
> Done.
> 
> Actually ... CrAppControlProtocol includes CrAppProcotol.  So I technically
> don't need both, but I figured it was better to be explicit.  So if you have
> firm opinions on that, I would be easily swayed.

I like the explicitness.

[Scott Hess - ex-Googler, 2011-11-30 17:19:54]

On 2011/11/30 15:29:44, rsesek wrote:
> lgtm

In case things go awry and some sheriff person ends up here ...

There is a presubmit rule WRT ObjC @implementation in base/.  Since this CL
changes an @implementation in base/test/, it fails presubmit, so I can't land
via commit queue.

This change should only affect the Mac build.  The Win try fail looks to be way
unrelated (probably flake).  The DEPS change added in the last patch is what
messed up linux and win_sync, but I believe that the change has good coverage
from the bots where it did succeed (failing to patch even when given a specific
svn revision seems like a problem with the bot, to me).

[Scott Hess - ex-Googler, 2011-11-30 17:30:30]

[Damn.  It.  OWNERS, how does it work?]

Mark, Nico, I need a base/ OWNERS approval for moving ScopedSendingEvent from
content/ to base/, along with folding in the test framework support involved.

[Nico, 2011-11-30 18:04:13]

lgtm stamp.

Thanks for working on this!

http://codereview.chromium.org/8724004/diff/7001/base/test/mock_chrome_applic...
File base/test/mock_chrome_application_mac.mm (left):

http://codereview.chromium.org/8724004/diff/7001/base/test/mock_chrome_applic...
base/test/mock_chrome_application_mac.mm:21: // this class is used by remoting/
for a non-test use.
Is this still true?

[Scott Hess - ex-Googler, 2011-12-01 19:28:51]

Unfortunately, the original landing of this was reverted by hayato@ because it
was breaking test_shell.  Really?  The trybots were green ... digging deeper,
test_shell_tests was crashing (http://crbug.com/106066 to track that).

darin@ and tony@, you're here as webkit/OWNERS.  The new implementation of
MockCrApp is a superset of the test_shell implementation CrApplication.  Test
shell already contains MockCrApp by virtue of using the base/ gtest framework.

Rest of you, I re-opened the CL because the fix doesn't impact any of the
existing review.  Call me on it if you think that's just not cricket.

http://codereview.chromium.org/8724004/diff/7001/base/test/mock_chrome_applic...
File base/test/mock_chrome_application_mac.mm (left):

http://codereview.chromium.org/8724004/diff/7001/base/test/mock_chrome_applic...
base/test/mock_chrome_application_mac.mm:21: // this class is used by remoting/
for a non-test use.
On 2011/11/30 18:04:13, Nico wrote:
> Is this still true? 

It's still being used by remoting/, but I'm becoming less convinced about the
desirability of restricting it to gtest-only.  DRT and test_shell have an almost
identical class, and it seems somewhat reasonable to me to allow test-oriented
clients of message_pump_mac to pull this in even outside of a gtest context.

In practice it's not a huge issue, even with ASSERT_TRUE you'd fail so many
tests that it would be impractical to fix it with test-level annotations, so
you'd have to roll something back.

[Mark Mentovai, 2011-12-01 19:32:30]

LGTM for OWNERS approval wherever you need it.

[tony, 2011-12-01 19:35:56]

LGTM stamp for webkit/tools/test_shell/

[Scott Hess - ex-Googler, 2011-12-01 20:36:39]

On 2011/12/01 19:35:56, tony wrote:
> LGTM stamp for webkit/tools/test_shell/

Sigh.  webkit_unit_tests also affected.  I'll be moving forward betting it's
DRT?

[Scott Hess - ex-Googler, 2011-12-01 22:00:12]

On 2011/12/01 20:36:39, shess wrote:
> On 2011/12/01 19:35:56, tony wrote:
> > LGTM stamp for webkit/tools/test_shell/
> 
> Sigh.  webkit_unit_tests also affected.  I'll be moving forward betting it's
> DRT?

OK, so webkit_unit_tests works when converted to MockCrApp, but DumpRenderTree
does not link because it doesn't include the base/ testing framework.  I believe
that neither TestShell nor DumpRenderTree (nor webkit_unit_tests) really need to
have the MessagePumpMac handling in question, so at some point it should be
possible to not need any custom NSApp in this code.  So, for now I've posted a
change which adds CrAppControlProtocol to CrDrtApplication, which will fix the
build breakage, without adding a bunch of sketchy new dependencies.

[dvyukov, 2011-12-08 12:11:01]

http://codereview.chromium.org/8724004/diff/20006/content/content_tests.gypi
File content/content_tests.gypi (left):

http://codereview.chromium.org/8724004/diff/20006/content/content_tests.gypi#...
content/content_tests.gypi:231: 'common/gamepad_seqlock_unittest.cc',
What is the reason to remove gamepad_seqlock_unittest.cc?
I am going to add it back in http://codereview.chromium.org/8772004/

[Scott Hess - ex-Googler, 2011-12-08 14:29:45]

http://codereview.chromium.org/8724004/diff/20006/content/content_tests.gypi
File content/content_tests.gypi (left):

http://codereview.chromium.org/8724004/diff/20006/content/content_tests.gypi#...
content/content_tests.gypi:231: 'common/gamepad_seqlock_unittest.cc',
On 2011/12/08 12:11:01, dvyukov wrote:
> What is the reason to remove gamepad_seqlock_unittest.cc?
> I am going to add it back in http://codereview.chromium.org/8772004/

Holy Broken Merge, Batman!  I'm sorry about that - it was entirely
unintentional.  Let me know if you want me to push it back in before the other
CL.

[dvyukov, 2011-12-08 14:37:33]

On 2011/12/08 14:29:45, shess wrote:
> http://codereview.chromium.org/8724004/diff/20006/content/content_tests.gypi
> File content/content_tests.gypi (left):
> 
>
http://codereview.chromium.org/8724004/diff/20006/content/content_tests.gypi#...
> content/content_tests.gypi:231: 'common/gamepad_seqlock_unittest.cc',
> On 2011/12/08 12:11:01, dvyukov wrote:
> > What is the reason to remove gamepad_seqlock_unittest.cc?
> > I am going to add it back in http://codereview.chromium.org/8772004/
> 
> Holy Broken Merge, Batman!  I'm sorry about that - it was entirely
> unintentional.  Let me know if you want me to push it back in before the other
> CL.

No problem, I've already included it in my CL that I am going to submit soon...
I just need to fix that mess that merge done with my CL :)