Bug 88497 – Move form-related functions of Document to FormController

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Move form-related functions of Document to FormController

[Kent Tamura]

Created attachment 146195 [details]
Patch

[Kentaro Hara]

Comment on attachment 146195 [details]
Patch

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

Lightening Document looks really good!

> Source/WebCore/dom/Document.cpp:1647
> +Vector<String> Document::formElementsState()
> +{
> +    if (!m_formController)
> +        return Vector<String>();
> +    return formController()->formElementsState();
> +}
> +
> +void Document::setStateForNewFormElements(const Vector<String>& stateVector)
> +{
> +    if (!stateVector.size() && !m_formController)
> +        return;
> +    formController()->setStateForNewFormElements(stateVector);
> +}

It seems these methods are used by loader/HistoryController.cpp only. How about removing these methods from Document, and just call document()->formController()->formElementsState() in loader/HistoryController.cpp?

[Kentaro Hara]

Comment on attachment 146195 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:1647
>> +}
> 
> It seems these methods are used by loader/HistoryController.cpp only. How about removing these methods from Document, and just call document()->formController()->formElementsState() in loader/HistoryController.cpp?

Maybe the reason why you put the methods on Document is that you do not want to create FormController if formElementsState() is called before setStateForNewFormElements()? How often would it happen?

[Kent Tamura]

Comment on attachment 146195 [details]
Patch

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

>>> Source/WebCore/dom/Document.cpp:1647
>>> +}
>> 
>> It seems these methods are used by loader/HistoryController.cpp only. How about removing these methods from Document, and just call document()->formController()->formElementsState() in loader/HistoryController.cpp?
> 
> Maybe the reason why you put the methods on Document is that you do not want to create FormController if formElementsState() is called before setStateForNewFormElements()? How often would it happen?

Yes, the reason is that I don't want to create FormController as possible. FormController is not needed if a document contains no form controls.

formElementState() is called whenever a document is unloaded in order to save form state into HistoryItem.  So, if we call document->formController()->formElementsState() directly in HistoryController, A FormController would be created every time.  It would be wasteful if the document had no form elements and formController() was not created.

setStateForNewFormElements() is called whenever a document is loaded to restore form state from HistoryItem regardless of existence of form elements in the document.  The document might have no form elements and a FormController might be never used.  So I'd like to avoid instantiation of FormController.

[Kentaro Hara]

Comment on attachment 146195 [details]
Patch

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

The change looks good to me.

morrita-san: please r+ it if you think OK.

>>>> Source/WebCore/dom/Document.cpp:1647
>>>> +}
>>> 
>>> It seems these methods are used by loader/HistoryController.cpp only. How about removing these methods from Document, and just call document()->formController()->formElementsState() in loader/HistoryController.cpp?
>> 
>> Maybe the reason why you put the methods on Document is that you do not want to create FormController if formElementsState() is called before setStateForNewFormElements()? How often would it happen?
> 
> Yes, the reason is that I don't want to create FormController as possible. FormController is not needed if a document contains no form controls.
> 
> formElementState() is called whenever a document is unloaded in order to save form state into HistoryItem.  So, if we call document->formController()->formElementsState() directly in HistoryController, A FormController would be created every time.  It would be wasteful if the document had no form elements and formController() was not created.
> 
> setStateForNewFormElements() is called whenever a document is loaded to restore form state from HistoryItem regardless of existence of form elements in the document.  The document might have no form elements and a FormController might be never used.  So I'd like to avoid instantiation of FormController.

Makes sense. Thanks for the clarification.

[Hajime Morrita]

Comment on attachment 146195 [details]
Patch

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

FormController could be under html/ considering that all its clients are under html.
Then it could be a member of HTMLDocument instead of Document.

> Source/WebCore/dom/Document.h:495
> +    Vector<String> formElementsState();

Why not const?

> Source/WebCore/dom/FormController.cpp:3
> + * Copyright (C) 2010, 2011 Google Inc. All rights reserved.

It's 2012

[Kent Tamura]

(In reply to comment #6)
> (From update of attachment 146195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146195&action=review
> 
> FormController could be under html/ considering that all its clients are under html.
> Then it could be a member of HTMLDocument instead of Document.

I can move FormController.{cpp,h} to html/.  But Document needs to have it because SVGDocument can have form controls in <foreignContent>.

> > Source/WebCore/dom/Document.h:495
> > +    Vector<String> formElementsState();
> 
> Why not const?

It updates Document::m_formController.

[Kent Tamura]

(In reply to comment #7)
> > > +    Vector<String> formElementsState();
> > 
> > Why not const?
> 
> It updates Document::m_formController.

Ah, I can avoid to use formController().  So it can be const.

[Kent Tamura]

Created attachment 146495 [details]
Patch 2

- Move FormController.* to html/
- Update copyright headers
- make formElementsState const

[WebKit Review Bot]

Comment on attachment 146495 [details]
Patch 2

Clearing flags on attachment: 146495

Committed r119816: <http://trac.webkit.org/changeset/119816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.