Add a basic linux client for Blimp.

Add a basic linux client for Blimp.

Add a simple linux front end for blimp.  This ties into all of the
features but does not handle input events yet.

BUG=534894

[David Trainor- moved to gerrit, 2015-12-16 01:20:33]

Still working on a crasher... I think it has to do with me not fully
understanding X11 and the X11Window class (says it's a threading issue).

[David Trainor- moved to gerrit, 2015-12-16 01:27:18]

Ok... think I fixed the crasher.  Still confused though :/.

[David Trainor- moved to gerrit, 2015-12-16 01:31:52]

dtrainor@chromium.org changed reviewers:
+ haibinlu@chromium.org, nyquist@chromium.org, wez@chromium.org

[David Trainor- moved to gerrit, 2015-12-16 01:31:53]

Basic linux client... Not super clean, but feel free to take a look and I'll try
to address the comments before I go.  Piping through input events is going to be
a bit more annoying because we have to handle translating the aura input events
into generic blink events.  There is a file that does it but it's tied tightly
to aura.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
File blimp/client/linux/blimp_display_manager.h (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.h:28: virtual void OnClosed() = 0;
I actually probably don't need this... I can shut down by directly accessing the
current message loop (I do that in the delegate anyway...).  If we're okay with
this just being a test binary that might be simpler.

[haibinlu, 2015-12-17 00:32:59]

lgtm

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:89: executable("blimp_shell") {
blimp_client_shell?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session_linux.cc (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session_linux.cc:16:
blimp_display_manager_.reset(new BlimpDisplayManager(gfx::Size(800, 600),
nit. define a const for size?

[nyquist, 2015-12-17 03:00:21]

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:89: executable("blimp_shell") {
On 2015/12/17 00:32:58, haibinlu wrote:
> blimp_client_shell?

Good question. I guess currently it follows 'content_shell'. Then again,
//content isn't split between client and engine.

Will we ever have a blimp_engine_shell though?

I don't feel strongly. Either way is fine with me.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:107: "//ui/events/platform/x11",
Optional nit: Could this follow the same style as the previous line?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/DEPS
File blimp/client/DEPS (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/DEPS#newco...
blimp/client/DEPS:20: "+ui/platform_window",
Nit: Could this live in //blimp/client/linux instead?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
File blimp/client/blimp_startup.cc (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
blimp/client/blimp_startup.cc:35: logging::SetLogItems(false,   // Process ID
Why do we not want to log the process and thread ID on the engine?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session_linux.cc (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session_linux.cc:17: this,
Is it safe to pass |this| as an argument in your own constructor? I guess we
don't assume that the delegate is used in the constructor of BlimpDisplayManager
anyway? Given it currently only contains ::OnClosed I guess that's fine since
that is currently only called through the destructor of X11Window AFAIK.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session_linux.h (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session_linux.h:28:
scoped_ptr<ui::PlatformEventSource> event_source_;
For the uninitiated, could you add a comment about this and why it has to be
available before we create the BlimpDisplayManager as the delegate of X11Window?

[Wez, 2016-01-05 19:15:02]

It looks like Haibin landed one of the earlier patch-sets before Tommy's and my
review comments, so we'll need to rebase and land a follow-up patch with the
various fixes.

Haibin, in future if you take over a CL, please remember to close the original
review, with a link to the new CL, so that people don't mistakenly add further
comments to it!

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:89: executable("blimp_shell") {
On 2015/12/17 at 03:00:21, nyquist - OOO till 2016-01-04 wrote:
> On 2015/12/17 00:32:58, haibinlu wrote:
> > blimp_client_shell?
> 
> Good question. I guess currently it follows 'content_shell'. Then again,
//content isn't split between client and engine.
> 
> Will we ever have a blimp_engine_shell though?
> 
> I don't feel strongly. Either way is fine with me.

No, the client+engine are equivalent to content_shell, in effect - it'd be
reasonable to call this blimp_shell, but probably more understandable to call it
simply blimp_client.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:107: "//ui/events/platform/x11",
On 2015/12/17 at 03:00:21, nyquist - OOO till 2016-01-04 wrote:
> Optional nit: Could this follow the same style as the previous line?

I think we can just run the gn formatter over this file to get the preferred
style?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/android/ja...
File blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java
(right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/android/ja...
blimp/client/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java:100:
sLibraryLoadResult = new Boolean(startResult);
Seems strange that we're taking a "start result" and treating it as a "library
load result" - what does nativeStartBlimp() actually do...?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
File blimp/client/blimp_startup.cc (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
blimp/client/blimp_startup.cc:17: LAZY_INSTANCE_INITIALIZER;
Add a comment to explain the purpose of this member.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
blimp/client/blimp_startup.cc:35: logging::SetLogItems(false,   // Process ID
On 2015/12/17 at 03:00:21, nyquist - OOO till 2016-01-04 wrote:
> Why do we not want to log the process and thread ID on the engine?

This is blimp_client logging initialization - there is only one process, so we
probably want to log the thread Id and timestamp, not the process ID nor tick
count.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
blimp/client/blimp_startup.cc:44: // TODO(dtrainor): Initialize ICU?
Clarify why we might need to initialize ICU here.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
blimp/client/blimp_startup.cc:45: 
nit: Remove the spurious blank line.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
blimp/client/blimp_startup.cc:48: SkGraphics::Init();
The function name is InitializeMainMessageLoop, but we're also initializing some
global Skia and gfx state, so please add a comment to clarify what's going on
here.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
File blimp/client/blimp_startup.h (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/blimp_star...
blimp/client/blimp_startup.h:14: BLIMP_CLIENT_EXPORT bool
InitializeMainMessageLoop();
nit: Suggest having one-line comments to clarify the purpose of these, and e.g.
the meaning of the return-code from InitializeMainMessageLoop.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
File blimp/client/linux/blimp_display_manager.cc (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.cc:28: platform_window_->Show();
nit: Do we want to show the platform window _before_ we finish setting the
compositor size, etc?  Won't that mean that we get a blank window that then has
some flickery stuff happen in it at startup?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.cc:43: void
BlimpDisplayManager::OnDamageRect(const gfx::Rect& damaged_region) {}
Doesn't damage need to be passed to the compositor to trigger re-rendering?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.cc:57: delegate_->OnClosed();
We could almost just exit the process directly in this case, and wire up proper
teardown later, if we want.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.cc:73:
blimp_compositor_->SetAcceleratedWidget(widget);
If the widget has become kNullAcceleratedWidget then don't we need to do
anything to stop the compositor from potentially touching the old, presumably
now invalid, widget?

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
File blimp/client/linux/blimp_display_manager.h (right):

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.h:36: TabControlFeature*
tab_control_feature);
nit: Document object lifetime/ownership constrains for the various bare-pointer
parameters.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.h:39: // ui::PlatformWindowDelegate:
nit: ui::PlatformWindowDelegate implementation.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.h:53: float device_pixel_ratio_;
One-liner comment to explain what this is used for (looks like it caches the
ratio for the current accelerated widget - in which case how come we hold the
ration but not the widget?) and consider inline initialization.

https://codereview.chromium.org/1528243002/diff/20001/blimp/client/linux/blim...
blimp/client/linux/blimp_display_manager.h:56: TabControlFeature*
tab_control_feature_;
Inline initialize to nullptr.