Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1973)

Issue 385108: Transfer CF downloads to the host browser.... (Closed)

Created:
11 years, 1 month ago by tommi (sloooow) - chröme
Modified:
9 years, 7 months ago
Reviewers:
amit, stoyan
CC:
chromium-reviews_googlegroups.com, ben+cc_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 8

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+180 lines, -39 lines) Patch
M chrome/browser/external_tab_container.cc View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M chrome_frame/chrome_active_document.cc View 4 3 chunks +19 lines, -21 lines 0 comments Download
M chrome_frame/chrome_frame_activex_base.h View 1 2 3 4 9 chunks +20 lines, -16 lines 0 comments Download
M chrome_frame/extra_system_apis.h View 1 2 3 4 1 chunk +45 lines, -0 lines 0 comments Download
M chrome_frame/urlmon_url_request.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M chrome_frame/urlmon_url_request.cc View 1 2 3 4 3 chunks +91 lines, -2 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
tommi (sloooow) - chröme
11 years, 1 month ago (2009-11-13 21:24:53 UTC) #1
tommi (sloooow) - chröme
11 years, 1 month ago (2009-11-13 23:15:21 UTC) #2
stoyan
lgtm
11 years, 1 month ago (2009-11-13 23:36:13 UTC) #3
tommi (sloooow) - chröme
thanks stoyan - IE7 threw me a curveball - RevokeBindStatusCallback doesn't revoke the callback and ...
11 years, 1 month ago (2009-11-14 03:59:07 UTC) #4
amit
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 ...
11 years, 1 month ago (2009-11-18 07:04:10 UTC) #5
tommi (sloooow) - chröme
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 ...
11 years, 1 month ago (2009-11-18 18:35:39 UTC) #6
amit
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 ...
11 years, 1 month ago (2009-11-18 20:27:28 UTC) #7
tommi (sloooow) - chröme
On Wed, Nov 18, 2009 at 12:27 PM, <amit@chromium.org> wrote: > > http://codereview.chromium.org/385108/diff/3001/4002 > File ...
11 years, 1 month ago (2009-11-18 23:35:59 UTC) #8
amit
On Wed, Nov 18, 2009 at 3:35 PM, Tommi <tommi@chromium.org> wrote: > On Wed, Nov ...
11 years, 1 month ago (2009-11-19 00:29:34 UTC) #9
tommi (sloooow) - chröme
so, all of downcasting, GetRequestType(), virtual ToYourDerivedImplementationNameHere(), DownloadInHost(void* host) are pretty ugly. One way of ...
11 years, 1 month ago (2009-11-19 18:35:52 UTC) #10
amit
11 years, 1 month ago (2009-11-19 19:45:46 UTC) #11
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
>>>>
>>>
>>>
>>
>

Powered by Google App Engine
This is Rietveld 408576698