Upstream ContentView.getNavigationHistory()

Upstream ContentView.getNavigationHistory()

This function was added to support WebView.copyBackForwardList(). It
basically copies much of the content of the view's NavigationController
into the appropriate Java data structures.

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

[Jesse Greenwald, 2012-08-08 18:37:51]

yfriedman - can you take a look at this upstreaming CL?

thanks

[Ted C, 2012-08-08 20:41:33]

lgtm

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

http://codereview.chromium.org/10836155/diff/1002/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1114:
private void addToNavigationHistory(Object context, String url, String
virtualUrl,
context is a confusing name w/ regards to the view context.  something like
history might be better.

http://codereview.chromium.org/10836155/diff/1002/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1120:
public NavigationHistory getNavigationHistory() {
javadocz!

http://codereview.chromium.org/10836155/diff/1002/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/NavigationEntry.java
(right):

http://codereview.chromium.org/10836155/diff/1002/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/NavigationEntry.java:23:
/* package */NavigationEntry(String url, String virtualUrl, String originalUrl,
String title,
space after */

[Jesse Greenwald, 2012-08-08 20:54:54]

https://chromiumcodereview.appspot.com/10836155/diff/1002/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/10836155/diff/1002/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1114:
private void addToNavigationHistory(Object context, String url, String
virtualUrl,
On 2012/08/08 20:41:33, Ted C wrote:
> context is a confusing name w/ regards to the view context.  something like
> history might be better.

Done.

https://chromiumcodereview.appspot.com/10836155/diff/1002/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1120:
public NavigationHistory getNavigationHistory() {
On 2012/08/08 20:41:33, Ted C wrote:
> javadocz!

Done.

https://chromiumcodereview.appspot.com/10836155/diff/1002/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/NavigationEntry.java
(right):

https://chromiumcodereview.appspot.com/10836155/diff/1002/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/NavigationEntry.java:23:
/* package */NavigationEntry(String url, String virtualUrl, String originalUrl,
String title,
On 2012/08/08 20:41:33, Ted C wrote:
> space after */

Done.

[Yaron, 2012-08-08 22:50:49]

I'm a bit concerned about having a Java read-only copy of the navigation history
lying around. Seems like it might start being used for things other than webview
when those things could probably be working directly with the navigation
controller on the native side. Especially because it's arguably expensive to
construct because you have to marshal the bitmaps and copy a lot of state. Could
we keep this within a webview class or must it be exposed here?

[Jesse Greenwald, 2012-08-09 22:01:17]

Typically, we had been putting code that is webview specific directly into
ContentViewCore. Although lately things have been moving around quite a bit.
Maybe +joth can provide say whether this should go in ContentViewCore now or if
there's somewhere better to put it.

[joth, 2012-08-09 22:11:52]

On 2012/08/09 22:01:17, Jesse Greenwald wrote:
> Typically, we had been putting code that is webview specific directly into
> ContentViewCore. Although lately things have been moving around quite a bit.
> Maybe +joth can provide say whether this should go in ContentViewCore now or
if
> there's somewhere better to put it.

yep stuff has certainly been moving a lot.
Now we have crrev.com/150780 we can start bringing up WV-only java code into
android_webview.

Looking at ContentViewCoreImpl::GetNavigationHistory, this only uses public APIs
from content/public? If so, then yes I agree we can push these java files & JNI
bridging up to android_webview.
Usually boot strapping dance needed though to bring up that new java folder plus
associated javatests & a test runner.

[Ted C, 2012-08-09 22:58:00]

On 2012/08/09 22:11:52, joth wrote:
> On 2012/08/09 22:01:17, Jesse Greenwald wrote:
> > Typically, we had been putting code that is webview specific directly into
> > ContentViewCore. Although lately things have been moving around quite a bit.
> > Maybe +joth can provide say whether this should go in ContentViewCore now or
> if
> > there's somewhere better to put it.
> 
> yep stuff has certainly been moving a lot.
> Now we have crrev.com/150780 we can start bringing up WV-only java code into
> android_webview.
> 
> Looking at ContentViewCoreImpl::GetNavigationHistory, this only uses public
APIs
> from content/public? If so, then yes I agree we can push these java files &
JNI
> bridging up to android_webview.
> Usually boot strapping dance needed though to bring up that new java folder
plus
> associated javatests & a test runner.

FWIW, a long wanted feature I've wanted in Chrome on Android is that when you
long press on the back button we show a history like on the desktop, so this
would allow us to easily build this feature.

[Jesse Greenwald, 2012-08-09 23:08:36]

On 2012/08/09 22:58:00, Ted C wrote:
> On 2012/08/09 22:11:52, joth wrote:
> > On 2012/08/09 22:01:17, Jesse Greenwald wrote:
> > > Typically, we had been putting code that is webview specific directly into
> > > ContentViewCore. Although lately things have been moving around quite a
bit.
> > > Maybe +joth can provide say whether this should go in ContentViewCore now
or
> > if
> > > there's somewhere better to put it.
> > 
> > yep stuff has certainly been moving a lot.
> > Now we have crrev.com/150780 we can start bringing up WV-only java code into
> > android_webview.
> > 
> > Looking at ContentViewCoreImpl::GetNavigationHistory, this only uses public
> APIs
> > from content/public? If so, then yes I agree we can push these java files &
> JNI
> > bridging up to android_webview.
> > Usually boot strapping dance needed though to bring up that new java folder
> plus
> > associated javatests & a test runner.
> 
> FWIW, a long wanted feature I've wanted in Chrome on Android is that when you
> long press on the back button we show a history like on the desktop, so this
> would allow us to easily build this feature.

I'm fine putting this wherever.. if it should end up under android_webview, is
torne already working on the Java bring-up of it?

[joth, 2012-08-10 17:57:14]

LGTM. Lets land this in content:
- nav history is a good match for existing content layer responsibilities
- if used more heavily in chrome android we'd want to optimize it more (defer
the deep copy until necessary; refcount shared bitmaps), but those optimizations
would be equally useful for all users of the API anyway.

[Yaron, 2012-08-10 17:59:58]

On 2012/08/10 17:57:14, joth wrote:
> LGTM. Lets land this in content:
> - nav history is a good match for existing content layer responsibilities
> - if used more heavily in chrome android we'd want to optimize it more (defer
> the deep copy until necessary; refcount shared bitmaps), but those
optimizations
> would be equally useful for all users of the API anyway.

Sure. sorry, wasn't trying to be obstructionist, just wanted to see if there was
a better home. Regarding Ted's point: I think that could still be done in the
native layer without exposing this.

[commit-bot: I haz the power, 2012-08-10 18:03:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/10836155/8001

[commit-bot: I haz the power, 2012-08-10 18:03:05]

Presubmit check for 10836155-8001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/android

[Yaron, 2012-08-10 18:06:32]

official lgtm

[commit-bot: I haz the power, 2012-08-10 18:06:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/10836155/8001

[commit-bot: I haz the power, 2012-08-10 18:17:10]

Try job failure for 10836155-8001 on linux_chromeos for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

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

[commit-bot: I haz the power, 2012-08-10 22:29:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/10836155/8001

[commit-bot: I haz the power, 2012-08-10 23:53:48]

Change committed as 151155