New issue
Advanced search Search tips

Issue 654034 link

Starred by 1 user
Status: Fixed
Owner:
dcheng@chromium.org
Closed: Feb 2017
Cc:
csharrison@chromium.org
dcheng@chromium.org
pwnall@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove WebURLResponse::suggestedFilename and WebURLResponse::setSuggestedFilename from Blink public API

Project Member Reported by dcheng@chromium.org, Oct 7 2016 
For every request, we do a suggested filename calculation. This is showing up in profiles as a major part of WebURLLoaderImpl::Context::OnReceivedResponse.

csharrison@ prepared an initial patch to make it not generated for everything [1]. When looking at it, we realized that the only reason we need this is for drag and drop: when an image is dragged out of Chrome into the file manager, we use the suggestedFilename calculated in OnReceivedResponse().

This is really wasteful and it doesn't make any sense to do it here: we should be doing this work in the browser-side instead, especially since the browser-side already *also* calls GetSuggetedFilename in some paths [2].

[1] https://codereview.chromium.org/2398053003/
[2] https://cs.chromium.org/chromium/src/content/browser/web_contents/web_contents_view_aura.cc?rcl=0&l=153

Comment 1 by pwnall@chromium.org, Oct 31 2016

Status: Available (was: Untriaged)

Comment 2 by csharrison@chromium.org, Jan 4 2017 

PerformanceTests/XMLHttpRequest/send.html shows that the call to get the suggested filename is taking ~3.5% of CPU time on the main thread (on Linux using perf). I wonder if we could just land [1] or increase the priority of this work?

Comment 3 by csharrison@chromium.org, Jan 4 2017 

Oops, sorry stupid arithmetic error. It's more like 1.7% of the main thread's time.

Comment 4 by pwnall@chromium.org, Jan 5 2017 

I must admit I have no clue how 1.7% ranks among performance improvements. If it's a big deal, please bump up the issue's priority, and I'll get to it sooner :)

Comment 5 by dcheng@chromium.org, Feb 5 2017

Owner: dcheng@chromium.org
Status: Started (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 8 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3dd8561d357d05ba538137ed8736b8732e27c0d8

commit 3dd8561d357d05ba538137ed8736b8732e27c0d8
Author: dcheng <dcheng@chromium.org>
Date: Wed Feb 08 10:46:23 2017

Only generate suggested filenames when actually dragging an image.

ResourceResponse::suggestedFilename() is only used when dragging an
image out of a WebView, yet it was still calculated for every resource
response.

As previously implemented, there were a number of other issues:
- The suggested filename calculation was implemented in //content/child.
  However, in Blink, DataTransfer didn't know that the calculation, as
  implemented, would never be empty. Thus, it implemented fallback paths
  for an empty filename, even though it was never possible to reach it.
- In //content/browser, the code to start the actual drag also tried to
  handle the empty filename case, even though the fallback path should
  never be hit.
- Blink also had filename validation code that was used only by the
  image dragging code.

This CL plumbs the information needed to calculate the filename:
  - the image URL
  - the filename extension, as reported by the image decoder
  - the Content-Disposition header, if any
up into the browser process and performs the calculation there. This
allows all the logic regarding filename validation to be centralized in
one location.

Note that this CL does not plumb the filename extension through as a
MIME type. This is intentional: though this would make the code more
generic so future CLs could add support for dragging non-image binary
data, consulting the MIME registry to determine the right filename
extension often involves filesystem I/O, which should be avoided on the
UI thread.

BUG= 654034 

Review-Url: https://codereview.chromium.org/2674953003
Cr-Commit-Position: refs/heads/master@{#448946}

[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/components/test_runner/event_sender.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/browser/web_contents/web_contents_view_aura.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/browser/web_contents/web_drag_source_mac.mm
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/common/drag_traits.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/public/common/BUILD.gn
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/public/common/DEPS
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/public/common/drop_data.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/public/common/drop_data.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/renderer/drop_data_builder.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/renderer/render_widget.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataObject.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataObject.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataObjectItem.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataObjectItem.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataTransfer.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataTransfer.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/loader/ProgressTrackerTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResourceTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/page/DragController.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/clipboard/ClipboardUtilities.h
[delete] https://crrev.com/fb3db819dfcac399579102ecb93917c48d6769ce/third_party/WebKit/Source/platform/clipboard/ClipboardUtilitiesPosix.cpp
[delete] https://crrev.com/fb3db819dfcac399579102ecb93917c48d6769ce/third_party/WebKit/Source/platform/clipboard/ClipboardUtilitiesTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/clipboard/ClipboardUtilitiesWin.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/loader/fetch/MemoryCacheCorrectnessTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/network/ResourceResponse.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/network/ResourceResponse.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/public/platform/WebDragData.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/public/platform/WebURLResponse.h
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 8 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3dd8561d357d05ba538137ed8736b8732e27c0d8

commit 3dd8561d357d05ba538137ed8736b8732e27c0d8
Author: dcheng <dcheng@chromium.org>
Date: Wed Feb 08 10:46:23 2017

Only generate suggested filenames when actually dragging an image.

ResourceResponse::suggestedFilename() is only used when dragging an
image out of a WebView, yet it was still calculated for every resource
response.

As previously implemented, there were a number of other issues:
- The suggested filename calculation was implemented in //content/child.
  However, in Blink, DataTransfer didn't know that the calculation, as
  implemented, would never be empty. Thus, it implemented fallback paths
  for an empty filename, even though it was never possible to reach it.
- In //content/browser, the code to start the actual drag also tried to
  handle the empty filename case, even though the fallback path should
  never be hit.
- Blink also had filename validation code that was used only by the
  image dragging code.

This CL plumbs the information needed to calculate the filename:
  - the image URL
  - the filename extension, as reported by the image decoder
  - the Content-Disposition header, if any
up into the browser process and performs the calculation there. This
allows all the logic regarding filename validation to be centralized in
one location.

Note that this CL does not plumb the filename extension through as a
MIME type. This is intentional: though this would make the code more
generic so future CLs could add support for dragging non-image binary
data, consulting the MIME registry to determine the right filename
extension often involves filesystem I/O, which should be avoided on the
UI thread.

BUG= 654034 

Review-Url: https://codereview.chromium.org/2674953003
Cr-Commit-Position: refs/heads/master@{#448946}

[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/components/test_runner/event_sender.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/browser/web_contents/web_contents_view_aura.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/browser/web_contents/web_drag_source_mac.mm
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/common/drag_traits.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/public/common/BUILD.gn
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/public/common/DEPS
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/public/common/drop_data.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/public/common/drop_data.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/renderer/drop_data_builder.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/content/renderer/render_widget.cc
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataObject.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataObject.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataObjectItem.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataObjectItem.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataTransfer.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/clipboard/DataTransfer.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/loader/ProgressTrackerTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResourceTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/core/page/DragController.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/clipboard/ClipboardUtilities.h
[delete] https://crrev.com/fb3db819dfcac399579102ecb93917c48d6769ce/third_party/WebKit/Source/platform/clipboard/ClipboardUtilitiesPosix.cpp
[delete] https://crrev.com/fb3db819dfcac399579102ecb93917c48d6769ce/third_party/WebKit/Source/platform/clipboard/ClipboardUtilitiesTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/clipboard/ClipboardUtilitiesWin.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/loader/fetch/MemoryCacheCorrectnessTest.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/network/ResourceResponse.cpp
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/Source/platform/network/ResourceResponse.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/public/platform/WebDragData.h
[modify] https://crrev.com/3dd8561d357d05ba538137ed8736b8732e27c0d8/third_party/WebKit/public/platform/WebURLResponse.h

Comment 8 by dcheng@chromium.org, Feb 8 2017

Status: Fixed (was: Started)
►

Sign in to add a comment