Allow DOMUI pages to call window.open(), giving DOMUI privileges to the new

Allow DOMUI pages to call window.open(), giving DOMUI privileges to the new
window (assuming it is on the same site instance).

BUG=17636
TEST=no

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25250

[brettw, 2009-09-02 17:47:45]

http://codereview.chromium.org/173649/diff/4001/5010
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/173649/diff/4001/5010#newcode1283
Line 1283: if (opener_dom_ui_type_ == DOMUIFactory::GetDOMUIType(GetURL())) {
Is there a test for if this comparison fails? Seems like something that would be
good to make sure doesn't regress.

http://codereview.chromium.org/173649/diff/4001/5012
File chrome/browser/tab_contents/tab_contents_view.cc (right):

http://codereview.chromium.org/173649/diff/4001/5012#newcode38
Line 38: DOMUIFactory::GetDOMUIType(tab_contents_->GetURL()));
It seems weird to get the DOM UI type based on the URL of the source tab. Can't
we just ask the DOMUI for that tab? (It seems like DOMUIs should know their
type.) I don't feel strongly about this if it's a lot of work, it just seemed
weird to me to have to do this.

[Matt Perry, 2009-09-02 20:42:18]

http://codereview.chromium.org/173649/diff/4001/5010
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/173649/diff/4001/5010#newcode1283
Line 1283: if (opener_dom_ui_type_ == DOMUIFactory::GetDOMUIType(GetURL())) {
On 2009/09/02 17:47:45, brettw wrote:
> Is there a test for if this comparison fails? Seems like something that would
be
> good to make sure doesn't regress.

I added a test, and it uncovered a bug, so I fixed that too (see
render_view_host.*).

http://codereview.chromium.org/173649/diff/4001/5012
File chrome/browser/tab_contents/tab_contents_view.cc (right):

http://codereview.chromium.org/173649/diff/4001/5012#newcode38
Line 38: DOMUIFactory::GetDOMUIType(tab_contents_->GetURL()));
On 2009/09/02 17:47:45, brettw wrote:
> It seems weird to get the DOM UI type based on the URL of the source tab.
Can't
> we just ask the DOMUI for that tab? (It seems like DOMUIs should know their
> type.) I don't feel strongly about this if it's a lot of work, it just seemed
> weird to me to have to do this.

The main reason I do it this way is for toolstrips - they don't have a DOMUI,
since DOMUI is tab-centric. But we still want tabs opened via window.open to
have extension privileges.

[brettw, 2009-09-02 20:58:46]

LGTM

http://codereview.chromium.org/173649/diff/4001/5010
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/173649/diff/4001/5010#newcode1283
Line 1283: if (opener_dom_ui_type_ == DOMUIFactory::GetDOMUIType(GetURL())) {
On 2009/09/02 20:42:19, Matt Perry wrote:
> On 2009/09/02 17:47:45, brettw wrote:
> > Is there a test for if this comparison fails? Seems like something that
would
> be
> > good to make sure doesn't regress.
> 
> I added a test, and it uncovered a bug, so I fixed that too (see
> render_view_host.*).

Woot, thanks!

http://codereview.chromium.org/173649/diff/4001/5012
File chrome/browser/tab_contents/tab_contents_view.cc (right):

http://codereview.chromium.org/173649/diff/4001/5012#newcode38
Line 38: DOMUIFactory::GetDOMUIType(tab_contents_->GetURL()));
On 2009/09/02 20:42:19, Matt Perry wrote:
> On 2009/09/02 17:47:45, brettw wrote:
> > It seems weird to get the DOM UI type based on the URL of the source tab.
> Can't
> > we just ask the DOMUI for that tab? (It seems like DOMUIs should know their
> > type.) I don't feel strongly about this if it's a lot of work, it just
seemed
> > weird to me to have to do this.
> 
> The main reason I do it this way is for toolstrips - they don't have a DOMUI,
> since DOMUI is tab-centric. But we still want tabs opened via window.open to
> have extension privileges.

Done.

http://codereview.chromium.org/173649/diff/9001/9002
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/173649/diff/9001/9002#newcode23
Line 23: typedef DOMUI* (*DOMUIFactoryFunction)(TabContents* tab_contents,
Can you document the return value of this, i.e. that it can be NULL and that the
caller owns the pointer?

http://codereview.chromium.org/173649/diff/9001/9002#newcode43
Line 43: static DOMUIFactoryFunction GetDOMUIFactoryFunction(const GURL& url) {
Can you document the return value here, and that it may be NULL.

http://codereview.chromium.org/173649/diff/9001/9002#newcode44
Line 44: // Currently, any gears: URL means an HTML dialog.
Need an extra space at the beginning here.

http://codereview.chromium.org/173649/diff/9001/9003
File chrome/browser/dom_ui/dom_ui_factory.h (right):

http://codereview.chromium.org/173649/diff/9001/9003#newcode14
Line 14: typedef void* DOMUITypeID;
Can you document this? I'm thinking something like "An opaque identifier used to
identify a DOMUI. External code can only compare this to DOMUIFactory::kNoDOMUI
and to compare two DOMUITypeIDs to each other for equality, otherwise it is for
internal DOMUI workings only."

http://codereview.chromium.org/173649/diff/9001/9007
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/173649/diff/9001/9007#newcode1612
Line 1612: BlockExtensionRequest(request_id);
Can you comment the cases where this can happen?