Bug 79383 - Remove obsolete File attributes
Summary: Remove obsolete File attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-23 11:46 PST by Alexey Proskuryakov
Modified: 2012-03-21 10:27 PDT (History)
11 users (show)

See Also:


Attachments
proposed patch (2.46 KB, patch)
2012-02-23 12:17 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
with build fix (4.02 KB, patch)
2012-02-23 12:32 PST, Alexey Proskuryakov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
with updated tests (36.22 KB, patch)
2012-02-23 15:45 PST, Alexey Proskuryakov
morrita: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
updated for test changes (36.27 KB, patch)
2012-03-20 20:56 PDT, Alexey Proskuryakov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
spell reviewer name per committers.py, not per Bugzilla name (36.27 KB, patch)
2012-03-20 21:03 PDT, Alexey Proskuryakov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-02-23 11:46:28 PST
// FIXME: obsolete attributes. To be removed.
        readonly attribute DOMString fileName;
        readonly attribute unsigned long long fileSize;
Comment 1 Alexey Proskuryakov 2012-02-23 12:17:46 PST
Created attachment 128531 [details]
proposed patch

Talked to Tim Hatcher on IRC, he's fine with removing these from public interfaces.
Comment 2 Alexey Proskuryakov 2012-02-23 12:32:19 PST
Created attachment 128537 [details]
with build fix
Comment 3 Jian Li 2012-02-23 12:51:32 PST
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?
Comment 4 Alexey Proskuryakov 2012-02-23 13:55:39 PST
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 5 WebKit Review Bot 2012-02-23 15:33:03 PST
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
Comment 6 Alexey Proskuryakov 2012-02-23 15:45:20 PST
Created attachment 128573 [details]
with updated tests
Comment 7 Darin Fisher (:fishd, Google) 2012-02-23 17:45:57 PST
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?
Comment 8 Alexey Proskuryakov 2012-02-23 19:23:50 PST
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!
Comment 9 WebKit Commit Bot 2012-02-24 02:05:26 PST
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.
Comment 10 Alexey Proskuryakov 2012-02-24 09:33:14 PST
Firefox 10 doesn't support these - Jonas says that they were removed a few versions ago.
Comment 11 Darin Fisher (:fishd, Google) 2012-02-24 11:49:09 PST
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?
Comment 12 Alexey Proskuryakov 2012-02-24 11:57:01 PST
This seems trivial to revert if needed, I'd prefer removing to hiding if it's OK with you.
Comment 13 Hajime Morrita 2012-03-15 21:12:26 PDT
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 14 WebKit Review Bot 2012-03-20 20:07:45 PDT
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
Comment 15 Alexey Proskuryakov 2012-03-20 20:56:03 PDT
Created attachment 132960 [details]
updated for test changes
Comment 16 WebKit Review Bot 2012-03-20 20:58:25 PDT
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
Comment 17 Alexey Proskuryakov 2012-03-20 21:03:25 PDT
Created attachment 132962 [details]
spell reviewer name per committers.py, not per Bugzilla name
Comment 18 Hajime Morrita 2012-03-20 23:32:42 PDT
(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 19 WebKit Review Bot 2012-03-20 23:34:45 PDT
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
Comment 20 Alexey Proskuryakov 2012-03-21 10:27:05 PDT
Landed manually in <http://trac.webkit.org/changeset/111569>.