Refactor the implementation of the webNavigation extension API.

Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

This CL depends on new APIs added to NavigationHandle in https://codereview.chromium.org/1667163002/.

BUG=584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e419e2171ce22acc291c5c30c251633daeb0740d
Cr-Commit-Position: refs/heads/master@{#374505}

[nasko, 2016-02-04 23:42:21]

Description was changed from

==========
Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

BUG=584493
==========

to

==========
Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

This CL depends on new APIs added to NavigationHandle in
https://codereview.chromium.org/1667163002/.

BUG=584493
==========

[nasko, 2016-02-04 23:42:37]

nasko@chromium.org changed reviewers:
+ creis@chromium.org, rdevlin.cronin@chromium.org

[nasko, 2016-02-04 23:42:38]

Hey Charlie and Devlin,
This CL is the long promised refactor of webNavigation API to use the
NavigationHandle API. Can you review it for me?
Thanks in advance!
Nasko

[nasko, 2016-02-05 17:49:31]

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.h
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.h:25: class
FrameNavigationState {
I renamed this into FrameLoadingState in my big CL, but decided it isn't worth
the churn in this one. I will rename it as a follow up, to ensure it properly
reflects its new semantics.

[Devlin, 2016-02-05 18:30:28]

Should we update the docs to indicate that processId is invalid for these
events?

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(left):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:111:
NOTREACHED();
Why is this okay now?

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:151:
DCHECK(it != frame_host_state_map_.end());
We typically don't encourage coding like
if (unexpected_condition) {
  DCHECK()
  // Safely handle unexpected condition
}
// Behave normally

(with the exception of, e.g., IPC messages and the like).  But this API seems to
do it a lot - is there any particular reason?

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.h
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.h:102: bool
is_loading;        // True if there is a navigation going on.
this comment is weird, given that this is set to true from
DidFinishNavigation().  I think we should change it.  (See also other comment in
DidFinishNavigation)

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:281: void
WebNavigationTabObserver::DidFinishNavigation(
I'm sure it's just that I'm not as familiar with all these notifications, but
the flow of:
DidFinishNavigation -> StartTrackingDocumentLoad (which sets is_loading to true
on the navigation frame state - indicating "there is a navigation going on")
seems... odd.

I'm not really sure there's anything much to be done, but maybe we could add a
super short comment here, and/or rephrase the comment for is_loading?

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:285:
HandleError(navigation_handle);
HandleError vs DispatchOnErrorOccurred is a little weird, especially when
HandleError() just starts tracking the document load and then calls
DispatchOnErrorOccurred().

Maybe this would be cleaner as

bool did_commit = handle->HasCommitted();
bool is_error = !did_commit || handle->IsErrorPage();
if (is_error) {
  if (did_commit)
    StartTrackingDocumentLoad();
  DispatchOnErrorOccurred();
} else {
  HandleCommit();
}

or:
if (!handle->HasCommitted() || handle->IsErrorPage())
  HandleError();  // Add check for HasCommitted to HandleError()
else
  HandleCommit();

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:71:
int GetFrameId(content::NavigationHandle* navigation_handle) {
It's a shame that we duplicate the logic between here and
ExtensionApiFrameIdMap(), and, ideally, I'd prefer only the latter to need to
know that extension api frame ids are the FTN ID *or* 0 for the main frame *or*
-1 for an invalid frame, and so on.  Would it be too weird to add a
Get[Parent]FrameId(content::NavigationHandle*) in ExtensionApiFrameIdMap?

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:72:
if (navigation_handle->IsInMainFrame()) {
nit: If we keep this, I'd prefer a ternary here.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:97:
dict->SetInteger(keys::kTabIdKey, ExtensionTabUtil::GetTabId(
This is typically prettier when we use
api::web_navigation::OnBeforeNavigate::Details (also for others).

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:134:
transition_type = ui::PageTransitionFromInt(
Out of curiosity, why isn't this done as part of the transition type that
NavigationHandle::GetPageTransition() returns?

https://codereview.chromium.org/1670673003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessFragment/test_crossProcessFragment.js
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webnavigation/crossProcessFragment/test_crossProcessFragment.js:15:
let areExtensionsIsolated = config.isolateExtensions;
I could have sworn let only worked with "use strict";.  Maybe I'm
misremembering?

https://codereview.chromium.org/1670673003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js
(left):

https://codereview.chromium.org/1670673003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:114:
// There is no onBeforeNavigate and onErrorOccurred (like the other
Does this need to be updated? (since there are no tests that have
onBefore->onError->onBefore)

[nasko, 2016-02-05 23:41:22]

Thanks for pointing out the new and improved syntax for generating output for
events. It made it a lot more readable!

I didn't convert onCommitted, as it need some more messing around to properly
convert the PageTransitions type. I can try that later or in a separate CL.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(left):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:111:
NOTREACHED();
On 2016/02/05 18:30:28, Devlin wrote:
> Why is this okay now?

I am no longer tracking anything until the commit point. In order to know
whether the navigation was a reference fragment one, I need to check for
existing state before calling StartTrackingDocumentLoad. In that world, it is
expected for the map to not have any state for the frame, as this would be the
first navigation/document load in it, so it is fine to just return an empty
GURL.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:151:
DCHECK(it != frame_host_state_map_.end());
On 2016/02/05 18:30:28, Devlin wrote:
> We typically don't encourage coding like
> if (unexpected_condition) {
>   DCHECK()
>   // Safely handle unexpected condition
> }
> // Behave normally
> 
> (with the exception of, e.g., IPC messages and the like).  But this API seems
to
> do it a lot - is there any particular reason?

Unsure. It was written long ago, so probably remnant of the time. I would like
to not touch those parts, as it will make the refactor even bigger. If you feel
strongly about the syntax, I can do a follow up CL.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.h
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.h:102: bool
is_loading;        // True if there is a navigation going on.
On 2016/02/05 18:30:28, Devlin wrote:
> this comment is weird, given that this is set to true from
> DidFinishNavigation().  I think we should change it.  (See also other comment
in
> DidFinishNavigation)

In general, the model we want to move to is have clear separation between the
different stages of a "navigation". The navigation stage - the resource request
until commit time, and the document loading stage - once we've committed and
started parsing the document, until all subresource/subframe loads are complete.

I missed updating the comment.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:281: void
WebNavigationTabObserver::DidFinishNavigation(
On 2016/02/05 18:30:28, Devlin wrote:
> I'm sure it's just that I'm not as familiar with all these notifications, but
> the flow of:
> DidFinishNavigation -> StartTrackingDocumentLoad (which sets is_loading to
true
> on the navigation frame state - indicating "there is a navigation going on")
> seems... odd.
> 
> I'm not really sure there's anything much to be done, but maybe we could add a
> super short comment here, and/or rephrase the comment for is_loading?

I rephrased the comment on is_loading, which I missed initially.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:285:
HandleError(navigation_handle);
On 2016/02/05 18:30:28, Devlin wrote:
> HandleError vs DispatchOnErrorOccurred is a little weird, especially when
> HandleError() just starts tracking the document load and then calls
> DispatchOnErrorOccurred().
> 
> Maybe this would be cleaner as
> 
> bool did_commit = handle->HasCommitted();
> bool is_error = !did_commit || handle->IsErrorPage();
> if (is_error) {
>   if (did_commit)
>     StartTrackingDocumentLoad();
>   DispatchOnErrorOccurred();
> } else {
>   HandleCommit();
> }
> 
> or:
> if (!handle->HasCommitted() || handle->IsErrorPage())
>   HandleError();  // Add check for HasCommitted to HandleError()
> else
>   HandleCommit();

Done.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:71:
int GetFrameId(content::NavigationHandle* navigation_handle) {
On 2016/02/05 18:30:28, Devlin wrote:
> It's a shame that we duplicate the logic between here and
> ExtensionApiFrameIdMap(), and, ideally, I'd prefer only the latter to need to
> know that extension api frame ids are the FTN ID *or* 0 for the main frame
*or*
> -1 for an invalid frame, and so on.  Would it be too weird to add a
> Get[Parent]FrameId(content::NavigationHandle*) in ExtensionApiFrameIdMap?

Totally makes sense. Done.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:72:
if (navigation_handle->IsInMainFrame()) {
On 2016/02/05 18:30:28, Devlin wrote:
> nit: If we keep this, I'd prefer a ternary here.

Done.

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:97:
dict->SetInteger(keys::kTabIdKey, ExtensionTabUtil::GetTabId(
On 2016/02/05 18:30:28, Devlin wrote:
> This is typically prettier when we use
> api::web_navigation::OnBeforeNavigate::Details (also for others).

You asked for it! :)

https://codereview.chromium.org/1670673003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:134:
transition_type = ui::PageTransitionFromInt(
On 2016/02/05 18:30:28, Devlin wrote:
> Out of curiosity, why isn't this done as part of the transition type that
> NavigationHandle::GetPageTransition() returns?

Not sure. I'd rather not touch that though :), or at least not part of this CL.

https://codereview.chromium.org/1670673003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessFragment/test_crossProcessFragment.js
(right):

https://codereview.chromium.org/1670673003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webnavigation/crossProcessFragment/test_crossProcessFragment.js:15:
let areExtensionsIsolated = config.isolateExtensions;
On 2016/02/05 18:30:28, Devlin wrote:
> I could have sworn let only worked with "use strict";.  Maybe I'm
> misremembering?

Tends to work and I copied it from other places we use this statement. I could
switch it to use var, it won't change much. I usually like to keep things the
same when I copy/paste, so it can be easily grepped for in the future.

https://codereview.chromium.org/1670673003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js
(left):

https://codereview.chromium.org/1670673003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:114:
// There is no onBeforeNavigate and onErrorOccurred (like the other
On 2016/02/05 18:30:28, Devlin wrote:
> Does this need to be updated? (since there are no tests that have
> onBefore->onError->onBefore)

Done.

[Devlin, 2016-02-09 17:35:40]

lgtm

https://codereview.chromium.org/1670673003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/1670673003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:275: if
(!FrameNavigationState::IsValidUrl(navigation_handle->GetURL()))
nit: combine with above if?

https://codereview.chromium.org/1670673003/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1670673003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/web_navigation.json:140: "processId": {"type":
"integer", "description": "The value of -1, since the process which will render
the resulting document is not known until onCommit."},
I'd phrase this to include "deprecated" so it's clear that we're not just
passing -1 for funzies.  There's even a "deprecated" key - see e.g. line 37.

https://codereview.chromium.org/1670673003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/web_navigation.json:247: "processId": {"type":
"integer", "description": "The value of -1."},
ditto.  We also don't object to copy-pasting in docs, because they're more
likely to be used as quick references than detailed reading, so don't assume
people read the above entries.

https://codereview.chromium.org/1670673003/diff/60001/extensions/browser/exte...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1670673003/diff/60001/extensions/browser/exte...
extensions/browser/extension_api_frame_id_map.cc:81: 
nit: add // static (if you could also do this for existing static methods,
that'd be great).

https://codereview.chromium.org/1670673003/diff/60001/extensions/browser/exte...
extensions/browser/extension_api_frame_id_map.cc:84:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
Do these actually need to be done on the UI thread?  I mean, NavigationHandle is
a UI thread concept, but if this is the only place we're checking it, we have
bigger problems. :)  I'd prefer this class to only deal with checking for it's
own internal state (i.e., the map).

[nasko, 2016-02-09 17:53:54]

https://codereview.chromium.org/1670673003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/1670673003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:275: if
(!FrameNavigationState::IsValidUrl(navigation_handle->GetURL()))
On 2016/02/09 17:35:40, Devlin wrote:
> nit: combine with above if?

Done.

https://codereview.chromium.org/1670673003/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1670673003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/web_navigation.json:140: "processId": {"type":
"integer", "description": "The value of -1, since the process which will render
the resulting document is not known until onCommit."},
On 2016/02/09 17:35:40, Devlin wrote:
> I'd phrase this to include "deprecated" so it's clear that we're not just
> passing -1 for funzies.  There's even a "deprecated" key - see e.g. line 37.

Done.

https://codereview.chromium.org/1670673003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/web_navigation.json:247: "processId": {"type":
"integer", "description": "The value of -1."},
On 2016/02/09 17:35:40, Devlin wrote:
> ditto.  We also don't object to copy-pasting in docs, because they're more
> likely to be used as quick references than detailed reading, so don't assume
> people read the above entries.

Done.

https://codereview.chromium.org/1670673003/diff/60001/extensions/browser/exte...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1670673003/diff/60001/extensions/browser/exte...
extensions/browser/extension_api_frame_id_map.cc:81: 
On 2016/02/09 17:35:40, Devlin wrote:
> nit: add // static (if you could also do this for existing static methods,
> that'd be great).

Done.

https://codereview.chromium.org/1670673003/diff/60001/extensions/browser/exte...
extensions/browser/extension_api_frame_id_map.cc:84:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2016/02/09 17:35:40, Devlin wrote:
> Do these actually need to be done on the UI thread?  I mean, NavigationHandle
is
> a UI thread concept, but if this is the only place we're checking it, we have
> bigger problems. :)  I'd prefer this class to only deal with checking for it's
> own internal state (i.e., the map).

All the other Get* methods that access RFH also do the UI check, so I added it
to be consistent. If you want, I can remove it from all of them, but I'd rather
we be consistent - all have it or none.

[Devlin, 2016-02-09 18:01:16]

https://codereview.chromium.org/1670673003/diff/60001/extensions/browser/exte...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1670673003/diff/60001/extensions/browser/exte...
extensions/browser/extension_api_frame_id_map.cc:84:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2016/02/09 17:53:54, nasko wrote:
> On 2016/02/09 17:35:40, Devlin wrote:
> > Do these actually need to be done on the UI thread?  I mean,
NavigationHandle
> is
> > a UI thread concept, but if this is the only place we're checking it, we
have
> > bigger problems. :)  I'd prefer this class to only deal with checking for
it's
> > own internal state (i.e., the map).
> 
> All the other Get* methods that access RFH also do the UI check, so I added it
> to be consistent. If you want, I can remove it from all of them, but I'd
rather
> we be consistent - all have it or none.

Let's remove it for all static methods, where by definition there's no internal
state to be accessed.

[nasko, 2016-02-09 18:04:42]

Description was changed from

==========
Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

This CL depends on new APIs added to NavigationHandle in
https://codereview.chromium.org/1667163002/.

BUG=584493
==========

to

==========
Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

This CL depends on new APIs added to NavigationHandle in
https://codereview.chromium.org/1667163002/.

BUG=584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-02-09 18:04:54]

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

[commit-bot: I haz the power, 2016-02-09 18:05:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670673003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670673003/100001

[commit-bot: I haz the power, 2016-02-09 20:01:03]

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

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

Dry run: This issue passed the CQ dry run.

[nasko, 2016-02-09 20:04:08]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-02-09 20:04:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1670673003/#ps100001
(title: "Remove UI thread DCHECKs.")

[commit-bot: I haz the power, 2016-02-09 20:09:26]

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

[commit-bot: I haz the power, 2016-02-09 20:09:26]

This CL has an open dependency (Issue 1667163002 Patch 60001). Please resolve
the dependency and try again.

[nasko, 2016-02-09 22:18:11]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2016-02-09 22:19:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670673003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670673003/100001

[commit-bot: I haz the power, 2016-02-09 22:41:18]

Description was changed from

==========
Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

This CL depends on new APIs added to NavigationHandle in
https://codereview.chromium.org/1667163002/.

BUG=584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

This CL depends on new APIs added to NavigationHandle in
https://codereview.chromium.org/1667163002/.

BUG=584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-02-09 22:41:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-09 22:43:00]

Description was changed from

==========
Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

This CL depends on new APIs added to NavigationHandle in
https://codereview.chromium.org/1667163002/.

BUG=584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactor the implementation of the webNavigation extension API.

The webNavigation extension API does not work properly with out-of-process
iframes and will be broken with the upcoming changes in navigation
code (PlzNavigate). This CL is aiming to refactor the implementation to
use the NavigationHandle API, which is compatible with PlzNavigate and
helps hide some of the nasty details of cross-process navigations.

This CL depends on new APIs added to NavigationHandle in
https://codereview.chromium.org/1667163002/.

BUG=584493
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e419e2171ce22acc291c5c30c251633daeb0740d
Cr-Commit-Position: refs/heads/master@{#374505}
==========

[commit-bot: I haz the power, 2016-02-09 22:43:01]

Patchset 6 (id:??) landed as
https://crrev.com/e419e2171ce22acc291c5c30c251633daeb0740d
Cr-Commit-Position: refs/heads/master@{#374505}