WebContentsObserver and TabObserver update for PlzNavigate

WebContentsObserver and TabObserver update for PlzNavigate

This CL updates the WebContentsObserver and TabObserver interface to
add the new methods for PlzNavigate. The old and deprecated methods are
still there and will be removed in a subsequent CL including the usages
of the interface.

BUG=676139
Review-Url: https://codereview.chromium.org/2604543002
Cr-Commit-Position: refs/heads/master@{#442132}
Committed: https://chromium.googlesource.com/chromium/src/+/a4b7ae7967b24e8963eedc201171c34695d88f06

[shaktisahu, 2016-12-23 07:25:43]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-12-23 07:25:48]

The CQ bit was unchecked by shaktisahu@chromium.org

[shaktisahu, 2016-12-23 07:26:20]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 07:26:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 07:50:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 07:50:32]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[shaktisahu, 2016-12-23 07:57:06]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 07:57:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 08:05:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 08:05:09]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[shaktisahu, 2016-12-23 08:09:58]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 08:10:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 08:46:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 08:46:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[shaktisahu, 2016-12-29 04:02:28]

Patchset #2 (id:20001) has been deleted

[shaktisahu, 2016-12-29 04:03:12]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 04:03:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shaktisahu, 2016-12-29 04:50:16]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 04:50:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 04:52:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 04:52:21]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shaktisahu, 2016-12-29 06:24:23]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 06:24:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shaktisahu, 2016-12-29 06:24:40]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-12-29 06:48:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 06:48:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[shaktisahu, 2017-01-04 00:39:30]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 00:40:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 02:02:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 02:02:21]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[shaktisahu, 2017-01-05 23:43:37]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 23:44:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 00:24:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 00:24:47]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-01-06 21:17:43]

Description was changed from

==========
TabObserver update for PlzNavigate

This CL updates the TabWebContentsObserver and TabObserver interface to
use the new methods and get rid off the deprecated methods. All the
users of TabObserver were updated accordingly.

BUG=676139
==========

to

==========
WebContentsObserver and TabObserver update for PlzNavigate

This CL updates the WebContentsObserver and TabObserver interface to
add the new methods for PlzNavigate. The old and deprecated methods are
still there and will be removed in a subsequent CL including the usages
of the interface.

BUG=676139
==========

[shaktisahu, 2017-01-06 21:18:33]

Description was changed from

==========
WebContentsObserver and TabObserver update for PlzNavigate

This CL updates the WebContentsObserver and TabObserver interface to
add the new methods for PlzNavigate. The old and deprecated methods are
still there and will be removed in a subsequent CL including the usages
of the interface.

BUG=676139
==========

to

==========
WebContentsObserver and TabObserver update for PlzNavigate

This CL updates the WebContentsObserver and TabObserver interface to
add the new methods for PlzNavigate. The old and deprecated methods are
still there and will be removed in a subsequent CL including the usages
of the interface.

BUG=676139
==========

[shaktisahu, 2017-01-06 21:34:58]

shaktisahu@chromium.org changed reviewers:
+ tedchoc@chromium.org

[shaktisahu, 2017-01-06 21:34:59]

tedchoc@ - PTAL. As we spoke offline, I have split the CL. This is the first
part. I just have copied over most of the stuff
https://codereview.chromium.org/2598163002/ and added the methods to
TabObserver.

None of the call sites are hooked up with the new interface.

[Ted C, 2017-01-06 21:50:58]

tedchoc@chromium.org changed reviewers:
+ sky@chromium.org

[Ted C, 2017-01-06 21:51:00]

+sky for my comment in web_contents_observer_proxy.

https://codereview.chromium.org/2604543002/diff/120001/content/browser/androi...

https://codereview.chromium.org/2604543002/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/2604543002/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.cc:231: is_reload =
ui::PageTransitionCoreTypeIs(
pass the page transition type directly instead of is_reload.

There is a problem where this should be an invalid value before committed.  In
that case, I would consider passing -1 to the java WebContentsObserverProxy, but
I might change the java interface to pass a capital I Integer in the param list
and convert the -1 to null in that case.

Another option would be to add a new enum in c++ of

PAGE_TRANSITION_INVALID = -1; or something.  Might be cleaner and avoid the java
checking, but then you'd need to get sign of of all those owners.  I don't know
if it is worth floating the value by them first or if we should just isolate
this to java for now.

https://codereview.chromium.org/2604543002/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java
(right):

https://codereview.chromium.org/2604543002/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:51:
*                             document (for example scrolling to a named anchor
or PopState).
reduce intent to align with "Whether the " above.

https://codereview.chromium.org/2604543002/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java:56:
boolean hasCommitted, boolean isSamePage, boolean isReload, int errorCode) {}
Here if we pass Integer pageTransitionType, I would annotate it with @Nullable.

[sky, 2017-01-06 23:14:43]

https://codereview.chromium.org/2604543002/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/2604543002/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.cc:231: is_reload =
ui::PageTransitionCoreTypeIs(
On 2017/01/06 21:50:59, Ted C wrote:
> pass the page transition type directly instead of is_reload.
> 
> There is a problem where this should be an invalid value before committed.  In
> that case, I would consider passing -1 to the java WebContentsObserverProxy,
but
> I might change the java interface to pass a capital I Integer in the param
list
> and convert the -1 to null in that case.
> 
> Another option would be to add a new enum in c++ of
> 
> PAGE_TRANSITION_INVALID = -1; or something.  Might be cleaner and avoid the
java
> checking, but then you'd need to get sign of of all those owners.  I don't
know
> if it is worth floating the value by them first or if we should just isolate
> this to java for now.

If we add INVALID it means all existing code would have to deal with INVALID,
which it doesn't know. That would be confusing to many places and likely add a
slew of DCHECKs. As we've got this far within INVALID I'm inclined not to add it
now.

[Ted C, 2017-01-07 00:50:10]

On 2017/01/06 23:14:43, sky wrote:
>
https://codereview.chromium.org/2604543002/diff/120001/content/browser/androi...
> File content/browser/android/web_contents_observer_proxy.cc (right):
> 
>
https://codereview.chromium.org/2604543002/diff/120001/content/browser/androi...
> content/browser/android/web_contents_observer_proxy.cc:231: is_reload =
> ui::PageTransitionCoreTypeIs(
> On 2017/01/06 21:50:59, Ted C wrote:
> > pass the page transition type directly instead of is_reload.
> > 
> > There is a problem where this should be an invalid value before committed. 
In
> > that case, I would consider passing -1 to the java WebContentsObserverProxy,
> but
> > I might change the java interface to pass a capital I Integer in the param
> list
> > and convert the -1 to null in that case.
> > 
> > Another option would be to add a new enum in c++ of
> > 
> > PAGE_TRANSITION_INVALID = -1; or something.  Might be cleaner and avoid the
> java
> > checking, but then you'd need to get sign of of all those owners.  I don't
> know
> > if it is worth floating the value by them first or if we should just isolate
> > this to java for now.
> 
> If we add INVALID it means all existing code would have to deal with INVALID,
> which it doesn't know. That would be confusing to many places and likely add a
> slew of DCHECKs. As we've got this far within INVALID I'm inclined not to add
it
> now.

sgtm!  we'll isolate to java for now

[shaktisahu, 2017-01-07 00:53:21]

tedchoc@ - PTAL

[Ted C, 2017-01-07 00:57:06]

lgtm w/ a couple nits

https://codereview.chromium.org/2604543002/diff/140001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/TabObserver.java
(right):

https://codereview.chromium.org/2604543002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabObserver.java:266: * 
                           document (for example scrolling to a named anchor or
PopState).
same alignment nit (Whether the ...)

https://codereview.chromium.org/2604543002/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/2604543002/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:89:
Integer pageTransition = transition == -1 ? null : Integer.valueOf(transition);
I don't believe you need Integer.valueOf here as it will autobox it to Integer
for you.

[shaktisahu, 2017-01-07 01:02:48]

https://codereview.chromium.org/2604543002/diff/140001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/TabObserver.java
(right):

https://codereview.chromium.org/2604543002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabObserver.java:266: * 
                           document (for example scrolling to a named anchor or
PopState).
On 2017/01/07 00:57:05, Ted C wrote:
> same alignment nit (Whether the ...)

Done.

https://codereview.chromium.org/2604543002/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/2604543002/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:89:
Integer pageTransition = transition == -1 ? null : Integer.valueOf(transition);
On 2017/01/07 00:57:05, Ted C wrote:
> I don't believe you need Integer.valueOf here as it will autobox it to Integer
> for you.

Done. Thanks!

[shaktisahu, 2017-01-07 01:03:37]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-01-07 01:03:37]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2604543002/#ps160001
(title: "nits")

[commit-bot: I haz the power, 2017-01-07 01:04:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 01:41:24]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483751017477380,
"parent_rev": "3e2c0df8485250d7bd94ab90fc6a260bf89cef71", "commit_rev":
"a4b7ae7967b24e8963eedc201171c34695d88f06"}

[commit-bot: I haz the power, 2017-01-07 01:41:52]

Description was changed from

==========
WebContentsObserver and TabObserver update for PlzNavigate

This CL updates the WebContentsObserver and TabObserver interface to
add the new methods for PlzNavigate. The old and deprecated methods are
still there and will be removed in a subsequent CL including the usages
of the interface.

BUG=676139
==========

to

==========
WebContentsObserver and TabObserver update for PlzNavigate

This CL updates the WebContentsObserver and TabObserver interface to
add the new methods for PlzNavigate. The old and deprecated methods are
still there and will be removed in a subsequent CL including the usages
of the interface.

BUG=676139

Review-Url: https://codereview.chromium.org/2604543002
Cr-Commit-Position: refs/heads/master@{#442132}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4b7ae7967b24e8963eedc201171...
==========

[commit-bot: I haz the power, 2017-01-07 01:41:54]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/a4b7ae7967b24e8963eedc201171...