Only generate suggested filenames when actually dragging an image.

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}
Committed: https://chromium.googlesource.com/chromium/src/+/3dd8561d357d05ba538137ed8736b8732e27c0d8

[dcheng, 2017-02-04 10:53:28]

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

[commit-bot: I haz the power, 2017-02-04 10:53:38]

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

[commit-bot: I haz the power, 2017-02-04 10:57:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 10:57:55]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[dcheng, 2017-02-04 11:00:51]

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

[commit-bot: I haz the power, 2017-02-04 11:01:00]

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

[commit-bot: I haz the power, 2017-02-04 11:13:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 11:13:09]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-02-04 20:34:06]

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

[commit-bot: I haz the power, 2017-02-04 20:34:19]

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

[commit-bot: I haz the power, 2017-02-04 20:56:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 20:56:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dcheng, 2017-02-04 21:01:15]

Description was changed from

==========
Defer suggested filename generation to drag initiation.

It's silly to calculate for this every single resource response, since
it's only used when dragging images.

BUG=654034
==========

to

==========
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
==========

[dcheng, 2017-02-04 21:01:16]

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

[dcheng, 2017-02-04 21:05:44]

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

[commit-bot: I haz the power, 2017-02-04 21:05:50]

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

[commit-bot: I haz the power, 2017-02-04 21:28:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 21:29:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-02-05 01:26:00]

dcheng@chromium.org changed reviewers:
- csharrison@chromium.org, esprehn@chromium.org

[dcheng, 2017-02-05 01:27:02]

dcheng@chromium.org changed reviewers:
+ avi@chromium.org, csharrison@chromium.org, esprehn@chromium.org

[dcheng, 2017-02-05 01:27:56]

dcheng@chromium.org changed reviewers:
+ kinuko@chromium.org
- avi@chromium.org, esprehn@chromium.org

[dcheng, 2017-02-05 01:28:05]

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

[commit-bot: I haz the power, 2017-02-05 01:28:13]

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

[commit-bot: I haz the power, 2017-02-05 01:50:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 01:50:25]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dcheng, 2017-02-05 02:17:28]

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

[commit-bot: I haz the power, 2017-02-05 02:17:41]

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

[commit-bot: I haz the power, 2017-02-05 02:47:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 02:47:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-02-05 03:14:40]

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

[commit-bot: I haz the power, 2017-02-05 03:14:50]

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

[commit-bot: I haz the power, 2017-02-05 03:37:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 03:37:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-02-05 04:25:50]

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

[commit-bot: I haz the power, 2017-02-05 04:25:54]

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

[commit-bot: I haz the power, 2017-02-05 04:47:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 04:47:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dcheng, 2017-02-05 04:48:27]

kinuko, please take a look when you have time.

+csharrison FYI

[dcheng, 2017-02-05 05:46:23]

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

[commit-bot: I haz the power, 2017-02-05 05:46:32]

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

[commit-bot: I haz the power, 2017-02-05 06:19:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 06:19:22]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dcheng, 2017-02-05 06:30:44]

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

[commit-bot: I haz the power, 2017-02-05 06:30:50]

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

[commit-bot: I haz the power, 2017-02-05 08:33:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-05 08:33:51]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-06 22:24:58]

lgtm

Great to see us removing these wasteful code paths!

https://codereview.chromium.org/2674953003/diff/200001/components/test_runner...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/2674953003/diff/200001/components/test_runner...
components/test_runner/event_sender.cc:1912: filename =
filename.ReplaceExtension(filename_extension.utf8());
nit: could we use WebStringToFilePath(filename_extension).value() instead?

https://codereview.chromium.org/2674953003/diff/200001/content/renderer/drop_...
File content/renderer/drop_data_builder.cc (right):

https://codereview.chromium.org/2674953003/diff/200001/content/renderer/drop_...
content/renderer/drop_data_builder.cc:67:
item.binaryDataFilenameExtension.utf8();
nit: WebStringToFilePath(...).value();

[Charlie Harrison, 2017-02-06 22:51:10]

Awesome work, many thanks for taking this on :)

lgtm/2

[dcheng, 2017-02-08 00:01:05]

https://codereview.chromium.org/2674953003/diff/200001/components/test_runner...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/2674953003/diff/200001/components/test_runner...
components/test_runner/event_sender.cc:1912: filename =
filename.ReplaceExtension(filename_extension.utf8());
On 2017/02/06 22:24:58, kinuko wrote:
> nit: could we use WebStringToFilePath(filename_extension).value() instead?

I considered this, but a FilePath isn't really "just" a string. While it's not
implemented like this today, it's possible that a future world would add
canonicalization to the constructor, which might change the returned string from
FilePath::value().

Another alternative I considered is WebStringToFilePathStringType()... if you
think that would be better, I'm happy to add it.

[dcheng, 2017-02-08 01:00:26]

dcheng@chromium.org changed reviewers:
+ asanka@chromium.org, tkent@chromium.org

[dcheng, 2017-02-08 01:00:27]

+tkent for //components/test_runner
+asanka for new DEPS on //components/mime_util from //content/public

[kinuko, 2017-02-08 01:14:41]

On 2017/02/08 00:01:05, dcheng wrote:
>
https://codereview.chromium.org/2674953003/diff/200001/components/test_runner...
> File components/test_runner/event_sender.cc (right):
> 
>
https://codereview.chromium.org/2674953003/diff/200001/components/test_runner...
> components/test_runner/event_sender.cc:1912: filename =
> filename.ReplaceExtension(filename_extension.utf8());
> On 2017/02/06 22:24:58, kinuko wrote:
> > nit: could we use WebStringToFilePath(filename_extension).value() instead?
> 
> I considered this, but a FilePath isn't really "just" a string. While it's not
> implemented like this today, it's possible that a future world would add
> canonicalization to the constructor, which might change the returned string
from
> FilePath::value().
> 
> Another alternative I considered is WebStringToFilePathStringType()... if you
> think that would be better, I'm happy to add it.

If we start to have more code like this that'd be probably cleaner, but I'm fine
with the current code too for now.

[tkent, 2017-02-08 01:23:06]

On 2017/02/08 at 01:00:27, dcheng wrote:
> +tkent for //components/test_runner

lgtm

[asanka, 2017-02-08 04:20:21]

https://codereview.chromium.org/2674953003/diff/200001/content/public/common/...
File content/public/common/DEPS (right):

https://codereview.chromium.org/2674953003/diff/200001/content/public/common/...
content/public/common/DEPS:4: "+components/mime_util",
Has there been a change to the rule that /content/public cannot depend on
//components?

[dcheng, 2017-02-08 04:29:18]

On 2017/02/08 04:20:21, asanka wrote:
>
https://codereview.chromium.org/2674953003/diff/200001/content/public/common/...
> File content/public/common/DEPS (right):
> 
>
https://codereview.chromium.org/2674953003/diff/200001/content/public/common/...
> content/public/common/DEPS:4: "+components/mime_util",
> Has there been a change to the rule that /content/public cannot depend on
> //components?

My understanding is the policy is in //content/DEPS:

  "-components",
  # Content can depend on components that are:
  #   1) related to the implementation of the web platform
  #   2) shared code between third_party/WebKit and content
  # It should not depend on chrome features or implementation details, i.e. the
  # original components/ directories which was code split out from chrome/ to be
  # shared with iOS. This includes, but isn't limited to, browser features such
  # as autofill or extensions, and chrome implementation details such as
  # settings, packaging details, installation or crash reporting.

This isn't code that was split up from chrome to be shared with iOS; this is
code that is meant to be shared between Blink and content. Thus, I think it's OK
(and I'm specifically only allowing this new DEP in .cc files, to avoid it from
leaking into headers).

[asanka, 2017-02-08 04:47:33]

lgtm

[dcheng, 2017-02-08 04:50:35]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-02-08 04:50:54]

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

[commit-bot: I haz the power, 2017-02-08 06:35:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 06:35:18]

Failed to apply patch for third_party/WebKit/Source/platform/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/platform/BUILD.gn:1702
  error: third_party/WebKit/Source/platform/BUILD.gn: patch does not apply

Patch:       third_party/WebKit/Source/platform/BUILD.gn
Index: third_party/WebKit/Source/platform/BUILD.gn
diff --git a/third_party/WebKit/Source/platform/BUILD.gn
b/third_party/WebKit/Source/platform/BUILD.gn
index
ce7a9af6404d50e8ac4f79ca4c656b56822a580d..d60e5d08964f19afc1aadfa5bc29ab2948320f75
100644
--- a/third_party/WebKit/Source/platform/BUILD.gn
+++ b/third_party/WebKit/Source/platform/BUILD.gn
@@ -491,8 +491,6 @@ component("platform") {
     "clipboard/ClipboardMimeTypes.h",
     "clipboard/ClipboardUtilities.cpp",
     "clipboard/ClipboardUtilities.h",
-    "clipboard/ClipboardUtilitiesPosix.cpp",
-    "clipboard/ClipboardUtilitiesWin.cpp",
     "cpu/mips/CommonMacrosMSA.h",
     "credentialmanager/PlatformCredential.cpp",
     "credentialmanager/PlatformCredential.h",
@@ -1512,9 +1510,8 @@ component("platform") {
   }
 
   if (is_win) {
+    sources += [ "clipboard/ClipboardUtilitiesWin.cpp" ]
     sources -= [
-      "clipboard/ClipboardUtilitiesPosix.cpp",
-
       # Uses LocaleWin instead.
       "text/LocaleICU.cpp",
       "text/LocaleICU.h",
@@ -1524,10 +1521,7 @@ component("platform") {
       "/wd4724",  # Modulo by 0.
     ]
   } else {
-    sources -= [
-      "clipboard/ClipboardUtilitiesWin.cpp",
-      "text/LocaleWin.cpp",
-    ]
+    sources -= [ "text/LocaleWin.cpp" ]
   }
 
   if (is_android) {
@@ -1702,7 +1696,6 @@ test("blink_platform_unittests") {
     "animation/CompositorFloatAnimationCurveTest.cpp",
     "animation/TimingFunctionTest.cpp",
     "blob/BlobDataTest.cpp",
-    "clipboard/ClipboardUtilitiesTest.cpp",
     "exported/FilePathConversionTest.cpp",
     "exported/WebStringTest.cpp",
     "feature_policy/FeaturePolicyTest.cpp",

[dcheng, 2017-02-08 07:38:47]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-02-08 07:38:48]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
kinuko@chromium.org, csharrison@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2674953003/#ps210001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-08 07:39:01]

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

[commit-bot: I haz the power, 2017-02-08 09:18:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 09:18:36]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dcheng, 2017-02-08 09:23:16]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-02-08 09:23:34]

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

[commit-bot: I haz the power, 2017-02-08 10:46:27]

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1486545796368780,
"parent_rev": "fb3db819dfcac399579102ecb93917c48d6769ce", "commit_rev":
"3dd8561d357d05ba538137ed8736b8732e27c0d8"}

[commit-bot: I haz the power, 2017-02-08 10:47:06]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/3dd8561d357d05ba538137ed8736...
==========

[commit-bot: I haz the power, 2017-02-08 10:47:13]

Committed patchset #12 (id:210001) as
https://chromium.googlesource.com/chromium/src/+/3dd8561d357d05ba538137ed8736...