Mandoline: Remove native_viewport.mojom

Mandoline: Remove native_viewport.mojom

native_viewport mojom + app add a lot of unnecessary complexity. For now, this should be a part of the view_manager. A future patch will get rid of native_viewport completely and will teach display_manager to dispense platform_windows.

BUG=487881
Test=internal change only.

Committed: https://crrev.com/3a4eab3f2f4fd8f7c7eb4d195c0e86a47e085d98
Cr-Commit-Position: refs/heads/master@{#333183}

[Fady Samuel, 2015-06-04 21:43:49]

fsamuel@chromium.org changed reviewers:
+ sky@chromium.org

[Fady Samuel, 2015-06-04 22:02:03]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-04 22:03:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/20001

[commit-bot: I haz the power, 2015-06-04 22:10:06]

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

[commit-bot: I haz the power, 2015-06-04 22:10:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Fady Samuel, 2015-06-04 22:18:18]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-04 22:19:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/40001

[Fady Samuel, 2015-06-04 22:22:55]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-04 22:24:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/60001

[sky, 2015-06-04 22:37:58]

I don't see you actually removing native_viewport.mojom in this patch.

https://codereview.chromium.org/1149083010/diff/60001/components/view_manager...
File components/view_manager/event_dispatcher.cc (right):

https://codereview.chromium.org/1149083010/diff/60001/components/view_manager...
components/view_manager/event_dispatcher.cc:34: const mojo::Callback<void()>&
callback) {
I don't think you need callback anymore. I'm ok with a TODO if you want to do a
cleanup pass later.

https://codereview.chromium.org/1149083010/diff/60001/components/view_manager...
File components/view_manager/native_viewport/native_viewport.cc (right):

https://codereview.chromium.org/1149083010/diff/60001/components/view_manager...
components/view_manager/native_viewport/native_viewport.cc:117:
mojo::Callback<void()> callback;
Alot of this code can be cleaned up too.

https://codereview.chromium.org/1149083010/diff/60001/components/view_manager...
File components/view_manager/native_viewport/native_viewport.h (right):

https://codereview.chromium.org/1149083010/diff/60001/components/view_manager...
components/view_manager/native_viewport/native_viewport.h:34: EventDispatcher*
event_dispatcher,
I would prefer there to still be an interface and not have NativeViewport
directly use EventDispatcher. In fact it's probably best to have an interface
for all the things NV calls out for:
. event dispatch.
. destroyed.
. metrics.

https://codereview.chromium.org/1149083010/diff/60001/components/view_manager...
File components/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1149083010/diff/60001/components/view_manager...
components/view_manager/public/interfaces/view_manager.mojom:15: Size size;
size_in_pixels (to make it clear).

[commit-bot: I haz the power, 2015-06-04 22:39:01]

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

[commit-bot: I haz the power, 2015-06-04 22:39:02]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Fady Samuel, 2015-06-05 15:30:18]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 15:31:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/80001

[commit-bot: I haz the power, 2015-06-05 16:41:57]

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

[commit-bot: I haz the power, 2015-06-05 16:41:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2015-06-05 20:22:42]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 20:24:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/100001

[Fady Samuel, 2015-06-05 20:25:48]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 20:27:30]

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

[commit-bot: I haz the power, 2015-06-05 21:02:37]

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

[commit-bot: I haz the power, 2015-06-05 21:02:38]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2015-06-05 22:14:20]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[Fady Samuel, 2015-06-05 22:14:31]

PTAL Scott!

https://chromiumcodereview.appspot.com/1149083010/diff/60001/components/view_...
File components/view_manager/event_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/1149083010/diff/60001/components/view_...
components/view_manager/event_dispatcher.cc:34: const mojo::Callback<void()>&
callback) {
On 2015/06/04 22:37:57, sky wrote:
> I don't think you need callback anymore. I'm ok with a TODO if you want to do
a
> cleanup pass later.

Done.

https://chromiumcodereview.appspot.com/1149083010/diff/60001/components/view_...
File components/view_manager/native_viewport/native_viewport.cc (right):

https://chromiumcodereview.appspot.com/1149083010/diff/60001/components/view_...
components/view_manager/native_viewport/native_viewport.cc:117:
mojo::Callback<void()> callback;
On 2015/06/04 22:37:58, sky wrote:
> Alot of this code can be cleaned up too.

Done.

https://chromiumcodereview.appspot.com/1149083010/diff/60001/components/view_...
File components/view_manager/native_viewport/native_viewport.h (right):

https://chromiumcodereview.appspot.com/1149083010/diff/60001/components/view_...
components/view_manager/native_viewport/native_viewport.h:34: EventDispatcher*
event_dispatcher,
On 2015/06/04 22:37:58, sky wrote:
> I would prefer there to still be an interface and not have NativeViewport
> directly use EventDispatcher. In fact it's probably best to have an interface
> for all the things NV calls out for:
> . event dispatch.
> . destroyed.
> . metrics.

Done.

https://chromiumcodereview.appspot.com/1149083010/diff/60001/components/view_...
File components/view_manager/public/interfaces/view_manager.mojom (right):

https://chromiumcodereview.appspot.com/1149083010/diff/60001/components/view_...
components/view_manager/public/interfaces/view_manager.mojom:15: Size size;
On 2015/06/04 22:37:58, sky wrote:
> size_in_pixels (to make it clear).

Done.

[commit-bot: I haz the power, 2015-06-05 22:15:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/180001

[commit-bot: I haz the power, 2015-06-05 22:25:25]

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

[commit-bot: I haz the power, 2015-06-05 22:25:26]

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

[Fady Samuel, 2015-06-05 22:29:58]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 22:31:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/80002

[Fady Samuel, 2015-06-05 22:33:39]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 22:34:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/210001

[commit-bot: I haz the power, 2015-06-05 22:43:04]

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

[commit-bot: I haz the power, 2015-06-05 22:43:05]

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

[Fady Samuel, 2015-06-05 23:09:32]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 23:10:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083010/230001

[commit-bot: I haz the power, 2015-06-05 23:18:34]

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

[commit-bot: I haz the power, 2015-06-05 23:18:36]

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

[sky, 2015-06-05 23:25:51]

LGTM with the following changes.

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
File components/view_manager/display_manager.cc (right):

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
components/view_manager/display_manager.cc:108: 
nit: one newline is enough here.

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
components/view_manager/display_manager.cc:176: if (!event_dispatcher_)
Do we ever pass in null?

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
components/view_manager/display_manager.cc:184: float device_scale_factor) {
It seems like we may get here and the metrics haven't changed. Since this class
caches them could you early out if they haven't changed? Or as NativeViewport
does some caching make it cache it all and only invoke when truly changed.

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
File components/view_manager/event_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
components/view_manager/event_dispatcher.cc:48: 
IMO this relates to the previous code, so no newline is necessary.

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
File components/view_manager/native_viewport/native_viewport.h (right):

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
components/view_manager/native_viewport/native_viewport.h:30: virtual
~Delegate() {}
nit: protected

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
File components/view_manager/view_manager_app.cc (right):

https://chromiumcodereview.appspot.com/1149083010/diff/210001/components/view...
components/view_manager/view_manager_app.cc:52: if (!gpu_state_.get())
Why the check here? Isn't this the only place that sets gpu_state_? I'm ok with
a DCHECK (unless I'm missing something).

[Fady Samuel, 2015-06-05 23:34:52]

Addressed comments. CQ'ing.

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
File components/view_manager/display_manager.cc (right):

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
components/view_manager/display_manager.cc:108: 
On 2015/06/05 23:25:50, sky wrote:
> nit: one newline is enough here.

Done.

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
components/view_manager/display_manager.cc:176: if (!event_dispatcher_)
On 2015/06/05 23:25:50, sky wrote:
> Do we ever pass in null?

No, removed the check.

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
components/view_manager/display_manager.cc:184: float device_scale_factor) {
On 2015/06/05 23:25:50, sky wrote:
> It seems like we may get here and the metrics haven't changed. Since this
class
> caches them could you early out if they haven't changed? Or as NativeViewport
> does some caching make it cache it all and only invoke when truly changed.

Done.

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
File components/view_manager/event_dispatcher.cc (right):

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
components/view_manager/event_dispatcher.cc:48: 
On 2015/06/05 23:25:50, sky wrote:
> IMO this relates to the previous code, so no newline is necessary.

Done.

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
File components/view_manager/native_viewport/native_viewport.h (right):

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
components/view_manager/native_viewport/native_viewport.h:30: virtual
~Delegate() {}
On 2015/06/05 23:25:50, sky wrote:
> nit: protected

Native viewport wasn't doing much so I just got rid of it.

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
File components/view_manager/view_manager_app.cc (right):

https://codereview.chromium.org/1149083010/diff/210001/components/view_manage...
components/view_manager/view_manager_app.cc:52: if (!gpu_state_.get())
On 2015/06/05 23:25:50, sky wrote:
> Why the check here? Isn't this the only place that sets gpu_state_? I'm ok
with
> a DCHECK (unless I'm missing something).

No it's not. GpuImpl service also needs a gpu_state.

[Fady Samuel, 2015-06-05 23:34:58]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-06-05 23:34:59]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1149083010/#ps250001
(title: "Addressed nits")

[commit-bot: I haz the power, 2015-06-05 23:36:31]

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

[commit-bot: I haz the power, 2015-06-06 00:16:53]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-06-06 00:17:58]

Patchset 14 (id:??) landed as
https://crrev.com/3a4eab3f2f4fd8f7c7eb4d195c0e86a47e085d98
Cr-Commit-Position: refs/heads/master@{#333183}

[henrika (OOO until Aug 14), 2015-06-08 06:56:42]

A revert of this CL (patchset #14 id:250001) has been created in
https://codereview.chromium.org/1164553004/ by henrika@chromium.org.

The reason for reverting is: This patch seems to break GN bots on Windows. See
e.g. http://build.chromium.org/p/chromium.win/builders/Win8%20GN/builds/8167.