Compute ResourceResponse::suggestedFileName on the fly.

Compute ResourceResponse::suggestedFileName on the fly.

This was showing up in profiles as a major part of
WebURLLoaderImpl::Context::OnReceivedResponse.

[Charlie Harrison, 2016-10-06 12:39:00]

csharrison@chromium.org changed reviewers:
+ dcheng@chromium.org

[Charlie Harrison, 2016-10-06 12:39:00]

dcheng, I'm struggling a bit here to find ways to get around calling a static
net/base function with a GURL parameter from blink.

I imagine I'll have to make a place in //content/child or //content/renderer
with this function, and call it from that layer with a WebURL, but I'm not sure
the canonical place for that (Platform?).

I could also do something terrible like pass the KURL as a std::string. I would
really like to avoid that.

[kinuko, 2016-10-06 18:12:07]

On 2016/10/06 12:39:00, Charlie Harrison wrote:
> dcheng, I'm struggling a bit here to find ways to get around calling a static
> net/base function with a GURL parameter from blink.
> 
> I imagine I'll have to make a place in //content/child or //content/renderer
> with this function, and call it from that layer with a WebURL, but I'm not
sure
> the canonical place for that (Platform?).
> 
> I could also do something terrible like pass the KURL as a std::string. I
would
> really like to avoid that.

Somehow just spotted this one--

one way to make it work would be including public/platform/URLConversion.h and
WebStringToGURL(url().getString()).

[Charlie Harrison, 2016-10-06 18:14:42]

On 2016/10/06 18:12:07, kinuko (slow) wrote:
> On 2016/10/06 12:39:00, Charlie Harrison wrote:
> > dcheng, I'm struggling a bit here to find ways to get around calling a
static
> > net/base function with a GURL parameter from blink.
> > 
> > I imagine I'll have to make a place in //content/child or //content/renderer
> > with this function, and call it from that layer with a WebURL, but I'm not
> sure
> > the canonical place for that (Platform?).
> > 
> > I could also do something terrible like pass the KURL as a std::string. I
> would
> > really like to avoid that.
> 
> Somehow just spotted this one--
> 
> one way to make it work would be including public/platform/URLConversion.h and
> WebStringToGURL(url().getString()).

:( I'm trying to remove cases where we re-parse the URLs wholesale, but this is
a rare enough case where I guess this is ok.

[dcheng, 2016-10-06 18:24:09]

Is this used for anything other than copy image?

If that's the only use case for this, maybe it'd to just plumb through the
information needed for this call for that use case by adding some fields to
DataTransfer.

[Charlie Harrison, 2016-10-06 19:02:02]

On 2016/10/06 18:24:09, dcheng wrote:
> Is this used for anything other than copy image?
> 
> If that's the only use case for this, maybe it'd to just plumb through the
> information needed for this call for that use case by adding some fields to
> DataTransfer.

Sorry, I don't fully understand your suggestion. This is only used for copy
image, so pulling all logic out of ResourceResponse SGTM, with the caveat that
platform/network is the only Blink directory I know of that can pull in
//net/base via DEPS.

Are you suggesting adding a wrapper in platform/network/NetworkUtils which calls
through to net? We could call that in DataTransfer.

[Charlie Harrison, 2016-10-06 21:57:16]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 21:57:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-10-06 21:59:04]

I ended up implementing this, so ptal if I interpreted you correctly.

Since the url conversion is not happening per-request I am still happy.

[dcheng, 2016-10-06 22:04:41]

On 2016/10/06 21:59:04, Charlie Harrison wrote:
> I ended up implementing this, so ptal if I interpreted you correctly.
> 
> Since the url conversion is not happening per-request I am still happy.

Sorry, I had to do some code archaeology.

Basically, I propose changing DataTransfer::addSharedBuffer() so we plumb
through whatever information net::GetSuggestedFilename() needs. This special
DataTransfer item is basically only used for dragging out images. Then we can
just do this calculation in //content in
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content....

In fact, if you read the Chromium-side, it *also* calls
net::GetSuggestedFilename()... so maybe we should just be doing this once.

[Charlie Harrison, 2016-10-06 22:36:05]

On 2016/10/06 22:04:41, dcheng wrote:
> On 2016/10/06 21:59:04, Charlie Harrison wrote:
> > I ended up implementing this, so ptal if I interpreted you correctly.
> > 
> > Since the url conversion is not happening per-request I am still happy.
> 
> Sorry, I had to do some code archaeology.
> 
> Basically, I propose changing DataTransfer::addSharedBuffer() so we plumb
> through whatever information net::GetSuggestedFilename() needs. This special
> DataTransfer item is basically only used for dragging out images. Then we can
> just do this calculation in //content in
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content....
> 
> In fact, if you read the Chromium-side, it *also* calls
> net::GetSuggestedFilename()... so maybe we should just be doing this once.

Ok that makes sense. I'm a bit nervous that this will cause issues because we'll
be creating a subset of WebDragDatas without titles.

AFAICT we do call net::GetSuggestedFilename but only for mac / win platforms. It
looks like for mac we are calling it with the same url just without the
content-disposition.

[dcheng, 2016-10-07 04:37:15]

On 2016/10/06 22:36:05, Charlie Harrison wrote:
> On 2016/10/06 22:04:41, dcheng wrote:
> > On 2016/10/06 21:59:04, Charlie Harrison wrote:
> > > I ended up implementing this, so ptal if I interpreted you correctly.
> > > 
> > > Since the url conversion is not happening per-request I am still happy.
> > 
> > Sorry, I had to do some code archaeology.
> > 
> > Basically, I propose changing DataTransfer::addSharedBuffer() so we plumb
> > through whatever information net::GetSuggestedFilename() needs. This special
> > DataTransfer item is basically only used for dragging out images. Then we
can
> > just do this calculation in //content in
> >
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content....
> > 
> > In fact, if you read the Chromium-side, it *also* calls
> > net::GetSuggestedFilename()... so maybe we should just be doing this once.
> 
> Ok that makes sense. I'm a bit nervous that this will cause issues because
we'll
> be creating a subset of WebDragDatas without titles.

Hmm, where do you see this happening? I think there are already potential cases
today where the generation can fail, so as long as it's not worse, that's
probably OK.

> 
> AFAICT we do call net::GetSuggestedFilename but only for mac / win platforms.
It
> looks like for mac we are calling it with the same url just without the
> content-disposition.

[dcheng, 2016-10-07 04:38:37]

On 2016/10/07 04:37:15, dcheng wrote:
> On 2016/10/06 22:36:05, Charlie Harrison wrote:
> > On 2016/10/06 22:04:41, dcheng wrote:
> > > On 2016/10/06 21:59:04, Charlie Harrison wrote:
> > > > I ended up implementing this, so ptal if I interpreted you correctly.
> > > > 
> > > > Since the url conversion is not happening per-request I am still happy.
> > > 
> > > Sorry, I had to do some code archaeology.
> > > 
> > > Basically, I propose changing DataTransfer::addSharedBuffer() so we plumb
> > > through whatever information net::GetSuggestedFilename() needs. This
special
> > > DataTransfer item is basically only used for dragging out images. Then we
> can
> > > just do this calculation in //content in
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content....
> > > 
> > > In fact, if you read the Chromium-side, it *also* calls
> > > net::GetSuggestedFilename()... so maybe we should just be doing this once.
> > 
> > Ok that makes sense. I'm a bit nervous that this will cause issues because
> we'll
> > be creating a subset of WebDragDatas without titles.
> 
> Hmm, where do you see this happening? I think there are already potential
cases
> today where the generation can fail, so as long as it's not worse, that's
> probably OK.
> 
> > 
> > AFAICT we do call net::GetSuggestedFilename but only for mac / win
platforms.
> It
> > looks like for mac we are calling it with the same url just without the
> > content-disposition.

Assuming you mean Win/Linux, Mac has a different path, but in theory, there's no
reason we couldn't fix it there too, right?

[Charlie Harrison, 2016-10-07 12:19:42]

So, we will be clearing the |title| attribute of WebDragData, and (essentially),
putting it back in on the roundtrip once we've calculated the title in the
browser process.

I need to double check if e.g. the title needs to be used in the intermediate
objects (DataObject, WebDragData) before it is initialized in the browser.

I think this is safe though, so I will look into it.

[Charlie Harrison, 2016-10-07 17:18:58]

So, I think we should do this refactor, but I don't have time to do it now.
Would you accept the patch's current design with an added TODO linking to a
crbug?

I think all filename and file extension (including validation) computation will
have to move to the browser process.

[Charlie Harrison, 2017-03-03 20:57:26]

Deprecated, dcheng solved this one.

[Charlie Harrison, 2017-03-03 20:57:31]

Description was changed from

==========
Compute ResourceResponse::suggestedFileName on the fly.

This was showing up in profiles as a major part of
WebURLLoaderImpl::Context::OnReceivedResponse.
==========

to

==========
Compute ResourceResponse::suggestedFileName on the fly.

This was showing up in profiles as a major part of
WebURLLoaderImpl::Context::OnReceivedResponse.
==========