Allow links inside <extensionoptions> to be opened in new tabs

Allow links inside <extensionoptions> to be opened in new tabs

Extension options pages often have links to external pages as well as
internal extension pages. When embedded in an <extensionoptions> element
open external links in a new tab, and internal links in the embedded
view (unless specified otherwise).

BUG=386838
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291241

[ericzeng, 2014-08-14 23:05:01]

PTAL

[not at google - send to devlin, 2014-08-14 23:14:37]

You should be able to test this.

Fady any idea who might be able to answer the Referrer question?

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:207:
content::Referrer(),
I'm not sure if there should be a Referrer here. This link is definitely coming
from somewhere - the URL of the host - but I'm not sure what the
WebReferrerPolicy should be. Nor if it's actually relevant in this case.

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:212:
browser->window()->Show();
Ideally we'd respect choices like "open in new tab" etc. What are the context
menu options on the link? How do the arguments change?

https://codereview.chromium.org/471053003/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/471053003/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1516: } else {
you might want to undo this, so that you don't need a content owner to approve
this patch :)

[Fady Samuel, 2014-08-14 23:24:21]

This CL doesn't seem right. I just left the office. I'll review in depth later
tonight.

[ericzeng, 2014-08-14 23:39:46]

On 2014/08/14 23:24:21, Fady Samuel wrote:
> This CL doesn't seem right. I just left the office. I'll review in depth later
> tonight.

I gave it my best shot :/
I'm not confident that this is the right way to do it either, I couldn't find a
definitive place in the guest view/browser plugin where it handles opening
links. I think what may be going on is that a new webcontents is being created
for the linked content, but then a new tab is just opened without using that
webcontents. However it seems to work (mostly) and I'll see about tests.

[Fady Samuel, 2014-08-15 14:14:32]

I would implement WebContentsDelegate::ShouldCreateWebContents in
ExtensionOptionsGuest. Grab the URL, load it in the browser and return false.

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

[ericzeng, 2014-08-15 20:08:23]

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:207:
content::Referrer(),
On 2014/08/14 23:14:37, kalman wrote:
> I'm not sure if there should be a Referrer here. This link is definitely
coming
> from somewhere - the URL of the host - but I'm not sure what the
> WebReferrerPolicy should be. Nor if it's actually relevant in this case.

Are you saying that I should be finding what the referrer would be if this was
in a tab and using that instead?

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:212:
browser->window()->Show();
On 2014/08/14 23:14:37, kalman wrote:
> Ideally we'd respect choices like "open in new tab" etc. What are the context
> menu options on the link? How do the arguments change?

The link context menu has all of the normal options - open link in new tab, open
link in new window, open link in incognito window, save link as... etc. None of
the navigation options work right now, I'll see if I can fix that in the next
patch.

https://codereview.chromium.org/471053003/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/471053003/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1516: } else {
On 2014/08/14 23:14:37, kalman wrote:
> you might want to undo this, so that you don't need a content owner to approve
> this patch :)

ugh

[not at google - send to devlin, 2014-08-15 20:14:32]

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:207:
content::Referrer(),
On 2014/08/15 20:08:22, ericzeng wrote:
> On 2014/08/14 23:14:37, kalman wrote:
> > I'm not sure if there should be a Referrer here. This link is definitely
> coming
> > from somewhere - the URL of the host - but I'm not sure what the
> > WebReferrerPolicy should be. Nor if it's actually relevant in this case.
> 
> Are you saying that I should be finding what the referrer would be if this was
> in a tab and using that instead?

I would have thought that in a tab the referrer would be the
chrome-extensions:// page, but I actually don't see that being set for
chrome-extensions:// even in a tab. I guess we hide it, probably deliberately.
So you have nothing to worry about here.

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:212:
browser->window()->Show();
On 2014/08/15 20:08:22, ericzeng wrote:
> On 2014/08/14 23:14:37, kalman wrote:
> > Ideally we'd respect choices like "open in new tab" etc. What are the
context
> > menu options on the link? How do the arguments change?
> 
> The link context menu has all of the normal options - open link in new tab,
open
> link in new window, open link in incognito window, save link as... etc. None
of
> the navigation options work right now, I'll see if I can fix that in the next
> patch. 

SGTM.

[Fady Samuel, 2014-08-15 20:22:06]

lgtm with nit! You're making awesome progress Eric!

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/471053003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:212:
browser->window()->Show();
On 2014/08/15 20:08:22, ericzeng wrote:
> On 2014/08/14 23:14:37, kalman wrote:
> > Ideally we'd respect choices like "open in new tab" etc. What are the
context
> > menu options on the link? How do the arguments change?
> 
> The link context menu has all of the normal options - open link in new tab,
open
> link in new window, open link in incognito window, save link as... etc. None
of
> the navigation options work right now, I'll see if I can fix that in the next
> patch. 

Take a look at the link below for more information on how to customize the
context menu of your element.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/gue...

https://codereview.chromium.org/471053003/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/471053003/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:195:
Browser* browser =
A comment on what this does would be very helpful for future readers. :-)

Also comment on why we're returning false: Since we are loading the link in a
browser window, we do not want to create a new WebContents here.

[not at google - send to devlin, 2014-08-15 20:31:38]

lgtm from me as well

https://codereview.chromium.org/471053003/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/471053003/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:203:
browser->window()->Show();
You should also add a TODO to respect the user's choice about how they want the
link opened. *Hopefully* there is code somewhere which implements that for you
without you needing to duplicate it. But, for example, you wouldn't go to the
new tab if it was a ctrl-click.

Btw - why the need for Show() at all? If the browser is in the foreground
shouldn't the URL it opens be automatically in the foreground?

[Fady Samuel, 2014-08-15 20:35:51]

https://codereview.chromium.org/471053003/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/471053003/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:203:
browser->window()->Show();
On 2014/08/15 20:31:38, kalman wrote:
> You should also add a TODO to respect the user's choice about how they want
the
> link opened. *Hopefully* there is code somewhere which implements that for you
> without you needing to duplicate it. But, for example, you wouldn't go to the
> new tab if it was a ctrl-click.
> 
> Btw - why the need for Show() at all? If the browser is in the foreground
> shouldn't the URL it opens be automatically in the foreground?

Hmm, this can be determined by the disposition of the new window. Unfortunately,
it doesn't seem like the window disposition is passed in on
ShouldCreateWebContents. We might need to add that to the content API.

[ericzeng, 2014-08-15 20:36:48]

Is this really ready to commit? It still feels a little hacky, it doesn't work
on a few links, and I don't have tests.

[not at google - send to devlin, 2014-08-15 20:37:24]

Oh yeah. Tests. Doh.

[Fady Samuel, 2014-08-15 20:39:39]

Sorry, I got a little too carried away. :-)

I'm curious to know what kind of links fail.

[ericzeng, 2014-08-15 20:55:58]

On 2014/08/15 20:39:39, Fady Samuel wrote:
> Sorry, I got a little too carried away. :-)
> 
> I'm curious to know what kind of links fail.

There are two particular links in AdBlockPlus that aren't working but other
links in the options page work fine. There's also an extension called WOT whose
options page normally immediately redirects to an external page using
window.open(), but when embedded nothing happens. I have yet to determine why
any of these are broken - the broken adblock links look fine to me, and I'm able
to use window.open() to create new tabs from the guest view using devtools.

[ericzeng, 2014-08-20 20:00:31]

Alright, the tests run if I use LoadExtension() instead of InstallExtension().

https://codereview.chromium.org/471053003/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/471053003/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:195:
Browser* browser =
On 2014/08/15 20:22:06, Fady Samuel wrote:
> A comment on what this does would be very helpful for future readers. :-)
> 
> Also comment on why we're returning false: Since we are loading the link in a
> browser window, we do not want to create a new WebContents here.

Done.

[not at google - send to devlin, 2014-08-20 20:01:20]

woo! ship it.

[ericzeng, 2014-08-21 17:36:39]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-21 17:38:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ericzeng@chromium.org/471053003/100001

[ericzeng, 2014-08-21 17:49:47]

The CQ bit was unchecked by ericzeng@chromium.org

[ericzeng, 2014-08-21 17:52:08]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-21 17:54:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ericzeng@chromium.org/471053003/120001

[commit-bot: I haz the power, 2014-08-21 23:28:12]

Committed patchset #7 (120001) as 291241