Bug 88497 - Move form-related functions of Document to FormController
Summary: Move form-related functions of Document to FormController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-06 21:39 PDT by Kent Tamura
Modified: 2012-06-08 02:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (40.18 KB, patch)
2012-06-06 21:53 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (40.17 KB, patch)
2012-06-08 00:03 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-06-06 21:39:37 PDT
Move form-related functions of Document to FormController
Comment 1 Kent Tamura 2012-06-06 21:53:18 PDT
Created attachment 146195 [details]
Patch
Comment 2 Kentaro Hara 2012-06-07 00:16:00 PDT
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?
Comment 3 Kentaro Hara 2012-06-07 00:19:51 PDT
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?
Comment 4 Kent Tamura 2012-06-07 00:31:26 PDT
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.
Comment 5 Kentaro Hara 2012-06-07 00:35:01 PDT
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.
Comment 6 Hajime Morrita 2012-06-07 17:24:18 PDT
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
Comment 7 Kent Tamura 2012-06-07 17:30:32 PDT
(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.
Comment 8 Kent Tamura 2012-06-07 17:33:55 PDT
(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.
Comment 9 Kent Tamura 2012-06-08 00:03:58 PDT
Created attachment 146495 [details]
Patch 2

- Move FormController.* to html/
- Update copyright headers
- make formElementsState const
Comment 10 WebKit Review Bot 2012-06-08 02:54:13 PDT
Comment on attachment 146495 [details]
Patch 2

Clearing flags on attachment: 146495

Committed r119816: <http://trac.webkit.org/changeset/119816>
Comment 11 WebKit Review Bot 2012-06-08 02:54:27 PDT
All reviewed patches have been landed.  Closing bug.