| Summary: | Remove obsolete File attributes | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||||||
| Component: | WebCore Misc. | Assignee: | Alexey Proskuryakov <ap> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | aa, abarth, commit-queue, dglazkov, dimich, fishd, jianli, morrita, ojan, tkent, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 128531 [details]
proposed patch
Talked to Tim Hatcher on IRC, he's fine with removing these from public interfaces.
Created attachment 128537 [details]
with build fix
This is a breaking change. Alex, do you know how many web pages might be still accessing fileName and fileSize? Fisher, is it fine to stop supporting these obsolete attributes in Chromium? I do not know how much content would break, but if we ever plan to drop these attributes, now is a great time, in my opinion. Comment on attachment 128537 [details] with build fix Attachment 128537 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11603448 New failing tests: http/tests/security/clipboard/clipboard-file-access.html editing/pasteboard/file-input-files-access.html Created attachment 128573 [details]
with updated tests
It seems like it would be helpful to announce deprecation / removal of APIs on webkit-dev, and give port maintainers a chance to review the change. For Chromium, we might want to look to see if any Chrome Extensions are using these attributes before we remove them. See for example the webkit-dev thread about removing HTML notifications from WebKit. I'm also a little bit concerned about the possibility of fileName and fileSize being used on the web since Firefox also implements those attributes: https://developer.mozilla.org/en/DOM/File Alexey, have you spoken with any Firefox devs about removing these attributes? What makes you confident that we can make this change without breaking web sites? Just getting my feet wet by following a comment left by someone else, CC'ing people in the know and waiting for feedback. Thanks for the pointers! Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API. Firefox 10 doesn't support these - Jonas says that they were removed a few versions ago. I'm glad to hear that Firefox has removed them too. On the Chromium side, we can certainly be aggressive about disabling these in our nightly / dev channel builds. Hopefully, if it breaks any websites (or extensions), that'll be enough for people to notice and file bug reports. Would it make sense to do a smaller patch up front that just hides the DOM properties? Should we worry about our ability to revert this patch if we find a month from now that it is doom-and-gloom? This seems trivial to revert if needed, I'd prefer removing to hiding if it's OK with you. Comment on attachment 128573 [details]
with updated tests
Looks like there has been no further objection over weeks.
I agree that it's easy to revert this patch because the API is basically just alias or new name.
r+ing.
Comment on attachment 128573 [details] with updated tests Rejecting attachment 128573 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: youtTests/http/tests/security/clipboard/script-tests/clipboard-file-access.js is not empty after patch, as expected 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/http/tests/security/clipboard/script-tests/clipboard-file-access.js.rej rm 'LayoutTests/http/tests/security/clipboard/script-tests/clipboard-file-access.js' Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Hajime Mor..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12035183 Created attachment 132960 [details]
updated for test changes
Comment on attachment 132960 [details] updated for test changes Rejecting attachment 132960 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Hajime Morrita found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12071120 Created attachment 132962 [details]
spell reviewer name per committers.py, not per Bugzilla name
(In reply to comment #17) > Created an attachment (id=132962) [details] > spell reviewer name per committers.py, not per Bugzilla name Oops. I'll align this once the patch landed. Comment on attachment 132962 [details] spell reviewer name per committers.py, not per Bugzilla name Rejecting attachment 132962 [details] from commit-queue. New failing tests: http/tests/security/clipboard/clipboard-file-access.html Full output: http://queues.webkit.org/results/12035269 Landed manually in <http://trac.webkit.org/changeset/111569>. |
// FIXME: obsolete attributes. To be removed. readonly attribute DOMString fileName; readonly attribute unsigned long long fileSize;