Convert view manager to surfaces with uploading shim in client lib

Convert view manager to surfaces with uploading shim in client lib

This converts the view manager protocol over to specify view contents via
surfaces instead of shuffling a bitmap. To ease the transition, this provides a
wrapper in the client library that accepts an SkBitmap and uploads it into a
texture wrapped in a surface so existing code still works. Next I'll port the
clients over to providing surfaces themselves and remove this shim.

BUG=

Committed: https://crrev.com/529c759bf153b8eec2178244ddd3e87e5790b7c4
Cr-Commit-Position: refs/heads/master@{#294501}

[jamesr, 2014-09-03 22:06:55]

jamesr@chromium.org changed reviewers:
+ ben@chromium.org, sky@chromium.org

[jamesr, 2014-09-03 22:06:56]

This converts the view manager service completely over to using surfaces for
rendering and adds a shim on the client side to upload a SetViewContents() call
into a surface.

known issues:

- red and blue channels are flipped (most noticable in html_viewer)
- everything seems to be shifted up by the height of the titlebar. this only
impacts graphics, hit testing happens on the true bounds

[sky, 2014-09-03 22:45:29]

https://codereview.chromium.org/534843002/diff/20001/mojo/mojo_services.gypi
File mojo/mojo_services.gypi (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/mojo_services.gypi#...
mojo/mojo_services.gypi:721: '../ui/aura/aura.gyp:aura',
You should be able to remove some of these depdendencies now. skia, aura and
maybe more.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/connection_manager.cc (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/connection_manager.cc:40:
root_->SetBounds(gfx::Rect(800, 600));
Can we move this to the native viewport service? The native viewport service
should be the one imposing the initial size. Especially important for devices
where we want fullscreen.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/display_manager.cc (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/display_manager.cc:29: void DrawViewTree(Pass* pass,
const ServerView* view, gfx::Point offset) {
const gfx::Point& offset.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/display_manager.cc:72: bounds_(800, 600),
Same comment about size here.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/display_manager.cc:91:
dirty_rect_.Union(ConvertRectToRoot(view, bounds));
This should do nothing if view (and all its ancestors) aren't visible.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/display_manager.cc:140:
connection_manager_->root()->SetBounds(bounds_);
Doesn't this need to propagate bounds to bounds_?

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/server_view_delegate.h (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/server_view_delegate.h:34: virtual void
OnViewSurfaceChanged(const ServerView* view) = 0;
nit: OnViewSurfaceIdChanged.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_init_service_context.h (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_init_service_context.h:8: #include <set>
nit: shouldn't be needed anymore.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_init_service_context.h:11: #include
"base/memory/scoped_vector.h"
nit: shouldn't be needed anymore.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:15: #include
"third_party/skia/include/core/SkBitmap.h"
nit: remove

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:16: #include
"ui/gfx/codec/png_codec.h"
nit: remove.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:406: if (!view ||
!access_policy_->CanSetViewContents(view)) {
CanSetViewContents->CanSetViewSurfaceId

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.h (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.h:27: 
nit: nuke.

[jamesr, 2014-09-03 23:32:48]

Will try to fix up the bounds a bit, but it's weird since the initial bounds are
picked by the native viewport client which sets the size but the native viewport
client can then later get resize events from the native viewport itself.  The
old code happened to set the root's bounds when the native viewport initialized
itself since that's when the WindowTreeHostImpl could spin up, but now the view
manager is ready to go sooner.

https://codereview.chromium.org/534843002/diff/20001/mojo/mojo_services.gypi
File mojo/mojo_services.gypi (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/mojo_services.gypi#...
mojo/mojo_services.gypi:721: '../ui/aura/aura.gyp:aura',
On 2014/09/03 22:45:28, sky wrote:
> You should be able to remove some of these depdendencies now. skia, aura and
> maybe more.

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/connection_manager.cc (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/connection_manager.cc:40:
root_->SetBounds(gfx::Rect(800, 600));
On 2014/09/03 22:45:28, sky wrote:
> Can we move this to the native viewport service? The native viewport service
> should be the one imposing the initial size. Especially important for devices
> where we want fullscreen.

The way NVS works today is the client picks the size that the native viewport is
set to initially, so we have to pick.

Setting the root's bounds here is pretty weird.  I'll see if it can be better

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/display_manager.cc (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/display_manager.cc:29: void DrawViewTree(Pass* pass,
const ServerView* view, gfx::Point offset) {
On 2014/09/03 22:45:28, sky wrote:
> const gfx::Point& offset.

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/display_manager.cc:72: bounds_(800, 600),
On 2014/09/03 22:45:28, sky wrote:
> Same comment about size here.

We're passing the bounds to NativeViewport in the Create() call

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/display_manager.cc:91:
dirty_rect_.Union(ConvertRectToRoot(view, bounds));
On 2014/09/03 22:45:28, sky wrote:
> This should do nothing if view (and all its ancestors) aren't visible.

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/display_manager.cc:140:
connection_manager_->root()->SetBounds(bounds_);
On 2014/09/03 22:45:28, sky wrote:
> Doesn't this need to propagate bounds to bounds_?

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/server_view_delegate.h (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/server_view_delegate.h:34: virtual void
OnViewSurfaceChanged(const ServerView* view) = 0;
On 2014/09/03 22:45:28, sky wrote:
> nit: OnViewSurfaceIdChanged.

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_init_service_context.h (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_init_service_context.h:8: #include <set>
On 2014/09/03 22:45:28, sky wrote:
> nit: shouldn't be needed anymore.

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_init_service_context.h:11: #include
"base/memory/scoped_vector.h"
On 2014/09/03 22:45:28, sky wrote:
> nit: shouldn't be needed anymore.

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:15: #include
"third_party/skia/include/core/SkBitmap.h"
On 2014/09/03 22:45:28, sky wrote:
> nit: remove

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:16: #include
"ui/gfx/codec/png_codec.h"
On 2014/09/03 22:45:28, sky wrote:
> nit: remove.

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:406: if (!view ||
!access_policy_->CanSetViewContents(view)) {
On 2014/09/03 22:45:28, sky wrote:
> CanSetViewContents->CanSetViewSurfaceId

Done.

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.h (right):

https://codereview.chromium.org/534843002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.h:27: 
On 2014/09/03 22:45:28, sky wrote:
> nit: nuke.

Done.

[sky, 2014-09-03 23:36:18]

LGTM - I didn't review the view_manager_lib changes. I assumed you added Ben for
those. We can fix up initial bounds later on.

[jamesr, 2014-09-05 00:03:14]

OK, comments addressed.  This also fixes the red<->blue color channel shifting
so web contents don't look quite as garish.

Ben - could you review the client side additions and approve the new DEPS
changes?

Two other issues still exist with this patch:
*) Without --enable-harfbuzz-rendertext mojo shell sometimes crashes or behaves
badly when running mojo:mojo_demo_launcher because multiple threads try to call
into pango at the same time and pango initialization (and probably general
operation) is very far from threadsafe.  The harfbuzz backend seems unaffected

*) For some reason everything is visually shifted up by 30px on my box.  I'm
still trying to track this down.

[Ben Goodger (Google), 2014-09-08 18:15:06]

On 2014/09/05 00:03:14, jamesr wrote:
> OK, comments addressed.  This also fixes the red<->blue color channel shifting
> so web contents don't look quite as garish.
> 
> Ben - could you review the client side additions and approve the new DEPS
> changes?
> 
> Two other issues still exist with this patch:
> *) Without --enable-harfbuzz-rendertext mojo shell sometimes crashes or
behaves
> badly when running mojo:mojo_demo_launcher because multiple threads try to
call
> into pango at the same time and pango initialization (and probably general
> operation) is very far from threadsafe.  The harfbuzz backend seems unaffected
> 
> *) For some reason everything is visually shifted up by 30px on my box.  I'm
> still trying to track this down.

Looking now.

[Ben Goodger (Google), 2014-09-08 18:26:44]

lgtm !

https://codereview.chromium.org/534843002/diff/40001/mojo/services/public/int...
File mojo/services/public/interfaces/native_viewport/native_viewport.mojom
(right):

https://codereview.chromium.org/534843002/diff/40001/mojo/services/public/int...
mojo/services/public/interfaces/native_viewport/native_viewport.mojom:14:
Create(Size bounds);
rename bounds -> size

https://codereview.chromium.org/534843002/diff/40001/mojo/services/public/int...
mojo/services/public/interfaces/native_viewport/native_viewport.mojom:18:
SetBounds(Size bounds);
etc.

[jamesr, 2014-09-10 22:54:45]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:18:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/534843002/80002

[commit-bot: I haz the power, 2014-09-11 00:13:25]

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

[commit-bot: I haz the power, 2014-09-11 00:13:26]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[jamesr, 2014-09-11 17:31:12]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:40:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/534843002/110001

[commit-bot: I haz the power, 2014-09-11 18:21:14]

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

[commit-bot: I haz the power, 2014-09-11 18:21:15]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54767)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59678)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe 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...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jamesr, 2014-09-11 18:31:59]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:33:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/534843002/130001

[commit-bot: I haz the power, 2014-09-11 18:45:12]

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

[commit-bot: I haz the power, 2014-09-11 18:45:13]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54795)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59714)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe 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_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jamesr, 2014-09-11 19:32:12]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-09-11 19:33:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/534843002/150001

[jamesr, 2014-09-11 19:55:59]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-09-11 20:07:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/534843002/170001

[commit-bot: I haz the power, 2014-09-11 21:00:41]

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

[commit-bot: I haz the power, 2014-09-11 21:00:43]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jamesr, 2014-09-11 22:21:19]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-09-11 22:23:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/534843002/190001

[commit-bot: I haz the power, 2014-09-12 00:20:23]

Committed patchset #11 (id:190001) as 787491611a1ed9f29e5e2eb3fc1d74f6b511a45b

[commit-bot: I haz the power, 2014-09-12 00:23:38]

Patchset 11 (id:??) landed as
https://crrev.com/529c759bf153b8eec2178244ddd3e87e5790b7c4
Cr-Commit-Position: refs/heads/master@{#294501}