Export GetLastCommittedEntryIndex and RemoveEntryAtIndex to Java layer

Export GetLastCommittedEntryIndex and RemoveEntryAtIndex to Java layer

BUG=426679
Committed: https://crrev.com/45bc8c5795b4cedfea42beb9d71fc8e3ebd1692d
Cr-Commit-Position: refs/heads/master@{#302408}

[Jaekyun Seok (inactive), 2014-10-24 10:30:46]

jaekyun@chromium.org changed reviewers:
+ kkimlabs@chromium.org, tedchoc@chromium.org

[Jaekyun Seok (inactive), 2014-10-24 10:30:46]

Please review this change.

[Jaekyun Seok (inactive), 2014-10-24 10:33:15]

FYI, this change was made for
https://chrome-internal-review.googlesource.com/#/c/180952/ .

[Ted C, 2014-10-24 17:45:49]

lgtm

[Kibeom Kim (inactive), 2014-10-27 08:38:59]

lgtm

[Jaekyun Seok (inactive), 2014-10-29 03:48:52]

jaekyun@chromium.org changed reviewers:
+ creis@chromium.org, sky@chromium.org

[Jaekyun Seok (inactive), 2014-10-29 03:48:53]

Please review this change.

[Ted C, 2014-10-29 04:13:43]

On 2014/10/29 03:48:53, Jaekyun Seok wrote:
> Please review this change.

FYI, you should say what you want people to review.  I assume you need OWNERS
approval for content/browser/frame_host, but you want to be explicit about that
to get a faster turn around.

[Jaekyun Seok (inactive), 2014-10-29 04:21:35]

Sorry for my bad review etiquette. I'm not yet familiar with getting review in
chromium.

I will keep your advice in mind.

Scott and Charlie, could you please review my changes on
content/browser/frame_host/navigation_controller_android.h/cc?

I exported GetLastCommittedEntryIndex and RemoveEntryAtIndex to Java layer.

[Charlie Reis, 2014-10-29 17:06:48]

Should be fine, but I'd like to understand how the remove call will be used.

https://codereview.chromium.org/675133002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_android.h (right):

https://codereview.chromium.org/675133002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_android.h:81: jboolean
RemoveEntryAtIndex(JNIEnv* env, jobject obj, jint index);
Just curious, why do you need RemoveEntryAtIndex?  Anything that removes entries
at an arbitrary position is a bit subtle in practice, so I'd like to be sure
there's a good reason for it.  (We've had to restrict the Prune methods using
CanPruneAllButLastCommitted, for example.)

(Also, I think I just spotted a bug in that method: it doesn't update the
pending_entry_index_ after removal.  I'll file and fix that, though.)

[Charlie Reis, 2014-10-29 20:20:55]

https://codereview.chromium.org/675133002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_android.h (right):

https://codereview.chromium.org/675133002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_android.h:81: jboolean
RemoveEntryAtIndex(JNIEnv* env, jobject obj, jint index);
On 2014/10/29 17:06:47, Charlie Reis wrote:
> Just curious, why do you need RemoveEntryAtIndex?  Anything that removes
entries
> at an arbitrary position is a bit subtle in practice, so I'd like to be sure
> there's a good reason for it.  (We've had to restrict the Prune methods using
> CanPruneAllButLastCommitted, for example.)
> 
> (Also, I think I just spotted a bug in that method: it doesn't update the
> pending_entry_index_ after removal.  I'll file and fix that, though.)

For reference, I filed http://crbug.com/428380 for it.  It turns out it's not
buggy but has potentially unexpected behavior: it clears the pending entry. 
That may affect your use case, which is part of why I'm curious about it.

https://codereview.chromium.org/675133002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
(right):

https://codereview.chromium.org/675133002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:151:
* Removes the entry at the specified |index|.
Can you also update this to reflect
https://codereview.chromium.org/684173004/diff/1/content/public/browser/navig...

It's important to note that this will clear pending and transient entries if it
succeeds.

[Jaekyun Seok (inactive), 2014-10-30 04:42:34]

PTAL.

https://codereview.chromium.org/675133002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java
(right):

https://codereview.chromium.org/675133002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java:151:
* Removes the entry at the specified |index|.
On 2014/10/29 20:20:55, Charlie Reis wrote:
> Can you also update this to reflect
>
https://codereview.chromium.org/684173004/diff/1/content/public/browser/navig...
> 
> It's important to note that this will clear pending and transient entries if
it
> succeeds.

Done.

[Charlie Reis, 2014-10-31 20:38:52]

Thanks for updating the comment and explaining the use case (offline).  LGTM.

[Jaekyun Seok (inactive), 2014-11-02 23:24:09]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2014-11-02 23:25:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/675133002/20001

[commit-bot: I haz the power, 2014-11-03 01:00:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-03 01:01:07]

Patchset 2 (id:??) landed as
https://crrev.com/45bc8c5795b4cedfea42beb9d71fc8e3ebd1692d
Cr-Commit-Position: refs/heads/master@{#302408}