Support for chrome.experimental.popup API in ExternalTabContainer views

A collection of fixes allowing the chrome.experimental.popup.* set of APIs to function in circumstances where there is no Browser instance present.  This is a symptom of a tab-contents view hosted in an ExternalTabContainer.The major change here is the removal of the explicit dependency on a Browser instance across all of the delegates involved when showing a pop-up API.  I modified the following delegates:- ExtensionPopupHost::Delegate- TabContentsDelegate- ExtensionFunctionDispatcher::DelegateBecause the pop-up requires a Profile, and a gfx::NativeWindow, I added methods to the above interfaces to provide them.BUG=noneTEST=ExtensionApiTest.FLAKY_Popup

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

[Jeff Timanus, 2009-11-25 02:11:26]

Hi Guys,

Please take a moment to review this change.  I have managed to get the pop-up
API working when hosted in an ExternalTabContainer view.

This work involved exploring further into the depths of Chrome than my previous
work, so please comment on any mistakes/incorrect assumptions I may have made. 

Also, I've found that the unit-tests are not running very well on my local
machine.  Does the following call stack look familiar?  It seems that Chrome is
having difficulty calling dispatchOnLoad, defined in event_bindings.js.

 	browser_tests.exe!DebugUtil::BreakDebugger()  Line 222	C++
 	browser_tests.exe!logging::LogMessage::~LogMessage()  Line 540	C++

	browser_tests.exe!bindings_utils::CallFunctionInContext(v8::Handle<v8::Context>
context={...}, const
std::basic_string<char,std::char_traits<char>,std::allocator<char> > &
function_name="dispatchOnLoad", int argc=1, v8::Handle<v8::Value> *
argv=0x07f9ed24)  Line 132	C++
 	browser_tests.exe!EventBindings::HandleContextCreated(WebKit::WebFrame *
frame=0x07fbba00, bool content_script=false)  Line 265 + 0x3c bytes	C++
 	browser_tests.exe!RenderView::didCreateScriptContext(WebKit::WebFrame *
frame=0x07fbba00)  Line 2529 + 0xb bytes	C++

	browser_tests.exe!WebKit::FrameLoaderClientImpl::didCreateScriptContextForFrame()
 Line 130 + 0x27 bytes	C++
 	browser_tests.exe!WebCore::V8Proxy::initContextIfNeeded()  Line 1125 + 0x26
bytes	C++
 	browser_tests.exe!WebCore::V8Proxy::mainWorldContext(WebCore::Frame *
frame=0x07fe6000)  Line 1223	C++
 	browser_tests.exe!WebCore::ScriptController::bindToWindowObject(WebCore::Frame
* frame=0x07fe6000, const WebCore::String & key={...}, NPObject *
object=0x0801ca80)  Line 258 + 0xd bytes	C++
 	browser_tests.exe!WebKit::WebFrameImpl::bindToWindowObject(const
WebKit::WebString & name={...}, NPObject * object=0x0801ca80)  Line 571	C++
 	browser_tests.exe!CppBoundClass::BindToJavascript(WebKit::WebFrame *
frame=0x07fbba00, const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
classname="domAutomationController")  Line 328 + 0x2b bytes	C++
>	browser_tests.exe!RenderView::BindDOMAutomationController(WebKit::WebFrame *
frame=0x07fbba00)  Line 1234 + 0x23 bytes	C++
 	browser_tests.exe!RenderView::didClearWindowObject(WebKit::WebFrame *
frame=0x07fbba00)  Line 2281	C++

	browser_tests.exe!WebKit::FrameLoaderClientImpl::dispatchDidClearWindowObjectInWorld(WebCore::DOMWrapperWorld
* __formal=0x065fa668)  Line 114 + 0x24 bytes	C++

	browser_tests.exe!WebCore::FrameLoader::dispatchDidClearWindowObjectInWorld(WebCore::DOMWrapperWorld
* world=0x065fa668)  Line 3887 + 0x1c bytes	C++

	browser_tests.exe!WebCore::FrameLoader::dispatchDidClearWindowObjectsInAllWorlds()
 Line 3879 + 0x17 bytes	C++
 	browser_tests.exe!WebCore::FrameLoader::receivedFirstData()  Line 745	C++
 	browser_tests.exe!WebCore::FrameLoader::setEncoding(const WebCore::String &
name={...}, bool userChosen=false)  Line 1457	C++
 	browser_tests.exe!WebKit::WebFrameImpl::commitDocumentData(const char *
data=0x07540000, unsigned int dataLen=2406)  Line 867	C++

	browser_tests.exe!WebKit::FrameLoaderClientImpl::committedLoad(WebCore::DocumentLoader
* loader=0x07b8eb00, const char * data=0x07540000, int length=2406)  Line 992 +
0x20 bytes	C++
 	browser_tests.exe!WebCore::FrameLoader::committedLoad(WebCore::DocumentLoader
* loader=0x07b8eb00, const char * data=0x07540000, int length=2406)  Line 3209 +
0x24 bytes	C++
 	browser_tests.exe!WebCore::DocumentLoader::commitLoad(const char *
data=0x07540000, int length=2406)  Line 343	C++
 	browser_tests.exe!WebCore::DocumentLoader::receivedData(const char *
data=0x07540000, int length=2406)  Line 355	C++
 	browser_tests.exe!WebCore::FrameLoader::receivedData(const char *
data=0x07540000, int length=2406)  Line 2062	C++
 	browser_tests.exe!WebCore::MainResourceLoader::addData(const char *
data=0x07540000, int length=2406, bool allAtOnce=false)  Line 144	C++
 	browser_tests.exe!WebCore::ResourceLoader::didReceiveData(const char *
data=0x07540000, int length=2406, __int64 lengthReceived=2406, bool
allAtOnce=false)  Line 248 + 0x1b bytes	C++
 	browser_tests.exe!WebCore::MainResourceLoader::didReceiveData(const char *
data=0x07540000, int length=2406, __int64 lengthReceived=2406, bool
allAtOnce=false)  Line 375	C++

	browser_tests.exe!WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle
* __formal=0x0c00e210, const char * data=0x07540000, int length=2406, int
lengthReceived=2406)  Line 398 + 0x1f bytes	C++

	browser_tests.exe!WebCore::ResourceHandleInternal::didReceiveData(WebKit::WebURLLoader
* __formal=0x0801d188, const char * data=0x07540000, int dataLength=2406)  Line
141 + 0x31 bytes	C++
 	browser_tests.exe!webkit_glue::WebURLLoaderImpl::Context::OnReceivedData(const
char * data=0x07540000, int len=2406)  Line 479 + 0x26 bytes	C++
 	browser_tests.exe!ResourceDispatcher::OnReceivedData(const IPC::Message &
message={...}, int request_id=0, void * shm_handle=0x000003a0, int
data_len=2406)  Line 402 + 0x1b bytes	C++
 	browser_tests.exe!IPC::MessageWithTuple<Tuple3<int,void *,int>
>::Dispatch<ResourceDispatcher,int,void *,int>(const IPC::Message *
msg=0x0803a828, ResourceDispatcher * obj=0x06607a50, void (const IPC::Message &,
int, void *, int)* func=0x02aaba40)  Line 1035 + 0x18 bytes	C++
 	browser_tests.exe!ResourceDispatcher::DispatchMessageW(const IPC::Message &
message={...})  Line 543 + 0x12 bytes	C++
 	browser_tests.exe!ResourceDispatcher::OnMessageReceived(const IPC::Message &
message={...})  Line 319	C++
 	browser_tests.exe!ChildThread::OnMessageReceived(const IPC::Message &
msg={...})  Line 98 + 0x19 bytes	C++
 	browser_tests.exe!IPC::ChannelProxy::Context::OnDispatchMessage(const
IPC::Message & message={...})  Line 204 + 0x1b bytes	C++
 	browser_tests.exe!DispatchToMethod<IPC::ChannelProxy::Context,void (__thiscall
IPC::ChannelProxy::Context::*)(IPC::Message const
&),IPC::Message>(IPC::ChannelProxy::Context * obj=0x066136c0, void (const
IPC::Message &)* method=0x0272c3a0, const Tuple1<IPC::Message> & arg={...}) 
Line 422 + 0xf bytes	C++
 	browser_tests.exe!RunnableMethod<IPC::ChannelProxy::Context,void (__thiscall
IPC::ChannelProxy::Context::*)(IPC::Message const &),Tuple1<IPC::Message>
>::Run()  Line 284 + 0x1e bytes	C++
 	browser_tests.exe!MessageLoop::RunTask(Task * task=0x0803a800)  Line 320 + 0xf
bytes	C++
 	browser_tests.exe!MessageLoop::DeferOrRunPendingTask(const
MessageLoop::PendingTask & pending_task={...})  Line 331	C++
 	browser_tests.exe!MessageLoop::DoWork()  Line 435 + 0xc bytes	C++
 	browser_tests.exe!base::MessagePumpForUI::DoRunLoop()  Line 209 + 0x1d
bytes	C++

	browser_tests.exe!base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate
* delegate=0x07f9fc18, base::MessagePumpWin::Dispatcher * dispatcher=0x00000000)
 Line 52 + 0xf bytes	C++
 	browser_tests.exe!base::MessagePumpWin::Run(base::MessagePump::Delegate *
delegate=0x07f9fc18)  Line 78 + 0x1c bytes	C++
 	browser_tests.exe!MessageLoop::RunInternal()  Line 205 + 0x2a bytes	C++
 	browser_tests.exe!MessageLoop::RunHandler()  Line 178	C++
 	browser_tests.exe!MessageLoop::Run()  Line 156	C++
 	browser_tests.exe!base::Thread::Run(MessageLoop * message_loop=0x07f9fc18) 
Line 134	C++
 	browser_tests.exe!base::Thread::ThreadMain()  Line 155 + 0x16 bytes	C++
 	browser_tests.exe!`anonymous namespace'::ThreadFunc(void * closure=0x06614780)
 Line 26 + 0xf bytes	C++
 	kernel32.dll!77134911() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for
kernel32.dll]	
 	ntdll.dll!772fe4b6() 	
 	ntdll.dll!772fe489()

[Jeff Timanus, 2009-11-30 17:02:15]

Hi Guys,

Once the return-from-vacation frenzy settles down, could you please take a
moment to review this CL?

Thanks,

Jeff

[Erik does not do reviews, 2009-12-01 20:48:26]

Sorry for the delay getting back to you on this.

I had started to do a more detailed review, but I'm backing off because I
have one comment that may require a lot of changes.  We can't use
views:Widget as part of the general interface.  It's specific to
TOOLKIT_VIEWS and won't work for Linux and Mac.  Rather than a Widget, why
not use a NativeWindow instead?

Erik


On Mon, Nov 30, 2009 at 9:02 AM, <twiz@chromium.org> wrote:

> Hi Guys,
>
> Once the return-from-vacation frenzy settles down, could you please take a
> moment to review this CL?
>
> Thanks,
>
> Jeff
>
>
> http://codereview.chromium.org/434046
>

[twiz, 2009-12-01 22:34:34]

Thanks, Erik.

I'll look into making use of a NativeWindow (which is just a typedef for a
HWND) in place of views::Widget.

Jeff

On Tue, Dec 1, 2009 at 3:48 PM, Erik Kay <erikkay@chromium.org> wrote:

> Sorry for the delay getting back to you on this.
>
> I had started to do a more detailed review, but I'm backing off because I
> have one comment that may require a lot of changes.  We can't use
> views:Widget as part of the general interface.  It's specific to
> TOOLKIT_VIEWS and won't work for Linux and Mac.  Rather than a Widget, why
> not use a NativeWindow instead?
>
> Erik
>
>
> On Mon, Nov 30, 2009 at 9:02 AM, <twiz@chromium.org> wrote:
>
>> Hi Guys,
>>
>> Once the return-from-vacation frenzy settles down, could you please take a
>> moment to review this CL?
>>
>> Thanks,
>>
>> Jeff
>>
>>
>> http://codereview.chromium.org/434046
>>
>
>


-- 
Software Developer
Google Canada
(514) 670-8756

[Jeff Timanus, 2009-12-04 03:27:23]

Erik,

I made the change you suggested.  All of the delegates that I modified now pass
gfx::NativeView objects.

I stumbled across a nasty bug in the views::WidgetWin implementation when making
this change.  There was an incorrect reinterpret_cast when attempting to convert
from a native-view to a views::Widget object.  

Please review this CL as soon as you have an opportunity.

Thanks,

Jeff

[Erik does not do reviews, 2009-12-05 21:08:23]

Again, sorry for continuing to be slow here.  Expect continued slow response for
another week or so.

http://codereview.chromium.org/434046/diff/13002/12013
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/434046/diff/13002/12013#newcode68
chrome/browser/extensions/extension_dom_ui.cc:68: if (contents) {
for a DOMUI is it ever possible for this to be NULL?  If not, I'd say that we
shouldn't bother to check, we'll just crash on the defref instead.

http://codereview.chromium.org/434046/diff/13002/12013#newcode88
chrome/browser/extensions/extension_dom_ui.cc:88: if (!native_window &&
tab_contents() && tab_contents()->delegate()) {
for a DOMUI, it seems like we should always go through the tab_contents(),
shouldn't we?

http://codereview.chromium.org/434046/diff/13002/12007
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/434046/diff/13002/12007#newcode600
chrome/browser/extensions/extension_host.cc:600: if (browser) {
remove {}

http://codereview.chromium.org/434046/diff/13002/12007#newcode603
chrome/browser/extensions/extension_host.cc:603: } else if (extension_host_type_
== ViewType::EXTENSION_BACKGROUND_PAGE) {
change this test to != and make the contents be NOTREACHED.  Since you defaulted
window_id to -1, you don't need the extra branch here.

http://codereview.chromium.org/434046/diff/13002/12009
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/434046/diff/13002/12009#newcode69
chrome/browser/extensions/extension_host.h:69: // is a valid argument for
|browser|.
explain what NULL means in that case

http://codereview.chromium.org/434046/diff/13002/12006
File chrome/browser/extensions/extension_popup_host.cc (right):

http://codereview.chromium.org/434046/diff/13002/12006#newcode33
chrome/browser/extensions/extension_popup_host.cc:33: if (browser) {
nit: remove {}

http://codereview.chromium.org/434046/diff/13002/12006#newcode51
chrome/browser/extensions/extension_popup_host.cc:51: DCHECK(profile);
won't this always trip for ExternalTabContainers?

http://codereview.chromium.org/434046/diff/13002/12006#newcode57
chrome/browser/extensions/extension_popup_host.cc:57: // Don't signal
notification during destruction.  The delegate may be stale
is there a better way we can handle this?  maybe some way we can NULL out the
delegate before we get here?

http://codereview.chromium.org/434046/diff/13002/12004
File chrome/browser/extensions/extension_popup_host.h (right):

http://codereview.chromium.org/434046/diff/13002/12004#newcode57
chrome/browser/extensions/extension_popup_host.h:57: // as true to propagate
notifications to the PopupEventRouter.
add comment explinaing under what conditions is it desirable not to?

http://codereview.chromium.org/434046/diff/13002/12003
File chrome/browser/extensions/extension_process_manager.cc (left):

http://codereview.chromium.org/434046/diff/13002/12003#oldcode60
chrome/browser/extensions/extension_process_manager.cc:60: DCHECK(browser);
is a NULL browser valid for all process types?  if not, we should DCHECK those

http://codereview.chromium.org/434046/diff/13002/12003#oldcode71
chrome/browser/extensions/extension_process_manager.cc:71: DCHECK(browser);
same here

http://codereview.chromium.org/434046/diff/13002/12020
File chrome/browser/external_tab_container.h (right):

http://codereview.chromium.org/434046/diff/13002/12020#newcode119
chrome/browser/external_tab_container.h:119: virtual gfx::NativeWindow
GetFrameNativeWindow();
any reason for newlines between these?  This one at least is still a
continuation of TabContentsDelegate right?

http://codereview.chromium.org/434046/diff/13002/12010
File chrome/browser/views/browser_bubble.cc (right):

http://codereview.chromium.org/434046/diff/13002/12010#newcode72
chrome/browser/views/browser_bubble.cc:72: attached_ = true;
why did you move this?  If there's no browser, then there's nothing to be
attached to, right?

http://codereview.chromium.org/434046/diff/13002/12012
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/434046/diff/13002/12012#newcode153
chrome/renderer/extensions/extension_process_bindings.cc:153: //
same-browser-window origin.
do you need to bypass the whole check?  Or just the -1 part.

http://codereview.chromium.org/434046/diff/13002/12024
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/434046/diff/13002/12024#newcode475
views/widget/widget_win.cc:475: SetNativeWindowProperty(kWidgetKey,
static_cast<Widget*>(this));
scary.  Any idea why we haven't hit this before?

[Jeff Timanus, 2009-12-08 01:02:49]

Thanks for your review, Erik.  

I added a bug for the browser_id issue discussed in
extension_process_bindings.cc.  It seems that ExternalTabContainer views are not
propagating the browser id properly to all of the render views.  My current fix
bypasses the problem.

Please review again.

http://codereview.chromium.org/434046/diff/13002/12013
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/434046/diff/13002/12013#newcode68
chrome/browser/extensions/extension_dom_ui.cc:68: if (contents) {
On 2009/12/05 21:08:23, Erik Kay wrote:
> for a DOMUI is it ever possible for this to be NULL?  If not, I'd say that we
> shouldn't bother to check, we'll just crash on the defref instead.

I was just being cautious here.  I'll take out the check.

http://codereview.chromium.org/434046/diff/13002/12013#newcode88
chrome/browser/extensions/extension_dom_ui.cc:88: if (!native_window &&
tab_contents() && tab_contents()->delegate()) {
On 2009/12/05 21:08:23, Erik Kay wrote:
> for a DOMUI, it seems like we should always go through the tab_contents(),
> shouldn't we?

I added the TabContentsDelegate::GetFrameNativeWindow method.  The default
implementation returns NULL.  It is only when an ExternalTabContainer is used as
the delegate that this returns a non-null value.

The alternative would be to take the default implementation (which returns the
frame of the browser instance, if one is present) out of
ExtensionFunctionDispatcher::Delegate, and place it in the TabContentsDelegate
class.

Would you prefer this design?  I purposefully put the default implementation in
ExtensionFunctionDispatcher::Delegate, because it is used both here, and by
ExtensionHost.

http://codereview.chromium.org/434046/diff/13002/12007
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/434046/diff/13002/12007#newcode600
chrome/browser/extensions/extension_host.cc:600: if (browser) {
On 2009/12/05 21:08:23, Erik Kay wrote:
> remove {}

Done.

http://codereview.chromium.org/434046/diff/13002/12007#newcode603
chrome/browser/extensions/extension_host.cc:603: } else if (extension_host_type_
== ViewType::EXTENSION_BACKGROUND_PAGE) {
On 2009/12/05 21:08:23, Erik Kay wrote:
> change this test to != and make the contents be NOTREACHED.  Since you
defaulted
> window_id to -1, you don't need the extra branch here.

Done.

http://codereview.chromium.org/434046/diff/13002/12009
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/434046/diff/13002/12009#newcode69
chrome/browser/extensions/extension_host.h:69: // is a valid argument for
|browser|.
On 2009/12/05 21:08:23, Erik Kay wrote:
> explain what NULL means in that case

Done.

http://codereview.chromium.org/434046/diff/13002/12006
File chrome/browser/extensions/extension_popup_host.cc (right):

http://codereview.chromium.org/434046/diff/13002/12006#newcode33
chrome/browser/extensions/extension_popup_host.cc:33: if (browser) {
On 2009/12/05 21:08:23, Erik Kay wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/434046/diff/13002/12006#newcode51
chrome/browser/extensions/extension_popup_host.cc:51: DCHECK(profile);
On 2009/12/05 21:08:23, Erik Kay wrote:
> won't this always trip for ExternalTabContainers?

No, ExternalTabContainers always have a profile.

http://codereview.chromium.org/434046/diff/13002/12006#newcode57
chrome/browser/extensions/extension_popup_host.cc:57: // Don't signal
notification during destruction.  The delegate may be stale
On 2009/12/05 21:08:23, Erik Kay wrote:
> is there a better way we can handle this?  maybe some way we can NULL out the
> delegate before we get here?

I added code to the destructor of PopupDelegate that explicitly resets the
delegate pointer of the ExtensionPopupHost to NULL, which prevents the errors
described here.

http://codereview.chromium.org/434046/diff/13002/12004
File chrome/browser/extensions/extension_popup_host.h (right):

http://codereview.chromium.org/434046/diff/13002/12004#newcode57
chrome/browser/extensions/extension_popup_host.h:57: // as true to propagate
notifications to the PopupEventRouter.
On 2009/12/05 21:08:23, Erik Kay wrote:
> add comment explinaing under what conditions is it desirable not to?

No longer relevant.

http://codereview.chromium.org/434046/diff/13002/12003
File chrome/browser/extensions/extension_process_manager.cc (left):

http://codereview.chromium.org/434046/diff/13002/12003#oldcode60
chrome/browser/extensions/extension_process_manager.cc:60: DCHECK(browser);
On 2009/12/05 21:08:23, Erik Kay wrote:
> is a NULL browser valid for all process types?  if not, we should DCHECK those
Good idea.  Done.

http://codereview.chromium.org/434046/diff/13002/12003#oldcode71
chrome/browser/extensions/extension_process_manager.cc:71: DCHECK(browser);
On 2009/12/05 21:08:23, Erik Kay wrote:
> same here

Done.

http://codereview.chromium.org/434046/diff/13002/12020
File chrome/browser/external_tab_container.h (right):

http://codereview.chromium.org/434046/diff/13002/12020#newcode119
chrome/browser/external_tab_container.h:119: virtual gfx::NativeWindow
GetFrameNativeWindow();
On 2009/12/05 21:08:23, Erik Kay wrote:
> any reason for newlines between these?  This one at least is still a
> continuation of TabContentsDelegate right?

No reason.  I was just trying to fit in with the declarations around it.  The
subsequent routines are also part of TabContentsDelegate.  I also thought
leaving a space after the inline function was clearer.  

Line removed.

http://codereview.chromium.org/434046/diff/13002/12010
File chrome/browser/views/browser_bubble.cc (right):

http://codereview.chromium.org/434046/diff/13002/12010#newcode72
chrome/browser/views/browser_bubble.cc:72: attached_ = true;
On 2009/12/05 21:08:23, Erik Kay wrote:
> why did you move this?  If there's no browser, then there's nothing to be
> attached to, right?
I moved this so that the DCHECK in DetachFromBrowser() would not trip if there
was no browser_view.  I wanted to keep the semantics of these routines intact -
they keep track of balanced attach-detach calls, even if there is no
browser_view.

http://codereview.chromium.org/434046/diff/13002/12012
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/434046/diff/13002/12012#newcode153
chrome/renderer/extensions/extension_process_bindings.cc:153: //
same-browser-window origin.
On 2009/12/05 21:08:23, Erik Kay wrote:
> do you need to bypass the whole check?  Or just the -1 part.

We need to bypass the whole check.  I've found that when displaying a popup from
an ExternalTabContainer, the browser id returned from the following sequence is
different from the ids returned during the iteration:

(in ExtensionImpl::PopupViewFinder...)
RenderView* render_view  = bindings_utils::GetRenderViewForCurrentContext();
int browser_window_id = render_view->browser_window_id();

This behaviour is incorrect.  

See the bug I added:  29646

http://codereview.chromium.org/434046/diff/13002/12024
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/434046/diff/13002/12024#newcode475
views/widget/widget_win.cc:475: SetNativeWindowProperty(kWidgetKey,
static_cast<Widget*>(this));
On 2009/12/05 21:08:23, Erik Kay wrote:
> scary.  Any idea why we haven't hit this before?

I'm quite surprised we haven't hit this before.  The only explanation is that
I'm the first to use this API since the inheritance ordering of WidgetWin was
changed.

[Jeff Timanus, 2009-12-09 19:13:09]

Hi Evan & Peter,

Evan, could you please review this CL as soon as possible?

Peter, you may want to have a look at the change I made to the WidgetWin
implementation.

Thanks,

Jeff

[Evan Stade, 2009-12-09 19:35:36]

overall lg to me, but please wait for Peter's comments

http://codereview.chromium.org/434046/diff/14004/18012
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/434046/diff/14004/18012#newcode86
chrome/browser/extensions/extension_dom_ui.cc:86: if (tab_contents_delegate) {
nit: no curlies for single line ifs

http://codereview.chromium.org/434046/diff/14004/18007
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/434046/diff/14004/18007#newcode339
chrome/browser/extensions/extension_function_dispatcher.cc:339: return
(delegate_) ? delegate_->GetFrameNativeWindow() : NULL;
nit: no () aroudn delegate_

http://codereview.chromium.org/434046/diff/14004/18006
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/434046/diff/14004/18006#newcode606
chrome/browser/extensions/extension_host.cc:606: Browser* browser =
const_cast<ExtensionHost* >(this)->GetBrowser();
I don't like this const cast. I know it's not your fault, but I'm pretty sure
you can get rid of it by sprinkling const on enough functions.

http://codereview.chromium.org/434046/diff/14004/18003
File chrome/browser/extensions/extension_popup_host.h (right):

http://codereview.chromium.org/434046/diff/14004/18003#newcode89
chrome/browser/extensions/extension_popup_host.h:89: friend class PopupDelegate;
you only need this to set the delegate to null, right? I'd prefer a public
DelegateDestroyed() method.

http://codereview.chromium.org/434046/diff/14004/18002
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/434046/diff/14004/18002#newcode52
chrome/browser/extensions/extension_process_manager.cc:52:
CloseBackgroundHosts();
looks like this will cause a double delete for the case where we get a
BROWSER_CLOSED notification...? Should you make CloseBackgroundHosts also clear
the vector

http://codereview.chromium.org/434046/diff/14004/18002#newcode261
chrome/browser/extensions/extension_process_manager.cc:261:
ExtensionHostSet::iterator current = iter++;
nit: move iter++ to for statement header

http://codereview.chromium.org/434046/diff/14004/18011
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/434046/diff/14004/18011#newcode155
chrome/renderer/extensions/extension_process_bindings.cc:155: // match that of
the arguments to the accumulator.  See bug:  29646
please make this a link: http://crbug.com/29646

http://codereview.chromium.org/434046/diff/14004/18011#newcode159
chrome/renderer/extensions/extension_process_bindings.cc:159:
render_view->browser_window_id() != browser_window_id_)
nit: curlies for multi-line conditionals

[Jeff Timanus, 2009-12-09 20:44:37]

I addressed your remarks, please look again.

Peter, your comments are not showing up in the web-tool, but they are addressed.

Thanks,

Jeff

http://codereview.chromium.org/434046/diff/14004/18012
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/434046/diff/14004/18012#newcode86
chrome/browser/extensions/extension_dom_ui.cc:86: if (tab_contents_delegate) {
On 2009/12/09 19:35:36, Evan Stade wrote:
> nit: no curlies for single line ifs

Done.

http://codereview.chromium.org/434046/diff/14004/18007
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/434046/diff/14004/18007#newcode339
chrome/browser/extensions/extension_function_dispatcher.cc:339: return
(delegate_) ? delegate_->GetFrameNativeWindow() : NULL;
On 2009/12/09 19:35:36, Evan Stade wrote:
> nit: no () aroudn delegate_

Done.

http://codereview.chromium.org/434046/diff/14004/18006
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/434046/diff/14004/18006#newcode606
chrome/browser/extensions/extension_host.cc:606: Browser* browser =
const_cast<ExtensionHost* >(this)->GetBrowser();
On 2009/12/09 19:35:36, Evan Stade wrote:
> I don't like this const cast. I know it's not your fault, but I'm pretty sure
> you can get rid of it by sprinkling const on enough functions.

Done.

http://codereview.chromium.org/434046/diff/14004/18003
File chrome/browser/extensions/extension_popup_host.h (right):

http://codereview.chromium.org/434046/diff/14004/18003#newcode89
chrome/browser/extensions/extension_popup_host.h:89: friend class PopupDelegate;
On 2009/12/09 19:35:36, Evan Stade wrote:
> you only need this to set the delegate to null, right? I'd prefer a public
> DelegateDestroyed() method.

Done.

http://codereview.chromium.org/434046/diff/14004/18002
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/434046/diff/14004/18002#newcode52
chrome/browser/extensions/extension_process_manager.cc:52:
CloseBackgroundHosts();
On 2009/12/09 19:35:36, Evan Stade wrote:
> looks like this will cause a double delete for the case where we get a
> BROWSER_CLOSED notification...? Should you make CloseBackgroundHosts also
clear
> the vector

I just double checked this.  It is safe, because the ExtensionHost destructor
sends a synchronous notification, EXTENSION_HOST_DESTROYED.  This notification
is caught by the ExtensionProcessManager, and explicitly removes the host from
the vector.

http://codereview.chromium.org/434046/diff/14004/18002#newcode261
chrome/browser/extensions/extension_process_manager.cc:261:
ExtensionHostSet::iterator current = iter++;
On 2009/12/09 19:35:36, Evan Stade wrote:
> nit: move iter++ to for statement header

I don't think that the ++iter can be moved.  Because of the sequence of event
notifications, we need to increment the iterator before the host is destroyed. 
We could change the loop logic to be one-behind, but that may complicate things
more than its worth.

http://codereview.chromium.org/434046/diff/14004/18009
File chrome/browser/views/browser_bubble.cc (right):

http://codereview.chromium.org/434046/diff/14004/18009#newcode24
chrome/browser/views/browser_bubble.cc:24: 
In response to: pkasting@chromium.org

Can |frame| or |window| ever be NULL here?  The old code assumed neither
could.  Was it wrong?  Has something changed?

If not, and if your newly-added DCHECK is correct (that we can't fail to
get a browser view), then we should remove the conditionals from the
callers.  And probably just delete this function and write the relevant
chain of unconditional calls into the callers.

------ Response -----

Yes, ExternalTabContainer, which is a type of Widget, will return a NULL window,
so that check is necessary.  However, we should always have a frame, so I
removed that check.

If we do have a window then we should ALWAYS have a browser_view.

Was just being defensive.

http://codereview.chromium.org/434046/diff/14004/18011
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/434046/diff/14004/18011#newcode155
chrome/renderer/extensions/extension_process_bindings.cc:155: // match that of
the arguments to the accumulator.  See bug:  29646
On 2009/12/09 19:35:36, Evan Stade wrote:
> please make this a link: http://crbug.com/29646

Done.

http://codereview.chromium.org/434046/diff/14004/18011#newcode159
chrome/renderer/extensions/extension_process_bindings.cc:159:
render_view->browser_window_id() != browser_window_id_)
On 2009/12/09 19:35:36, Evan Stade wrote:
> nit: curlies for multi-line conditionals

Done.

[Peter Kasting, 2009-12-09 20:50:31]

(These were already sent by email and addressed, I'm trying to get Rietveld to
note them now)

http://codereview.chromium.org/434046/diff/14004/18009
File chrome/browser/views/browser_bubble.cc (right):

http://codereview.chromium.org/434046/diff/14004/18009#newcode17
chrome/browser/views/browser_bubble.cc:17: if (window) {
Can |frame| or |window| ever be NULL here?  The old code assumed neither could. 
Was it wrong?  Has something changed?

If not, and if your newly-added DCHECK is correct (that we can't fail to get a
browser view), then we should remove the conditionals from the callers.  And
probably just delete this function and write the relevant chain of unconditional
calls into the callers.

http://codereview.chromium.org/434046/diff/14004/18009#newcode69
chrome/browser/views/browser_bubble.cc:69: if (browser_view) {
Nit: Remove {}

http://codereview.chromium.org/434046/diff/14004/18018
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/434046/diff/14004/18018#newcode1357
chrome/browser/views/location_bar_view.cc:1357: gfx::NativeWindow frame_window =
browser->window()->GetNativeHandle();
Nit: I'd just roll this temp into the call, and remove the newly added blank
line.

[Peter Kasting, 2009-12-09 20:54:59]

On 2009/12/09 20:44:37, Jeff Timanus wrote:
> http://codereview.chromium.org/434046/diff/14004/18009#newcode24
> chrome/browser/views/browser_bubble.cc:24: 
> In response to: mailto:pkasting@chromium.org
> 
> Can |frame| or |window| ever be NULL here?  The old code assumed neither
> could.  Was it wrong?  Has something changed?
> 
> ------ Response -----
> 
> Yes, ExternalTabContainer, which is a type of Widget, will return a NULL
window,
> so that check is necessary.

And this NULL-return is new with your patch, right?  I'm just trying to
understand how the old code got away with not checking this.

[Jeff Timanus, 2009-12-09 21:09:22]

On 2009/12/09 20:54:59, Peter Kasting wrote:
> On 2009/12/09 20:44:37, Jeff Timanus wrote:
> > http://codereview.chromium.org/434046/diff/14004/18009#newcode24
> > chrome/browser/views/browser_bubble.cc:24: 
> > In response to: mailto:pkasting@chromium.org
> > 
> > Can |frame| or |window| ever be NULL here?  The old code assumed neither
> > could.  Was it wrong?  Has something changed?
> > 
> > ------ Response -----
> > 
> > Yes, ExternalTabContainer, which is a type of Widget, will return a NULL
> window,
> > so that check is necessary.
> 
> And this NULL-return is new with your patch, right?  I'm just trying to
> understand how the old code got away with not checking this.

Yes, the NULL return is new with my patch.

The old code didn't get away at all when this path was executed within  an
ExternalTabContainer.  I had to make many changes to push the required data
around.  This was not important, because, previously, BrowserBubble instances
were never created from within ExternalTabContainer objects.

[Jeff Timanus, 2009-12-09 21:16:22]

http://codereview.chromium.org/434046/diff/14004/18006
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/434046/diff/14004/18006#newcode606
chrome/browser/extensions/extension_host.cc:606: Browser* browser =
const_cast<ExtensionHost* >(this)->GetBrowser();
On 2009/12/09 20:44:37, Jeff Timanus wrote:
> On 2009/12/09 19:35:36, Evan Stade wrote:
> > I don't like this const cast. I know it's not your fault, but I'm pretty
sure
> > you can get rid of it by sprinkling const on enough functions.
> 
> Done.

Does Chrome have a standard wrt transitive const?  I made the change you
suggest, but almost all of my applications of const to member functions have not
been transitive const.

[Evan Stade, 2009-12-09 21:26:01]

> Does Chrome have a standard wrt transitive const?  I made the change you
> suggest, but almost all of my applications of const to member functions have
not
> been transitive const.

There are many places that aren't const but could/should be, but I've never
heard of a moratorium on non-transitive const.

[Evan Stade, 2009-12-09 21:26:10]

http://codereview.chromium.org/434046/diff/14004/18002
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/434046/diff/14004/18002#newcode52
chrome/browser/extensions/extension_process_manager.cc:52:
CloseBackgroundHosts();
On 2009/12/09 20:44:37, Jeff Timanus wrote:
> On 2009/12/09 19:35:36, Evan Stade wrote:
> > looks like this will cause a double delete for the case where we get a
> > BROWSER_CLOSED notification...? Should you make CloseBackgroundHosts also
> clear
> > the vector
> 
> I just double checked this.  It is safe, because the ExtensionHost destructor
> sends a synchronous notification, EXTENSION_HOST_DESTROYED.  This notification
> is caught by the ExtensionProcessManager, and explicitly removes the host from
> the vector.
> 

ok thanks for checking

http://codereview.chromium.org/434046/diff/14004/18002#newcode261
chrome/browser/extensions/extension_process_manager.cc:261:
ExtensionHostSet::iterator current = iter++;
On 2009/12/09 20:44:37, Jeff Timanus wrote:
> On 2009/12/09 19:35:36, Evan Stade wrote:
> > nit: move iter++ to for statement header
> 
> I don't think that the ++iter can be moved.  Because of the sequence of event
> notifications, we need to increment the iterator before the host is destroyed.

> We could change the loop logic to be one-behind, but that may complicate
things
> more than its worth.

ah, ok

[Peter Kasting, 2009-12-09 21:34:42]

On 2009/12/09 21:16:22, Jeff Timanus wrote:
> Does Chrome have a standard wrt transitive const?  I made the change you
> suggest, but almost all of my applications of const to member functions have
not
> been transitive const.

I don't know what transitive const means, though I can guess.

We prefer logical const to physical const.  I'm generally OK with saying
something is "logically const" if it allows you to muck with the contents of
objects that members point to, as long as that won't be meaningful to the parent
object.  For example, if some parent function reads *foo_ and acts on it, then a
function returning &foo_ should not be const.  But if the parent is a linked
list of Foos, then mucking with the contents of one of them doesn't really
affect the linked list, and thus marking that function const would be fine.

[Evan Stade, 2009-12-09 21:53:06]

lgtm

[Ben Goodger (Google), 2010-06-03 23:21:17]

Hi Jeff,

I know this code review is ancient but I just discovered the presence of these
methods and svn annotate led me to this CL :-)

It would help to know what you're trying to do - why do you need the browser and
the frame native window?

I am re-CC'ing Brett since I would be surprised if he actually saw this.

Thanks!

-Ben

http://codereview.chromium.org/434046/diff/26025/27020
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/434046/diff/26025/27020#newcode256
chrome/browser/tab_contents/tab_contents_delegate.h:256: virtual Browser*
GetBrowser() { return NULL; }
This is a layering violation and is just about the worst thing you can do in
Chrome code short of shooting a kitten.

We need to correct this since it looks like people (e.g. dhg@) are already
beginning to use this for other purposes.

TabContents should never rely on Browser, it is at a lower level of abstraction.

http://codereview.chromium.org/434046/diff/26025/27020#newcode259
chrome/browser/tab_contents/tab_contents_delegate.h:259: virtual
gfx::NativeWindow GetFrameNativeWindow() { return NULL; }
See the above comment. I am not clear on the need for there to be assumption in
the TabContents API of such a relationship (presence of a "frame native
window").

[Jeff Timanus, 2010-06-04 18:16:15]

Hi Ben,

Yes, there is some ugly complexity in the implementation of the experimental
extension popup API.  A lot of this is a result of the need to display popups in
ExtensionHosts, and ExtensionDOMUI views.  (And in Chrome-Frame extension views
as well.)

Please see my response below.  

Jeff

http://codereview.chromium.org/434046/diff/26025/27020
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/434046/diff/26025/27020#newcode256
chrome/browser/tab_contents/tab_contents_delegate.h:256: virtual Browser*
GetBrowser() { return NULL; }
On 2010/06/03 23:21:18, Ben Goodger wrote:
> This is a layering violation and is just about the worst thing you can do in
> Chrome code short of shooting a kitten.
Yikes!  And I love kittens!  (We should be able to resolve this issue - some
changes will be needed in the ExtensionPopup::Show and associated routines.)

> 
> We need to correct this since it looks like people (e.g. dhg@) are already
> beginning to use this for other purposes.
> 
> TabContents should never rely on Browser, it is at a lower level of
abstraction.

I added this method because of the need for these parameters during display of
an ExtensionPopup view.  (See browser\views\extensions\extension_popup.{h|cc}) 
The experimental extension popup API only has access to an
ExtensionFunctionDispatcher, and needed a means to gain access to these values. 
See browser\extensions\extension_pop_api.cc.

The TabContentsDelegate is used to implement the
ExtensionFunctionDispatcher::Delegate interface in both ExtensionDOMUI, and
ExtensionHost.  This is because popups can be launched both from ExtensionHosts
(toolstrips), or from a TabContents view that has been navigated to an extension
url.

Looking at the use of the Browser in the construction of the ExtensionPopup, it
seems to be required during the construction of the ExtensionHost for use within
the popup.  See ExtensionProcessManager::CreateView.  Perhaps it would be
possible to decouple these functions from the browser, but further inspection
will be required.

The native window that frames the view that is launching the popup is used to
find the corresponding widget.  This is subsequently used during positioning of
the popup (which is a top-level window).  The widget is used to convert the
coordinates of the position of the popup, which are in the coordinate frame of
the frame to coordinates on the screen.

I hope that makes some sense.  Please let me know if you need more
clarification.  I was not aware, at the time, of the abstraction boundary
between the TabContentsDelegate and Browser classes.  

I'll investigate if there is a means to have the ExtensionFunctionDispatcher
(which I presume can know all about the Browser) provide a different access
point for the browser and framing window.

[Ben Goodger (Google), 2010-07-07 18:56:39]

Jeff,

What's the status on this? It's now been over a month.

-Ben

On Mon, Jun 14, 2010 at 2:40 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> OK. You should work with the other extensions folk to fix this then asap.
>
> -Ben
>
> On Fri, Jun 4, 2010 at 11:16 AM,  <twiz@chromium.org> wrote:
>> Hi Ben,
>>
>> Yes, there is some ugly complexity in the implementation of the experimental
>> extension popup API.  A lot of this is a result of the need to display
>> popups in
>> ExtensionHosts, and ExtensionDOMUI views.  (And in Chrome-Frame extension
>> views
>> as well.)
>>
>> Please see my response below.
>>
>> Jeff
>>
>>
>> http://codereview.chromium.org/434046/diff/26025/27020
>> File chrome/browser/tab_contents/tab_contents_delegate.h (right):
>>
>> http://codereview.chromium.org/434046/diff/26025/27020#newcode256
>> chrome/browser/tab_contents/tab_contents_delegate.h:256: virtual
>> Browser* GetBrowser() { return NULL; }
>> On 2010/06/03 23:21:18, Ben Goodger wrote:
>>>
>>> This is a layering violation and is just about the worst thing you can
>>
>> do in
>>>
>>> Chrome code short of shooting a kitten.
>>
>> Yikes!  And I love kittens!  (We should be able to resolve this issue -
>> some changes will be needed in the ExtensionPopup::Show and associated
>> routines.)
>>
>>
>>> We need to correct this since it looks like people (e.g. dhg@) are
>>
>> already
>>>
>>> beginning to use this for other purposes.
>>
>>> TabContents should never rely on Browser, it is at a lower level of
>>
>> abstraction.
>>
>> I added this method because of the need for these parameters during
>> display of an ExtensionPopup view.  (See
>> browser\views\extensions\extension_popup.{h|cc})  The experimental
>> extension popup API only has access to an ExtensionFunctionDispatcher,
>> and needed a means to gain access to these values.  See
>> browser\extensions\extension_pop_api.cc.
>>
>> The TabContentsDelegate is used to implement the
>> ExtensionFunctionDispatcher::Delegate interface in both ExtensionDOMUI,
>> and ExtensionHost.  This is because popups can be launched both from
>> ExtensionHosts (toolstrips), or from a TabContents view that has been
>> navigated to an extension url.
>>
>> Looking at the use of the Browser in the construction of the
>> ExtensionPopup, it seems to be required during the construction of the
>> ExtensionHost for use within the popup.  See
>> ExtensionProcessManager::CreateView.  Perhaps it would be possible to
>> decouple these functions from the browser, but further inspection will
>> be required.
>>
>> The native window that frames the view that is launching the popup is
>> used to find the corresponding widget.  This is subsequently used during
>> positioning of the popup (which is a top-level window).  The widget is
>> used to convert the coordinates of the position of the popup, which are
>> in the coordinate frame of the frame to coordinates on the screen.
>>
>> I hope that makes some sense.  Please let me know if you need more
>> clarification.  I was not aware, at the time, of the abstraction
>> boundary between the TabContentsDelegate and Browser classes.
>>
>> I'll investigate if there is a means to have the
>> ExtensionFunctionDispatcher (which I presume can know all about the
>> Browser) provide a different access point for the browser and framing
>> window.
>>
>> http://codereview.chromium.org/434046/show
>>
>

[twiz, 2010-07-07 19:22:30]

Hi Ben,

Unfortunately, I've been distracted with other important Chrome-Frame issues
for the past while - specifically getting host networking support for
document.cookie requests.  That task is wrapping up, and next on my list was
to correct the design of the popup internals.

Jeff

On Wed, Jul 7, 2010 at 2:56 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> Jeff,
>
> What's the status on this? It's now been over a month.
>
> -Ben
>
> On Mon, Jun 14, 2010 at 2:40 PM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > OK. You should work with the other extensions folk to fix this then asap.
> >
> > -Ben
> >
> > On Fri, Jun 4, 2010 at 11:16 AM,  <twiz@chromium.org> wrote:
> >> Hi Ben,
> >>
> >> Yes, there is some ugly complexity in the implementation of the
> experimental
> >> extension popup API.  A lot of this is a result of the need to display
> >> popups in
> >> ExtensionHosts, and ExtensionDOMUI views.  (And in Chrome-Frame
> extension
> >> views
> >> as well.)
> >>
> >> Please see my response below.
> >>
> >> Jeff
> >>
> >>
> >> http://codereview.chromium.org/434046/diff/26025/27020
> >> File chrome/browser/tab_contents/tab_contents_delegate.h (right):
> >>
> >> http://codereview.chromium.org/434046/diff/26025/27020#newcode256
> >> chrome/browser/tab_contents/tab_contents_delegate.h:256: virtual
> >> Browser* GetBrowser() { return NULL; }
> >> On 2010/06/03 23:21:18, Ben Goodger wrote:
> >>>
> >>> This is a layering violation and is just about the worst thing you can
> >>
> >> do in
> >>>
> >>> Chrome code short of shooting a kitten.
> >>
> >> Yikes!  And I love kittens!  (We should be able to resolve this issue -
> >> some changes will be needed in the ExtensionPopup::Show and associated
> >> routines.)
> >>
> >>
> >>> We need to correct this since it looks like people (e.g. dhg@) are
> >>
> >> already
> >>>
> >>> beginning to use this for other purposes.
> >>
> >>> TabContents should never rely on Browser, it is at a lower level of
> >>
> >> abstraction.
> >>
> >> I added this method because of the need for these parameters during
> >> display of an ExtensionPopup view.  (See
> >> browser\views\extensions\extension_popup.{h|cc})  The experimental
> >> extension popup API only has access to an ExtensionFunctionDispatcher,
> >> and needed a means to gain access to these values.  See
> >> browser\extensions\extension_pop_api.cc.
> >>
> >> The TabContentsDelegate is used to implement the
> >> ExtensionFunctionDispatcher::Delegate interface in both ExtensionDOMUI,
> >> and ExtensionHost.  This is because popups can be launched both from
> >> ExtensionHosts (toolstrips), or from a TabContents view that has been
> >> navigated to an extension url.
> >>
> >> Looking at the use of the Browser in the construction of the
> >> ExtensionPopup, it seems to be required during the construction of the
> >> ExtensionHost for use within the popup.  See
> >> ExtensionProcessManager::CreateView.  Perhaps it would be possible to
> >> decouple these functions from the browser, but further inspection will
> >> be required.
> >>
> >> The native window that frames the view that is launching the popup is
> >> used to find the corresponding widget.  This is subsequently used during
> >> positioning of the popup (which is a top-level window).  The widget is
> >> used to convert the coordinates of the position of the popup, which are
> >> in the coordinate frame of the frame to coordinates on the screen.
> >>
> >> I hope that makes some sense.  Please let me know if you need more
> >> clarification.  I was not aware, at the time, of the abstraction
> >> boundary between the TabContentsDelegate and Browser classes.
> >>
> >> I'll investigate if there is a means to have the
> >> ExtensionFunctionDispatcher (which I presume can know all about the
> >> Browser) provide a different access point for the browser and framing
> >> window.
> >>
> >> http://codereview.chromium.org/434046/show
> >>
> >
>



-- 
Software Developer
Google Canada
(514) 670-8756