Consolidate ContentViewCore::Load* functions

Consolidate ContentViewCore::Load* functions

Consolidate all ContentViewCore::Load* methods into a single LoadUrl
call that takes a NavigationController::LoadURLParam.

Consolidate all jni ContentViewCore::Load* methods by adding a Java
LoadUrlParams. The data is copied to a native AndroidLoadUrlParams
object in native jni code.

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

[boliu, 2012-08-09 00:20:28]

Depends on http://codereview.chromium.org/10830144/ which has not landed yet.

[joth, 2012-08-09 00:33:55]

http://codereview.chromium.org/10823207/diff/5002/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

http://codereview.chromium.org/10823207/diff/5002/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:40:
public LoadUrlParams(String url) {
we could add:
// check initializeConstants was called.
assert LOAD_TYPE_DEFAULT != LOAD_TYPE_BROWSER_INITIATED_HTTP_POST;

http://codereview.chromium.org/10823207/diff/5002/content/public/browser/andr...
File content/public/browser/android/content_view_core.h (right):

http://codereview.chromium.org/10823207/diff/5002/content/public/browser/andr...
content/public/browser/android/content_view_core.h:39: // Methods called from
Java via JNI
these don't need to be in the base class. They are implementation detail, and
can remain non-virtual (and private?) in the impl class.

http://codereview.chromium.org/10823207/diff/5002/content/public/browser/andr...
content/public/browser/android/content_view_core.h:53: // Methods called from
native code
this comment shouldn't be needed, as this base class interface is only for
native code.

http://codereview.chromium.org/10823207/diff/5002/content/public/browser/andr...
content/public/browser/android/content_view_core.h:56: virtual void
LoadUrl(NavigationController::LoadURLParams& params) = 0;
(does LoadUrl actually get called from native? I think it's only needed on java
side.)

[boliu, 2012-08-09 01:22:00]

http://codereview.chromium.org/10823207/diff/5002/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

http://codereview.chromium.org/10823207/diff/5002/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:40:
public LoadUrlParams(String url) {
On 2012/08/09 00:33:55, joth wrote:
> we could add:
> // check initializeConstants was called.
> assert LOAD_TYPE_DEFAULT != LOAD_TYPE_BROWSER_INITIATED_HTTP_POST;  

Done.

http://codereview.chromium.org/10823207/diff/5002/content/public/browser/andr...
File content/public/browser/android/content_view_core.h (right):

http://codereview.chromium.org/10823207/diff/5002/content/public/browser/andr...
content/public/browser/android/content_view_core.h:39: // Methods called from
Java via JNI
Generated ContentViewCore_jni.h uses this through ContentViewCore, but
apparently that can be changed by renaming the definition of the native method
in Java. This file is no longer modified in this CL.

[Yaron, 2012-08-09 21:32:31]

http://codereview.chromium.org/10823207/diff/6028/content/browser/android/loa...
File content/browser/android/load_url_params.cc (right):

http://codereview.chromium.org/10823207/diff/6028/content/browser/android/loa...
content/browser/android/load_url_params.cc:14: using
content::NavigationController;
Why not just have this function in the content namespace?

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:367:
if (mNativeContentViewCore == 0) return;
Not part of your change, but this seems a little weird that we can just toss
away a loadUrl request. Can you add an error log.

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:14:
public class LoadUrlParams {
Add @JNINamespace("content")

[boliu, 2012-08-10 01:17:19]

http://codereview.chromium.org/10823207/diff/6028/content/browser/android/loa...
File content/browser/android/load_url_params.cc (right):

http://codereview.chromium.org/10823207/diff/6028/content/browser/android/loa...
content/browser/android/load_url_params.cc:14: using
content::NavigationController;
RegisterConstants is in the anonymous namespace so it cannot be called from
other files and such. Is this not appropriate here?

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:367:
if (mNativeContentViewCore == 0) return;
This check is littered throughout this file. I'm guessing this is used for
protection during initialization and should never happen during actual usage. So
I don't think I should add a log for this cl. But we might decide to do this for
all the checks here in a separate cl.

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:14:
public class LoadUrlParams {
On 2012/08/09 21:32:31, Yaron wrote:
> Add @JNINamespace("content")

Done.

[Yaron, 2012-08-14 01:51:04]

lgtm

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:367:
if (mNativeContentViewCore == 0) return;
On 2012/08/10 01:17:19, boliu wrote:
> This check is littered throughout this file. I'm guessing this is used for
> protection during initialization and should never happen during actual usage.
So
> I don't think I should add a log for this cl. But we might decide to do this
for
> all the checks here in a separate cl.

Ya.. I think we should try and remove it throughout.

[boliu, 2012-08-14 23:00:33]

Hi Charlie, this is a refactor of Android only code that comes with a few gyp
changes. Could you take a look?

[Charlie Reis, 2012-08-15 17:48:38]

LGTM, with nits and one question about how to keep in sync.

http://codereview.chromium.org/10823207/diff/13001/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

http://codereview.chromium.org/10823207/diff/13001/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:217: if
(base_url_for_data_url)
nit: These need braces, since the blocks don't fit on one line.

http://codereview.chromium.org/10823207/diff/13001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

http://codereview.chromium.org/10823207/diff/13001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:17:
// Should match NavigationController::LoadUrlType exactly. See comments
Is there any way we can prevent this from getting out of sync?  If not an
automated way, maybe at least with a comment in navigation_controller.h?

http://codereview.chromium.org/10823207/diff/13001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:30:
// Fields with counterparts in NavigationController::LoadURLParams
nit: Period.

[boliu, 2012-08-15 18:36:19]

http://codereview.chromium.org/10823207/diff/13001/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

http://codereview.chromium.org/10823207/diff/13001/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:217: if
(base_url_for_data_url)
On 2012/08/15 17:48:38, creis wrote:
> nit: These need braces, since the blocks don't fit on one line.

Done.

http://codereview.chromium.org/10823207/diff/13001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

http://codereview.chromium.org/10823207/diff/13001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:17:
// Should match NavigationController::LoadUrlType exactly. See comments
Added comments at end of the enums in navigation_controller.h. PTAL

We don't have a way to generate java code from native code but maybe we should
add one for as other constants are being upstreamed.

http://codereview.chromium.org/10823207/diff/13001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:30:
// Fields with counterparts in NavigationController::LoadURLParams
On 2012/08/15 17:48:38, creis wrote:
> nit: Period.

Done.

[Charlie Reis, 2012-08-15 18:40:15]

Thanks.  Please also have an associated bug number for this CL.

LGTM.

[commit-bot: I haz the power, 2012-08-15 19:33:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/10823207/1013

[commit-bot: I haz the power, 2012-08-15 20:07:05]

Try job failure for 10823207-1013 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-08-15 20:54:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/10823207/1013

[commit-bot: I haz the power, 2012-08-15 23:33:17]

Change committed as 151784

[joth, 2012-08-16 01:26:42]

(late reply to yaron's comment)

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

http://codereview.chromium.org/10823207/diff/6028/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:367:
if (mNativeContentViewCore == 0) return;
On 2012/08/14 01:51:04, Yaron wrote:
> On 2012/08/10 01:17:19, boliu wrote:
> > This check is littered throughout this file. I'm guessing this is used for
> > protection during initialization and should never happen during actual
usage.
> So
> > I don't think I should add a log for this cl. But we might decide to do this
> for
> > all the checks here in a separate cl.
> 
> Ya.. I think we should try and remove it throughout.

it's for webview compat. Today, you can get away with:-
 WebView v = new WebView...
 v.destroy();
 v.loadUrl(..);  // silently ignored.

On the old plan, we had to mimic that behavior here. Now, we have a few other
layers we could handle it in. Before you change the design here, please
coordinate moving it somewhere else (so we don't loose one without ensuring the
other can actually meet all needs).

[mkosiba (inactive), 2012-08-16 13:43:01]

https://chromiumcodereview.appspot.com/10823207/diff/1013/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://chromiumcodereview.appspot.com/10823207/diff/1013/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:55:

any particular reason you omitted the Data url constructor?

[boliu, 2012-08-16 14:16:10]

https://chromiumcodereview.appspot.com/10823207/diff/1013/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://chromiumcodereview.appspot.com/10823207/diff/1013/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:55:

On 2012/08/16 13:43:01, Martin Kosiba wrote:
> any particular reason you omitted the Data url constructor?

The downstream version of this class has additional static factories for
creating the data urls. I didn't include in the upstream CL because they are not
being used by any code upstream yet.

If you mean why the base/virtual urls are initialized to null, it's because null
here translates to empty string in the native LoadURLParams which is the
default.