classList is defined in Element but setting the class attribute needs to trigger classAttributeChanged which is only available on StyledElement
Created attachment 158898 [details] Patch
Comment on attachment 158898 [details] Patch Attachment 158898 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13516539 New failing tests: css3/selectors3/xml/css3-modsel-139.xml css3/selectors3/xhtml/css3-modsel-113.xml fast/dom/getElementsByClassName/011.xml css3/selectors3/xhtml/css3-modsel-113b.xml fast/parser/xhtml-alternate-entities.xml css3/selectors3/xhtml/css3-modsel-139.xml css3/selectors3/xml/css3-modsel-139b.xml css3/selectors3/xml/css3-modsel-113b.xml css3/selectors3/xhtml/css3-modsel-139b.xml css3/selectors3/xml/css3-modsel-113.xml
Created attachment 158920 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 158898 [details] Patch I'll look into the failures tomorrow
Created attachment 159948 [details] Patch
Comment on attachment 159948 [details] Patch Attachment 159948 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13567305 New failing tests: fast/parser/xhtml-alternate-entities.xml
Created attachment 159956 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 159948 [details] Patch Attachment 159948 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13569299 New failing tests: fast/parser/xhtml-alternate-entities.xml
Created attachment 159965 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 159983 [details] Patch
Comment on attachment 159983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159983&action=review > Source/WebCore/dom/ClassNodeList.cpp:57 > + // TODO: DOM4 allows getElementsByClassName to return non StyledElement. s/TODO/FIXME
Comment on attachment 159983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159983&action=review > Source/WebCore/ChangeLog:9 > + Class attribute logic needs to be moved to Element (from StyledElement) > + https://bugs.webkit.org/show_bug.cgi?id=93665 > + > + Reviewed by NOBODY (OOPS!). > + > + Before this change classAttributeChanged was only called for StyledElement. With this refactoring > + it gets called for all Elements when the class attribute changes. Does this fix some bug or is it just a refactoring? Please clarify. Ideally the bug title is changed to reflect whatever behavior being fixed if there is any behavior change. > Source/WebCore/dom/ClassNodeList.cpp:57 > + // TODO: DOM4 allows getElementsByClassName to return non StyledElement. We use FIXME instead of TODO.
Comment on attachment 159983 [details] Patch I don't think we can land the patch as is since it's unclear whether this patch is changing the behavior or just refactoring the code.
(In reply to comment #13) > (From update of attachment 159983 [details]) > I don't think we can land the patch as is since it's unclear whether this patch is changing the behavior or just refactoring the code. I thought it was clear from the test that it makes classList work for svg/xml elements. You just want that clearly spelled out in the ChangeLog?
What I mean to say, it it makes setting the class attribute and then reading out the classList work for svg/xml elements.
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 159983 [details] [details]) > > I don't think we can land the patch as is since it's unclear whether this patch is changing the behavior or just refactoring the code. > > I thought it was clear from the test that it makes classList work for svg/xml elements. You just want that clearly spelled out in the ChangeLog? Well, it wasn't clear from the bug title & the change log entry whether this was a simple refactoring with an addition of test or this was fixing some bug for which we're adding a test.
Created attachment 159990 [details] Patch
Comment on attachment 159990 [details] Patch You should probably use Ojan's r+ but r=me anyhow.
Created attachment 159991 [details] Patch for landing
(In reply to comment #18) > (From update of attachment 159990 [details]) > You should probably use Ojan's r+ but r=me anyhow. You are confusing me... I just did --reviewer "Ojan Vafai". I guess it is not a big deal but I can redo it if you really care
Comment on attachment 159991 [details] Patch for landing Clearing flags on attachment: 159991 Committed r126349: <http://trac.webkit.org/changeset/126349>
All reviewed patches have been landed. Closing bug.
(In reply to comment #20) > (In reply to comment #18) > > (From update of attachment 159990 [details] [details]) > > You should probably use Ojan's r+ but r=me anyhow. > > You are confusing me... I just did --reviewer "Ojan Vafai". I guess it is not a big deal but I can redo it if you really care Yeah, that's what I meant to ask you :) I just set r+ on the patch for the bugzilla's sake.