[Extensions] Remove ExtensionWebUI

[Extensions] Remove ExtensionWebUI

ExtensionWebUI only did two things:
- Set the link transition type for the bookmark manager to
  PAGE_TRANSITION_AUTO_BOOKMARK.
- Instantiated the BookmarkManagerPrivateDragEventRouter.

Neither of these things should require WebUI, and having extension
WebContents have WebUI is bad.

Instead, handle the link transition in OverrideOpenUrlParams (as well
as modifying the referrer and the renderer-initiated, as we
currently do for WebUI navigations), and make the
BookmarkManagerPrivateDragEventRouter a WebContentsUserData
instantiated from extensions::TabHelper.

BUG=659798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[Devlin, 2016-10-31 22:59:47]

Description was changed from

==========
webui
BUG=
==========

to

==========
webui
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Devlin, 2016-10-31 23:00:25]

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

[commit-bot: I haz the power, 2016-10-31 23:01:02]

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-11-01 00:32:38]

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

[commit-bot: I haz the power, 2016-11-01 00:32:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2016-11-01 16:52:45]

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

[commit-bot: I haz the power, 2016-11-01 16:53:02]

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-11-01 17:31:07]

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

[commit-bot: I haz the power, 2016-11-01 17:31:08]

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

[Devlin, 2016-11-02 19:41:32]

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

[commit-bot: I haz the power, 2016-11-02 19:42:07]

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

[Devlin, 2016-11-02 21:55:48]

Description was changed from

==========
webui
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[Extensions] Remove ExtensionWebUI

ExtensionWebUI only did two things:
- Set the link transition type for the bookmark manager to
  PAGE_TRANSITION_AUTO_BOOKMARK.
- Instantiated the BookmarkManagerPrivateDragEventRouter.

Neither of these things should require WebUI, and having extension
WebContents have WebUI is bad.

Instead, handle the link transition in OverrideOpenUrlParams (as well
as modifying the referrer and the renderer-initiated, as we
currently do for WebUI navigations), and make the
BookmarkManagerPrivateDragEventRouter a WebContentsUserData
instantiated from extensions::TabHelper.

BUG=659798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Devlin, 2016-11-02 22:01:42]

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

[Devlin, 2016-11-02 22:01:43]

Charlie, mind taking a look?

https://codereview.chromium.org/2468673003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2468673003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:561:
if (extension->is_extension())
Note: this was the behavior prior to
https://chromium.googlesource.com/chromium/src/+/f9bc386d7f73366b45cb85848d27...,
and in retrospect probably should have been maintained.

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:786: void
NavigatorImpl::RequestTransferURL(
There's no ContentBrowserClient::OverrideTransferURL-type method, so we lose the
ability to update the transition type and set the referer here.  Any
suggestions?  Would a new method on ContentBrowserClient be the way to go,
or...?

[commit-bot: I haz the power, 2016-11-02 22:30:39]

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

[commit-bot: I haz the power, 2016-11-02 22:30:40]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-11-03 17:03:14]

Thanks!  I'll try to review today, but I'm trying to first wrap up a few
questions about the implications of the previous CL in https://crbug.com/660498.

[Charlie Reis, 2016-11-04 23:31:59]

This is great.

We've found on https://crbug.com/660498 that the is_renderer_initiated = false
part had no effect on whether extensions can navigate to chrome:// URLs or not,
so I'm less concerned about regressions.  And that's great, because I love the
idea of not having WebUI objects for them anymore.

LGTM with one suggestion on the OverrideOpenURLParams issue.

https://codereview.chromium.org/2468673003/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2468673003/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1234: params->referrer =
content::Referrer();
It's a bit unfortunate that we have 4 copies of this code, but I guess each one
is a bit different.  I suppose we can live with it.  :)

https://codereview.chromium.org/2468673003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2468673003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:561:
if (extension->is_extension())
On 2016/11/02 22:01:43, Devlin (slow) wrote:
> Note: this was the behavior prior to
>
https://chromium.googlesource.com/chromium/src/+/f9bc386d7f73366b45cb85848d27...,
> and in retrospect probably should have been maintained.

Agreed.

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:775: params.transition =
render_frame_host->web_ui()->GetLinkTransitionType();
Sanity check: No other actual WebUI pages rely on this?  I see the NTP is
mentioned here, but that got switched away from WebUI quite some time ago.  If
not, I'm thrilled to remove some of this complexity.

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:786: void
NavigatorImpl::RequestTransferURL(
On 2016/11/02 22:01:43, Devlin (slow) wrote:
> There's no ContentBrowserClient::OverrideTransferURL-type method, so we lose
the
> ability to update the transition type and set the referer here.  Any
> suggestions?  Would a new method on ContentBrowserClient be the way to go,
> or...?

Interesting.  It would be nice to unify these if possible.  The only current
implementation of OverrideOpenURLParams is for exactly this behavior (for the
NTP).  I wonder if we should rename it to OverrideNavigationParams and pass in
references to transition, is_renderer_initiated, and referrer.  Then we could
call it from both RequestOpenURL and RequestTransferURL, and support the NTP,
bookmarks, and other extension cases.

It's a bit less general purpose for the RequestOpenURLParams case, but I think
that may be ok.  Thoughts?

[Devlin, 2016-11-05 01:23:16]

rdevlin.cronin@chromium.org changed reviewers:
+ asargent@chromium.org, dbeam@chromium.org

[Devlin, 2016-11-05 01:23:17]

+dbeam for webui
+asargent for extensions

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:775: params.transition =
render_frame_host->web_ui()->GetLinkTransitionType();
On 2016/11/04 23:31:59, Charlie Reis (OOO til 11-11) wrote:
> Sanity check: No other actual WebUI pages rely on this?  I see the NTP is
> mentioned here, but that got switched away from WebUI quite some time ago.  If
> not, I'm thrilled to remove some of this complexity.

No other WebUI ever sets a transition type (and the default value is set to
link) and no tests fail, so I'm pretty sure not.

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2468673003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:786: void
NavigatorImpl::RequestTransferURL(
On 2016/11/04 23:31:59, Charlie Reis (OOO til 11-11) wrote:
> On 2016/11/02 22:01:43, Devlin (slow) wrote:
> > There's no ContentBrowserClient::OverrideTransferURL-type method, so we lose
> the
> > ability to update the transition type and set the referer here.  Any
> > suggestions?  Would a new method on ContentBrowserClient be the way to go,
> > or...?
> 
> Interesting.  It would be nice to unify these if possible.  The only current
> implementation of OverrideOpenURLParams is for exactly this behavior (for the
> NTP).  I wonder if we should rename it to OverrideNavigationParams and pass in
> references to transition, is_renderer_initiated, and referrer.  Then we could
> call it from both RequestOpenURL and RequestTransferURL, and support the NTP,
> bookmarks, and other extension cases.
> 
> It's a bit less general purpose for the RequestOpenURLParams case, but I think
> that may be ok.  Thoughts?

Sounds good to me!  Done.

https://codereview.chromium.org/2468673003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_web_ui.h (right):

https://codereview.chromium.org/2468673003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_web_ui.h:29: // TODO(devlin): Rename this
class to ExtensionURLOverrides.
Dan: I have a patch ready to do this (you can even see it in PS2), but I
realized it made this CL a lot more fragmented.  I'll do it immediately in a
followup. :)

[asargent_no_longer_on_chrome, 2016-11-07 20:52:16]

lgtm

[Charlie Reis, 2016-11-07 21:00:03]

LGTM!

https://codereview.chromium.org/2468673003/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2468673003/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:239: // opening new urls and
transfer urls.
nit: opening new URLs and when transferring URLs across processes.

[Devlin, 2016-11-07 21:01:59]

rdevlin.cronin@chromium.org changed reviewers:
+ michaelpg@chromium.org
- dbeam@chromium.org

[Devlin, 2016-11-07 21:02:00]

Forgot that Dan's out today.  dbeam -> cc, +michaelpg.  Michael, mind looking at
c/b/ui/webui?

[michaelpg, 2016-11-08 23:36:41]

c/b/ui/webui lgtm

[Devlin, 2016-11-09 02:01:50]

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

[commit-bot: I haz the power, 2016-11-09 02:02:37]

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-11-09 03:58:32]

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

[commit-bot: I haz the power, 2016-11-09 03:58:32]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-09 15:45:54]

https://codereview.chromium.org/2468673003/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2468673003/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:239: // opening new urls and
transfer urls.
On 2016/11/07 21:00:03, Charlie Reis (OOO til 11-11) wrote:
> nit: opening new URLs and when transferring URLs across processes.

Done.

[Devlin, 2016-11-09 15:46:05]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-11-09 15:46:06]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
asargent@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2468673003/#ps100001
(title: "nit")

[commit-bot: I haz the power, 2016-11-09 15:46:17]

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-11-09 15:50:49]

Description was changed from

==========
[Extensions] Remove ExtensionWebUI

ExtensionWebUI only did two things:
- Set the link transition type for the bookmark manager to
  PAGE_TRANSITION_AUTO_BOOKMARK.
- Instantiated the BookmarkManagerPrivateDragEventRouter.

Neither of these things should require WebUI, and having extension
WebContents have WebUI is bad.

Instead, handle the link transition in OverrideOpenUrlParams (as well
as modifying the referrer and the renderer-initiated, as we
currently do for WebUI navigations), and make the
BookmarkManagerPrivateDragEventRouter a WebContentsUserData
instantiated from extensions::TabHelper.

BUG=659798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[Extensions] Remove ExtensionWebUI

ExtensionWebUI only did two things:
- Set the link transition type for the bookmark manager to
  PAGE_TRANSITION_AUTO_BOOKMARK.
- Instantiated the BookmarkManagerPrivateDragEventRouter.

Neither of these things should require WebUI, and having extension
WebContents have WebUI is bad.

Instead, handle the link transition in OverrideOpenUrlParams (as well
as modifying the referrer and the renderer-initiated, as we
currently do for WebUI navigations), and make the
BookmarkManagerPrivateDragEventRouter a WebContentsUserData
instantiated from extensions::TabHelper.

BUG=659798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-09 15:50:50]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-09 15:54:18]

Description was changed from

==========
[Extensions] Remove ExtensionWebUI

ExtensionWebUI only did two things:
- Set the link transition type for the bookmark manager to
  PAGE_TRANSITION_AUTO_BOOKMARK.
- Instantiated the BookmarkManagerPrivateDragEventRouter.

Neither of these things should require WebUI, and having extension
WebContents have WebUI is bad.

Instead, handle the link transition in OverrideOpenUrlParams (as well
as modifying the referrer and the renderer-initiated, as we
currently do for WebUI navigations), and make the
BookmarkManagerPrivateDragEventRouter a WebContentsUserData
instantiated from extensions::TabHelper.

BUG=659798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[Extensions] Remove ExtensionWebUI

ExtensionWebUI only did two things:
- Set the link transition type for the bookmark manager to
  PAGE_TRANSITION_AUTO_BOOKMARK.
- Instantiated the BookmarkManagerPrivateDragEventRouter.

Neither of these things should require WebUI, and having extension
WebContents have WebUI is bad.

Instead, handle the link transition in OverrideOpenUrlParams (as well
as modifying the referrer and the renderer-initiated, as we
currently do for WebUI navigations), and make the
BookmarkManagerPrivateDragEventRouter a WebContentsUserData
instantiated from extensions::TabHelper.

BUG=659798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-11-09 15:54:19]

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