Bug 29363

Created attachment 70950 [details] A trial patch V0

Hi, This patch is my first attempt to support HTML5 output element. SInce I'm not familiar with adding elements to WebKit, this patch would do something bad, or at least have problems. So It would be definitely helpful for me if you can take a look at the patch and pointed out the problems. This patch also doesn't contain CSS default property for the element. I'll add it later. Thanks,

Comment on attachment 70950 [details] A trial patch V0 View in context: https://bugs.webkit.org/attachment.cgi?id=70950&action=review Thank you for working on this. The patch is too large to review. I recommend to split this into - DOMSettableTokenList implementation and JSC binding, - V8 binding for DOMSettableTokenList, - "form" attribute support for form control elements, and - <output> element support. > WebCore/WebCore.xcodeproj/project.pbxproj:14519 > + 4AD7D6631267D56B0020D6E8 /* HTMLOutputElement.cpp */, You should run WebKitTools/Scripts/sort-Xcode-project-file when you add files to an Xcode project.

Hi Kent-san, > The patch is too large to review. I recommend to split this into > - DOMSettableTokenList implementation and JSC binding, > - V8 binding for DOMSettableTokenList, > - "form" attribute support for form control elements, and > - <output> element support. Thank you for your advice. I'll split this following your advice. > You should run WebKitTools/Scripts/sort-Xcode-project-file when you add files to an Xcode project. I didn't know that. Thank you letting me know that! Regards,

Created attachment 72521 [details] Patch V1

(In reply to comment #5) Hi, I've revised the patch. Although the form attribute support, which blocks this issue, is not addressed yet, I think that adding output element before adding form attribute would be more convenient for writing tests (since there is no elements which have the form attribute for now). I'm planning to add form attribute support after this patch is successfully landed. Thanks,

Comment on attachment 72521 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=72521&action=review > LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:40 > +element.htmlFor.add('x'); shouldBeEqualToString('element.htmlFor.toString()', 'x'); Insert a line-break after ";" > LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:78 > +debug('Ensure that we can handle empty form attribute correctly'); Would you add debug('') before this line, or add "-" at the beginning of the message like other tests for readability of the test result please? > LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:80 > +list = element.htmlFor; Should prepend "var " > LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:91 > +element.htmlFor.add('x') What's the expected result of these remove() and add()? > LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-htmlfor.js:1 > +description('Tests for the htmlFor attribute of the output element.'); Many test cases in this file look equivalent to tests in dom-settable-token-list.js. Are they needed? > LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:7 > +output = document.createElement('output'); Redundant spaces before "=" > WebCore/html/HTMLOutputElement.cpp:83 > +void HTMLOutputElement::childrenChanged(bool createdByParser, Node* /*beforeChange*/, Node* /*afterChange*/, int /*childCountDelta*/) You don't need to keep unused parameter names. > WebCore/html/HTMLOutputElement.cpp:133 > + setTextContent(value, ec); Better to have ASSERT(!m_isSetTextContentInProgress) > WebCore/html/HTMLOutputElement.idl:35 > + readonly attribute boolean willValidate; You need a test for willValidate behavior.

Also, we need to add a default style for <output> to WebCore/css/html.css. Probably display:inline or display:inilne-block is sufficient. Firefox seems to use display:inline.

Comment on attachment 72521 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=72521&action=review Hi Kent-san, Thank you for your review. I'll send revised patch following your comments. >> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:40 >> +element.htmlFor.add('x'); shouldBeEqualToString('element.htmlFor.toString()', 'x'); > > Insert a line-break after ";" done. >> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:78 >> +debug('Ensure that we can handle empty form attribute correctly'); > > Would you add debug('') before this line, or add "-" at the beginning of the message like other tests for readability of the test result please? I've added "-" at the beginning of all debug messages. >> LayoutTests/fast/dom/HTMLOutputElement/script-tests/dom-settable-token-list.js:91 >> +element.htmlFor.add('x') > > What's the expected result of these remove() and add()? I forgot to add expectations. Added expectations. Sorry for confusing. >> LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-htmlfor.js:1 >> +description('Tests for the htmlFor attribute of the output element.'); > > Many test cases in this file look equivalent to tests in dom-settable-token-list.js. Are they needed? Removed. >> LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:7 >> +output = document.createElement('output'); > > Redundant spaces before "=" Fixed. >> WebCore/html/HTMLOutputElement.cpp:83 >> +void HTMLOutputElement::childrenChanged(bool createdByParser, Node* /*beforeChange*/, Node* /*afterChange*/, int /*childCountDelta*/) > > You don't need to keep unused parameter names. Thank you for letting me know that. Removed unused parameter names. >> WebCore/html/HTMLOutputElement.idl:35 >> + readonly attribute boolean willValidate; > > You need a test for willValidate behavior. Thank you for the advice. I've added a test.

Created attachment 72626 [details] Patch V2

> > WebCore/html/HTMLOutputElement.cpp:133 > > + setTextContent(value, ec); > > Better to have ASSERT(!m_isSetTextContentInProgress) Done. > Also, we need to add a default style for <output> to WebCore/css/html.css. > Probably display:inline or display:inilne-block is sufficient. > Firefox seems to use display:inline. Thank you for notifying that. I forgot it. I've added display:inline for <output>. In addition, I also forgot to add an attribute for HTMLOutputElement to dom/page/Window.idl and added it in the revised patch. Thanks,

Comment on attachment 72626 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=72626&action=review > LayoutTests/fast/dom/HTMLOutputElement/htmloutputelement-validity-expected.txt:1 > +CONSOLE MESSAGE: line 25: true Something wrong. > WebCore/html/HTMLOutputElement.h:44 > + virtual bool isEnumeratable() const { return true; } Need a test for this. However, I couldn't find existing tests for from control enumeration. We might need a new test for all of form control types. These override functions (isEnumeratable, willValidate, reset, childrenChanged) should be moved to private:.

Comment on attachment 72626 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=72626&action=review > WebCore/ChangeLog:7 > + Support for &lt;output&gt; element No need to use entity references :-)

Should this be inside #if?

(In reply to comment #14) > Should this be inside #if? Does Apple have any special reasons to do so? If not, I don't think we need to enclose the code by #if ENABLE(). The implementation doesn't need platform-specific code, and will be completed by this patch except "form" attribute support.

Comment on attachment 72626 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=72626&action=review Kent-san, Thank you your quick review. I'll revise the patch but I have a question on tests for form control enumeration. If I understand the spec correctly, the spec <http://www.w3.org/TR/html5/forms.html#category-listed> says that following elements would be listed in elements property of form elements: <button>, <fieldset>, <input>, <keygen>, <object>, <output>, <select>, and <textarea>. But for the current implementation, <fieldset>, <keygen> and <object> are not contained in the elements property. Which behavior should I add in the tests? >> LayoutTests/fast/dom/HTMLOutputElement/htmloutputelement-validity-expected.txt:1 >> +CONSOLE MESSAGE: line 25: true > > Something wrong. I'm sorry I forgot delete this debug information. I'll remove this. >> WebCore/ChangeLog:7 >> + Support for &lt;output&gt; element > > No need to use entity references :-) Oh, thank you notifying this. I'll fix this. >> WebCore/html/HTMLOutputElement.h:44 >> + virtual bool isEnumeratable() const { return true; } > > Need a test for this. > However, I couldn't find existing tests for from control enumeration. We might need a new test for all of form control types. > > These override functions (isEnumeratable, willValidate, reset, childrenChanged) should be moved to private:. I've moved these override functions to private except for willValidate() since it is called by JSHTMLOutputElement class. I'm going to try to write tests for form control enumeration but I have some questions, as I mentioned above. It is great if you could give some advice on this.

(In reply to comment #16) > Thank you your quick review. I'll revise the patch but I have a question on tests for form control enumeration. If I understand the spec correctly, the spec <http://www.w3.org/TR/html5/forms.html#category-listed> says that following elements would be listed in elements property of form elements: <button>, <fieldset>, <input>, <keygen>, <object>, <output>, <select>, and <textarea>. But for the current implementation, <fieldset>, <keygen> and <object> are not contained in the elements property. Which behavior should I add in the tests? ok, we need to discuss what we should do. Let's discuss at new bug. You don't need to add an enumeration test in this patch.

(In reply to comment #17) > ok, we need to discuss what we should do. Let's discuss at new bug. You don't need to add an enumeration test in this patch. I've posted this as https://bugs.webkit.org/show_bug.cgi?id=48821. Thank you for your suggestion!

Created attachment 72638 [details] Patch V3

(In reply to comment #19) > Created an attachment (id=72638) [details] > Patch V3 Looks good. I'll set r+ on the next work day if Apple people has no comments on #if guard.

Comment on attachment 72638 [details] Patch V3 Rejecting patch 72638 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: ... fast/doctypes ............ fast/dom .................................................................................................................................................................................................................................. fast/dom/prototype-inheritance-2.html -> failed Exiting early after 1 failures. 6883 tests run. 269.32s total testing time 6882 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 2 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/5078009

Created attachment 72910 [details] Patch V4

Comment on attachment 72910 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=72910&action=review > LayoutTests/fast/dom/prototype-inheritance-2-expected.txt:-443 > - Why is the last line removed? > LayoutTests/fast/dom/prototype-inheritance-expected.txt:-837 > - ditto.

Comment on attachment 72910 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=72910&action=review > LayoutTests/ChangeLog:28 > + * fast/dom/Window/window-properties-expected.txt: Updated expectation. > + * fast/dom/Window/window-property-descriptors-expected.txt: Updated expectation. > + * fast/dom/prototype-inheritance-2-expected.txt: Updated expectation. > + * fast/dom/prototype-inheritance-expected.txt: Updated expectation. We need to update the followings too. LayoutTests/platform/chromium/fast/dom/prototype-inheritance-expected.txt LayoutTests/platform/gtk/fast/dom/prototype-inheritance-expected.txt LayoutTests/platform/qt/fast/dom/prototype-inheritance-expected.txt LayoutTests/platform/win/fast/dom/prototype-inheritance-2-expected.txt LayoutTests/platform/win/fast/dom/prototype-inheritance-expected.txt LayoutTests/platform/gtk/fast/dom/Window/window-properties-expected.txt LayoutTests/platform/gtk/fast/dom/Window/window-property-descriptors-expected.txt LayoutTests/platform/qt/fast/dom/Window/window-properties-expected.txt LayoutTests/platform/qt/fast/dom/Window/window-property-descriptors-expected.txt LayoutTests/platform/win/fast/dom/Window/window-property-descriptors-expected.txt

Comment on attachment 72910 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=72910&action=review >> LayoutTests/fast/dom/prototype-inheritance-2-expected.txt:-443 >> - > > Why is the last line removed? I'm sorry for confusing, but I've just run new-run-webkit-tests and copied from -actual.txt to -expected.txt. Is this a bug of the new-run-webkit-tests? Anyway, I'll re-send the patch. Sorry again the inconvenience.

(In reply to comment #25) > I'm sorry for confusing, but I've just run new-run-webkit-tests and copied from -actual.txt to -expected.txt. Is this a bug of the new-run-webkit-tests? Anyway, I'll re-send the patch. Sorry again the inconvenience. If you work on Mac, I recommend to use run-webkit-tests (not new-). % run-webkit-tests --debug/--release --reset fast/dom/prototype-inheritance.html updates the expectation. new-run-webkit-tests on Mac is not popular yet.

Created attachment 72914 [details] Patch V5

Comment on attachment 72914 [details] Patch V5 ok

(In reply to comment #26) > If you work on Mac, I recommend to use run-webkit-tests (not new-). > % run-webkit-tests --debug/--release --reset fast/dom/prototype-inheritance.html > updates the expectation. new-run-webkit-tests on Mac is not popular yet. I see. Thank you for your advice. I'll do in this way next time:-) I've posted revised patch, which will modify platform-dependent expectation files. Thank you for letting me know that.

Attachment 72638 [details] did not build on win: Build output: http://queues.webkit.org/results/5186010

Comment on attachment 72914 [details] Patch V5 We need to update WebCore/WebCore.vcproj/WebCore.vcproj

Created attachment 72922 [details] Fixed windows build

(In reply to comment #31) > We need to update WebCore/WebCore.vcproj/WebCore.vcproj Sorry for bothering you.. I've updated it.

Comment on attachment 72914 [details] Patch V5 Rejecting patch 72914 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: ............... fast/inspector-support ...... fast/invalid ................................... fast/js ....................................................................................................................................................................... fast/js/global-constructors.html -> failed Exiting early after 1 failures. 8885 tests run. 186.23s total testing time 8884 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/5122019

Comment on attachment 72922 [details] Fixed windows build Rejecting patch 72922 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: ............... fast/inspector-support ...... fast/invalid ................................... fast/js ....................................................................................................................................................................... fast/js/global-constructors.html -> failed Exiting early after 1 failures. 8885 tests run. 163.49s total testing time 8884 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/5183009

Comment on attachment 72922 [details] Fixed windows build Rejecting patch 72922 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: ............... fast/inspector-support ...... fast/invalid ................................... fast/js ....................................................................................................................................................................... fast/js/global-constructors.html -> failed Exiting early after 1 failures. 8885 tests run. 187.80s total testing time 8884 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/5173011

Created attachment 72930 [details] Patch

Comment on attachment 72930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72930&action=review > LayoutTests/ChangeLog:29 > + * fast/js/global-constructors-expected.txt: Updated expectation. You need to update the followings too. LayoutTests/platform/gtk/fast/js/global-constructors-expected.txt LayoutTests/platform/qt/fast/js/global-constructors-expected.txt LayoutTests/platform/win/fast/js/global-constructors-expected.txt

Created attachment 72937 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 72937 [details]: http/tests/appcache/manifest-redirect.html Please file bugs against the tests. These tests were authored by ap@webkit.org. The commit-queue is continuing to process your patch.

Comment on attachment 72937 [details] Patch Clearing flags on attachment: 72937 Committed r71373: <http://trac.webkit.org/changeset/71373>

All reviewed patches have been landed. Closing bug.