Created attachment 142214 [details] proposed patch

Comment on attachment 142214 [details] proposed patch Attachment 142214 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12694970

Comment on attachment 142214 [details] proposed patch Attachment 142214 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12701826

Comment on attachment 142214 [details] proposed patch Attachment 142214 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12722139 New failing tests: editing/pasteboard/paste-noscript.html editing/pasteboard/paste-noscript-xhtml.xhtml

Created attachment 142256 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 142214 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=142214&action=review Marking r- due to build breakage. > Source/WebCore/html/HTMLFormCollection.h:38 > - static PassOwnPtr<HTMLFormCollection> create(HTMLFormElement*); > + static PassOwnPtr<HTMLFormCollection> create(Node*); Why does this take a Node, not an Element?

(In reply to comment #0) > Refactor HTMLFormCollection to be independent of HTMLFormElement. > This is needed for implementing HTMLFieldSetElement's elements attribute. Are you planing to update HTMLFieldSetElement so that it has a collection of element pointers?

(In reply to comment #7) > (In reply to comment #0) > > Refactor HTMLFormCollection to be independent of HTMLFormElement. > > This is needed for implementing HTMLFieldSetElement's elements attribute. > > Are you planing to update HTMLFieldSetElement so that it has a collection of element pointers? Yes and maybe the collection can be refreshed if document version changes.

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #0) > > > Refactor HTMLFormCollection to be independent of HTMLFormElement. > > > This is needed for implementing HTMLFieldSetElement's elements attribute. > > > > Are you planing to update HTMLFieldSetElement so that it has a collection of element pointers? > > Yes and maybe the collection can be refreshed if document version changes. I see. I wondered which one was better; A) HTMLFieldSetElement has a collection of form controls, and use HTMLFormCollection. B) HTMLFieldSetElement has no collection of form controls, and use HTMLCollection. Probably A is simpler because of RadioNodeList.

(In reply to comment #9) > > I wondered which one was better; > A) HTMLFieldSetElement has a collection of form controls, and use HTMLFormCollection. > B) HTMLFieldSetElement has no collection of form controls, and use HTMLCollection. > > Probably A is simpler because of RadioNodeList. Yes because of RadioNodeList, option A is better. Please do let me know if you have further suggestions.

Created attachment 142472 [details] updated patch Fixed build and the failing tests have to addressed. Hopefully it will pass all builds.

(In reply to comment #6) Thanks for review. > Marking r- due to build breakage. Fixed. > Why does this take a Node, not an Element? Updated in latest patch. (In reply to comment #11) > Created an attachment (id=142472) [details] > updated patch > > Fixed build and the failing tests have to addressed. Hopefully it will pass all builds. Typo, please read as "Fixed build and the failing tests have *been* addressed."

Comment on attachment 142472 [details] updated patch Attachment 142472 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12723342

Comment on attachment 142472 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=142472&action=review > Source/WebCore/dom/Node.h:588 > + const Vector<FormAssociatedElement*>* formControlElements(); > + const Vector<HTMLImageElement*>* formImageElements(); > + unsigned numberOfFormControlElements(); > + I don't think Node should have them. If you'd like to have common ways to get them in HTMLCollection, you should define these function like: static unsigned numberOfFormControlElements(HTMLElement* base); > Source/WebCore/html/HTMLFormElement.h:162 > - Vector<FormAssociatedElement*> m_associatedElements; > - Vector<HTMLImageElement*> m_imageElements; > + mutable OwnPtr<Vector<FormAssociatedElement*> > m_associatedElements; > + mutable OwnPtr<Vector<HTMLImageElement*> > m_imageElements; Why do you need to change them to OwnPtr<>? In general, ChangeLog entry should explain reasons of the change.

(In reply to comment #14) > (From update of attachment 142472 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142472&action=review > Thanks for reviewing this patch. > I don't think Node should have them. If you'd like to have common ways to get them in HTMLCollection, you should define these function like: > static unsigned numberOfFormControlElements(HTMLElement* base); Yes, having these functions like suggested in HTMLFormCollection would be better. > > > Source/WebCore/html/HTMLFormElement.h:162 > > - Vector<FormAssociatedElement*> m_associatedElements; > > - Vector<HTMLImageElement*> m_imageElements; > > + mutable OwnPtr<Vector<FormAssociatedElement*> > m_associatedElements; > > + mutable OwnPtr<Vector<HTMLImageElement*> > m_imageElements; > > Why do you need to change them to OwnPtr<>? Only reason being, able to 'return 0' in case of failed case of toFormElement()conversion. I think this would not be needed if we make the above change. We can assert if not an form element in conversion. > In general, ChangeLog entry should explain reasons of the change. Will surely add explanation.

Created attachment 142687 [details] updated patch Updated as per reviews comments.

Comment on attachment 142687 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=142687&action=review > Source/WebCore/html/HTMLFormCollection.cpp:53 > +static const Vector<FormAssociatedElement*>& formControlElements(HTMLElement* base) You moved these function to HTMLFormCollection. So we can make them member functions of HTMLFormCollection and the 'base' argument is not needed. > Source/WebCore/html/HTMLFormCollection.cpp:55 > + ASSERT(base && base->hasTagName(formTag)); nit: You should separate this ASSERT into two. ASSERT(base); ASSERT(base->hasTagName(formTag)); in order to know which condition fails. > Source/WebCore/html/HTMLFormElement.h:149 > > - friend class HTMLFormCollection; > - > typedef HashMap<RefPtr<AtomicStringImpl>, RefPtr<HTMLFormControlElement> > AliasMap; great!

Created attachment 142974 [details] updated patch Updated as per reviews comments.

Comment on attachment 142974 [details] updated patch Looks nice.

Comment on attachment 142974 [details] updated patch Clearing flags on attachment: 142974 Committed r117754: <http://trac.webkit.org/changeset/117754>

All reviewed patches have been landed. Closing bug.

(In reply to comment #19) > (From update of attachment 142974 [details]) > Looks nice. Thanks!!