Embedded Extension Options: Open all external links in a new tab

Embedded Extension Options: Open all external links in a new tab

Implement WebContentsDelegate::OpenURLFromTab in ExtensionOptionsGuest
to handle links that do not have the target=_blank attribute. Force all
links to URLs outside of the extension to be opened in a new tab.

This also handles <meta> redirects and setting window.location from the
options page by opening them in a new tab.

BUG=409344
Committed: https://crrev.com/3b21259d93fdc6cd23ede873b15468bd35a047bf
Cr-Commit-Position: refs/heads/master@{#293612}

[ericzeng, 2014-09-05 20:32:58]

ericzeng@chromium.org changed reviewers:
+ fsamuel@chromium.org

[ericzeng, 2014-09-05 20:32:59]

PTAL

[Fady Samuel, 2014-09-05 21:11:57]

LGTM with nits.

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

https://codereview.chromium.org/546073003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:165: if
((!params.url.SchemeIs(extensions::kExtensionScheme) ||
Some comments describing what you're doing would be nice here.

https://codereview.chromium.org/546073003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:167:
params.disposition == CURRENT_TAB) {
Early exit preferred for readability.

[ericzeng, 2014-09-05 21:17:19]

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

https://codereview.chromium.org/546073003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:165: if
((!params.url.SchemeIs(extensions::kExtensionScheme) ||
On 2014/09/05 21:11:57, Fady Samuel wrote:
> Some comments describing what you're doing would be nice here.

Done.

https://codereview.chromium.org/546073003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:167:
params.disposition == CURRENT_TAB) {
On 2014/09/05 21:11:57, Fady Samuel wrote:
> Early exit preferred for readability.

How do I early exit for this condition? Wouldn't flipping the directionality be
just as verbose?

[Fady Samuel, 2014-09-05 21:18:01]

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

https://codereview.chromium.org/546073003/diff/1/chrome/browser/guest_view/ex...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:167:
params.disposition == CURRENT_TAB) {
On 2014/09/05 21:17:19, ericzeng wrote:
> On 2014/09/05 21:11:57, Fady Samuel wrote:
> > Early exit preferred for readability.
> 
> How do I early exit for this condition? Wouldn't flipping the directionality
be
> just as verbose?

You're probably right...lgtm

[ericzeng, 2014-09-05 21:18:52]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-09-05 21:19:48]

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

[commit-bot: I haz the power, 2014-09-06 03:22:27]

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

[commit-bot: I haz the power, 2014-09-06 03:22:28]

Exceeded time limit waiting for builds to trigger.

[ericzeng, 2014-09-06 03:23:02]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-09-06 03:24:35]

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

[commit-bot: I haz the power, 2014-09-06 09:28:01]

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

[commit-bot: I haz the power, 2014-09-06 09:28:02]

Exceeded time limit waiting for builds to trigger.

[Fady Samuel, 2014-09-06 13:19:35]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-09-06 13:20:34]

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

[commit-bot: I haz the power, 2014-09-06 13:21:58]

Committed patchset #2 (id:20001) as a1df6cb89853ac22b8b63a0aeb7e337aa2a95da8

[commit-bot: I haz the power, 2014-09-10 03:42:24]

Patchset 2 (id:??) landed as
https://crrev.com/3b21259d93fdc6cd23ede873b15468bd35a047bf
Cr-Commit-Position: refs/heads/master@{#293612}