Convert ExtensionLoaderHandler to use the new navigation callbacks.

Convert ExtensionLoaderHandler to use the new navigation callbacks.

BUG=682002
Review-Url: https://codereview.chromium.org/2648803002
Cr-Commit-Position: refs/heads/master@{#445550}
Committed: https://chromium.googlesource.com/chromium/src/+/062a42c6769d9d6f6a3ce699c0acc758636f924e

[jam, 2017-01-20 18:31:11]

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

[jam, 2017-01-20 18:31:18]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-20 18:31:23]

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-20 18:43:12]

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

[commit-bot: I haz the power, 2017-01-20 18:43:13]

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

[jam, 2017-01-20 19:09:55]

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

[commit-bot: I haz the power, 2017-01-20 19:10:34]

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-20 19:49:01]

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

[commit-bot: I haz the power, 2017-01-20 19:49:01]

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

[jam, 2017-01-20 23:45:41]

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

[jam, 2017-01-20 23:45:52]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-20 23:45:59]

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

[jam, 2017-01-20 23:46:06]

Description was changed from

==========
Convert ExtensionLoaderHandler to use the new navigation callbacks.

BUG=682002
==========

to

==========
Convert ExtensionLoaderHandler to use the new navigation callbacks.

BUG=682002
==========

[jam, 2017-01-20 23:46:06]

jam@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[jam, 2017-01-20 23:59:50]

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

[commit-bot: I haz the power, 2017-01-20 23:59:58]

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-21 01:33:24]

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

[commit-bot: I haz the power, 2017-01-21 01:33:25]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-01-21 02:01:17]

https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:173: void
ExtensionLoaderHandler::ReadyToCommitNavigation(
The cheatsheet linked in the bug and the comments in WebContentsObserver seem to
indicate that DidStartNavigationToPendingEntry is replaced by DidStartNavigation
- why's this differ?

https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:175: if
(!navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage())
Why do we need the IsInMainFrame() check?  And won't IsSamePage() always be
false for PAGE_TRANSITION_RELOAD? (which, separately, is a little confusing,
since it is the same page, but that's a different discussion...)

[jam, 2017-01-21 05:30:57]

https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:173: void
ExtensionLoaderHandler::ReadyToCommitNavigation(
On 2017/01/21 02:01:17, Devlin (slow) wrote:
> The cheatsheet linked in the bug and the comments in WebContentsObserver seem
to
> indicate that DidStartNavigationToPendingEntry is replaced by
DidStartNavigation
> - why's this differ?

Generally that's the replacement. In this case, the method needs the transition
type. That's not available in DidStartNavigation. that could be done, but only
with PlzNavigate, so I filed 683402 to track making this getter available early
once PlzNavigate is the only code path. So in the meantime, this is as early as
possible to get the transition type.

Note I've manually tested this cl and the other one, with and without
PlzNavigate, to make sure the user behavior stays the same.

https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:175: if
(!navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage())
On 2017/01/21 02:01:17, Devlin (slow) wrote:
> Why do we need the IsInMainFrame() check?

To keep the same behavior as before, since DidStartNavigationToPendingEntry
fired only for main frames while DidStartNavigation also fires for subframes.

>  And won't IsSamePage() always be
> false for PAGE_TRANSITION_RELOAD? (which, separately, is a little confusing,
> since it is the same page, but that's a different discussion...)

Yep that's true; I had kept it in to keep the same behavior as before but you're
right it's redundant. Removed.

[jam, 2017-01-21 05:31:04]

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

[commit-bot: I haz the power, 2017-01-21 05:31: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, 2017-01-21 06:28:47]

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

[commit-bot: I haz the power, 2017-01-21 06:28:48]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-21 07:17:54]

https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:173: void
ExtensionLoaderHandler::ReadyToCommitNavigation(
actually, turns out that my change for ContentFaviconDriver necessitated
plumbing more reload information to NavigationHandle which is available at
construction.

hold off reviewing this, when that other cl lands I can change this and the
other extensions cl to use DidStartNavigation.

[Devlin, 2017-01-23 17:19:10]

On 2017/01/21 07:17:54, jam wrote:
>
https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):
> 
>
https://codereview.chromium.org/2648803002/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/extensions/extension_loader_handler.cc:173: void
> ExtensionLoaderHandler::ReadyToCommitNavigation(
> actually, turns out that my change for ContentFaviconDriver necessitated
> plumbing more reload information to NavigationHandle which is available at
> construction.
> 
> hold off reviewing this, when that other cl lands I can change this and the
> other extensions cl to use DidStartNavigation.

acknowledged; ping again when ready. :)

[jam, 2017-01-23 22:28:50]

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

[commit-bot: I haz the power, 2017-01-23 22:29:28]

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-23 23:22:22]

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

[commit-bot: I haz the power, 2017-01-23 23:22:23]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-23 23:46:05]

ptal

[Devlin, 2017-01-23 23:55:41]

On 2017/01/23 23:46:05, jam wrote:
> ptal

Super simple!  I like it.

LGTM

[jam, 2017-01-23 23:56:25]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-01-23 23:57:03]

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-24 00:04:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485215785452140,
"parent_rev": "ec77ce738b4415bb5d2bf6aa2779931a06676dfe", "commit_rev":
"062a42c6769d9d6f6a3ce699c0acc758636f924e"}

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

Description was changed from

==========
Convert ExtensionLoaderHandler to use the new navigation callbacks.

BUG=682002
==========

to

==========
Convert ExtensionLoaderHandler to use the new navigation callbacks.

BUG=682002

Review-Url: https://codereview.chromium.org/2648803002
Cr-Commit-Position: refs/heads/master@{#445550}
Committed:
https://chromium.googlesource.com/chromium/src/+/062a42c6769d9d6f6a3ce699c0ac...
==========

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

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/062a42c6769d9d6f6a3ce699c0ac...