Bug 26016 - HTMLOptionElement::ownerSelectElement() needs to consider keygen
: HTMLOptionElement::ownerSelectElement() needs to consider keygen
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-05-25 19:58 PST by
Modified: 2010-04-29 02:33 PST (History)


Attachments
fix bug (1.22 KB, patch)
2009-05-25 20:21 PST, Grace Kloba
darin: review-
Review Patch | Details | Formatted Diff | Diff
layout test file (687 bytes, text/html)
2009-05-26 17:20 PST, Grace Kloba
no flags Details
new patch file with layout test (3.68 KB, patch)
2009-05-26 18:05 PST, Grace Kloba
no flags Review Patch | Details | Formatted Diff | Diff
remove a tab from the previous layout file (3.68 KB, patch)
2009-05-26 18:23 PST, Grace Kloba
no flags Review Patch | Details | Formatted Diff | Diff
remove another tab from the Changelog (3.69 KB, patch)
2009-05-26 20:06 PST, Grace Kloba
abarth: review-
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Proposed patch: Fix the test case (4.10 KB, patch)
2010-04-15 17:21 PST, Jan Erik Hanssen
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch: Fix the test case and the style error of the last patch (4.11 KB, patch)
2010-04-15 18:04 PST, Jan Erik Hanssen
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-05-25 19:58:37 PST
HTMLKeygenElement is derived from HTMLKeygenElement. But HTMLOptionElement::ownerSelectElement() only checks the selectTag.
------- Comment #1 From 2009-05-25 20:01:03 PST -------
Sorry I meant HTMLKeygenElement is derived from HTMLSelectElement.
------- Comment #2 From 2009-05-25 20:21:22 PST -------
Created an attachment (id=30663) [details]
fix bug

Not sure how to write layout test as Safari uses SelectElement::setSelectedIndex instead of HTMLOptionElement::setSelected for the menu list.
------- Comment #3 From 2009-05-26 07:31:52 PST -------
(In reply to comment #2)
> Not sure how to write layout test as Safari uses
> SelectElement::setSelectedIndex instead of HTMLOptionElement::setSelected for
> the menu list.

Should be easy. HTMLOptionElement::setSelected can be called directly from JavaScript by setting the value of the selected attribute on the option element.
------- Comment #4 From 2009-05-26 07:36:00 PST -------
(From update of attachment 30663 [details])
Fix looks fine. Please add a regression test.

Since ownerSelectElement is used in accessKeyAction, index, and setSelected there are multiple ways you could test this simply in a regression test.
------- Comment #5 From 2009-05-26 17:20:04 PST -------
Created an attachment (id=30688) [details]
layout test file

Here is the layout test file. Which directory do you suggest me to put it?
------- Comment #6 From 2009-05-26 17:21:30 PST -------
(In reply to comment #5)
> Which directory do you suggest me to put it?

Thanks for asking.

LayoutTests/fast/forms is a good choice. Another possibility would be LayoutTests/fast/dom/HTMLKeygenElement.
------- Comment #7 From 2009-05-26 18:05:34 PST -------
Created an attachment (id=30689) [details]
new patch file with layout test
------- Comment #8 From 2009-05-26 18:23:05 PST -------
Created an attachment (id=30690) [details]
remove a tab from the previous layout file
------- Comment #9 From 2009-05-26 20:06:13 PST -------
Created an attachment (id=30693) [details]
remove another tab from the Changelog
------- Comment #10 From 2009-05-28 17:19:03 PST -------
I'd suggest adding "bool isSelectElement(Element*);" to SelectElement.h, and use "isSelectElement" in that function instead of comparing against select/keygen tags. There already is isOptionElement/isOptionGroupElement, and isSelectElement could serve your needs easily. What do you think Darin?
------- Comment #11 From 2009-06-02 01:09:02 PST -------
Will land.
------- Comment #12 From 2009-06-02 01:38:12 PST -------
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/dom/HTMLKeygenElement
Adding         LayoutTests/fast/dom/HTMLKeygenElement/keygen-option-select-expected.txt
Adding         LayoutTests/fast/dom/HTMLKeygenElement/keygen-option-select.html
Sending        WebCore/ChangeLog
Sending        WebCore/html/HTMLOptionElement.cpp
Transmitting file data .....
Committed revision 44354.
------- Comment #13 From 2009-06-02 02:13:42 PST -------
(From update of attachment 30693 [details])
The enclosed patch does not pass.
------- Comment #14 From 2009-06-02 02:13:56 PST -------
Reverting...
------- Comment #15 From 2009-06-15 10:52:14 PST -------
What part doesn't the patch pass?
------- Comment #16 From 2010-03-31 10:58:28 PST -------
(From update of attachment 30693 [details])
Re-nominate for review to run through the EWS.
------- Comment #17 From 2010-03-31 10:59:45 PST -------
BTW, darin@apple.com marked the patch review+ on 2009-06-01 at 00:38:57 PST.  We seemed to have trouble landing it, but we have much better technology now.
------- Comment #18 From 2010-04-01 00:13:12 PST -------
(From update of attachment 30693 [details])
Still looks good.  Forwarding Darin Adler's r+.
------- Comment #19 From 2010-04-01 01:41:40 PST -------
(From update of attachment 30693 [details])
Rejecting patch 30693 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12596 test cases.
fast/dom/HTMLKeygenElement/keygen-option-select.html -> failed

Exiting early after 1 failures. 5903 tests run.
90.95s total testing time

5902 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1569137
------- Comment #20 From 2010-04-01 09:16:01 PST -------
(From update of attachment 30693 [details])
Looks like the problem was that the test doesn't pass.
------- Comment #21 From 2010-04-15 17:21:10 PST -------
Created an attachment (id=53493) [details]
Proposed patch: Fix the test case
------- Comment #22 From 2010-04-15 17:24:37 PST -------
Attachment 53493 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #23 From 2010-04-15 18:04:09 PST -------
Created an attachment (id=53496) [details]
Proposed patch: Fix the test case and the style error of the last patch

Fixed the style error of the last patch
------- Comment #24 From 2010-04-28 04:33:04 PST -------
Ping? The patch had an r+ originally, the current patch just fixes a style error in the test file.

Thanks!
------- Comment #25 From 2010-04-28 15:06:51 PST -------
(From update of attachment 53496 [details])
Forwarding r+.  Thanks!
------- Comment #26 From 2010-04-29 02:33:51 PST -------
(From update of attachment 53496 [details])
Clearing flags on attachment: 53496

Committed r58490: <http://trac.webkit.org/changeset/58490>
------- Comment #27 From 2010-04-29 02:33:58 PST -------
All reviewed patches have been landed.  Closing bug.