Update DataTransfer.Files upon change to it's items list [chromium/src : main]
Utkarsh Pathak (Gerrit)
Utkarsh Pathak added 2 comments![]( "Open in Gerrit")
if (auto* file = DynamicTo<File>(blob)) {Do we need this dynamic conversion? Since `GetAsFile()` method gives a File* I think we should be able to use it directly, no need for conversion from parent class blob to child class File. Just use File* instead of blob and check if this File* is not nullptr in the if condition.
var dt = new DataTransfer();nit: consider using let for better scoping and reduced risks
Related details
- Rohan Raja
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pranav Modi (Gerrit)
Pranav Modi added 1 comment![]( "Open in Gerrit")
var filelist = dt.files;+1 on utkarsh suggestion. Use of const is also suggested if the values might not need to be reassigned hereafter.
Related details
- Rohan Raja
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rohan Raja (Gerrit)
Rohan Raja added 3 comments![]( "Open in Gerrit")
Do we need this dynamic conversion? Since `GetAsFile()` method gives a File* I think we should be able to use it directly, no need for conversion from parent class blob to child class File. Just use File* instead of blob and check if this File* is not nullptr in the if condition.
Makes sense, thanks for pointing out.
nit: consider using let for better scoping and reduced risks
use const
+1 on utkarsh suggestion. Use of const is also suggested if the values might not need to be reassigned hereafter.
- Pranav Modi
- Rakesh Goulikar
- Utkarsh Pathak
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christine Hollingsworth (Gerrit)
Christine Hollingsworth added 3 comments![]( "Open in Gerrit")
Almost LGTM pending open question - thanks!
File* file = data_object_->Item(i)->GetAsFile();Just curious, is there a reason for updating this logic to remove the Blob --> File conversion piece?
if (file != nullptr) {Nit: can be simplified to `if (file) { ...`
Related details
- Ayu Ishii
- Pranav Modi
- Rakesh Goulikar
- Rohan Raja
- Utkarsh Pathak
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ayu Ishii (Gerrit)
Ayu Ishii added 2 comments![]( "Open in Gerrit")
if (CanReadData()) {Can we return early for `!CanReadData`? Prefer less deeply nested code for readability.
<!DOCTYPE html>Should this be under `wpt` tests, especially since this should work the same way across browsers?
README mentions "'Web platform tests' (WPT) are the preferred form of web tests and are located at web_tests/external/wpt. Tests that should work across browsers go there. Other directories are for Chrome-specific tests only."
Related details
Rohan Raja (Gerrit)
Rohan Raja added 4 comments![]( "Open in Gerrit")
Can we return early for `!CanReadData`? Prefer less deeply nested code for readability.
Done
File* file = data_object_->Item(i)->GetAsFile();Just curious, is there a reason for updating this logic to remove the Blob --> File conversion piece?
This change was pointed out during the initial review of this CL and it seems to make sense to prevent redundant conversion to Blob and then back again to File type since GetAsFile already returns a File*.
Nit: can be simplified to `if (file) { ...`
Done, thanks
Should this be under `wpt` tests, especially since this should work the same way across browsers?
README mentions "'Web platform tests' (WPT) are the preferred form of web tests and are located at web_tests/external/wpt. Tests that should work across browsers go there. Other directories are for Chrome-specific tests only."
- Ayu Ishii
- Christine Hollingsworth
- Pranav Modi
- Rakesh Goulikar
- Utkarsh Pathak
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christine Hollingsworth (Gerrit)
Christine Hollingsworth voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christine Hollingsworth
| Code-Review | +1 |
1 comment
Related details
- Ayu Ishii
- Pranav Modi
- Rakesh Goulikar
- Rohan Raja
- Utkarsh Pathak
Ayu Ishii (Gerrit)
Ayu Ishii voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ayu Ishii
| Code-Review | +1 |
1 comment
- Pranav Modi
- Rakesh Goulikar
- Rohan Raja
- Utkarsh Pathak
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/49127.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Pranav Modi
- Rakesh Goulikar
- Rohan Raja
- Utkarsh Pathak
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rohan Raja (Gerrit)
Rohan Raja voted Commit-Queue+2![]( "Open in Gerrit")
- Pranav Modi
- Rakesh Goulikar
- Utkarsh Pathak
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Update DataTransfer.Files upon change to it's items list
Refactored "blink::DataTransfer" class to update "files_" member when
it's DataObject's item list is changed, instead of doing the same in
the files getter. This change ensures that files is updated
immediately when the DataTransfer's item list changes.
This fixes the issue where if a reference to DataTransfer.files is
stored in a separate JS variable, then any file items added to that
DataTransfer object are not reflected in the stored JS variable, until
the DataTransfer object's files getter is invoked. The issue is not
present in Safari and Firefox.
- M third_party/blink/renderer/core/clipboard/data_transfer.cc
- A third_party/blink/web_tests/external/wpt/clipboard-apis/data-transfer-file-list-change-reference-updates.html
Code-Review: +1 by Ayu Ishii, +1 by Christine Hollingsworth
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/49127
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |