Move form-related functions of Document to FormController
Created attachment 146195 [details] Patch
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 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 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 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 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
(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.
(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.
Created attachment 146495 [details] Patch 2 - Move FormController.* to html/ - Update copyright headers - make formElementsState const
Comment on attachment 146495 [details] Patch 2 Clearing flags on attachment: 146495 Committed r119816: <http://trac.webkit.org/changeset/119816>
All reviewed patches have been landed. Closing bug.