android content shell bringup.

content/public/browser/content_view.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[jam, 2012/06/08 23:45:09]

since this file is just for Android, perhaps content_view_android.h is clearer
and similar to what we name platform-specific files?

[John Grabowski, 2012/06/12 01:46:30]

We have somewhat conflicting standards here.  When we have a native class mirror
a Java class, we try and name it the same.  Also, we put class Foo in a file
named Foo.cc.  For similar cases, the files live in
content/browser/android/Foo.h.  However, this needs to be in content/public.  

Perhaps I can create a new content/public/browser/android directory that
contains this file?

[jam, 2012/06/12 17:44:02]

that sounds good to me. I just checked and that's what the WebKit API does, so
we should follow them for consistency.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_PUBLIC_BROWSER_CONTENT_VIEW_H_
+#define CONTENT_PUBLIC_BROWSER_CONTENT_VIEW_H_
+#pragma once
+
+#include <jni.h>
+#include <vector>
+
+#include "base/android/jni_helper.h"
+#include "base/memory/weak_ptr.h"
+
+class ContentViewClient;

[jam, 2012/06/08 23:45:09]

nit: not used

+
+namespace content {
+class JavaScriptDialogCreator;
+}
+
+namespace content {
+// Native side of the ContentView.java, the primary FrameLayout of
+// Chromium on Android.  This is a public interface used by native
+// code outside of the content/ hierarchy.

[jam, 2012/06/08 23:45:09]

nit: "content/ hierarchy." -> "content module"

+//
+// TODO(jrg): this is a shell.  Upstream the rest.
+class ContentView : public base::SupportsWeakPtr<ContentView> {

[jam, 2012/06/08 23:45:09]

I'm curious why you need this to spuport weak pointer? who creates and owns
this, the embedder? if so, why do they need this?

[John Grabowski, 2012/06/12 01:46:30]

Those are details in methods we haven't upstreamed yet; we set this as a weak
pointer in WebContentsViewAndroid.

[jam, 2012/06/12 17:44:02]

even if it's still upstream, I'm wondering why you need a weak pointer here?
i.e. what is the ownership, and can't you WCVA be notified, or figure out
itself, when this goes away?

[John Grabowski, 2012/06/13 22:12:29]

From what I can tell.... a Java ContentView owns this native ContentView. 
However, WebContentsViewAndroid needs a pointer to it.  Presumably there are
cases (e.g. closing a tab in Java while a page is loading) where the pointer
gets deallocated async from a renderer message (e.g. SetTitle).  Does that seem
reasonable?

[jam, 2012/06/14 01:31:40]

Just to be clear, is this a case that happens, or are you giving an example that
might happen? If it can happen, then I'm not sure why because when we close
pages, we always cancel their requests immediately.

It still seems like this is a band-aid over lifetime issues that need to be
resolved, i.e. I'm not convinced yet that we need this on public interfaces :)
Do you have a specific example of where we need this that we can't solve in
other ways? 

[John Grabowski, 2012/06/14 04:20:49]

I don't know if that example is true.  I followed the downstream code until the
spot where there is an extra check to make sure a weak pointer of this object
hasn't become NULL, and I see such a check in methods like
WebContentsViewAndroid::SetTitle().  The only way I can see for it to be NULL is
if the owner (java ContentView) got deallocated, which would happen if a tab was
closed.

If you feel strongly this is an inappropriate change, and can't be done in a
follow-up CL, I will investigate/experiment downstream.

[jam, 2012/06/14 16:08:57]

I'd really prefer we avoid adding weakptr to public interfaces that the embedder
allocates since in my mind it indicates ownership semantics that aren't well
defined.

For the WebContentsViewAndroid::SetTitle() check, how could that be reached if
the tab is closed? Without knowing the callstack where this happens, it's hard
to know exactly what's going on.

So if you can, please skip the weakptr for now.

[John Grabowski, 2012/06/14 17:56:30]

OK.  Removed; added a TODO, merge warning, and bug number.

[no sievers, 2012/06/19 23:48:39]

One case I remember is interstitials and the way they work as in temporarily
replacing the WebContents+RenderWidgetHostView. There was no good way to get
back to the them when the ChromeView goes way and they might still have a stale
pointer to the ContentView. There might have been other cases, maybe something
with instant.


On 2012/06/08 23:45:09, John Abd-El-Malek wrote:

+ public:
+  ContentView();

[jam, 2012/06/08 23:45:09]

nit: don't need

[John Grabowski, 2012/06/12 01:46:30]

will be very soon...

[jam, 2012/06/12 17:44:02]

we only put interfaces in content/public. so I don't understand what you mean by
this comment?

[John Grabowski, 2012/06/13 22:12:29]

what I meant was, real soon the content_view ctor will have "stuff in it", so
yes it's empty now but not for long.  But right, that really means
content_view_impl will soon be non-empty.  Removing.

+  virtual void Destroy(JNIEnv* env, jobject obj) = 0;
+
+  static ContentView* Create(JNIEnv* env, jobject obj);
+  static ContentView* GetNativeContentView(JNIEnv* env, jobject obj);
+
+ protected:
+  virtual ~ContentView() = 0;

[jam, 2012/06/08 23:45:09]

nit: inline this

[John Grabowski, 2012/06/12 01:46:30]

I thought we were generally against inlining virtual dtors for size reasons?

[jam, 2012/06/12 17:44:02]

empty functions are fine though (compilers have empty stub functions
apparently). for the content api, we always just do

virtual ~Foo() {}

[John Grabowski, 2012/06/13 22:12:29]

done

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ContentView);

[jam, 2012/06/08 23:45:09]

nit: don't need on abstract classes, since you can't copy them

[John Grabowski, 2012/06/12 01:46:30]

(For history: this only became an issue when I split this into content_view.h
and content_view_impl.h, something we did for the content/ public reasons.)

+};
+
+bool RegisterContentView(JNIEnv* env);

[jam, 2012/06/08 23:45:09]

does this need to be exposed to embedders? I only see it called by
content/app/android/content_jni_registrar.cc. if it doesn't need to be public,
this should be moved to the impl.h

+
+};  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_CONTENT_VIEW_H_