mojo: first take on removing mojo_main_{chromium,standalone}.

mojo: first take on removing mojo_main_{chromium,standalone}.

* Adds ApplicationRunner{Chromium} to house common code.
* Makes 'application_{chromium, standalone}' target depend on the appropriate
environment target.  Since application_{c,s} house different Runner types
altogether, linking against the wrong application lib would yield unresolved
externals. linking against a {wrong} environment lib would now result in
duplicately defined symbols. So these changes help crbug.com/334543.

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

[tim (not reviewing), 2014-08-07 01:11:42]

sneak peek. I'm still tinkering with this but it's worth sending for feedback. 
I didn't see a strong reason to inherit from ApplicationImpl or share base
classes for the task of launching, so for the moment ApplicationRunner has-an
ApplicationImpl.

[DaveMoore, 2014-08-07 01:54:33]

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_impl.h (right):

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_impl.h:85: friend MojoResult
(::MojoMain)(MojoHandle);
Can this friend declaration go away?

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_runner.h (right):

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_runner.h:19: void
take_delegate(ApplicationDelegate* delegate);
take_delegate()? Why not set_delegate()? And why is the declaration different
from ApplicationRunnerChromium? And what cases don't have the delegate?

[tim (not reviewing), 2014-08-07 17:13:21]

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_impl.h (right):

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_impl.h:85: friend MojoResult
(::MojoMain)(MojoHandle);
On 2014/08/07 01:54:33, DaveMoore wrote:
> Can this friend declaration go away?

Yep.  We may also be able to do away with all the overloaded ctor and BindShell
variants.

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_runner.h (right):

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_runner.h:19: void
take_delegate(ApplicationDelegate* delegate);
On 2014/08/07 01:54:33, DaveMoore wrote:
> take_delegate()? Why not set_delegate()? And why is the declaration different
> from ApplicationRunnerChromium? And what cases don't have the delegate?

I wanted to convey ownership without being able to have scoped_ptr, except in
ARChromium where I can, which is why the two declarations are different. If this
is more awkward sounding than anything else we can change it to set_() with raw
pointers in both places. But ownership is important, because we want to control
when the different pieces are destroyed relative to each other.

I was originally thinking the delegate would be a constructor parameter, since
all the apps I've come across so far use a delegate. But Darin brought up the
fact that apps that do not vend services do not need a delegate, and that might
be a common case. (discussion @
https://docs.google.com/a/chromium.org/document/d/1Fmf6HLldq8tXQkJyaact68G_NK...
). 

Right now ApplicationImpl isn't really built to handle a null delegate, whether
you are vending services or connecting to services (e.g
ConfigureOutgoingConnection).

[DaveMoore, 2014-08-07 18:12:37]

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_runner.h (right):

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_runner.h:19: void
take_delegate(ApplicationDelegate* delegate);
What I don't like about it is that ARC inherits from AR, so it ends up with both
methods visible to clients. That seems confusing.

On 2014/08/07 17:13:21, timsteele wrote:
> On 2014/08/07 01:54:33, DaveMoore wrote:
> > take_delegate()? Why not set_delegate()? And why is the declaration
different
> > from ApplicationRunnerChromium? And what cases don't have the delegate?
> 
> I wanted to convey ownership without being able to have scoped_ptr, except in
> ARChromium where I can, which is why the two declarations are different. If
this
> is more awkward sounding than anything else we can change it to set_() with
raw
> pointers in both places. But ownership is important, because we want to
control
> when the different pieces are destroyed relative to each other.
> 
> I was originally thinking the delegate would be a constructor parameter, since
> all the apps I've come across so far use a delegate. But Darin brought up the
> fact that apps that do not vend services do not need a delegate, and that
might
> be a common case. (discussion @
>
https://docs.google.com/a/chromium.org/document/d/1Fmf6HLldq8tXQkJyaact68G_NK...
> ). 
> 
> Right now ApplicationImpl isn't really built to handle a null delegate,
whether
> you are vending services or connecting to services (e.g
> ConfigureOutgoingConnection).

[tim (not reviewing), 2014-08-07 18:46:49]

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_runner.h (right):

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_runner.h:19: void
take_delegate(ApplicationDelegate* delegate);
On 2014/08/07 18:12:36, DaveMoore wrote:
> What I don't like about it is that ARC inherits from AR, so it ends up with
both
> methods visible to clients. That seems confusing.
> 
> On 2014/08/07 17:13:21, timsteele wrote:
> > On 2014/08/07 01:54:33, DaveMoore wrote:
> > > take_delegate()? Why not set_delegate()? And why is the declaration
> different
> > > from ApplicationRunnerChromium? And what cases don't have the delegate?
> > 
> > I wanted to convey ownership without being able to have scoped_ptr, except
in
> > ARChromium where I can, which is why the two declarations are different. If
> this
> > is more awkward sounding than anything else we can change it to set_() with
> raw
> > pointers in both places. But ownership is important, because we want to
> control
> > when the different pieces are destroyed relative to each other.
> > 
> > I was originally thinking the delegate would be a constructor parameter,
since
> > all the apps I've come across so far use a delegate. But Darin brought up
the
> > fact that apps that do not vend services do not need a delegate, and that
> might
> > be a common case. (discussion @
> >
>
https://docs.google.com/a/chromium.org/document/d/1Fmf6HLldq8tXQkJyaact68G_NK...
> > ). 
> > 
> > Right now ApplicationImpl isn't really built to handle a null delegate,
> whether
> > you are vending services or connecting to services (e.g
> > ConfigureOutgoingConnection).
> 

Oh, but it doesn't inherit :) I explicitly avoided inheritance between AR, ARC,
and ApplicationImpl because I didn't see a need.  Note that AR and ARC are part
of different targets. You only get the one from the application target you
linked against, which now also implicitly brings in the correct environment_
target.

[DaveMoore, 2014-08-07 18:58:56]

On 2014/08/07 18:46:49, timsteele wrote:
>
https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
> File mojo/public/cpp/application/application_runner.h (right):
> 
>
https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
> mojo/public/cpp/application/application_runner.h:19: void
> take_delegate(ApplicationDelegate* delegate);
> On 2014/08/07 18:12:36, DaveMoore wrote:
> > What I don't like about it is that ARC inherits from AR, so it ends up with
> both
> > methods visible to clients. That seems confusing.
> > 
> > On 2014/08/07 17:13:21, timsteele wrote:
> > > On 2014/08/07 01:54:33, DaveMoore wrote:
> > > > take_delegate()? Why not set_delegate()? And why is the declaration
> > different
> > > > from ApplicationRunnerChromium? And what cases don't have the delegate?
> > > 
> > > I wanted to convey ownership without being able to have scoped_ptr, except
> in
> > > ARChromium where I can, which is why the two declarations are different.
If
> > this
> > > is more awkward sounding than anything else we can change it to set_()
with
> > raw
> > > pointers in both places. But ownership is important, because we want to
> > control
> > > when the different pieces are destroyed relative to each other.
> > > 
> > > I was originally thinking the delegate would be a constructor parameter,
> since
> > > all the apps I've come across so far use a delegate. But Darin brought up
> the
> > > fact that apps that do not vend services do not need a delegate, and that
> > might
> > > be a common case. (discussion @
> > >
> >
>
https://docs.google.com/a/chromium.org/document/d/1Fmf6HLldq8tXQkJyaact68G_NK...
> > > ). 
> > > 
> > > Right now ApplicationImpl isn't really built to handle a null delegate,
> > whether
> > > you are vending services or connecting to services (e.g
> > > ConfigureOutgoingConnection).
> > 
> 
> Oh, but it doesn't inherit :) I explicitly avoided inheritance between AR,
ARC,
> and ApplicationImpl because I didn't see a need.  Note that AR and ARC are
part
> of different targets. You only get the one from the application target you
> linked against, which now also implicitly brings in the correct environment_
> target.

Oh, you're right. I'm sorry. Then different apis is fine.

[darin (slow to review), 2014-08-07 20:08:48]

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_runner.h (right):

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_runner.h:19: void
take_delegate(ApplicationDelegate* delegate);
On 2014/08/07 17:13:21, timsteele wrote:
> On 2014/08/07 01:54:33, DaveMoore wrote:
> > take_delegate()? Why not set_delegate()? And why is the declaration
different
> > from ApplicationRunnerChromium? And what cases don't have the delegate?
> 
> I wanted to convey ownership without being able to have scoped_ptr, except in
> ARChromium where I can, which is why the two declarations are different. If
this
> is more awkward sounding than anything else we can change it to set_() with
raw
> pointers in both places. But ownership is important, because we want to
control
> when the different pieces are destroyed relative to each other.

take_delegate() bothers me too :)

Why do you need to control the lifetime of the ApplicationDelegate? How about we
just say that it has to outlive the ApplicationRunner?

[tim (not reviewing), 2014-08-07 22:05:32]

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_runner.h (right):

https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_runner.h:19: void
take_delegate(ApplicationDelegate* delegate);
On 2014/08/07 20:08:48, darin wrote:
> On 2014/08/07 17:13:21, timsteele wrote:
> > On 2014/08/07 01:54:33, DaveMoore wrote:
> > > take_delegate()? Why not set_delegate()? And why is the declaration
> different
> > > from ApplicationRunnerChromium? And what cases don't have the delegate?
> > 
> > I wanted to convey ownership without being able to have scoped_ptr, except
in
> > ARChromium where I can, which is why the two declarations are different. If
> this
> > is more awkward sounding than anything else we can change it to set_() with
> raw
> > pointers in both places. But ownership is important, because we want to
> control
> > when the different pieces are destroyed relative to each other.
> 
> take_delegate() bothers me too :)
> 
> Why do you need to control the lifetime of the ApplicationDelegate? How about
we
> just say that it has to outlive the ApplicationRunner?

Will rename. I guess I was thinking the ApplicationRunner would outlive the
delegate, and the delegate would be destroyed before things like Environment and
AtExitManager. One Runner per invocation of Run() that blocks until everything
is done and gone seemed simple to me.

I suppose we could add an ApplicationDelegate::Shutdown as the object lifetime
counterpart to ApplicationDelegate::Initialize, and say the delegate outlives
the Runner while having a chance to clean up before the world disappears.

[darin (slow to review), 2014-08-08 03:59:51]

On 2014/08/07 22:05:32, timsteele wrote:
>
https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
> File mojo/public/cpp/application/application_runner.h (right):
> 
>
https://codereview.chromium.org/441853002/diff/40001/mojo/public/cpp/applicat...
> mojo/public/cpp/application/application_runner.h:19: void
> take_delegate(ApplicationDelegate* delegate);
> On 2014/08/07 20:08:48, darin wrote:
> > On 2014/08/07 17:13:21, timsteele wrote:
> > > On 2014/08/07 01:54:33, DaveMoore wrote:
> > > > take_delegate()? Why not set_delegate()? And why is the declaration
> > different
> > > > from ApplicationRunnerChromium? And what cases don't have the delegate?
> > > 
> > > I wanted to convey ownership without being able to have scoped_ptr, except
> in
> > > ARChromium where I can, which is why the two declarations are different.
If
> > this
> > > is more awkward sounding than anything else we can change it to set_()
with
> > raw
> > > pointers in both places. But ownership is important, because we want to
> > control
> > > when the different pieces are destroyed relative to each other.
> > 
> > take_delegate() bothers me too :)
> > 
> > Why do you need to control the lifetime of the ApplicationDelegate? How
about
> we
> > just say that it has to outlive the ApplicationRunner?
> 
> Will rename. I guess I was thinking the ApplicationRunner would outlive the
> delegate, and the delegate would be destroyed before things like Environment
and
> AtExitManager. One Runner per invocation of Run() that blocks until everything
> is done and gone seemed simple to me.

Oh, I hadn't considered Environment and AtExitManager. Those should really
outlive
the ApplicationDelegate. Hmm...


> 
> I suppose we could add an ApplicationDelegate::Shutdown as the object lifetime
> counterpart to ApplicationDelegate::Initialize, and say the delegate outlives
> the Runner while having a chance to clean up before the world disappears.

[darin (slow to review), 2014-08-08 04:04:10]

How about instead of set_delegate or take_delegate, we just provide a variant of
the ApplicationRunner constructor that takes an ApplicationDelegate instance.
The documentation can clarify that this is an ownership transfer. Then, we can
probably write MojoMain functions that look like this in the common case:

extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(MojoHandle shell_handle)
{
  return mojo::ApplicationRunner(new
mojo::examples::WGetApp()).Run(shell_handle);
}

[tim (not reviewing), 2014-08-08 17:43:46]

On 2014/08/08 04:04:10, darin wrote:
> How about instead of set_delegate or take_delegate, we just provide a variant
of
> the ApplicationRunner constructor that takes an ApplicationDelegate instance.
> The documentation can clarify that this is an ownership transfer. Then, we can
> probably write MojoMain functions that look like this in the common case:
> 
> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(MojoHandle
shell_handle)
> {
>   return mojo::ApplicationRunner(new
> mojo::examples::WGetApp()).Run(shell_handle);
> }

I like that (although I've always interpreted the style-guide function
overloading ban to apply to constructors too). I would however like to avoid
having a pile of ctors, especially if we wind up with more optional parameters.
I'd prefer setters. Maybe as we add more we could use a builder-style pattern,
e.g

return ApplicationRunner()
    .set_delegate(new mojo::examples::WGetApp())
    .set_loop_type(base::MessageLoop::TYPE_DEFAULT)
    .Run(shell_handle);

I suppose this is also anti-styleguide :) But for now we don't actually have any
delegate-free apps and we don't have many params, so I can add the delegate as a
ctor param.

[darin (slow to review), 2014-08-08 18:03:06]

We can definitely evolve this again if need be. I'm a fan of the ctor
approach.

-Darin


On Fri, Aug 8, 2014 at 10:43 AM, <tim@chromium.org> wrote:

> On 2014/08/08 04:04:10, darin wrote:
>
>> How about instead of set_delegate or take_delegate, we just provide a
>> variant
>>
> of
>
>> the ApplicationRunner constructor that takes an ApplicationDelegate
>> instance.
>> The documentation can clarify that this is an ownership transfer. Then,
>> we can
>> probably write MojoMain functions that look like this in the common case:
>>
>
>  extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(MojoHandle
>>
> shell_handle)
>
>> {
>>    return mojo::ApplicationRunner(new
>> mojo::examples::WGetApp()).Run(shell_handle);
>> }
>>
>
> I like that (although I've always interpreted the style-guide function
> overloading ban to apply to constructors too). I would however like to
> avoid
> having a pile of ctors, especially if we wind up with more optional
> parameters.
> I'd prefer setters. Maybe as we add more we could use a builder-style
> pattern,
> e.g
>
> return ApplicationRunner()
>     .set_delegate(new mojo::examples::WGetApp())
>     .set_loop_type(base::MessageLoop::TYPE_DEFAULT)
>     .Run(shell_handle);
>
> I suppose this is also anti-styleguide :) But for now we don't actually
> have any
> delegate-free apps and we don't have many params, so I can add the
> delegate as a
> ctor param.
>
> https://codereview.chromium.org/441853002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tim (not reviewing), 2014-08-08 21:50:03]

Now ready for review.

[DaveMoore, 2014-08-08 22:27:17]

https://codereview.chromium.org/441853002/diff/80001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_runner.h (right):

https://codereview.chromium.org/441853002/diff/80001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_runner.h:35: MojoResult Run(MojoHandle
shell_handle);
If there's only one Run() method it should take a ScopedMessagePipeHandle. I
would keep this one as a convenience to call the other, with a comment that the
handle will be owned by the Runner. Same for ApplicationRunnerChromium.

https://codereview.chromium.org/441853002/diff/80001/mojo/services/view_manag...
File mojo/services/view_manager/main.cc (right):

https://codereview.chromium.org/441853002/diff/80001/mojo/services/view_manag...
mojo/services/view_manager/main.cc:49: new
mojo::service::ViewManagerApp)).Run(shell_handle);
Even if it saves a line I think this should read like this:

  mojo::ApplicationRunnerChromium runner(make_scoped_ptr(
      new mojo::LauncherApp));
  return runner.Run(shell_handle);

The Run() call is important and with what you have it gets buried. This of
course goes for all the uses.

In fact after seeing so many calls to make_scoped_delegate I think I would get
rid of it, make the api just take an ApplicationDelegate* and decide if it
outlives the Runner like this:

  mojo::service::ViewManagerApp app;
  return mojo::ApplicationRunnerChromium(&app).Run(shell_handle);

or is deleted by it:

  mojo::service::ViewManagerApp* app = new mojo::service::ViewManagerApp;
  return mojo::ApplicationRunnerChromium(app).Run(shell_handle);

[tim (not reviewing), 2014-08-08 22:59:52]

https://codereview.chromium.org/441853002/diff/80001/mojo/services/view_manag...
File mojo/services/view_manager/main.cc (right):

https://codereview.chromium.org/441853002/diff/80001/mojo/services/view_manag...
mojo/services/view_manager/main.cc:49: new
mojo::service::ViewManagerApp)).Run(shell_handle);
On 2014/08/08 22:27:17, DaveMoore wrote:
> Even if it saves a line I think this should read like this:
> 
>   mojo::ApplicationRunnerChromium runner(make_scoped_ptr(
>       new mojo::LauncherApp));
>   return runner.Run(shell_handle);
> 
> The Run() call is important and with what you have it gets buried. This of
> course goes for all the uses.
> 
> In fact after seeing so many calls to make_scoped_delegate I think I would get
> rid of it, make the api just take an ApplicationDelegate* and decide if it
> outlives the Runner like this:
> 
>   mojo::service::ViewManagerApp app;
>   return mojo::ApplicationRunnerChromium(&app).Run(shell_handle);
> 
> or is deleted by it:
> 
>   mojo::service::ViewManagerApp* app = new mojo::service::ViewManagerApp;
>   return mojo::ApplicationRunnerChromium(app).Run(shell_handle);
> 

The Runner will always own, delete, and outlive the delegate. If we were to make
it a raw pointer, it would be a bug to use it after Run(). The self-documenting
convention used in chrome for this is passing a scoped_ptr. I was tempted to
remove it too, but then I always am because it's always extra characters... :/ 
I figure it's better to just follow the rule, but if you insist we can kill it.

The alternative I proposed earlier to allow the delegate outlive the runner was
to add an ApplicationDelegate::Shutdown and use it as the analog to
Initialize(). We might wind up doing that anyway, but even if we did I (so far)
don't see a reason to allow the delegate to outlive the runner.

[tim (not reviewing), 2014-08-18 23:23:22]

Try this on for size. I removed make_scoped_delegate and seperated out the
construction / Run call to make it easier on the eyes.

[darin (slow to review), 2014-08-18 23:38:18]

LGTM!!

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/composito...
File mojo/examples/compositor_app/compositor_app.cc (right):

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/composito...
mojo/examples/compositor_app/compositor_app.cc:67:
mojo::ApplicationRunnerChromium runner(new mojo::examples::SampleApp());
nit: you seemed to be standardizing on dropping the trailing ()'s. might want to
be consistent.

https://codereview.chromium.org/441853002/diff/100001/mojo/services/network/m...
File mojo/services/network/main.cc (right):

https://codereview.chromium.org/441853002/diff/100001/mojo/services/network/m...
mojo/services/network/main.cc:56:
runner.set_loop_type(base::MessageLoop::TYPE_IO);
nit: spell it out: set_message_loop_type

[DaveMoore, 2014-08-18 23:41:03]

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
File mojo/examples/aura_demo/aura_demo.cc (right):

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
mojo/examples/aura_demo/aura_demo.cc:15: #include
"mojo/public/cpp/application/application_export.h"
Nit: I don't think you need application_impl.h. This goes for pretty much all of
the users of ApplicationRunner...

Also, where is application_export.h?

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
I wonder if we should wrap this with our own MojoMain() that calls something
like ApplicationRunner::CreateRunner() (maybe a better name). This would provide
a few benefits:
1) App writers would get a link time error instead of a runtime one if they
forgot to define one.
2) We could eliminate the need to import application_export.h
3) We would get type checking on the arg
4) The line would be simpler

MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {

[tim (not reviewing), 2014-08-19 00:06:30]

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
File mojo/examples/aura_demo/aura_demo.cc (right):

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
mojo/examples/aura_demo/aura_demo.cc:15: #include
"mojo/public/cpp/application/application_export.h"
On 2014/08/18 23:41:03, DaveMoore wrote:
> Nit: I don't think you need application_impl.h. This goes for pretty much all
of
> the users of ApplicationRunner...
> 
> Also, where is application_export.h?

Ah, yeah. I was removing it from everywhere possible but a bunch of places
actually use ApplicationImpl (most files containing MojoMain also implement the
ApplicationDelegate and many of those make use of |app| passed to Initialize).

This one (and two others) fell through the cracks though. Fixed.  Also added
app_export.h

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/composito...
File mojo/examples/compositor_app/compositor_app.cc (right):

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/composito...
mojo/examples/compositor_app/compositor_app.cc:67:
mojo::ApplicationRunnerChromium runner(new mojo::examples::SampleApp());
On 2014/08/18 23:38:18, darin wrote:
> nit: you seemed to be standardizing on dropping the trailing ()'s. might want
to
> be consistent.

Done.

https://codereview.chromium.org/441853002/diff/100001/mojo/services/network/m...
File mojo/services/network/main.cc (right):

https://codereview.chromium.org/441853002/diff/100001/mojo/services/network/m...
mojo/services/network/main.cc:56:
runner.set_loop_type(base::MessageLoop::TYPE_IO);
On 2014/08/18 23:38:18, darin wrote:
> nit: spell it out: set_message_loop_type

Done.

[tim (not reviewing), 2014-08-19 00:11:06]

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
File mojo/examples/aura_demo/aura_demo.cc (right):

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
On 2014/08/18 23:41:03, DaveMoore wrote:
> I wonder if we should wrap this with our own MojoMain() that calls something
> like ApplicationRunner::CreateRunner() (maybe a better name). This would
provide
> a few benefits:
> 1) App writers would get a link time error instead of a runtime one if they
> forgot to define one.

Where would this MojoMain live?  Right now a nice thing is that you must link
against the proper mojo_application_* target to get the associated
ApplicationRunner{Chromium}, and that brings in the proper environment (unlike
before where this was seperate).  I'd like to keep that coupled.

> 2) We could eliminate the need to import application_export.h
> 3) We would get type checking on the arg
> 4) The line would be simpler
> 
> MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {

[DaveMoore, 2014-08-19 03:47:35]

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
File mojo/examples/aura_demo/aura_demo.cc (right):

https://codereview.chromium.org/441853002/diff/100001/mojo/examples/aura_demo...
mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
How about putting MojoMain() in application_runner.cc and
application_runner_chromium.cc. That's where you would document what you have to
define so it would make sense.


On 2014/08/19 00:11:05, timsteele wrote:
> On 2014/08/18 23:41:03, DaveMoore wrote:
> > I wonder if we should wrap this with our own MojoMain() that calls something
> > like ApplicationRunner::CreateRunner() (maybe a better name). This would
> provide
> > a few benefits:
> > 1) App writers would get a link time error instead of a runtime one if they
> > forgot to define one.
> 
> Where would this MojoMain live?  Right now a nice thing is that you must link
> against the proper mojo_application_* target to get the associated
> ApplicationRunner{Chromium}, and that brings in the proper environment (unlike
> before where this was seperate).  I'd like to keep that coupled.
> 
> > 2) We could eliminate the need to import application_export.h
> > 3) We would get type checking on the arg
> > 4) The line would be simpler
> > 
> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>

[darin (slow to review), 2014-08-19 03:54:02]

Hmm, I really like having people define MojoMain themselves. It takes away
some of the mystery about how Mojo works.
On Aug 18, 2014 8:47 PM, <davemoore@chromium.org> wrote:

>
> https://codereview.chromium.org/441853002/diff/100001/
> mojo/examples/aura_demo/aura_demo.cc
> File mojo/examples/aura_demo/aura_demo.cc (right):
>
> https://codereview.chromium.org/441853002/diff/100001/
> mojo/examples/aura_demo/aura_demo.cc#newcode209
> mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
> How about putting MojoMain() in application_runner.cc and
> application_runner_chromium.cc. That's where you would document what you
> have to define so it would make sense.
>
>
> On 2014/08/19 00:11:05, timsteele wrote:
>
>> On 2014/08/18 23:41:03, DaveMoore wrote:
>> > I wonder if we should wrap this with our own MojoMain() that calls
>>
> something
>
>> > like ApplicationRunner::CreateRunner() (maybe a better name). This
>>
> would
>
>> provide
>> > a few benefits:
>> > 1) App writers would get a link time error instead of a runtime one
>>
> if they
>
>> > forgot to define one.
>>
>
>  Where would this MojoMain live?  Right now a nice thing is that you
>>
> must link
>
>> against the proper mojo_application_* target to get the associated
>> ApplicationRunner{Chromium}, and that brings in the proper environment
>>
> (unlike
>
>> before where this was seperate).  I'd like to keep that coupled.
>>
>
>  > 2) We could eliminate the need to import application_export.h
>> > 3) We would get type checking on the arg
>> > 4) The line would be simpler
>> >
>> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>
>
>
> https://codereview.chromium.org/441853002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2014-08-19 04:43:06]

I don't think this is the most mysterious thing in Mojo :-) And I find it more
mysterious to have a function with no type safety be the thing that's magically
called. 

Plus there's all the ugliness around exporting a C function in a cross platform
way from C++ code. I figure if you're using our Runner you can define our
function instead. Do you really want every single application to have to copy /
paste the line:

extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
    MojoHandle shell_handle) {

when it could be

MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
or
MojoResult ApplicationMain(MojoHandle shell_handle) {
or
MojoResult RunApplication(MojoHandle shell_handle) {

As an alternative maybe we could provide a macro like

MOJO_MAIN() to get the extern C, export and CDECL part right. APPLICATION_EXPORT
isn't really the right thing here anyway. Then maybe it could read

MojoResult MOJO_MAIN(MojoHandle shell_handle) {
}

It doesn't help with the signature. We could roll that in too, but that might be
too mysterious

DEFINE_MOJO_MAIN(shell_handle) {
}

If we did that it should probably go into a different header than
application_export.h. Maybe mojo_main.h.

On 2014/08/19 03:54:02, darin wrote:
> Hmm, I really like having people define MojoMain themselves. It takes away
> some of the mystery about how Mojo works.
> On Aug 18, 2014 8:47 PM, <mailto:davemoore@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/441853002/diff/100001/
> > mojo/examples/aura_demo/aura_demo.cc
> > File mojo/examples/aura_demo/aura_demo.cc (right):
> >
> > https://codereview.chromium.org/441853002/diff/100001/
> > mojo/examples/aura_demo/aura_demo.cc#newcode209
> > mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
> > How about putting MojoMain() in application_runner.cc and
> > application_runner_chromium.cc. That's where you would document what you
> > have to define so it would make sense.
> >
> >
> > On 2014/08/19 00:11:05, timsteele wrote:
> >
> >> On 2014/08/18 23:41:03, DaveMoore wrote:
> >> > I wonder if we should wrap this with our own MojoMain() that calls
> >>
> > something
> >
> >> > like ApplicationRunner::CreateRunner() (maybe a better name). This
> >>
> > would
> >
> >> provide
> >> > a few benefits:
> >> > 1) App writers would get a link time error instead of a runtime one
> >>
> > if they
> >
> >> > forgot to define one.
> >>
> >
> >  Where would this MojoMain live?  Right now a nice thing is that you
> >>
> > must link
> >
> >> against the proper mojo_application_* target to get the associated
> >> ApplicationRunner{Chromium}, and that brings in the proper environment
> >>
> > (unlike
> >
> >> before where this was seperate).  I'd like to keep that coupled.
> >>
> >
> >  > 2) We could eliminate the need to import application_export.h
> >> > 3) We would get type checking on the arg
> >> > 4) The line would be simpler
> >> >
> >> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
> >>
> >
> >
> > https://codereview.chromium.org/441853002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-08-19 05:32:51]

I think it is really nice to tell people they should implement a global
"main"-style function rather than some class method.

I agree it would be nice to avoid all of the decorations on MojoMain if
possible. None of that will make any sense in a NaCl environment anyways,
so perhaps what we should do is define a clean looking MojoMain.

Then we could make our DSO / DLL builds link in a private function that is
exported, which calls MojoMain. Just as we have MojoSetSystemThunks, we
could have a function like MojoCallMain that is exported from the DSO.

Alternatively, if we went the macro route, then it would open up options to
link multiple apps into the same DSO / DLL. We'd have the ability to
generate different function names for their "main" functions at build time
to avoid symbol conflicts at link time. I'm just not a big fan of macros :-/

-Darin


On Mon, Aug 18, 2014 at 9:43 PM, <davemoore@chromium.org> wrote:

> I don't think this is the most mysterious thing in Mojo :-) And I find it
> more
> mysterious to have a function with no type safety be the thing that's
> magically
> called.
>
> Plus there's all the ugliness around exporting a C function in a cross
> platform
> way from C++ code. I figure if you're using our Runner you can define our
> function instead. Do you really want every single application to have to
> copy /
> paste the line:
>
> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
>     MojoHandle shell_handle) {
>
> when it could be
>
> MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
> or
> MojoResult ApplicationMain(MojoHandle shell_handle) {
> or
> MojoResult RunApplication(MojoHandle shell_handle) {
>
> As an alternative maybe we could provide a macro like
>
> MOJO_MAIN() to get the extern C, export and CDECL part right.
> APPLICATION_EXPORT
> isn't really the right thing here anyway. Then maybe it could read
>
> MojoResult MOJO_MAIN(MojoHandle shell_handle) {
> }
>
> It doesn't help with the signature. We could roll that in too, but that
> might be
> too mysterious
>
> DEFINE_MOJO_MAIN(shell_handle) {
> }
>
> If we did that it should probably go into a different header than
> application_export.h. Maybe mojo_main.h.
>
>
> On 2014/08/19 03:54:02, darin wrote:
>
>> Hmm, I really like having people define MojoMain themselves. It takes away
>> some of the mystery about how Mojo works.
>> On Aug 18, 2014 8:47 PM, <mailto:davemoore@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.org/441853002/diff/100001/
>> > mojo/examples/aura_demo/aura_demo.cc
>> > File mojo/examples/aura_demo/aura_demo.cc (right):
>> >
>> > https://codereview.chromium.org/441853002/diff/100001/
>>
>> > mojo/examples/aura_demo/aura_demo.cc#newcode209
>> > mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
>> > How about putting MojoMain() in application_runner.cc and
>> > application_runner_chromium.cc. That's where you would document what
>> you
>> > have to define so it would make sense.
>> >
>> >
>> > On 2014/08/19 00:11:05, timsteele wrote:
>> >
>> >> On 2014/08/18 23:41:03, DaveMoore wrote:
>> >> > I wonder if we should wrap this with our own MojoMain() that calls
>> >>
>> > something
>> >
>> >> > like ApplicationRunner::CreateRunner() (maybe a better name). This
>> >>
>> > would
>> >
>> >> provide
>> >> > a few benefits:
>> >> > 1) App writers would get a link time error instead of a runtime one
>> >>
>> > if they
>> >
>> >> > forgot to define one.
>> >>
>> >
>> >  Where would this MojoMain live?  Right now a nice thing is that you
>> >>
>> > must link
>> >
>> >> against the proper mojo_application_* target to get the associated
>> >> ApplicationRunner{Chromium}, and that brings in the proper environment
>> >>
>> > (unlike
>> >
>> >> before where this was seperate).  I'd like to keep that coupled.
>> >>
>> >
>> >  > 2) We could eliminate the need to import application_export.h
>> >> > 3) We would get type checking on the arg
>> >> > 4) The line would be simpler
>> >> >
>> >> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>> >>
>> >
>> >
>> > https://codereview.chromium.org/441853002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/441853002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tim (not reviewing), 2014-08-19 16:09:18]

On 2014/08/19 05:32:51, darin wrote:
> I think it is really nice to tell people they should implement a global
> "main"-style function rather than some class method.

+1.  As a relative newcomer, writing a main-style function for an app that I was
writing had an incredibly reassuring quality to it. I could at least understand
what was happening at the earliest moments of my app.

> 
> I agree it would be nice to avoid all of the decorations on MojoMain if
> possible. None of that will make any sense in a NaCl environment anyways,
> so perhaps what we should do is define a clean looking MojoMain.
> 
> Then we could make our DSO / DLL builds link in a private function that is
> exported, which calls MojoMain. Just as we have MojoSetSystemThunks, we
> could have a function like MojoCallMain that is exported from the DSO.

So MojoCallMain would have the ugly decorations but be provided by us as part of
the application target? 
How does MojoCallMain find MojoMain? Would it be a static ApplicationRunner
method or something?
(Sorry if I'm missing the obvious here).

> 
> Alternatively, if we went the macro route, then it would open up options to
> link multiple apps into the same DSO / DLL. We'd have the ability to
> generate different function names for their "main" functions at build time
> to avoid symbol conflicts at link time. I'm just not a big fan of macros :-/
> 
> -Darin

[tim (not reviewing), 2014-08-19 16:13:32]

On 2014/08/19 16:09:18, timsteele wrote:
> On 2014/08/19 05:32:51, darin wrote:
> > I think it is really nice to tell people they should implement a global
> > "main"-style function rather than some class method.
> 
> +1.  As a relative newcomer, writing a main-style function for an app that I
was
> writing had an incredibly reassuring quality to it. I could at least
understand
> what was happening at the earliest moments of my app.
> 
> > 
> > I agree it would be nice to avoid all of the decorations on MojoMain if
> > possible. None of that will make any sense in a NaCl environment anyways,
> > so perhaps what we should do is define a clean looking MojoMain.
> > 
> > Then we could make our DSO / DLL builds link in a private function that is
> > exported, which calls MojoMain. Just as we have MojoSetSystemThunks, we
> > could have a function like MojoCallMain that is exported from the DSO.
> 
> So MojoCallMain would have the ugly decorations but be provided by us as part
of
> the application target? 
> How does MojoCallMain find MojoMain? Would it be a static ApplicationRunner
> method or something?
> (Sorry if I'm missing the obvious here).

Oh, I misread this paragraph as an alternative but it wasn't. Oops. 
So it would be a global main, I think I see now.
> 
> > 
> > Alternatively, if we went the macro route, then it would open up options to
> > link multiple apps into the same DSO / DLL. We'd have the ability to
> > generate different function names for their "main" functions at build time
> > to avoid symbol conflicts at link time. I'm just not a big fan of macros :-/
> > 
> > -Darin

[viettrungluu, 2014-08-19 16:30:23]

On Mon, Aug 18, 2014 at 9:43 PM, <davemoore@chromium.org> wrote:

> I don't think this is the most mysterious thing in Mojo :-) And I find it
> more
> mysterious to have a function with no type safety be the thing that's
> magically
> called.
>
> Plus there's all the ugliness around exporting a C function in a cross
> platform
> way from C++ code. I figure if you're using our Runner you can define our
> function instead. Do you really want every single application to have to
> copy /
> paste the line:
>
> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
>     MojoHandle shell_handle) {
>

AFAIK, it suffices to have the decorations in a declaration (and not the
definition), so we could make this easy by adding (the equivalent of)
"extern "C" APPLICATION EXPORT MojoResult CDECL MojoMain(MojoHandle);" to
some header file.


>
> when it could be
>
> MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
> or
> MojoResult ApplicationMain(MojoHandle shell_handle) {
> or
> MojoResult RunApplication(MojoHandle shell_handle) {
>
> As an alternative maybe we could provide a macro like
>
> MOJO_MAIN() to get the extern C, export and CDECL part right.
> APPLICATION_EXPORT
> isn't really the right thing here anyway. Then maybe it could read
>
> MojoResult MOJO_MAIN(MojoHandle shell_handle) {
> }
>
> It doesn't help with the signature. We could roll that in too, but that
> might be
> too mysterious
>
> DEFINE_MOJO_MAIN(shell_handle) {
> }
>
> If we did that it should probably go into a different header than
> application_export.h. Maybe mojo_main.h.
>
> On 2014/08/19 03:54:02, darin wrote:
>
>> Hmm, I really like having people define MojoMain themselves. It takes away
>> some of the mystery about how Mojo works.
>> On Aug 18, 2014 8:47 PM, <mailto:davemoore@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.org/441853002/diff/100001/
>> > mojo/examples/aura_demo/aura_demo.cc
>> > File mojo/examples/aura_demo/aura_demo.cc (right):
>> >
>> > https://codereview.chromium.org/441853002/diff/100001/
>> > mojo/examples/aura_demo/aura_demo.cc#newcode209
>> > mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
>> > How about putting MojoMain() in application_runner.cc and
>> > application_runner_chromium.cc. That's where you would document what
>> you
>> > have to define so it would make sense.
>>
>> >
>> >
>> > On 2014/08/19 00:11:05, timsteele wrote:
>> >
>> >> On 2014/08/18 23:41:03, DaveMoore wrote:
>> >> > I wonder if we should wrap this with our own MojoMain() that calls
>> >>
>> > something
>> >
>> >> > like ApplicationRunner::CreateRunner() (maybe a better name). This
>> >>
>> > would
>> >
>> >> provide
>> >> > a few benefits:
>> >> > 1) App writers would get a link time error instead of a runtime one
>> >>
>> > if they
>> >
>> >> > forgot to define one.
>> >>
>> >
>> >  Where would this MojoMain live?  Right now a nice thing is that you
>> >>
>> > must link
>> >
>> >> against the proper mojo_application_* target to get the associated
>> >> ApplicationRunner{Chromium}, and that brings in the proper environment
>> >>
>> > (unlike
>> >
>> >> before where this was seperate).  I'd like to keep that coupled.
>> >>
>> >
>> >  > 2) We could eliminate the need to import application_export.h
>> >> > 3) We would get type checking on the arg
>> >> > 4) The line would be simpler
>> >> >
>> >> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>> >>
>> >
>> >
>> > https://codereview.chromium.org/441853002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/441853002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-08-19 17:46:52]

On Tue, Aug 19, 2014 at 9:30 AM, Viet-Trung Luu <viettrungluu@chromium.org>
wrote:

> On Mon, Aug 18, 2014 at 9:43 PM, <davemoore@chromium.org> wrote:
>
>> I don't think this is the most mysterious thing in Mojo :-) And I find it
>> more
>> mysterious to have a function with no type safety be the thing that's
>> magically
>> called.
>>
>> Plus there's all the ugliness around exporting a C function in a cross
>> platform
>> way from C++ code. I figure if you're using our Runner you can define our
>> function instead. Do you really want every single application to have to
>> copy /
>> paste the line:
>>
>> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
>>     MojoHandle shell_handle) {
>>
>
> AFAIK, it suffices to have the decorations in a declaration (and not the
> definition), so we could make this easy by adding (the equivalent of)
> "extern "C" APPLICATION EXPORT MojoResult CDECL MojoMain(MojoHandle);" to
> some header file.
>

Oh yeah... that'd be way better. We should totally do that.

-Darin



>
>
>>
>> when it could be
>>
>> MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>> or
>> MojoResult ApplicationMain(MojoHandle shell_handle) {
>> or
>> MojoResult RunApplication(MojoHandle shell_handle) {
>>
>> As an alternative maybe we could provide a macro like
>>
>> MOJO_MAIN() to get the extern C, export and CDECL part right.
>> APPLICATION_EXPORT
>> isn't really the right thing here anyway. Then maybe it could read
>>
>> MojoResult MOJO_MAIN(MojoHandle shell_handle) {
>> }
>>
>> It doesn't help with the signature. We could roll that in too, but that
>> might be
>> too mysterious
>>
>> DEFINE_MOJO_MAIN(shell_handle) {
>> }
>>
>> If we did that it should probably go into a different header than
>> application_export.h. Maybe mojo_main.h.
>>
>> On 2014/08/19 03:54:02, darin wrote:
>>
>>> Hmm, I really like having people define MojoMain themselves. It takes
>>> away
>>> some of the mystery about how Mojo works.
>>> On Aug 18, 2014 8:47 PM, <mailto:davemoore@chromium.org> wrote:
>>>
>>
>>  >
>>> > https://codereview.chromium.org/441853002/diff/100001/
>>> > mojo/examples/aura_demo/aura_demo.cc
>>> > File mojo/examples/aura_demo/aura_demo.cc (right):
>>> >
>>> > https://codereview.chromium.org/441853002/diff/100001/
>>> > mojo/examples/aura_demo/aura_demo.cc#newcode209
>>> > mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
>>> > How about putting MojoMain() in application_runner.cc and
>>> > application_runner_chromium.cc. That's where you would document what
>>> you
>>> > have to define so it would make sense.
>>>
>>> >
>>> >
>>> > On 2014/08/19 00:11:05, timsteele wrote:
>>> >
>>> >> On 2014/08/18 23:41:03, DaveMoore wrote:
>>> >> > I wonder if we should wrap this with our own MojoMain() that calls
>>> >>
>>> > something
>>> >
>>> >> > like ApplicationRunner::CreateRunner() (maybe a better name). This
>>> >>
>>> > would
>>> >
>>> >> provide
>>> >> > a few benefits:
>>> >> > 1) App writers would get a link time error instead of a runtime one
>>> >>
>>> > if they
>>> >
>>> >> > forgot to define one.
>>> >>
>>> >
>>> >  Where would this MojoMain live?  Right now a nice thing is that you
>>> >>
>>> > must link
>>> >
>>> >> against the proper mojo_application_* target to get the associated
>>> >> ApplicationRunner{Chromium}, and that brings in the proper environment
>>> >>
>>> > (unlike
>>> >
>>> >> before where this was seperate).  I'd like to keep that coupled.
>>> >>
>>> >
>>> >  > 2) We could eliminate the need to import application_export.h
>>> >> > 3) We would get type checking on the arg
>>> >> > 4) The line would be simpler
>>> >> >
>>> >> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>> >>
>>> >
>>> >
>>> > https://codereview.chromium.org/441853002/
>>> >
>>>
>>
>>  To unsubscribe from this group and stop receiving emails from it, send an
>>>
>> email
>>
>>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>>
>>
>>
>> https://codereview.chromium.org/441853002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-08-19 17:52:55]

Yeah...that's the right thing to do. Put it in mojo_main.h. It'll be a bit
confusing if you forget to include it but I'm not sure what we can do about
that.


On Tue, Aug 19, 2014 at 10:46 AM, Darin Fisher <darin@chromium.org> wrote:

>
>
>
> On Tue, Aug 19, 2014 at 9:30 AM, Viet-Trung Luu <viettrungluu@chromium.org
> > wrote:
>
>> On Mon, Aug 18, 2014 at 9:43 PM, <davemoore@chromium.org> wrote:
>>
>>> I don't think this is the most mysterious thing in Mojo :-) And I find
>>> it more
>>> mysterious to have a function with no type safety be the thing that's
>>> magically
>>> called.
>>>
>>> Plus there's all the ugliness around exporting a C function in a cross
>>> platform
>>> way from C++ code. I figure if you're using our Runner you can define our
>>> function instead. Do you really want every single application to have to
>>> copy /
>>> paste the line:
>>>
>>> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
>>>     MojoHandle shell_handle) {
>>>
>>
>> AFAIK, it suffices to have the decorations in a declaration (and not the
>> definition), so we could make this easy by adding (the equivalent of)
>> "extern "C" APPLICATION EXPORT MojoResult CDECL MojoMain(MojoHandle);" to
>> some header file.
>>
>
> Oh yeah... that'd be way better. We should totally do that.
>
> -Darin
>
>
>
>>
>>
>>>
>>> when it could be
>>>
>>> MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>> or
>>> MojoResult ApplicationMain(MojoHandle shell_handle) {
>>> or
>>> MojoResult RunApplication(MojoHandle shell_handle) {
>>>
>>> As an alternative maybe we could provide a macro like
>>>
>>> MOJO_MAIN() to get the extern C, export and CDECL part right.
>>> APPLICATION_EXPORT
>>> isn't really the right thing here anyway. Then maybe it could read
>>>
>>> MojoResult MOJO_MAIN(MojoHandle shell_handle) {
>>> }
>>>
>>> It doesn't help with the signature. We could roll that in too, but that
>>> might be
>>> too mysterious
>>>
>>> DEFINE_MOJO_MAIN(shell_handle) {
>>> }
>>>
>>> If we did that it should probably go into a different header than
>>> application_export.h. Maybe mojo_main.h.
>>>
>>> On 2014/08/19 03:54:02, darin wrote:
>>>
>>>> Hmm, I really like having people define MojoMain themselves. It takes
>>>> away
>>>> some of the mystery about how Mojo works.
>>>> On Aug 18, 2014 8:47 PM, <mailto:davemoore@chromium.org> wrote:
>>>>
>>>
>>>  >
>>>> > https://codereview.chromium.org/441853002/diff/100001/
>>>> > mojo/examples/aura_demo/aura_demo.cc
>>>> > File mojo/examples/aura_demo/aura_demo.cc (right):
>>>> >
>>>> > https://codereview.chromium.org/441853002/diff/100001/
>>>> > mojo/examples/aura_demo/aura_demo.cc#newcode209
>>>> > mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
>>>> > How about putting MojoMain() in application_runner.cc and
>>>> > application_runner_chromium.cc. That's where you would document what
>>>> you
>>>> > have to define so it would make sense.
>>>>
>>>> >
>>>> >
>>>> > On 2014/08/19 00:11:05, timsteele wrote:
>>>> >
>>>> >> On 2014/08/18 23:41:03, DaveMoore wrote:
>>>> >> > I wonder if we should wrap this with our own MojoMain() that calls
>>>> >>
>>>> > something
>>>> >
>>>> >> > like ApplicationRunner::CreateRunner() (maybe a better name). This
>>>> >>
>>>> > would
>>>> >
>>>> >> provide
>>>> >> > a few benefits:
>>>> >> > 1) App writers would get a link time error instead of a runtime one
>>>> >>
>>>> > if they
>>>> >
>>>> >> > forgot to define one.
>>>> >>
>>>> >
>>>> >  Where would this MojoMain live?  Right now a nice thing is that you
>>>> >>
>>>> > must link
>>>> >
>>>> >> against the proper mojo_application_* target to get the associated
>>>> >> ApplicationRunner{Chromium}, and that brings in the proper
>>>> environment
>>>> >>
>>>> > (unlike
>>>> >
>>>> >> before where this was seperate).  I'd like to keep that coupled.
>>>> >>
>>>> >
>>>> >  > 2) We could eliminate the need to import application_export.h
>>>> >> > 3) We would get type checking on the arg
>>>> >> > 4) The line would be simpler
>>>> >> >
>>>> >> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>>> >>
>>>> >
>>>> >
>>>> > https://codereview.chromium.org/441853002/
>>>> >
>>>>
>>>
>>>  To unsubscribe from this group and stop receiving emails from it, send
>>>> an
>>>>
>>> email
>>>
>>>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>>>
>>>
>>>
>>> https://codereview.chromium.org/441853002/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2014-08-19 17:54:42]

If we want to make it fail-safe, we could stick it in mojo/c/system/types.h
(which you'd need for MojoHandle, etc.).

On Tue, Aug 19, 2014 at 10:52 AM, David Moore <davemoore@google.com> wrote:

> Yeah...that's the right thing to do. Put it in mojo_main.h. It'll be a bit
> confusing if you forget to include it but I'm not sure what we can do about
> that.
>
>
> On Tue, Aug 19, 2014 at 10:46 AM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>>
>> On Tue, Aug 19, 2014 at 9:30 AM, Viet-Trung Luu <
>> viettrungluu@chromium.org> wrote:
>>
>>> On Mon, Aug 18, 2014 at 9:43 PM, <davemoore@chromium.org> wrote:
>>>
>>>> I don't think this is the most mysterious thing in Mojo :-) And I find
>>>> it more
>>>> mysterious to have a function with no type safety be the thing that's
>>>> magically
>>>> called.
>>>>
>>>> Plus there's all the ugliness around exporting a C function in a cross
>>>> platform
>>>> way from C++ code. I figure if you're using our Runner you can define
>>>> our
>>>> function instead. Do you really want every single application to have
>>>> to copy /
>>>> paste the line:
>>>>
>>>> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
>>>>     MojoHandle shell_handle) {
>>>>
>>>
>>> AFAIK, it suffices to have the decorations in a declaration (and not the
>>> definition), so we could make this easy by adding (the equivalent of)
>>> "extern "C" APPLICATION EXPORT MojoResult CDECL MojoMain(MojoHandle);" to
>>> some header file.
>>>
>>
>> Oh yeah... that'd be way better. We should totally do that.
>>
>> -Darin
>>
>>
>>
>>>
>>>
>>>>
>>>> when it could be
>>>>
>>>> MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>>> or
>>>> MojoResult ApplicationMain(MojoHandle shell_handle) {
>>>> or
>>>> MojoResult RunApplication(MojoHandle shell_handle) {
>>>>
>>>> As an alternative maybe we could provide a macro like
>>>>
>>>> MOJO_MAIN() to get the extern C, export and CDECL part right.
>>>> APPLICATION_EXPORT
>>>> isn't really the right thing here anyway. Then maybe it could read
>>>>
>>>> MojoResult MOJO_MAIN(MojoHandle shell_handle) {
>>>> }
>>>>
>>>> It doesn't help with the signature. We could roll that in too, but that
>>>> might be
>>>> too mysterious
>>>>
>>>> DEFINE_MOJO_MAIN(shell_handle) {
>>>> }
>>>>
>>>> If we did that it should probably go into a different header than
>>>> application_export.h. Maybe mojo_main.h.
>>>>
>>>> On 2014/08/19 03:54:02, darin wrote:
>>>>
>>>>> Hmm, I really like having people define MojoMain themselves. It takes
>>>>> away
>>>>> some of the mystery about how Mojo works.
>>>>> On Aug 18, 2014 8:47 PM, <mailto:davemoore@chromium.org> wrote:
>>>>>
>>>>
>>>>  >
>>>>> > https://codereview.chromium.org/441853002/diff/100001/
>>>>> > mojo/examples/aura_demo/aura_demo.cc
>>>>> > File mojo/examples/aura_demo/aura_demo.cc (right):
>>>>> >
>>>>> > https://codereview.chromium.org/441853002/diff/100001/
>>>>> > mojo/examples/aura_demo/aura_demo.cc#newcode209
>>>>> > mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle) {
>>>>> > How about putting MojoMain() in application_runner.cc and
>>>>> > application_runner_chromium.cc. That's where you would document
>>>>> what you
>>>>> > have to define so it would make sense.
>>>>>
>>>>> >
>>>>> >
>>>>> > On 2014/08/19 00:11:05, timsteele wrote:
>>>>> >
>>>>> >> On 2014/08/18 23:41:03, DaveMoore wrote:
>>>>> >> > I wonder if we should wrap this with our own MojoMain() that calls
>>>>> >>
>>>>> > something
>>>>> >
>>>>> >> > like ApplicationRunner::CreateRunner() (maybe a better name).
>>>>> This
>>>>> >>
>>>>> > would
>>>>> >
>>>>> >> provide
>>>>> >> > a few benefits:
>>>>> >> > 1) App writers would get a link time error instead of a runtime
>>>>> one
>>>>> >>
>>>>> > if they
>>>>> >
>>>>> >> > forgot to define one.
>>>>> >>
>>>>> >
>>>>> >  Where would this MojoMain live?  Right now a nice thing is that you
>>>>> >>
>>>>> > must link
>>>>> >
>>>>> >> against the proper mojo_application_* target to get the associated
>>>>> >> ApplicationRunner{Chromium}, and that brings in the proper
>>>>> environment
>>>>> >>
>>>>> > (unlike
>>>>> >
>>>>> >> before where this was seperate).  I'd like to keep that coupled.
>>>>> >>
>>>>> >
>>>>> >  > 2) We could eliminate the need to import application_export.h
>>>>> >> > 3) We would get type checking on the arg
>>>>> >> > 4) The line would be simpler
>>>>> >> >
>>>>> >> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>>>> >>
>>>>> >
>>>>> >
>>>>> > https://codereview.chromium.org/441853002/
>>>>> >
>>>>>
>>>>
>>>>  To unsubscribe from this group and stop receiving emails from it, send
>>>>> an
>>>>>
>>>> email
>>>>
>>>>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>>>>
>>>>
>>>>
>>>> https://codereview.chromium.org/441853002/
>>>>
>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-08-19 17:59:55]

If we go with mojo/c/system/main.h, then we could include that from
mojo/c/system/core.h? Pretty much everyone should be including core.h,
right?

-Darin


On Tue, Aug 19, 2014 at 10:54 AM, Viet-Trung Luu <viettrungluu@chromium.org>
wrote:

> If we want to make it fail-safe, we could stick it in
> mojo/c/system/types.h (which you'd need for MojoHandle, etc.).
>
>
> On Tue, Aug 19, 2014 at 10:52 AM, David Moore <davemoore@google.com>
> wrote:
>
>> Yeah...that's the right thing to do. Put it in mojo_main.h. It'll be a
>> bit confusing if you forget to include it but I'm not sure what we can do
>> about that.
>>
>>
>> On Tue, Aug 19, 2014 at 10:46 AM, Darin Fisher <darin@chromium.org>
>> wrote:
>>
>>>
>>>
>>>
>>> On Tue, Aug 19, 2014 at 9:30 AM, Viet-Trung Luu <
>>> viettrungluu@chromium.org> wrote:
>>>
>>>> On Mon, Aug 18, 2014 at 9:43 PM, <davemoore@chromium.org> wrote:
>>>>
>>>>> I don't think this is the most mysterious thing in Mojo :-) And I find
>>>>> it more
>>>>> mysterious to have a function with no type safety be the thing that's
>>>>> magically
>>>>> called.
>>>>>
>>>>> Plus there's all the ugliness around exporting a C function in a cross
>>>>> platform
>>>>> way from C++ code. I figure if you're using our Runner you can define
>>>>> our
>>>>> function instead. Do you really want every single application to have
>>>>> to copy /
>>>>> paste the line:
>>>>>
>>>>> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
>>>>>     MojoHandle shell_handle) {
>>>>>
>>>>
>>>> AFAIK, it suffices to have the decorations in a declaration (and not
>>>> the definition), so we could make this easy by adding (the equivalent of)
>>>> "extern "C" APPLICATION EXPORT MojoResult CDECL MojoMain(MojoHandle);" to
>>>> some header file.
>>>>
>>>
>>> Oh yeah... that'd be way better. We should totally do that.
>>>
>>> -Darin
>>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>> when it could be
>>>>>
>>>>> MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>>>> or
>>>>> MojoResult ApplicationMain(MojoHandle shell_handle) {
>>>>> or
>>>>> MojoResult RunApplication(MojoHandle shell_handle) {
>>>>>
>>>>> As an alternative maybe we could provide a macro like
>>>>>
>>>>> MOJO_MAIN() to get the extern C, export and CDECL part right.
>>>>> APPLICATION_EXPORT
>>>>> isn't really the right thing here anyway. Then maybe it could read
>>>>>
>>>>> MojoResult MOJO_MAIN(MojoHandle shell_handle) {
>>>>> }
>>>>>
>>>>> It doesn't help with the signature. We could roll that in too, but
>>>>> that might be
>>>>> too mysterious
>>>>>
>>>>> DEFINE_MOJO_MAIN(shell_handle) {
>>>>> }
>>>>>
>>>>> If we did that it should probably go into a different header than
>>>>> application_export.h. Maybe mojo_main.h.
>>>>>
>>>>> On 2014/08/19 03:54:02, darin wrote:
>>>>>
>>>>>> Hmm, I really like having people define MojoMain themselves. It takes
>>>>>> away
>>>>>> some of the mystery about how Mojo works.
>>>>>> On Aug 18, 2014 8:47 PM, <mailto:davemoore@chromium.org> wrote:
>>>>>>
>>>>>
>>>>>  >
>>>>>> > https://codereview.chromium.org/441853002/diff/100001/
>>>>>> > mojo/examples/aura_demo/aura_demo.cc
>>>>>> > File mojo/examples/aura_demo/aura_demo.cc (right):
>>>>>> >
>>>>>> > https://codereview.chromium.org/441853002/diff/100001/
>>>>>> > mojo/examples/aura_demo/aura_demo.cc#newcode209
>>>>>> > mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle shell_handle)
>>>>>> {
>>>>>> > How about putting MojoMain() in application_runner.cc and
>>>>>> > application_runner_chromium.cc. That's where you would document
>>>>>> what you
>>>>>> > have to define so it would make sense.
>>>>>>
>>>>>> >
>>>>>> >
>>>>>> > On 2014/08/19 00:11:05, timsteele wrote:
>>>>>> >
>>>>>> >> On 2014/08/18 23:41:03, DaveMoore wrote:
>>>>>> >> > I wonder if we should wrap this with our own MojoMain() that
>>>>>> calls
>>>>>> >>
>>>>>> > something
>>>>>> >
>>>>>> >> > like ApplicationRunner::CreateRunner() (maybe a better name).
>>>>>> This
>>>>>> >>
>>>>>> > would
>>>>>> >
>>>>>> >> provide
>>>>>> >> > a few benefits:
>>>>>> >> > 1) App writers would get a link time error instead of a runtime
>>>>>> one
>>>>>> >>
>>>>>> > if they
>>>>>> >
>>>>>> >> > forgot to define one.
>>>>>> >>
>>>>>> >
>>>>>> >  Where would this MojoMain live?  Right now a nice thing is that you
>>>>>> >>
>>>>>> > must link
>>>>>> >
>>>>>> >> against the proper mojo_application_* target to get the associated
>>>>>> >> ApplicationRunner{Chromium}, and that brings in the proper
>>>>>> environment
>>>>>> >>
>>>>>> > (unlike
>>>>>> >
>>>>>> >> before where this was seperate).  I'd like to keep that coupled.
>>>>>> >>
>>>>>> >
>>>>>> >  > 2) We could eliminate the need to import application_export.h
>>>>>> >> > 3) We would get type checking on the arg
>>>>>> >> > 4) The line would be simpler
>>>>>> >> >
>>>>>> >> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>>>>> >>
>>>>>> >
>>>>>> >
>>>>>> > https://codereview.chromium.org/441853002/
>>>>>> >
>>>>>>
>>>>>
>>>>>  To unsubscribe from this group and stop receiving emails from it,
>>>>>> send an
>>>>>>
>>>>> email
>>>>>
>>>>>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>>>>>
>>>>>
>>>>>
>>>>> https://codereview.chromium.org/441853002/
>>>>>
>>>>
>>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-08-19 18:00:15]

(I'm OK with types.h too.)


On Tue, Aug 19, 2014 at 10:59 AM, Darin Fisher <darin@chromium.org> wrote:

> If we go with mojo/c/system/main.h, then we could include that from
> mojo/c/system/core.h? Pretty much everyone should be including core.h,
> right?
>
> -Darin
>
>
> On Tue, Aug 19, 2014 at 10:54 AM, Viet-Trung Luu <
> viettrungluu@chromium.org> wrote:
>
>> If we want to make it fail-safe, we could stick it in
>> mojo/c/system/types.h (which you'd need for MojoHandle, etc.).
>>
>>
>> On Tue, Aug 19, 2014 at 10:52 AM, David Moore <davemoore@google.com>
>> wrote:
>>
>>> Yeah...that's the right thing to do. Put it in mojo_main.h. It'll be a
>>> bit confusing if you forget to include it but I'm not sure what we can do
>>> about that.
>>>
>>>
>>> On Tue, Aug 19, 2014 at 10:46 AM, Darin Fisher <darin@chromium.org>
>>> wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Tue, Aug 19, 2014 at 9:30 AM, Viet-Trung Luu <
>>>> viettrungluu@chromium.org> wrote:
>>>>
>>>>> On Mon, Aug 18, 2014 at 9:43 PM, <davemoore@chromium.org> wrote:
>>>>>
>>>>>> I don't think this is the most mysterious thing in Mojo :-) And I
>>>>>> find it more
>>>>>> mysterious to have a function with no type safety be the thing that's
>>>>>> magically
>>>>>> called.
>>>>>>
>>>>>> Plus there's all the ugliness around exporting a C function in a
>>>>>> cross platform
>>>>>> way from C++ code. I figure if you're using our Runner you can define
>>>>>> our
>>>>>> function instead. Do you really want every single application to have
>>>>>> to copy /
>>>>>> paste the line:
>>>>>>
>>>>>> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
>>>>>>     MojoHandle shell_handle) {
>>>>>>
>>>>>
>>>>> AFAIK, it suffices to have the decorations in a declaration (and not
>>>>> the definition), so we could make this easy by adding (the equivalent of)
>>>>> "extern "C" APPLICATION EXPORT MojoResult CDECL MojoMain(MojoHandle);" to
>>>>> some header file.
>>>>>
>>>>
>>>> Oh yeah... that'd be way better. We should totally do that.
>>>>
>>>> -Darin
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> when it could be
>>>>>>
>>>>>> MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>>>>> or
>>>>>> MojoResult ApplicationMain(MojoHandle shell_handle) {
>>>>>> or
>>>>>> MojoResult RunApplication(MojoHandle shell_handle) {
>>>>>>
>>>>>> As an alternative maybe we could provide a macro like
>>>>>>
>>>>>> MOJO_MAIN() to get the extern C, export and CDECL part right.
>>>>>> APPLICATION_EXPORT
>>>>>> isn't really the right thing here anyway. Then maybe it could read
>>>>>>
>>>>>> MojoResult MOJO_MAIN(MojoHandle shell_handle) {
>>>>>> }
>>>>>>
>>>>>> It doesn't help with the signature. We could roll that in too, but
>>>>>> that might be
>>>>>> too mysterious
>>>>>>
>>>>>> DEFINE_MOJO_MAIN(shell_handle) {
>>>>>> }
>>>>>>
>>>>>> If we did that it should probably go into a different header than
>>>>>> application_export.h. Maybe mojo_main.h.
>>>>>>
>>>>>> On 2014/08/19 03:54:02, darin wrote:
>>>>>>
>>>>>>> Hmm, I really like having people define MojoMain themselves. It
>>>>>>> takes away
>>>>>>> some of the mystery about how Mojo works.
>>>>>>> On Aug 18, 2014 8:47 PM, <mailto:davemoore@chromium.org> wrote:
>>>>>>>
>>>>>>
>>>>>>  >
>>>>>>> > https://codereview.chromium.org/441853002/diff/100001/
>>>>>>> > mojo/examples/aura_demo/aura_demo.cc
>>>>>>> > File mojo/examples/aura_demo/aura_demo.cc (right):
>>>>>>> >
>>>>>>> > https://codereview.chromium.org/441853002/diff/100001/
>>>>>>> > mojo/examples/aura_demo/aura_demo.cc#newcode209
>>>>>>> > mojo/examples/aura_demo/aura_demo.cc:209: MojoHandle
>>>>>>> shell_handle) {
>>>>>>> > How about putting MojoMain() in application_runner.cc and
>>>>>>> > application_runner_chromium.cc. That's where you would document
>>>>>>> what you
>>>>>>> > have to define so it would make sense.
>>>>>>>
>>>>>>> >
>>>>>>> >
>>>>>>> > On 2014/08/19 00:11:05, timsteele wrote:
>>>>>>> >
>>>>>>> >> On 2014/08/18 23:41:03, DaveMoore wrote:
>>>>>>> >> > I wonder if we should wrap this with our own MojoMain() that
>>>>>>> calls
>>>>>>> >>
>>>>>>> > something
>>>>>>> >
>>>>>>> >> > like ApplicationRunner::CreateRunner() (maybe a better name).
>>>>>>> This
>>>>>>> >>
>>>>>>> > would
>>>>>>> >
>>>>>>> >> provide
>>>>>>> >> > a few benefits:
>>>>>>> >> > 1) App writers would get a link time error instead of a runtime
>>>>>>> one
>>>>>>> >>
>>>>>>> > if they
>>>>>>> >
>>>>>>> >> > forgot to define one.
>>>>>>> >>
>>>>>>> >
>>>>>>> >  Where would this MojoMain live?  Right now a nice thing is that
>>>>>>> you
>>>>>>> >>
>>>>>>> > must link
>>>>>>> >
>>>>>>> >> against the proper mojo_application_* target to get the associated
>>>>>>> >> ApplicationRunner{Chromium}, and that brings in the proper
>>>>>>> environment
>>>>>>> >>
>>>>>>> > (unlike
>>>>>>> >
>>>>>>> >> before where this was seperate).  I'd like to keep that coupled.
>>>>>>> >>
>>>>>>> >
>>>>>>> >  > 2) We could eliminate the need to import application_export.h
>>>>>>> >> > 3) We would get type checking on the arg
>>>>>>> >> > 4) The line would be simpler
>>>>>>> >> >
>>>>>>> >> > MojoResult ApplicationRunner::Create(MojoHandle shell_handle) {
>>>>>>> >>
>>>>>>> >
>>>>>>> >
>>>>>>> > https://codereview.chromium.org/441853002/
>>>>>>> >
>>>>>>>
>>>>>>
>>>>>>  To unsubscribe from this group and stop receiving emails from it,
>>>>>>> send an
>>>>>>>
>>>>>> email
>>>>>>
>>>>>>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> https://codereview.chromium.org/441853002/
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tim (not reviewing), 2014-08-19 18:39:40]

On 2014/08/19 17:46:52, darin wrote:
> On Tue, Aug 19, 2014 at 9:30 AM, Viet-Trung Luu
<mailto:viettrungluu@chromium.org>
> wrote:
> 
> > On Mon, Aug 18, 2014 at 9:43 PM, <mailto:davemoore@chromium.org> wrote:
> >
> >> I don't think this is the most mysterious thing in Mojo :-) And I find it
> >> more
> >> mysterious to have a function with no type safety be the thing that's
> >> magically
> >> called.
> >>
> >> Plus there's all the ugliness around exporting a C function in a cross
> >> platform
> >> way from C++ code. I figure if you're using our Runner you can define our
> >> function instead. Do you really want every single application to have to
> >> copy /
> >> paste the line:
> >>
> >> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
> >>     MojoHandle shell_handle) {
> >>
> >
> > AFAIK, it suffices to have the decorations in a declaration (and not the
> > definition), so we could make this easy by adding (the equivalent of)
> > "extern "C" APPLICATION EXPORT MojoResult CDECL MojoMain(MojoHandle);" to
> > some header file.
> >
> 
> Oh yeah... that'd be way better. We should totally do that.

In fact that we already were! But I fell victim to copypastitis and added the
unnecessary decorations everywhere.  Cleaned up in latest patch set.

I think I'll now add a mojo_main header and include it from everywhere and
likely types.h as well.  I'd make a case for application_runner.h, but I guess
we don't need to completely force people to use a Runner vs manually set up the
environment, so something from system/ probably makes more sense.

[viettrungluu, 2014-08-19 19:11:04]

On Tue, Aug 19, 2014 at 11:39 AM, <tim@chromium.org> wrote:

> On 2014/08/19 17:46:52, darin wrote:
>
>> On Tue, Aug 19, 2014 at 9:30 AM, Viet-Trung Luu
>>
> <mailto:viettrungluu@chromium.org>
>
>> wrote:
>>
>
>  > On Mon, Aug 18, 2014 at 9:43 PM, <mailto:davemoore@chromium.org> wrote:
>> >
>> >> I don't think this is the most mysterious thing in Mojo :-) And I find
>> it
>> >> more
>> >> mysterious to have a function with no type safety be the thing that's
>> >> magically
>> >> called.
>> >>
>> >> Plus there's all the ugliness around exporting a C function in a cross
>> >> platform
>> >> way from C++ code. I figure if you're using our Runner you can define
>> our
>> >> function instead. Do you really want every single application to have
>> to
>> >> copy /
>> >> paste the line:
>> >>
>> >> extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain(
>> >>     MojoHandle shell_handle) {
>> >>
>> >
>> > AFAIK, it suffices to have the decorations in a declaration (and not the
>> > definition), so we could make this easy by adding (the equivalent of)
>> > "extern "C" APPLICATION EXPORT MojoResult CDECL MojoMain(MojoHandle);"
>> to
>> > some header file.
>> >
>>
>
>  Oh yeah... that'd be way better. We should totally do that.
>>
>
> In fact that we already were! But I fell victim to copypastitis and added
> the
> unnecessary decorations everywhere.  Cleaned up in latest patch set.
>
> I think I'll now add a mojo_main header and include it from everywhere and
> likely types.h as well.


Please just call it main.h if you do that.


> I'd make a case for application_runner.h, but I guess
> we don't need to completely force people to use a Runner vs manually set
> up the
> environment, so something from system/ probably makes more sense.
>
> https://codereview.chromium.org/441853002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tim (not reviewing), 2014-08-19 22:12:01]

PTAL.  I moved MojoMain from public/cpp/application to public/cpp/system/main.h
(not c/system). Simply because it was under cpp/ before, and afaict only used in
cplusplus contexts (?).  I'm only speculating on the directory structure and
what-should-go-where, though, so please point out what I should be doing if this
doesn't jive.

[darin (slow to review), 2014-08-19 22:24:52]

On 2014/08/19 22:12:01, timsteele wrote:
> PTAL.  I moved MojoMain from public/cpp/application to
public/cpp/system/main.h
> (not c/system). Simply because it was under cpp/ before, and afaict only used
in
> cplusplus contexts (?).  I'm only speculating on the directory structure and
> what-should-go-where, though, so please point out what I should be doing if
this
> doesn't jive.

It's an extern "C" function that defines the ABI. It is usable from a program
written in C. I think it should accordingly live in public/c/system/main.h.

-Darin

[viettrungluu, 2014-08-19 22:30:57]

https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
File mojo/public/cpp/system/main.h (right):

https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
mojo/public/cpp/system/main.h:10: #if defined(WIN32)
I think I'd write this as

#if defined(WIN32)
extern "C" __declspec(dllexport) MojoResult __cdecl MojoMain(...);
#else
...
#endif

rather than having this header "export" a whole bunch of macros for no
particular reason.

But this really should be in the public/c/system. (See functions.h for the
conditionals around the |extern "C"| bits; it's fine/equivalent to declare this
using |extern "C" { ... }| instead -- but the |extern "C"| part should be
omitted if you're actually compiling as C.)

(Maybe it should be part of functions.h. You could also put it directly in
types.h. You can't put it in its own header[*] and have types.h include that,
due to a circular dependency.)

[*] Unless you did something evil like put the #include after the "body" of the
include file.

https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
mojo/public/cpp/system/main.h:20: // DSOs should implement MojoMain directly.
Note that the fancy decorations
You shouldn't mention "DSOs", since the fact that anything is a DSO is a
platform-dependent detail. (E.g., on NaCl, it'd be "executable"/NEXE.)

https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
mojo/public/cpp/system/main.h:27: // See
public/cpp/application/application_runner* for recommended way of
I think you can omit this comment. cpp/system doesn't recommend anything, and
doesn't favor our framework.

[tim (not reviewing), 2014-08-20 18:02:06]

https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
File mojo/public/cpp/system/main.h (right):

https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
mojo/public/cpp/system/main.h:10: #if defined(WIN32)
On 2014/08/19 22:30:57, viettrungluu wrote:
> I think I'd write this as
> 
> #if defined(WIN32)
> extern "C" __declspec(dllexport) MojoResult __cdecl MojoMain(...);
> #else
> ...
> #endif
> 
> rather than having this header "export" a whole bunch of macros for no
> particular reason.
> 
> But this really should be in the public/c/system. (See functions.h for the
> conditionals around the |extern "C"| bits; it's fine/equivalent to declare
this
> using |extern "C" { ... }| instead -- but the |extern "C"| part should be
> omitted if you're actually compiling as C.)
> 
> (Maybe it should be part of functions.h. You could also put it directly in
> types.h. You can't put it in its own header[*] and have types.h include that,
> due to a circular dependency.)
> 
> [*] Unless you did something evil like put the #include after the "body" of
the
> include file.

Thanks for the explanation. I removed the extra exports and moved the
declaration to c/system. MojoMain still feels different to me than stuff in
functions.h/types.h/elsewhere, so I kept it in its own file...

I included it from core.h because core.h is already a catch-all for several
other files.

https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
mojo/public/cpp/system/main.h:20: // DSOs should implement MojoMain directly.
Note that the fancy decorations
On 2014/08/19 22:30:57, viettrungluu wrote:
> You shouldn't mention "DSOs", since the fact that anything is a DSO is a
> platform-dependent detail. (E.g., on NaCl, it'd be "executable"/NEXE.)

Noted. Done.

https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
mojo/public/cpp/system/main.h:27: // See
public/cpp/application/application_runner* for recommended way of
On 2014/08/19 22:30:57, viettrungluu wrote:
> I think you can omit this comment. cpp/system doesn't recommend anything, and
> doesn't favor our framework.

OK, done.

[viettrungluu, 2014-08-20 19:00:54]

On 2014/08/20 18:02:06, timsteele wrote:
>
https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
> File mojo/public/cpp/system/main.h (right):
> 
>
https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
> mojo/public/cpp/system/main.h:10: #if defined(WIN32)
> On 2014/08/19 22:30:57, viettrungluu wrote:
> > I think I'd write this as
> > 
> > #if defined(WIN32)
> > extern "C" __declspec(dllexport) MojoResult __cdecl MojoMain(...);
> > #else
> > ...
> > #endif
> > 
> > rather than having this header "export" a whole bunch of macros for no
> > particular reason.
> > 
> > But this really should be in the public/c/system. (See functions.h for the
> > conditionals around the |extern "C"| bits; it's fine/equivalent to declare
> this
> > using |extern "C" { ... }| instead -- but the |extern "C"| part should be
> > omitted if you're actually compiling as C.)
> > 
> > (Maybe it should be part of functions.h. You could also put it directly in
> > types.h. You can't put it in its own header[*] and have types.h include
that,
> > due to a circular dependency.)
> > 
> > [*] Unless you did something evil like put the #include after the "body" of
> the
> > include file.
> 
> Thanks for the explanation. I removed the extra exports and moved the
> declaration to c/system. MojoMain still feels different to me than stuff in
> functions.h/types.h/elsewhere, so I kept it in its own file...
> 
> I included it from core.h because core.h is already a catch-all for several
> other files.

OK, LGTM.

> 
>
https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
> mojo/public/cpp/system/main.h:20: // DSOs should implement MojoMain directly.
> Note that the fancy decorations
> On 2014/08/19 22:30:57, viettrungluu wrote:
> > You shouldn't mention "DSOs", since the fact that anything is a DSO is a
> > platform-dependent detail. (E.g., on NaCl, it'd be "executable"/NEXE.)
> 
> Noted. Done.
> 
>
https://codereview.chromium.org/441853002/diff/240001/mojo/public/cpp/system/...
> mojo/public/cpp/system/main.h:27: // See
> public/cpp/application/application_runner* for recommended way of
> On 2014/08/19 22:30:57, viettrungluu wrote:
> > I think you can omit this comment. cpp/system doesn't recommend anything,
and
> > doesn't favor our framework.
> 
> OK, done.

[tim (not reviewing), 2014-08-20 20:35:58]

The CQ bit was checked by tim@chromium.org

[tim (not reviewing), 2014-08-20 20:36:26]

The CQ bit was unchecked by tim@chromium.org

[tim (not reviewing), 2014-08-20 20:38:09]

On 2014/08/20 20:36:26, timsteele wrote:
> The CQ bit was unchecked by mailto:tim@chromium.org

Waiting for green try runs then committing.
Assuming Darin / Dave are OK with this now, but feel free to chime in if you'd
like changes now or as a follow up.

[tim (not reviewing), 2014-08-20 20:46:52]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-20 20:47:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/441853002/380001

[commit-bot: I haz the power, 2014-08-20 22:04:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-20 22:08:44]

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

[commit-bot: I haz the power, 2014-08-20 22:08:46]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[yzshen1, 2014-08-20 23:04:25]

https://codereview.chromium.org/441853002/diff/380001/mojo/mojo_public.gypi
File mojo/mojo_public.gypi (right):

https://codereview.chromium.org/441853002/diff/380001/mojo/mojo_public.gypi#n...
mojo/mojo_public.gypi:89:
'public/platform/native/gles2_impl_chcromium_sync_point_thunks.h',
This is probably not intended?

[tim (not reviewing), 2014-08-20 23:26:12]

https://codereview.chromium.org/441853002/diff/380001/mojo/mojo_public.gypi
File mojo/mojo_public.gypi (right):

https://codereview.chromium.org/441853002/diff/380001/mojo/mojo_public.gypi#n...
mojo/mojo_public.gypi:89:
'public/platform/native/gles2_impl_chcromium_sync_point_thunks.h',
On 2014/08/20 23:04:25, yzshen1 wrote:
> This is probably not intended?

oh no! thank you.

[tim (not reviewing), 2014-08-20 23:26:36]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-20 23:27:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/441853002/400001

[commit-bot: I haz the power, 2014-08-21 01:09:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-21 01:12:10]

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

[commit-bot: I haz the power, 2014-08-21 01:12:12]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[tim (not reviewing), 2014-08-21 02:39:10]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-21 02:41:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/441853002/400001

[commit-bot: I haz the power, 2014-08-21 04:10:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-21 10:46:37]

Committed patchset #13 (400001) as 291039