Port Moterm to Mozart.

Port Moterm to Mozart.

Much of the glue code has been eliminated by use of the new UI helpers.

BUG=
R=abarth@google.com, viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/5a846ea69f087b0b5938565b6f8e849b53a89788

[jeffbrown, 2015-12-31 11:21:18]

jeffbrown@google.com changed reviewers:
+ abarth@google.com, jamesr@chromium.org, viettrungluu@chromium.org

[abarth, 2016-01-10 03:31:57]

LGTM but you should get trung's sign-off too.

https://codereview.chromium.org/1556683004/diff/1/apps/moterm/moterm_view.cc
File apps/moterm/moterm_view.cc (right):

https://codereview.chromium.org/1556683004/diff/1/apps/moterm/moterm_view.cc#...
apps/moterm/moterm_view.cc:118: ScheduleDraw(false);
Should this be conditional on whether ProcessInput makes any modifications to
the model?

https://codereview.chromium.org/1556683004/diff/1/apps/moterm/moterm_view.cc#...
apps/moterm/moterm_view.cc:214: (!model_state_changes_.IsDirty() && !force &&
!force_next_draw_)) {
Oh, I see.  We check the model here.

[jeffbrown, 2016-01-26 08:33:41]

https://codereview.chromium.org/1556683004/diff/1/apps/moterm/moterm_view.cc
File apps/moterm/moterm_view.cc (right):

https://codereview.chromium.org/1556683004/diff/1/apps/moterm/moterm_view.cc#...
apps/moterm/moterm_view.cc:118: ScheduleDraw(false);
On 2016/01/10 03:31:57, abarth wrote:
> Should this be conditional on whether ProcessInput makes any modifications to
> the model?

Handles below.  I tried not to change the existing code too much in this patch.

https://codereview.chromium.org/1556683004/diff/1/apps/moterm/moterm_view.cc#...
apps/moterm/moterm_view.cc:214: (!model_state_changes_.IsDirty() && !force &&
!force_next_draw_)) {
On 2016/01/10 03:31:57, abarth wrote:
> Oh, I see.  We check the model here.

Acknowledged.

[jeffbrown, 2016-01-26 09:27:40]

address feedback

[viettrungluu, 2016-01-26 18:33:35]

lgtm w/nits

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/main.cc
File apps/moterm/main.cc (right):

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/main.cc#new...
apps/moterm/main.cc:10: #include "base/logging.h"
This include seems unnecessary.

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/main.cc#new...
apps/moterm/main.cc:11: #include "base/macros.h"
"

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/moterm_app.h
File apps/moterm/moterm_app.h (right):

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/moterm_app....
apps/moterm/moterm_app.h:22: DISALLOW_COPY_AND_ASSIGN(MotermApp);
You should include "base/macros.h" for this macro.

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/moterm_view.cc
File apps/moterm/moterm_view.cc (right):

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/moterm_view...
apps/moterm/moterm_view.cc:224: // TODO(vtl): If |!force|, draw only the dirty
region(s)?
Hmmm. This TODO doesn't quite make sense anymore. It's not obvious to me how
this information would be propagated. (Maybe we should track dirty regions, and
"force" would just make everything dirty.)

Anyhow, could you change this to "TODO(vtl): Draw only the dirty region(s)?".

https://codereview.chromium.org/1556683004/diff/20001/examples/moterm_example...
File examples/moterm_example_app/moterm_example_app.cc (right):

https://codereview.chromium.org/1556683004/diff/20001/examples/moterm_example...
examples/moterm_example_app/moterm_example_app.cc:37: static void
Fputs(mojo::files::File* file, const char* s) {
If you're going to bother making this static, then you may as well dump
everything other than MojoMain() inside an anonymous namespace. <shrug>

https://codereview.chromium.org/1556683004/diff/20001/examples/moterm_example...
examples/moterm_example_app/moterm_example_app.cc:48: explicit
MotermExampleAppView(
nit (preexisting condition, admittedly): no explicit required

[jeffbrown, 2016-01-26 23:10:44]

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/main.cc
File apps/moterm/main.cc (right):

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/main.cc#new...
apps/moterm/main.cc:10: #include "base/logging.h"
On 2016/01/26 18:33:35, viettrungluu wrote:
> This include seems unnecessary.

Done.

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/main.cc#new...
apps/moterm/main.cc:11: #include "base/macros.h"
On 2016/01/26 18:33:35, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/moterm_app.h
File apps/moterm/moterm_app.h (right):

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/moterm_app....
apps/moterm/moterm_app.h:22: DISALLOW_COPY_AND_ASSIGN(MotermApp);
On 2016/01/26 18:33:35, viettrungluu wrote:
> You should include "base/macros.h" for this macro.

Done.

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/moterm_view.cc
File apps/moterm/moterm_view.cc (right):

https://codereview.chromium.org/1556683004/diff/20001/apps/moterm/moterm_view...
apps/moterm/moterm_view.cc:224: // TODO(vtl): If |!force|, draw only the dirty
region(s)?
On 2016/01/26 18:33:35, viettrungluu wrote:
> Hmmm. This TODO doesn't quite make sense anymore. It's not obvious to me how
> this information would be propagated. (Maybe we should track dirty regions,
and
> "force" would just make everything dirty.)
> 
> Anyhow, could you change this to "TODO(vtl): Draw only the dirty region(s)?".

Done.

https://codereview.chromium.org/1556683004/diff/20001/examples/moterm_example...
File examples/moterm_example_app/moterm_example_app.cc (right):

https://codereview.chromium.org/1556683004/diff/20001/examples/moterm_example...
examples/moterm_example_app/moterm_example_app.cc:37: static void
Fputs(mojo::files::File* file, const char* s) {
On 2016/01/26 18:33:35, viettrungluu wrote:
> If you're going to bother making this static, then you may as well dump
> everything other than MojoMain() inside an anonymous namespace. <shrug>

Meh, I'll just drop the static.  :)

https://codereview.chromium.org/1556683004/diff/20001/examples/moterm_example...
examples/moterm_example_app/moterm_example_app.cc:48: explicit
MotermExampleAppView(
On 2016/01/26 18:33:35, viettrungluu wrote:
> nit (preexisting condition, admittedly): no explicit required

Done.

[jeffbrown, 2016-01-26 23:53:09]

Description was changed from

==========
Port Moterm to Mozart.

Much of the glue code has been eliminated by use of the new UI helpers.

BUG=
==========

to

==========
Port Moterm to Mozart.

Much of the glue code has been eliminated by use of the new UI helpers.

BUG=
R=abarth@google.com, viettrungluu@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/5a846ea69f087b0b5938565b6f8...
==========

[jeffbrown, 2016-01-26 23:53:13]

Committed patchset #3 (id:40001) manually as
5a846ea69f087b0b5938565b6f8e849b53a89788 (presubmit successful).