android content shell bringup.

android content shell bringup.

Starting classes for content view, content view client.  Includes some
relevant deps needed to build or pass a sanity check.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142309

[John Grabowski, 2012-06-08 23:19:42]

jam and jar for OWNERS
yaron for Android

[John Grabowski, 2012-06-08 23:26:05]

Ooh -- it looks like we can now launch a renderer??? 

When I start an intent with URL, I see a crash.  But the browser didn't crash,
which means a non-browser process crashed.  And then I see an OnTabCrashed()
call get made in the Browser.

Whoa!

[jar (doing other things), 2012-06-08 23:31:42]

Can you get someone that is familiar with JNI to review this first?  It is clear
that a lot of the JNI interface was abused in base (and is being corrected by
this code), and I'm not currently fluent in that interface (and suspect that the
surrounding code should be scrutinized).

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

http://codereview.chromium.org/10536066/diff/2002/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

http://codereview.chromium.org/10536066/diff/2002/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:411:
reinterpret_cast<void*>(const_cast<content::FileChooserParams*>(&params)));
80 chars

http://codereview.chromium.org/10536066/diff/2002/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:411:
reinterpret_cast<void*>(const_cast<content::FileChooserParams*>(&params)));
nit: are you sure you need to reinterpret_cast to void*?

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_util.cc (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_util.cc:10: #include
"content/public/browser/web_contents.h"
nit: here and in the rest of the change, the convention is to include _impl.h
headers for interfaces inside content. this is to match the WebKit API. the
reason is that later on if we need to cast to the impl, we don't need to change
the includes. that and the public headers are for embedders

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_util.cc:65: 
nit: blank line

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_util.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_util.h:14:
base::android::ScopedJavaLocalRef<jobject> ToJavaFileChooserParams(
nit: put in content namespace

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_client.cc (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:12: #include
"content/public/browser/content_view.h"
nit: order

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:56: if (!obj)
is this an error that shouldn't happen? if so, just DCHECK it it and don't
handle that error case

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:464: {
nit: brace in above line

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:468: case
ERR_UNSUPPORTED_AUTH_SCHEME:
nit: here and below, 2 space indents

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_client.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.h:28: using
content::NativeWebKeyboardEvent;
nit: using statements aren't allowed in headers per the style guide

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.h:32: // Success
nit: two space invent

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.h:33: ERROR_OK = 0,
nit: the convention we try to follow in content is same as WebKit api, which is
enum type's name followed by specific, i.e.

CONTENT_VIEW_CLIENT_ERROR_UNKNOWN
CONTENT_VIEW_CLIENT_ERROR_HOST_LOOKUP

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.h:68: class ContentViewClient :
public content::WebContentsDelegate {
nit: new code in content should be in the content namespace

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.h:107: 
nit: convention is to not have any blank lines between overridden functions

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.h:161: const FindMatchRect&
active_rect) OVERRIDE;
nit: here and below, 80 char limit. convention when OVERRIDE goes over is to
move the stuff before it to start indented (i.e. we dont want to start a line
with OVERRIDE)

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.h:178: static ContentViewClientError
ToContentViewClientError(int netError);
nit: style guide is net_error

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_impl.cc (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.cc:59: const
content::NotificationDetails& details) OVERRIDE {
nit: override is only in the header. also brace bracket

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_impl.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:13: #include
"base/memory/weak_ptr.h"
nit: don't need these 4 includes since content_view.h includes them

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:20: class JavaScriptDialogCreator;
nit: not used

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:30: jobject obj);
nit: tabbing

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
File content/public/browser/content_view.cc (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: don't need this file, so remove it (we only add cc files in the public
directory to avoid clang errors when necessary)

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
File content/public/browser/content_view.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
since this file is just for Android, perhaps content_view_android.h is clearer
and similar to what we name platform-specific files?

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:15: class ContentViewClient;
nit: not used

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:24: // code outside of the content/
hierarchy.
nit: "content/ hierarchy." -> "content module"

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
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?

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:29: ContentView();
nit: don't need

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:36: virtual ~ContentView() = 0;
nit: inline this

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:39: DISALLOW_COPY_AND_ASSIGN(ContentView);
nit: don't need on abstract classes, since you can't copy them

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:42: bool RegisterContentView(JNIEnv* env);
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

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
File content/public/common/file_chooser_params.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
content/public/common/file_chooser_params.h:52: // filesystem).  See
SelectFileDialog.java.
nit: this can fit on the previous line

I'm also confused about what this parameter means. it's named "capture" but you
say "Paramaterization of choice" which I can't tell what that is.

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
File content/public/common/find_match_rect_android.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
content/public/common/find_match_rect_android.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
are you planning on exposing this later? i only see it used inside content for
now.

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
content/public/common/find_match_rect_android.h:9: struct FindMatchRect {
nit: everything in content API has to be in content namespace

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
content/public/common/find_match_rect_android.h:12: FindMatchRect(float left,
float top, float right, float bottom);
where is the cc file for this? if you can do this inline, that's preferred. if
you must add a .cc file to avoid clang errors, then it would be in this
directory.

[Yaron, 2012-06-09 01:02:11]

mostly just nits.

jar: I'm familiar with JNI stuff and it looks fine. jrg is  updating this with a
newer copy from downstream.

Side point: I'm the only person here who's not j__ :)

http://codereview.chromium.org/10536066/diff/2002/content/public/android/java...
File content/public/android/java/org/chromium/content/browser/ContentView.java
(right):

http://codereview.chromium.org/10536066/diff/2002/content/public/android/java...
content/public/android/java/org/chromium/content/browser/ContentView.java:101:
private void initialize(Context context) {
Might be worthwhile to match the signature to avoid having it accidentally
overridden on downstream?

http://codereview.chromium.org/10536066/diff/2002/content/public/android/java...
File
content/public/android/java/org/chromium/content/browser/ContentViewClient.java
(right):

http://codereview.chromium.org/10536066/diff/2002/content/public/android/java...
content/public/android/java/org/chromium/content/browser/ContentViewClient.java:16:
import org.chromium.base.AccessedByNative;
I think you need to "git add" this file. It's not upstream

http://codereview.chromium.org/10536066/diff/2002/content/public/android/java...
content/public/android/java/org/chromium/content/browser/ContentViewClient.java:110:
// TODO(jrg): add onReceivedHttpAuthRequest() once ContentHttpAuthHandler is
upstreamed
Leave it in commented out?

http://codereview.chromium.org/10536066/diff/2002/content/public/android/java...
File
content/public/android/java/org/chromium/content/browser/FileChooserParams.java
(right):

http://codereview.chromium.org/10536066/diff/2002/content/public/android/java...
content/public/android/java/org/chromium/content/browser/FileChooserParams.java:16:
* See ContentUtil.java for conversion methods between the Java and native types.
content_util.cc

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
File content/public/common/find_match_rect_android.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
content/public/common/find_match_rect_android.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> are you planning on exposing this later? i only see it used inside content for
> now.

From downstream it looks like the native side is only used in content.

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
content/public/common/find_match_rect_android.h:12: FindMatchRect(float left,
float top, float right, float bottom);
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> where is the cc file for this? if you can do this inline, that's preferred. if
> you must add a .cc file to avoid clang errors, then it would be in this
> directory.

Downstream it's in the same dir. I guess you can add it in this patch, John.

[haitao.feng, 2012-06-11 05:38:50]

Thanks for uploading those files!

Yes, from adb logcat:
E/chromium( 1566): [ERROR:web_contents_view_android.cc(65)] Not implemented
reached in virtual void
WebContentsViewAndroid::OnTabCrashed(base::TerminationStatus, int)

You may consider uploading sandboxed_process_service.[h|cc],
sandboxed_process_launcher.[h|cc] to bring up renderer process.

On 2012/06/08 23:26:05, John Grabowski wrote:
> Ooh -- it looks like we can now launch a renderer??? 
> 
> When I start an intent with URL, I see a crash.  But the browser didn't crash,
> which means a non-browser process crashed.  And then I see an OnTabCrashed()
> call get made in the Browser.
> 
> Whoa!

[yongsheng, 2012-06-11 07:23:13]

On 2012/06/11 05:38:50, haitao.feng wrote:
> Thanks for uploading those files!
> 
> Yes, from adb logcat:
> E/chromium( 1566): [ERROR:web_contents_view_android.cc(65)] Not implemented
> reached in virtual void
> WebContentsViewAndroid::OnTabCrashed(base::TerminationStatus, int)
> 
> You may consider uploading sandboxed_process_service.[h|cc],
> sandboxed_process_launcher.[h|cc] to bring up renderer process.
This is really a good patch to help push forward content shell. 
To make content shell work, there are many other important files to be
added/implemented: 
render_widget_host_view_android.h/cc
backing_store_android.h/cc
web_contents_view_android.h/cc
So will you enable them in the next step?

[jar (doing other things), 2012-06-11 08:04:52]

base changes LGTM (mostly rubber stamped)

[Shouqun Liu, 2012-06-11 08:26:05]

On 2012/06/08 23:26:05, John Grabowski wrote:
> Ooh -- it looks like we can now launch a renderer??? 
> 
> When I start an intent with URL, I see a crash.  But the browser didn't crash,
> which means a non-browser process crashed.  And then I see an OnTabCrashed()
> call get made in the Browser.
> 
> Whoa!

yes, the GPU process and renderer process crash,  the upstream code does not
contain creation of process in android way.

[John Grabowski, 2012-06-12 01:46:29]

All feedback applied except where noted; thanks guys.
jam: have question about new location/name for content_view.h

http://chromiumcodereview.appspot.com/10536066/diff/2002/chrome/browser/file_...
File chrome/browser/file_select_helper.cc (right):

http://chromiumcodereview.appspot.com/10536066/diff/2002/chrome/browser/file_...
chrome/browser/file_select_helper.cc:411:
reinterpret_cast<void*>(const_cast<content::FileChooserParams*>(&params)));
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: are you sure you need to reinterpret_cast to void*?

I think no.

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/browser/andr...
File content/browser/android/content_util.cc (right):

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/browser/andr...
content/browser/android/content_util.cc:10: #include
"content/public/browser/web_contents.h"
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: here and in the rest of the change, the convention is to include _impl.h
> headers for interfaces inside content. this is to match the WebKit API. the
> reason is that later on if we need to cast to the impl, we don't need to
change
> the includes. that and the public headers are for embedders

Sure.  Is there a convention for what files have _impls?  I can see
web_contents_impl.h, but can't otherwise eyeball a list of #includes to know
which ones to convert over.

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/andro...
File content/public/android/java/org/chromium/content/browser/ContentView.java
(right):

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/andro...
content/public/android/java/org/chromium/content/browser/ContentView.java:101:
private void initialize(Context context) {
On 2012/06/09 01:02:11, Yaron wrote:
> Might be worthwhile to match the signature to avoid having it accidentally
> overridden on downstream?

I thought about it, but felt it was better to have clean "chunks" to upstream.
I will personally re-downstream this so it shouldn't be a problem.

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/andro...
File
content/public/android/java/org/chromium/content/browser/ContentViewClient.java
(right):

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/andro...
content/public/android/java/org/chromium/content/browser/ContentViewClient.java:16:
import org.chromium.base.AccessedByNative;
On 2012/06/09 01:02:11, Yaron wrote:
> I think you need to "git add" this file. It's not upstream

Oops!  Thx

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/andro...
content/public/android/java/org/chromium/content/browser/ContentViewClient.java:110:
// TODO(jrg): add onReceivedHttpAuthRequest() once ContentHttpAuthHandler is
upstreamed
On 2012/06/09 01:02:11, Yaron wrote:
> Leave it in commented out?

We probably have allowances for upstream transition, but in general upstream
doesn't like landing code which is commented out.

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/brows...
File content/public/browser/content_view.h (right):

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/brows...
content/public/browser/content_view.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> since this file is just for Android, perhaps content_view_android.h is clearer
> and similar to what we name platform-specific files?

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?

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/brows...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> 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?

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

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/brows...
content/public/browser/content_view.h:29: ContentView();
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: don't need

will be very soon...

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/brows...
content/public/browser/content_view.h:36: virtual ~ContentView() = 0;
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: inline this

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

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/brows...
content/public/browser/content_view.h:39: DISALLOW_COPY_AND_ASSIGN(ContentView);
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: don't need on abstract classes, since you can't copy them

(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.)

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/commo...
File content/public/common/file_chooser_params.h (right):

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/commo...
content/public/common/file_chooser_params.h:52: // filesystem).  See
SelectFileDialog.java.
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: this can fit on the previous line
> 
> I'm also confused about what this parameter means. it's named "capture" but
you
> say "Paramaterization of choice" which I can't tell what that is.

Sorry.  If "camera", will perform a capture from the camera (e.g. for select
image, this can be the "file location").  It is unclear how much I want to doc
the java in here but am open to suggestions for clarity.

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/commo...
File content/public/common/find_match_rect_android.h (right):

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/commo...
content/public/common/find_match_rect_android.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> are you planning on exposing this later? i only see it used inside content for
> now.

Don't think so.

[Yaron, 2012-06-12 04:04:03]

lgtm

[jam, 2012-06-12 17:44:01]

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_util.cc (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_util.cc:10: #include
"content/public/browser/web_contents.h"
On 2012/06/12 01:46:29, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > nit: here and in the rest of the change, the convention is to include
_impl.h
> > headers for interfaces inside content. this is to match the WebKit API. the
> > reason is that later on if we need to cast to the impl, we don't need to
> change
> > the includes. that and the public headers are for embedders
> 
> Sure.  Is there a convention for what files have _impls?  I can see
> web_contents_impl.h, but can't otherwise eyeball a list of #includes to know
> which ones to convert over.  

if there's an interface in content/public that is implemented in content,
there'll usually be an impl

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_client.cc (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:12: #include
"content/public/browser/content_view.h"
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: order

?

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:56: if (!obj)
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> is this an error that shouldn't happen? if so, just DCHECK it it and don't
> handle that error case

?

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:464: {
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: brace in above line

this is still not according to style guide, i.e 80 chars and brace bracket not
on previous line

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:468: case
ERR_UNSUPPORTED_AUTH_SCHEME:
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: here and below, 2 space indents

?

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_impl.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:13: #include
"base/memory/weak_ptr.h"
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: don't need these 4 includes since content_view.h includes them

?

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:20: class JavaScriptDialogCreator;
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: not used

?

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:30: jobject obj);
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: tabbing

?

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
File content/public/browser/content_view.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/06/12 01:46:30, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > since this file is just for Android, perhaps content_view_android.h is
clearer
> > and similar to what we name platform-specific files?
> 
> 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?

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

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
On 2012/06/12 01:46:30, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > 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?
> 
> Those are details in methods we haven't upstreamed yet; we set this as a weak
> pointer in WebContentsViewAndroid.

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?

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:29: ContentView();
On 2012/06/12 01:46:30, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > nit: don't need
> 
> will be very soon...

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

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:36: virtual ~ContentView() = 0;
On 2012/06/12 01:46:30, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > nit: inline this
> 
> I thought we were generally against inlining virtual dtors for size reasons?

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

virtual ~Foo() {}

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
File content/public/common/file_chooser_params.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
content/public/common/file_chooser_params.h:52: // filesystem).  See
SelectFileDialog.java.
On 2012/06/12 01:46:30, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > nit: this can fit on the previous line
> > 
> > I'm also confused about what this parameter means. it's named "capture" but
> you
> > say "Paramaterization of choice" which I can't tell what that is.
> 
> Sorry.  If "camera", will perform a capture from the camera (e.g. for select
> image, this can be the "file location").  It is unclear how much I want to doc
> the java in here but am open to suggestions for clarity.

so did you mean to update this?

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
File content/public/common/find_match_rect_android.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/find_...
content/public/common/find_match_rect_android.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/06/12 01:46:30, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > are you planning on exposing this later? i only see it used inside content
for
> > now.
> 
> Don't think so.

if it's not used outside of content, move this to content/common

http://codereview.chromium.org/10536066/diff/8003/content/browser/android/con...
File content/browser/android/content_util.cc (right):

http://codereview.chromium.org/10536066/diff/8003/content/browser/android/con...
content/browser/android/content_util.cc:35: JNIEnv* env, const
content::FileChooserParams& file_chooser_params) {
nit: here and below, don't need content:: anymore

http://codereview.chromium.org/10536066/diff/8003/content/browser/android/con...
File content/browser/android/content_util.h (right):

http://codereview.chromium.org/10536066/diff/8003/content/browser/android/con...
content/browser/android/content_util.h:18: const content::FileChooserParams&
file_chooser_params);
nit: don't need content::

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

Thanks for the detailed scrutiny!
Feedback applied with one or two questions.

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_client.cc (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:12: #include
"content/public/browser/content_view.h"
On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > nit: order
> 
> ?

Done.

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:56: if (!obj)
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> is this an error that shouldn't happen? if so, just DCHECK it it and don't
> handle that error case

Done.

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:464: {
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: brace in above line

Done.

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_client.cc:468: case
ERR_UNSUPPORTED_AUTH_SCHEME:
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: here and below, 2 space indents

Done.  (sorry looks like I missed this whole file on review)

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_impl.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:13: #include
"base/memory/weak_ptr.h"
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: don't need these 4 includes since content_view.h includes them

Wouldn't IWYU mean I should keep jni_helper.h and notification_observer.h?
(weak_ptr removed)

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:20: class JavaScriptDialogCreator;
On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> nit: not used

Removed

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:30: jobject obj);
On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > nit: tabbing
> 
> ?

fixed

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
File content/public/browser/content_view.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> On 2012/06/12 01:46:30, John Grabowski wrote:
> > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > 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?
> > 
> > Those are details in methods we haven't upstreamed yet; we set this as a
weak
> > pointer in WebContentsViewAndroid.
> 
> 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?

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?

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:29: ContentView();
On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> On 2012/06/12 01:46:30, John Grabowski wrote:
> > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > nit: don't need
> > 
> > will be very soon...
> 
> we only put interfaces in content/public. so I don't understand what you mean
by
> this comment?

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.

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:36: virtual ~ContentView() = 0;
On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> On 2012/06/12 01:46:30, John Grabowski wrote:
> > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > nit: inline this
> > 
> > I thought we were generally against inlining virtual dtors for size reasons?
> 
> empty functions are fine though (compilers have empty stub functions
> apparently). for the content api, we always just do
> 
> virtual ~Foo() {}

done

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
File content/public/common/file_chooser_params.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
content/public/common/file_chooser_params.h:52: // filesystem).  See
SelectFileDialog.java.
On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> On 2012/06/12 01:46:30, John Grabowski wrote:
> > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > nit: this can fit on the previous line
> > > 
> > > I'm also confused about what this parameter means. it's named "capture"
but
> > you
> > > say "Paramaterization of choice" which I can't tell what that is.
> > 
> > Sorry.  If "camera", will perform a capture from the camera (e.g. for select
> > image, this can be the "file location").  It is unclear how much I want to
doc
> > the java in here but am open to suggestions for clarity.
> 
> so did you mean to update this?

Like I said, it is unclear how much I want to doc the java interface in native
code.  I will add what you think is relevant (which appears to be more than I
have added already)?

http://codereview.chromium.org/10536066/diff/8003/content/browser/android/con...
File content/browser/android/content_util.cc (right):

http://codereview.chromium.org/10536066/diff/8003/content/browser/android/con...
content/browser/android/content_util.cc:35: JNIEnv* env, const
content::FileChooserParams& file_chooser_params) {
On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> nit: here and below, don't need content:: anymore

Done.

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

http://codereview.chromium.org/10536066/diff/2002/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

http://codereview.chromium.org/10536066/diff/2002/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:411:
reinterpret_cast<void*>(const_cast<content::FileChooserParams*>(&params)));
On 2012/06/12 01:46:29, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > nit: are you sure you need to reinterpret_cast to void*?
> 
> I think no.

any pointer is a void*, so you shouldn't need a static_cast either

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
File content/browser/android/content_view_impl.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/browser/android/con...
content/browser/android/content_view_impl.h:13: #include
"base/memory/weak_ptr.h"
On 2012/06/13 22:12:29, John Grabowski wrote:
> On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > nit: don't need these 4 includes since content_view.h includes them
> 
> Wouldn't IWYU mean I should keep jni_helper.h and notification_observer.h?
> (weak_ptr removed)

as far as i can tell, jni.h and jni_helper.h are used by the parent interface.
so there's no reason to include them in the derived interfaces.

<vector> isn't used by the parent, so it should move out of there.

notification_observer.h is fine now that it's not in the other header

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
File content/public/browser/content_view.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
On 2012/06/13 22:12:29, John Grabowski wrote:
> On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> > On 2012/06/12 01:46:30, John Grabowski wrote:
> > > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > > 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?
> > > 
> > > Those are details in methods we haven't upstreamed yet; we set this as a
> weak
> > > pointer in WebContentsViewAndroid.
> > 
> > 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?
> 
> 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?
> 

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?

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
File content/public/common/file_chooser_params.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
content/public/common/file_chooser_params.h:52: // filesystem).  See
SelectFileDialog.java.
On 2012/06/13 22:12:29, John Grabowski wrote:
> On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> > On 2012/06/12 01:46:30, John Grabowski wrote:
> > > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > > nit: this can fit on the previous line
> > > > 
> > > > I'm also confused about what this parameter means. it's named "capture"
> but
> > > you
> > > > say "Paramaterization of choice" which I can't tell what that is.
> > > 
> > > Sorry.  If "camera", will perform a capture from the camera (e.g. for
select
> > > image, this can be the "file location").  It is unclear how much I want to
> doc
> > > the java in here but am open to suggestions for clarity.
> > 
> > so did you mean to update this?
> 
> Like I said, it is unclear how much I want to doc the java interface in native
> code.  I will add what you think is relevant (which appears to be more than I
> have added already)?

It's just that this comment doesn't mean anything to me. Is this because I'm not
familiar with the android port?

 51   // Paramaterization of choice on the Java size (e.g. from
 52   // filesystem).  See SelectFileDialog.java.

small thing first: do you mean side instead of size?

but also, what does "Paramaterization of choice" mean??

and I still don't understand camera vs capture?

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
File content/browser/android/content_view_client.cc (right):

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
content/browser/android/content_view_client.cc:15: #include
"content/public/browser/android/content_view.h"
nit: content_view_impl

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
content/browser/android/content_view_client.cc:42: using namespace net;
this is against the style guide (just saw this). you have to list out each class
individually

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
content/browser/android/content_view_client.cc:63: content::WebContents* source,
const GURL& url) {
nit: get rid of content:: in this file, and also using content:: statements
above

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
File content/browser/android/content_view_client.h (right):

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
content/browser/android/content_view_client.h:30: enum ContentViewClientError {
why is this not in the content namespace?

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
content/browser/android/content_view_client.h:69: class ContentViewClient :
public content::WebContentsDelegate {
nit: in this whole class, get rid of "content::" now

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
File content/browser/android/content_view_impl.cc (right):

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
content/browser/android/content_view_impl.cc:58: const
content::NotificationSource& source,
nit: ditto for "content::" in this file

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
File content/browser/android/content_view_impl.h (right):

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
content/browser/android/content_view_impl.h:14: #include
"content/public/browser/android/content_view.h"
nit: order

http://codereview.chromium.org/10536066/diff/13002/content/browser/android/co...
content/browser/android/content_view_impl.h:22: public
content::NotificationObserver {
nit: here and below, no need for "content::"

http://codereview.chromium.org/10536066/diff/13002/content/public/browser/and...
File content/public/browser/android/content_view.h (right):

http://codereview.chromium.org/10536066/diff/13002/content/public/browser/and...
content/public/browser/android/content_view.h:16: class JavaScriptDialogCreator;
nit: get rid of all the includes and forward declarations that aren't used in
this file. i.e. vector, jni_helper.h, JavaScriptDialogCreator,

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

All feedback applied.  Thanks for your patience!
2 open issues (below)

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
File content/public/browser/content_view.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
On 2012/06/14 01:31:40, John Abd-El-Malek wrote:
> On 2012/06/13 22:12:29, John Grabowski wrote:
> > On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> > > On 2012/06/12 01:46:30, John Grabowski wrote:
> > > > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > > > 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?
> > > > 
> > > > Those are details in methods we haven't upstreamed yet; we set this as a
> > weak
> > > > pointer in WebContentsViewAndroid.
> > > 
> > > 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?
> > 
> > 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?
> > 
> 
> 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? 

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.

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
File content/public/common/file_chooser_params.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
content/public/common/file_chooser_params.h:52: // filesystem).  See
SelectFileDialog.java.
On 2012/06/14 01:31:40, John Abd-El-Malek wrote:
> On 2012/06/13 22:12:29, John Grabowski wrote:
> > On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> > > On 2012/06/12 01:46:30, John Grabowski wrote:
> > > > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > > > nit: this can fit on the previous line
> > > > > 
> > > > > I'm also confused about what this parameter means. it's named
"capture"
> > but
> > > > you
> > > > > say "Paramaterization of choice" which I can't tell what that is.
> > > > 
> > > > Sorry.  If "camera", will perform a capture from the camera (e.g. for
> select
> > > > image, this can be the "file location").  It is unclear how much I want
to
> > doc
> > > > the java in here but am open to suggestions for clarity.
> > > 
> > > so did you mean to update this?
> > 
> > Like I said, it is unclear how much I want to doc the java interface in
native
> > code.  I will add what you think is relevant (which appears to be more than
I
> > have added already)?
> 
> It's just that this comment doesn't mean anything to me. Is this because I'm
not
> familiar with the android port?
> 
>  51   // Paramaterization of choice on the Java size (e.g. from
>  52   // filesystem).  See SelectFileDialog.java.
> 
> small thing first: do you mean side instead of size?
> 
> but also, what does "Paramaterization of choice" mean??
> 
> and I still don't understand camera vs capture?

Yes, size not side.  I will email you the java file (not upstreamed yet).

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

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
File content/public/browser/content_view.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
On 2012/06/14 04:20:49, John Grabowski wrote:
> On 2012/06/14 01:31:40, John Abd-El-Malek wrote:
> > On 2012/06/13 22:12:29, John Grabowski wrote:
> > > On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> > > > On 2012/06/12 01:46:30, John Grabowski wrote:
> > > > > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > > > > 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?
> > > > > 
> > > > > Those are details in methods we haven't upstreamed yet; we set this as
a
> > > weak
> > > > > pointer in WebContentsViewAndroid.
> > > > 
> > > > 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?
> > > 
> > > 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?
> > > 
> > 
> > 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? 
> 
> 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.

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.

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
File content/public/common/file_chooser_params.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
content/public/common/file_chooser_params.h:52: // filesystem).  See
SelectFileDialog.java.
On 2012/06/14 04:20:49, John Grabowski wrote:
> On 2012/06/14 01:31:40, John Abd-El-Malek wrote:
> > On 2012/06/13 22:12:29, John Grabowski wrote:
> > > On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> > > > On 2012/06/12 01:46:30, John Grabowski wrote:
> > > > > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > > > > nit: this can fit on the previous line
> > > > > > 
> > > > > > I'm also confused about what this parameter means. it's named
> "capture"
> > > but
> > > > > you
> > > > > > say "Paramaterization of choice" which I can't tell what that is.
> > > > > 
> > > > > Sorry.  If "camera", will perform a capture from the camera (e.g. for
> > select
> > > > > image, this can be the "file location").  It is unclear how much I
want
> to
> > > doc
> > > > > the java in here but am open to suggestions for clarity.
> > > > 
> > > > so did you mean to update this?
> > > 
> > > Like I said, it is unclear how much I want to doc the java interface in
> native
> > > code.  I will add what you think is relevant (which appears to be more
than
> I
> > > have added already)?
> > 
> > It's just that this comment doesn't mean anything to me. Is this because I'm
> not
> > familiar with the android port?
> > 
> >  51   // Paramaterization of choice on the Java size (e.g. from
> >  52   // filesystem).  See SelectFileDialog.java.
> > 
> > small thing first: do you mean side instead of size?
> > 
> > but also, what does "Paramaterization of choice" mean??
> > 
> > and I still don't understand camera vs capture?
> 
> Yes, size not side.  I will email you the java file (not upstreamed yet).

My suggestion is really just to make this legible to someone reading this file
who's not familiar with the java side, since this struct is used on all
platforms. I agree no need to duplicate the java comment in its entirety.
Something like this would make it easier to understand:

// Used to select items other than files, i.e. camera/mic. See
SelectFileDialog.java for more details.

[jam, 2012-06-14 16:11:09]

http://codereview.chromium.org/10536066/diff/13002/content/public/browser/and...
File content/public/browser/android/content_view.h (right):

http://codereview.chromium.org/10536066/diff/13002/content/public/browser/and...
content/public/browser/android/content_view.h:16: class JavaScriptDialogCreator;
On 2012/06/14 01:31:41, John Abd-El-Malek wrote:
> nit: get rid of all the includes and forward declarations that aren't used in
> this file. i.e. vector, jni_helper.h, JavaScriptDialogCreator,

looks like this isn't done yet?

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

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
File content/public/browser/content_view.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/browser/cont...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
On 2012/06/14 16:08:57, John Abd-El-Malek wrote:
> On 2012/06/14 04:20:49, John Grabowski wrote:
> > On 2012/06/14 01:31:40, John Abd-El-Malek wrote:
> > > On 2012/06/13 22:12:29, John Grabowski wrote:
> > > > On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> > > > > On 2012/06/12 01:46:30, John Grabowski wrote:
> > > > > > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > > > > > 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?
> > > > > > 
> > > > > > Those are details in methods we haven't upstreamed yet; we set this
as
> a
> > > > weak
> > > > > > pointer in WebContentsViewAndroid.
> > > > > 
> > > > > 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?
> > > > 
> > > > 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?
> > > > 
> > > 
> > > 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? 
> > 
> > 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.
> 
> 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.

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

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
File content/public/common/file_chooser_params.h (right):

http://codereview.chromium.org/10536066/diff/2002/content/public/common/file_...
content/public/common/file_chooser_params.h:52: // filesystem).  See
SelectFileDialog.java.
On 2012/06/14 16:08:57, John Abd-El-Malek wrote:
> On 2012/06/14 04:20:49, John Grabowski wrote:
> > On 2012/06/14 01:31:40, John Abd-El-Malek wrote:
> > > On 2012/06/13 22:12:29, John Grabowski wrote:
> > > > On 2012/06/12 17:44:02, John Abd-El-Malek wrote:
> > > > > On 2012/06/12 01:46:30, John Grabowski wrote:
> > > > > > On 2012/06/08 23:45:09, John Abd-El-Malek wrote:
> > > > > > > nit: this can fit on the previous line
> > > > > > > 
> > > > > > > I'm also confused about what this parameter means. it's named
> > "capture"
> > > > but
> > > > > > you
> > > > > > > say "Paramaterization of choice" which I can't tell what that is.
> > > > > > 
> > > > > > Sorry.  If "camera", will perform a capture from the camera (e.g.
for
> > > select
> > > > > > image, this can be the "file location").  It is unclear how much I
> want
> > to
> > > > doc
> > > > > > the java in here but am open to suggestions for clarity.
> > > > > 
> > > > > so did you mean to update this?
> > > > 
> > > > Like I said, it is unclear how much I want to doc the java interface in
> > native
> > > > code.  I will add what you think is relevant (which appears to be more
> than
> > I
> > > > have added already)?
> > > 
> > > It's just that this comment doesn't mean anything to me. Is this because
I'm
> > not
> > > familiar with the android port?
> > > 
> > >  51   // Paramaterization of choice on the Java size (e.g. from
> > >  52   // filesystem).  See SelectFileDialog.java.
> > > 
> > > small thing first: do you mean side instead of size?
> > > 
> > > but also, what does "Paramaterization of choice" mean??
> > > 
> > > and I still don't understand camera vs capture?
> > 
> > Yes, size not side.  I will email you the java file (not upstreamed yet).
> 
> My suggestion is really just to make this legible to someone reading this file
> who's not familiar with the java side, since this struct is used on all
> platforms. I agree no need to duplicate the java comment in its entirety.
> Something like this would make it easier to understand:
> 
> // Used to select items other than files, i.e. camera/mic. See
> SelectFileDialog.java for more details.

Done

http://codereview.chromium.org/10536066/diff/13002/content/public/browser/and...
File content/public/browser/android/content_view.h (right):

http://codereview.chromium.org/10536066/diff/13002/content/public/browser/and...
content/public/browser/android/content_view.h:16: class JavaScriptDialogCreator;
On 2012/06/14 16:11:10, John Abd-El-Malek wrote:
> On 2012/06/14 01:31:41, John Abd-El-Malek wrote:
> > nit: get rid of all the includes and forward declarations that aren't used
in
> > this file. i.e. vector, jni_helper.h, JavaScriptDialogCreator,
> 
> looks like this isn't done yet?

Sorry... I was editing content/public/browser/content_view.h (original name of
this file) so my changes weren't uploaded.

[jam, 2012-06-14 19:01:49]

lgtm, thanks for your patience!

[John Grabowski, 2012-06-14 23:53:33]

Thanks jam

[commit-bot: I haz the power, 2012-06-14 23:53:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrg@chromium.org/10536066/13007

[commit-bot: I haz the power, 2012-06-15 01:41:43]

Change committed as 142309

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

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/brows...
File content/public/browser/content_view.h (right):

http://chromiumcodereview.appspot.com/10536066/diff/2002/content/public/brows...
content/public/browser/content_view.h:27: class ContentView : public
base::SupportsWeakPtr<ContentView> {
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:
> 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?