Transfer CF downloads to the host browser....

Transfer CF downloads to the host browser.TEST=Click a download link (not via redirect) in a web page displayed by Chrome Frame.  The request should be handled by the host browser.BUG=23561
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33022

[stoyan, 2009-11-13 23:36:13]

lgtm

[tommi (sloooow) - chröme, 2009-11-14 03:59:07]

thanks stoyan - IE7 threw me a curveball - RevokeBindStatusCallback doesn't
revoke the callback and a couple of other things.
So, I'll rework the patch and send out another review when it's ready.

On Fri, Nov 13, 2009 at 6:36 PM, <stoyan@chromium.org> wrote:

> lgtm
>
>
> http://codereview.chromium.org/385108
>

[amit, 2009-11-18 07:04:10]

lgtm

http://codereview.chromium.org/385108/diff/3001/4002
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/385108/diff/3001/4002#newcode466
Line 466: static_cast<UrlmonUrlRequest*>(request)->TransferToHost(doc_site_);
Ugg! I am not sure if static cast will work here as expected. In theory the map
can contain any kind of PluginUrlRequest and static_cast of no way of knowing
that at compile time.

I am wondering if ToUrlmonRequest() or ToNPAPIRequest() pattern is better than
static_cast here.

http://codereview.chromium.org/385108/diff/3001/4003
File chrome_frame/urlmon_url_request.cc (left):

http://codereview.chromium.org/385108/diff/3001/4003#oldcode231
Line 231: binding_ = NULL;
curious as to why this is removed.

http://codereview.chromium.org/385108/diff/3001/4003
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/385108/diff/3001/4003#newcode172
Line 172: DCHECK(wb2);
if (wb2) to be safe

http://codereview.chromium.org/385108/diff/3001/4003#newcode205
Line 205: hr = common_wbp2->NavigateWithBindCtx2(uri_obj, NULL, NULL, NULL,
NULL,
80

[tommi (sloooow) - chröme, 2009-11-18 18:35:39]

http://codereview.chromium.org/385108/diff/3001/4002
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/385108/diff/3001/4002#newcode466
Line 466: static_cast<UrlmonUrlRequest*>(request)->TransferToHost(doc_site_);
we did discuss this actually (siggi, robert and I).  the only thing static_cast
can decide upon at compile time is the fact that UrlmonUrlrequest does derive
from PluginUrlRequest. If it does not, there'll be a compile time error.  If
there's no error the functionality is basically reinterpret_cast.

http://codereview.chromium.org/385108/diff/3001/4003
File chrome_frame/urlmon_url_request.cc (left):

http://codereview.chromium.org/385108/diff/3001/4003#oldcode231
Line 231: binding_ = NULL;
On 2009/11/18 07:04:10, amit wrote:
> curious as to why this is removed.
removed because OnStopBinding is called before we get the download notification.
 So, we need to keep these variables around.  However, this is a good point and
I need to make sure that there are no circular references.  If there are, I'll
have to introduce an Uninitialize/Cleanup method.

[amit, 2009-11-18 20:27:28]

http://codereview.chromium.org/385108/diff/3001/4002
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/385108/diff/3001/4002#newcode466
Line 466: static_cast<UrlmonUrlRequest*>(request)->TransferToHost(doc_site_);
On 2009/11/18 18:35:39, tommi wrote:
> we did discuss this actually (siggi, robert and I).  the only thing
static_cast
> can decide upon at compile time is the fact that UrlmonUrlrequest does derive
> from PluginUrlRequest. If it does not, there'll be a compile time error.  If
> there's no error the functionality is basically reinterpret_cast.
Right, that's why I was wondering if introducing 'virtual ToUrlmonRequest()' or
'virtual ToNPAPIRequest()' is better than reinterpret_cast. That way we will use
that pointer only if it is of the correct type.

http://codereview.chromium.org/385108/diff/3001/4003
File chrome_frame/urlmon_url_request.cc (left):

http://codereview.chromium.org/385108/diff/3001/4003#oldcode231
Line 231: binding_ = NULL;
On 2009/11/18 18:35:39, tommi wrote:
> On 2009/11/18 07:04:10, amit wrote:
> > curious as to why this is removed.
> removed because OnStopBinding is called before we get the download
notification.
>  So, we need to keep these variables around.  However, this is a good point
and
> I need to make sure that there are no circular references.  If there are, I'll
> have to introduce an Uninitialize/Cleanup method.
> 

A NULL binding_ indicates that a request is done and this can cause side
effects. For e.g. if we get ReadAsync or StopAsync we will call EndRequest
again.

[tommi (sloooow) - chröme, 2009-11-18 23:35:59]

On Wed, Nov 18, 2009 at 12:27 PM, <amit@chromium.org> wrote:

>
> http://codereview.chromium.org/385108/diff/3001/4002
> File chrome_frame/chrome_frame_activex_base.h (right):
>
> http://codereview.chromium.org/385108/diff/3001/4002#newcode466
> Line 466:
> static_cast<UrlmonUrlRequest*>(request)->TransferToHost(doc_site_);
> On 2009/11/18 18:35:39, tommi wrote:
>
>> we did discuss this actually (siggi, robert and I).  the only thing
>>
> static_cast
>
>> can decide upon at compile time is the fact that UrlmonUrlrequest does
>>
> derive
>
>> from PluginUrlRequest. If it does not, there'll be a compile time
>>
> error.  If
>
>> there's no error the functionality is basically reinterpret_cast.
>>
> Right, that's why I was wondering if introducing 'virtual
> ToUrlmonRequest()' or 'virtual ToNPAPIRequest()' is better than
> reinterpret_cast. That way we will use that pointer only if it is of the
> correct type.


virtual RequestType GetRequestType(); ?


>
> http://codereview.chromium.org/385108/diff/3001/4003
> File chrome_frame/urlmon_url_request.cc (left):
>
> http://codereview.chromium.org/385108/diff/3001/4003#oldcode231
> Line 231: binding_ = NULL;
> On 2009/11/18 18:35:39, tommi wrote:
>
>> On 2009/11/18 07:04:10, amit wrote:
>> > curious as to why this is removed.
>> removed because OnStopBinding is called before we get the download
>>
> notification.
>
>>  So, we need to keep these variables around.  However, this is a good
>>
> point and
>
>> I need to make sure that there are no circular references.  If there
>>
> are, I'll
>
>> have to introduce an Uninitialize/Cleanup method.
>>
>
>
> A NULL binding_ indicates that a request is done and this can cause side
> effects. For e.g. if we get ReadAsync or StopAsync we will call
> EndRequest again.


thanks - I'll look into it


>
>
> http://codereview.chromium.org/385108
>

[amit, 2009-11-19 00:29:34]

On Wed, Nov 18, 2009 at 3:35 PM, Tommi <tommi@chromium.org> wrote:

> On Wed, Nov 18, 2009 at 12:27 PM, <amit@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/385108/diff/3001/4002
>> File chrome_frame/chrome_frame_activex_base.h (right):
>>
>> http://codereview.chromium.org/385108/diff/3001/4002#newcode466
>> Line 466:
>> static_cast<UrlmonUrlRequest*>(request)->TransferToHost(doc_site_);
>> On 2009/11/18 18:35:39, tommi wrote:
>>
>>> we did discuss this actually (siggi, robert and I).  the only thing
>>>
>> static_cast
>>
>>> can decide upon at compile time is the fact that UrlmonUrlrequest does
>>>
>> derive
>>
>>> from PluginUrlRequest. If it does not, there'll be a compile time
>>>
>> error.  If
>>
>>> there's no error the functionality is basically reinterpret_cast.
>>>
>> Right, that's why I was wondering if introducing 'virtual
>> ToUrlmonRequest()' or 'virtual ToNPAPIRequest()' is better than
>> reinterpret_cast. That way we will use that pointer only if it is of the
>> correct type.
>
>
> virtual RequestType GetRequestType(); ?
>
Well the return type has to be derived type like UrlmonUrlRequest, that's
why it's a iffy.  We had used this 'pattern' elsewhere in chrome for e.g.
old TabContents to WebContents casts

>
>
>>
>> http://codereview.chromium.org/385108/diff/3001/4003
>> File chrome_frame/urlmon_url_request.cc (left):
>>
>> http://codereview.chromium.org/385108/diff/3001/4003#oldcode231
>> Line 231: binding_ = NULL;
>> On 2009/11/18 18:35:39, tommi wrote:
>>
>>> On 2009/11/18 07:04:10, amit wrote:
>>> > curious as to why this is removed.
>>> removed because OnStopBinding is called before we get the download
>>>
>> notification.
>>
>>>  So, we need to keep these variables around.  However, this is a good
>>>
>> point and
>>
>>> I need to make sure that there are no circular references.  If there
>>>
>> are, I'll
>>
>>> have to introduce an Uninitialize/Cleanup method.
>>>
>>
>>
>> A NULL binding_ indicates that a request is done and this can cause side
>> effects. For e.g. if we get ReadAsync or StopAsync we will call
>> EndRequest again.
>
>
> thanks - I'll look into it
>
>
>>
>>
>> http://codereview.chromium.org/385108
>>
>
>

[tommi (sloooow) - chröme, 2009-11-19 18:35:52]

so, all of downcasting, GetRequestType(), virtual
ToYourDerivedImplementationNameHere(), DownloadInHost(void* host) are pretty
ugly.
One way of avoiding it could be to have the request map owned outside of a
generic class such as the automation client and there, store the most
derived type (e.g. using a template).  The automation client would have
access to the map but not own it and for its purposes just use methods of
the request base class (as it currently does).  The derived class type would
still be available when host specific request functionality is needed by the
host specific control classes.
If you agree, then I'd be happy to do that change but think it should be
done in a separate patch.  What do you think?

On Wed, Nov 18, 2009 at 4:22 PM, Amit Joshi <amit@chromium.org> wrote:

>
>
> On Wed, Nov 18, 2009 at 3:35 PM, Tommi <tommi@chromium.org> wrote:
>
>> On Wed, Nov 18, 2009 at 12:27 PM, <amit@chromium.org> wrote:
>>
>>>
>>> http://codereview.chromium.org/385108/diff/3001/4002
>>> File chrome_frame/chrome_frame_activex_base.h (right):
>>>
>>> http://codereview.chromium.org/385108/diff/3001/4002#newcode466
>>> Line 466:
>>> static_cast<UrlmonUrlRequest*>(request)->TransferToHost(doc_site_);
>>> On 2009/11/18 18:35:39, tommi wrote:
>>>
>>>> we did discuss this actually (siggi, robert and I).  the only thing
>>>>
>>> static_cast
>>>
>>>> can decide upon at compile time is the fact that UrlmonUrlrequest does
>>>>
>>> derive
>>>
>>>> from PluginUrlRequest. If it does not, there'll be a compile time
>>>>
>>> error.  If
>>>
>>>> there's no error the functionality is basically reinterpret_cast.
>>>>
>>> Right, that's why I was wondering if introducing 'virtual
>>> ToUrlmonRequest()' or 'virtual ToNPAPIRequest()' is better than
>>> reinterpret_cast. That way we will use that pointer only if it is of the
>>> correct type.
>>
>>
>> virtual RequestType GetRequestType(); ?
>>
> Well the return type has to be derived type like UrlmonUrlRequest, that's
> why it's a iffy.  We had used this 'pattern' elsewhere in chrome for e.g.
> old TabContents to WebContents casts
>
>>
>>
>>>
>>> http://codereview.chromium.org/385108/diff/3001/4003
>>> File chrome_frame/urlmon_url_request.cc (left):
>>>
>>> http://codereview.chromium.org/385108/diff/3001/4003#oldcode231
>>> Line 231: binding_ = NULL;
>>> On 2009/11/18 18:35:39, tommi wrote:
>>>
>>>> On 2009/11/18 07:04:10, amit wrote:
>>>> > curious as to why this is removed.
>>>> removed because OnStopBinding is called before we get the download
>>>>
>>> notification.
>>>
>>>>  So, we need to keep these variables around.  However, this is a good
>>>>
>>> point and
>>>
>>>> I need to make sure that there are no circular references.  If there
>>>>
>>> are, I'll
>>>
>>>> have to introduce an Uninitialize/Cleanup method.
>>>>
>>>
>>>
>>> A NULL binding_ indicates that a request is done and this can cause side
>>> effects. For e.g. if we get ReadAsync or StopAsync we will call
>>> EndRequest again.
>>
>>
>> thanks - I'll look into it
>>
>>
>>>
>>>
>>> http://codereview.chromium.org/385108
>>>
>>
>>
>

[amit, 2009-11-19 19:45:46]

OK with static cast, for now then :)

On Thu, Nov 19, 2009 at 10:35 AM, Tommi <tommi@chromium.org> wrote:

> so, all of downcasting, GetRequestType(), virtual
> ToYourDerivedImplementationNameHere(), DownloadInHost(void* host) are pretty
> ugly.
> One way of avoiding it could be to have the request map owned outside of a
> generic class such as the automation client and there, store the most
> derived type (e.g. using a template).  The automation client would have
> access to the map but not own it and for its purposes just use methods of
> the request base class (as it currently does).  The derived class type would
> still be available when host specific request functionality is needed by the
> host specific control classes.
> If you agree, then I'd be happy to do that change but think it should be
> done in a separate patch.  What do you think?
>
>
> On Wed, Nov 18, 2009 at 4:22 PM, Amit Joshi <amit@chromium.org> wrote:
>
>>
>>
>> On Wed, Nov 18, 2009 at 3:35 PM, Tommi <tommi@chromium.org> wrote:
>>
>>> On Wed, Nov 18, 2009 at 12:27 PM, <amit@chromium.org> wrote:
>>>
>>>>
>>>> http://codereview.chromium.org/385108/diff/3001/4002
>>>> File chrome_frame/chrome_frame_activex_base.h (right):
>>>>
>>>> http://codereview.chromium.org/385108/diff/3001/4002#newcode466
>>>> Line 466:
>>>> static_cast<UrlmonUrlRequest*>(request)->TransferToHost(doc_site_);
>>>> On 2009/11/18 18:35:39, tommi wrote:
>>>>
>>>>> we did discuss this actually (siggi, robert and I).  the only thing
>>>>>
>>>> static_cast
>>>>
>>>>> can decide upon at compile time is the fact that UrlmonUrlrequest does
>>>>>
>>>> derive
>>>>
>>>>> from PluginUrlRequest. If it does not, there'll be a compile time
>>>>>
>>>> error.  If
>>>>
>>>>> there's no error the functionality is basically reinterpret_cast.
>>>>>
>>>> Right, that's why I was wondering if introducing 'virtual
>>>> ToUrlmonRequest()' or 'virtual ToNPAPIRequest()' is better than
>>>> reinterpret_cast. That way we will use that pointer only if it is of the
>>>> correct type.
>>>
>>>
>>> virtual RequestType GetRequestType(); ?
>>>
>> Well the return type has to be derived type like UrlmonUrlRequest, that's
>> why it's a iffy.  We had used this 'pattern' elsewhere in chrome for e.g.
>> old TabContents to WebContents casts
>>
>>>
>>>
>>>>
>>>> http://codereview.chromium.org/385108/diff/3001/4003
>>>> File chrome_frame/urlmon_url_request.cc (left):
>>>>
>>>> http://codereview.chromium.org/385108/diff/3001/4003#oldcode231
>>>> Line 231: binding_ = NULL;
>>>> On 2009/11/18 18:35:39, tommi wrote:
>>>>
>>>>> On 2009/11/18 07:04:10, amit wrote:
>>>>> > curious as to why this is removed.
>>>>> removed because OnStopBinding is called before we get the download
>>>>>
>>>> notification.
>>>>
>>>>>  So, we need to keep these variables around.  However, this is a good
>>>>>
>>>> point and
>>>>
>>>>> I need to make sure that there are no circular references.  If there
>>>>>
>>>> are, I'll
>>>>
>>>>> have to introduce an Uninitialize/Cleanup method.
>>>>>
>>>>
>>>>
>>>> A NULL binding_ indicates that a request is done and this can cause side
>>>> effects. For e.g. if we get ReadAsync or StopAsync we will call
>>>> EndRequest again.
>>>
>>>
>>> thanks - I'll look into it
>>>
>>>
>>>>
>>>>
>>>> http://codereview.chromium.org/385108
>>>>
>>>
>>>
>>
>