[Mac] Remove content/ CrApplication.

[Mac] Remove content/ CrApplication.

Pull the CrAppProtocol autorelease-pool handling down into 
MessagePumpCrApplication, which is selected at Create() if NSApp 
implements the right protocol. UsingCrApp() allows clients to 
confirm the correct setup (unfortunately, synchronizing NSApp 
initialization and MessagePump::Create() would be intrusive).

Also push CrAppProtocol and CrAppControlProtocol implementation into
BrowserCrApplication, and reparent that class from NSApplication.

Reparent ServiceCrApplication on NSApplication and rename.

Remove CrApplication registration from gpu, plugin, and renderer
mains.

Remove MockCrApp dependency from remoting sample code.

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

[Scott Hess - ex-Googler, 2011-12-02 04:01:39]

I had a more targetted incremental CL:
  http://codereview.chromium.org/8763017/
which concentrates on MessagePumpMac, and I thought wouldn't need content/
changes.  I thought wrong.  So heck with it, here's the entire thing.  This
should be the last change for this, so it would be awesome if you could take the
first shots, rather than having the long watchlist of interested parties arguing
amongst themselves (hi, all!).

[remoting/ crew, your change is truly minor, really.]

[Robert Sesek, 2011-12-02 17:10:17]

What a delightfully CrAppy CL. Just two nits.

Also, I'm trusting you (and codesearch) that it's okay to remove the other ptype
CrApps. It looks like the sending event stuff is only needed in the browser.

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

http://codereview.chromium.org/8771028/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:230: LOG(ERROR) << "NSApp
should be of type " << [[self className] UTF8String]
LOG(FATAL)?

http://codereview.chromium.org/8771028/diff/1/chrome/service/chrome_service_a...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/8771028/diff/1/chrome/service/chrome_service_a...
chrome/service/chrome_service_application_mac.mm:19: -(void)setCloudPrintHandler
{
nit: space after -

[Scott Hess - ex-Googler, 2011-12-02 19:15:24]

OK, that wasn't so bad.  This'll need OWNERS for remoting/, content/ and base/,
jam@ was the one requesting this change, dmaclach@ knows Mac.

And mark@ for base/ and general spiritual advice.

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

http://codereview.chromium.org/8771028/diff/1/chrome/browser/chrome_browser_a...
chrome/browser/chrome_browser_application_mac.mm:230: LOG(ERROR) << "NSApp
should be of type " << [[self className] UTF8String]
On 2011/12/02 17:10:17, rsesek wrote:
> LOG(FATAL)?

My goal was crash-if-debug, but warn-if-release.  I guess the only possible way
that someone could get here in release is if they didn't test some code path, so
crashing would be reasonable ... I guess.

OK, let me argue a different direction.  In the browser executable, not having
the right CrApp stuff can leads to various use-after-free cases, which are bad,
so I'd be willing to wire it as a crash in that case.  In all other ptypes, it's
likely they shouldn't be using this app subclass in the first place, so having
it crash is also reasonable.

Done, for both if() statements.

http://codereview.chromium.org/8771028/diff/1/chrome/service/chrome_service_a...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/8771028/diff/1/chrome/service/chrome_service_a...
chrome/service/chrome_service_application_mac.mm:19: -(void)setCloudPrintHandler
{
On 2011/12/02 17:10:17, rsesek wrote:
> nit: space after -

Done.

Whoa, hold the phone ... can you see any reason at all why this needs to be an
NSApplication in the first place?  I can see why the code previously required a
CrApplication.  OK, ignore me, problem for another day.

http://codereview.chromium.org/8771028/diff/5003/base/message_pump_mac.mm
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/8771028/diff/5003/base/message_pump_mac.mm#new...
base/message_pump_mac.mm:643: [NSApplication sharedApplication];
I wanted to call this out, WRT your comment about not needing CrApp in all the
other ptypes.  This line is here because either way the message pump requires an
NSApp.  So either the executable can arrange for a specific NSApp to be created
before the pump, or it implicitly accepts an NSApplication as appropriate,
forever, no regrets.

I'm adding a comment, here.

http://codereview.chromium.org/8771028/diff/7001/base/message_pump_mac.h
File base/message_pump_mac.h (right):

http://codereview.chromium.org/8771028/diff/7001/base/message_pump_mac.h#newc...
base/message_pump_mac.h:282: static bool IsHandlingSendEvent();
I'm slightly queasy about adding these static methods.  AFAICT, MessagePumpMac
is really being used as a namespace, so I'm following that.  But it kind of
seems like this might make more sense as:

MessagePump* base::mac::CreateMessagePump();
bool base::mac:UsingCrAppMessagePump();
bool base::mac::IsHandlingSendEvent();

[Scott Hess - ex-Googler, 2011-12-02 20:12:51]

David, I indeed had misunderstood something about the service sub-process,
recent change also pulled the NSApp initialization above the message_loop setup.
 Per earlier aside with Robert, I might followup with a change to make
ServiceApplication not an NSApplication, which side-steps the problem entirely,
but not in this CL.

[Robert Sesek, 2011-12-02 20:36:42]

lgtm

http://codereview.chromium.org/8771028/diff/1025/chrome/service/chrome_servic...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/8771028/diff/1025/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:98: NSLog(@"var: %@", var);
Stray NSLog

[jam, 2011-12-02 20:44:28]

content lgtm, thanks for doing this.

please update chrome/browser/DEPS

[dmac, 2011-12-02 20:48:00]

lgtm

[Scott Hess - ex-Googler, 2011-12-02 20:58:13]

Looping in apatrick@ and kbr@, for content/gpu/ OWNERS stamp.  Kinda wishing
PRESUBMIT gave me the set of OWNERS files I needed to satisfy.

http://codereview.chromium.org/8771028/diff/1025/chrome/service/chrome_servic...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/8771028/diff/1025/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:98: NSLog(@"var: %@", var);
On 2011/12/02 20:36:43, rsesek wrote:
> Stray NSLog

Oh, darn, git-commit -a bites again!  Thanks for catching that.

http://codereview.chromium.org/8771028/diff/3024/chrome/browser/DEPS
File chrome/browser/DEPS (left):

http://codereview.chromium.org/8771028/diff/3024/chrome/browser/DEPS#oldcode36
chrome/browser/DEPS:36: "+content/common/chrome_application_mac.h",
DEPS trimmed!

[Wez, 2011-12-02 21:22:10]

http://codereview.chromium.org/8771028/diff/1/base/message_pump_mac.h
File base/message_pump_mac.h (right):

http://codereview.chromium.org/8771028/diff/1/base/message_pump_mac.h#newcode267
base/message_pump_mac.h:267: // CrAppProtocol, creates an instances of
MessagePumpCrApplication.
s/instances/instance

[Ken Russell (switch to Gerrit), 2011-12-02 21:35:36]

CrApp was the best joke name in the code base!

LGTM

[Ken Russell (switch to Gerrit), 2011-12-02 21:36:25]

BTW, please test composited content like poster circle to ensure no regressions
in the GPU process.

[Scott Hess - ex-Googler, 2011-12-03 00:11:29]

On 2011/12/02 21:36:25, kbr wrote:
> BTW, please test composited content like poster circle to ensure no
regressions
> in the GPU process.

Updating with an offline exchange: Tested poster-circle.html and other .html in
that directory, and they looked the same in Chromium.app as my dev-channel
Chrome.app.

[Mark Mentovai, 2011-12-05 19:25:58]

LGTM for CrAppy owner approval.

[apatrick_chromium, 2011-12-05 22:00:27]

LGTM for content/gpu

[commit-bot: I haz the power, 2011-12-06 18:49:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/8771028/3024

[commit-bot: I haz the power, 2011-12-06 18:49:09]

Can't apply patch for file chrome/browser/DEPS.
While running patch -p1 --forward --force;
patching file chrome/browser/DEPS
Hunk #1 FAILED at 33.
1 out of 1 hunk FAILED -- saving rejects to file chrome/browser/DEPS.rej

[commit-bot: I haz the power, 2011-12-06 18:52:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/8771028/13001

[commit-bot: I haz the power, 2011-12-06 23:32:45]

Change committed as 113281