Bug 101898 - HTMLOutputElement::htmlFor should be readonly
Summary: HTMLOutputElement::htmlFor should be readonly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vineet Chaudhary (vineetc)
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2012-11-11 22:55 PST by Kentaro Hara
Modified: 2013-01-28 23:37 PST (History)
13 users (show)

See Also:


Attachments
patch (5.34 KB, patch)
2013-01-23 15:34 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
updated_patch (20.60 KB, patch)
2013-01-23 19:46 PST, Vineet Chaudhary (vineetc)
haraken: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch_for_landing (20.60 KB, patch)
2013-01-24 11:17 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
Patch (20.39 KB, patch)
2013-01-24 12:06 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
Patch (20.03 KB, patch)
2013-01-25 14:29 PST, Vineet Chaudhary (vineetc)
buildbot: commit-queue-
Details | Formatted Diff | Diff
another_try (20.23 KB, patch)
2013-01-28 15:27 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-11-11 22:55:24 PST
Spec: http://dev.w3.org/html5/spec-preview/the-output-element.html

The spec says htmlFor should be readonly, but WebKit implements it as no-readonly.

htmlFor is implemented in http://trac.webkit.org/changeset/71373

bashi, tkent: Is there any reason why htmlFor is implemented as no-readonly?
Comment 1 Kent Tamura 2012-11-11 23:03:51 PST
I think htmlFor was not read-only in old drafts.
Comment 2 Kenichi Ishibashi 2012-11-12 06:11:51 PST
I don't remember, but I think it wasn't readonly in old spec, too. I think we should make it readonly.

Please feel free to prepare the patch or assign the bug to me.
Comment 3 Vineet Chaudhary (vineetc) 2013-01-23 15:34:50 PST
Created attachment 184331 [details]
patch
Comment 4 Kent Tamura 2013-01-23 15:46:58 PST
Comment on attachment 184331 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184331&action=review

> Source/WebCore/ChangeLog:16
> +        * bindings/js/JSHTMLOutputElementCustom.cpp: Removed setHtmlFor() method.
> +        * bindings/v8/custom/V8HTMLOutputElementCustom.cpp: Removed htmlForAccessorSetter().

I think we can remove htmlFor() too.
Comment 5 Kent Tamura 2013-01-23 15:47:34 PST
Comment on attachment 184331 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184331&action=review

>> Source/WebCore/ChangeLog:16
>> +        * bindings/v8/custom/V8HTMLOutputElementCustom.cpp: Removed htmlForAccessorSetter().
> 
> I think we can remove htmlFor() too.

by removing [Custom] in the IDL.
Comment 6 Vineet Chaudhary (vineetc) 2013-01-23 15:56:15 PST
(In reply to comment #5)
> (From update of attachment 184331 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184331&action=review
> 
> >> Source/WebCore/ChangeLog:16
> >> +        * bindings/v8/custom/V8HTMLOutputElementCustom.cpp: Removed htmlForAccessorSetter().
> > 
> > I think we can remove htmlFor() too.
> 
> by removing [Custom] in the IDL.

Yes I just realized that we can remove custom bindings here thanks for pointing,
I will upload patch again.
Comment 7 Kentaro Hara 2013-01-23 15:58:54 PST
Comment on attachment 184331 [details]
patch

Thanks for fixing the bug. Would you check the behavior of other browsers (Firefox, Opera, IE)?
Comment 8 Vineet Chaudhary (vineetc) 2013-01-23 19:46:45 PST
Created attachment 184384 [details]
updated_patch

(In reply to comment #7)
> (From update of attachment 184331 [details])
> Thanks for fixing the bug. Would you check the behavior of other browsers (Firefox, Opera, IE)?

I checked the behavior with other browsers:

Read/Writable On
IE      : 8.0
Opera   : 12.12 (Ubuntu)

Readonly On
Firefox : 18.0
Comment 9 Kentaro Hara 2013-01-23 19:55:26 PST
Comment on attachment 184384 [details]
updated_patch

Given that Firefox follows the spec behavior, the change looks reasonable. Thanks for removing the custom bindings. (The original motivation that I filed this bug is that I wanted to remove the custom bindings.)
Comment 10 WebKit Review Bot 2013-01-24 11:07:58 PST
Comment on attachment 184384 [details]
updated_patch

Rejecting attachment 184384 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 184384, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
LOutputElementCustom.cpp'
patching file Source/WebCore/html/HTMLOutputElement.idl
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/HTMLOutputElement/dom-settable-token-list-expected.txt
patching file LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Kentaro Hara']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16072854
Comment 11 Vineet Chaudhary (vineetc) 2013-01-24 11:17:13 PST
Created attachment 184538 [details]
patch_for_landing

Sorry for previous patch it failed to apply.
Attaching same patch rebaselined.
Comment 12 Vineet Chaudhary (vineetc) 2013-01-24 11:29:30 PST
(In reply to comment #10)
> (From update of attachment 184384 [details])
> Rejecting attachment 184384 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 184384, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue
> 
> Last 500 characters of output:
> LOutputElementCustom.cpp'
> patching file Source/WebCore/html/HTMLOutputElement.idl
> patching file LayoutTests/ChangeLog
> Hunk #1 succeeded at 1 with fuzz 3.
> patching file LayoutTests/fast/dom/HTMLOutputElement/dom-settable-token-list-expected.txt
> patching file LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Kentaro Hara']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue
> 
> Full output: http://queues.webkit.org/results/16072854

Hi haraken,
I am able to apply patch locally but not sure why it is failing for bot.
Could you please point if I am missing something.
Comment 13 Vineet Chaudhary (vineetc) 2013-01-24 12:06:36 PST
Created attachment 184549 [details]
Patch
Comment 14 Kentaro Hara 2013-01-24 15:49:02 PST
Did you rebase your patch with the latest WebKit trunk?
Comment 15 Vineet Chaudhary (vineetc) 2013-01-24 16:00:38 PST
(In reply to comment #14)
> Did you rebase your patch with the latest WebKit trunk?

Yes I did that too also tried with svn-create-patch and webkit-patch both.
Comment 16 Kentaro Hara 2013-01-24 16:02:35 PST
Comment on attachment 184549 [details]
Patch

Hmm, then I wonder why bots are purple (It means the bots failed in applying your patch). Let's try to commit anyway.
Comment 17 WebKit Review Bot 2013-01-24 16:13:40 PST
Comment on attachment 184549 [details]
Patch

Rejecting attachment 184549 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 184549, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
LOutputElementCustom.cpp'
patching file Source/WebCore/html/HTMLOutputElement.idl
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/HTMLOutputElement/dom-settable-token-list-expected.txt
patching file LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Kentaro Hara']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16082923
Comment 18 Kent Tamura 2013-01-24 18:06:59 PST
(In reply to comment #13)
> Created an attachment (id=184549) [details]
> Patch

Looks like end-of-line code of some files are CR-LF.
Comment 19 Vineet Chaudhary (vineetc) 2013-01-25 14:29:12 PST
Created attachment 184810 [details]
Patch
Comment 20 Vineet Chaudhary (vineetc) 2013-01-25 14:38:22 PST
(In reply to comment #19)
> Created an attachment (id=184810) [details]
> Patch

(In reply to comment #18)
> (In reply to comment #13)
> > Created an attachment (id=184549) [details] [details]
> > Patch
> 
> Looks like end-of-line code of some files are CR-LF.

Thanks tkent it helped!! Editing vcxproj file in visual studio/windows helps.
Comment 21 Build Bot 2013-01-25 15:32:56 PST
Comment on attachment 184810 [details]
Patch

Attachment 184810 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16123515
Comment 22 Build Bot 2013-01-25 16:05:21 PST
Comment on attachment 184810 [details]
Patch

Attachment 184810 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16117579

New failing tests:
fast/dom/HTMLOutputElement/dom-settable-token-list.html
Comment 23 WebKit Review Bot 2013-01-25 16:14:03 PST
Comment on attachment 184810 [details]
Patch

Attachment 184810 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16119545

New failing tests:
fast/dom/HTMLOutputElement/dom-settable-token-list.html
Comment 24 Build Bot 2013-01-25 23:11:21 PST
Comment on attachment 184810 [details]
Patch

Attachment 184810 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16117711

New failing tests:
fast/dom/HTMLOutputElement/dom-settable-token-list.html
Comment 25 Vineet Chaudhary (vineetc) 2013-01-28 15:27:04 PST
Created attachment 185082 [details]
another_try

It seems patch failing to apply for WIN only.
I checked other patches includes *.vcxproj files and those also fails to apply patch for windows.
Can/should we land this patch manually? //Apologize for previous wrong patch.
Comment 26 Kentaro Hara 2013-01-28 16:09:14 PST
Comment on attachment 185082 [details]
another_try

Maybe it is a problem of CR+LF? Anyway the patch looks OK, so please feel free to try to land it manually, keeping watching the tree.
Comment 27 WebKit Review Bot 2013-01-28 23:33:15 PST
Comment on attachment 185082 [details]
another_try

Clearing flags on attachment: 185082

Committed r141063: <http://trac.webkit.org/changeset/141063>
Comment 28 WebKit Review Bot 2013-01-28 23:33:22 PST
All reviewed patches have been landed.  Closing bug.