Lazily generate HistoryItem's serialized form state (issue 239993011)

[jap...@chromium.org]

Reviewers: abarth,


https://codereview.chromium.org/239993011/diff/1/Source/core/html/HTMLFormControlElementWithState.cpp
File Source/core/html/HTMLFormControlElementWithState.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/HTMLFormControlElementWithState.cpp#newcode80
Source/core/html/HTMLFormControlElementWithState.cpp:80: return
inDocument() && shouldAutocomplete();
Remove the active check because we might be generating the saved state
as the document going away. That check is 5+ years old and I can't find
evidence it's necessary.

https://codereview.chromium.org/239993011/diff/1/Source/core/loader/HistoryItem.h
File Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/loader/HistoryItem.h#newcode115
Source/core/loader/HistoryItem.h:115: Vector<String>
m_documentStateVector;
We still need this because there would be a DocumentState object if
we're deserializing PageState sent from the browser process.

https://codereview.chromium.org/239993011/diff/1/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (left):

https://codereview.chromium.org/239993011/diff/1/Source/web/WebFrameImpl.cpp#oldcode1011
Source/web/WebFrameImpl.cpp:1011: frame()->loader().saveDocumentState();
This whole function is already dead as a result of moving history state
to content/renderer.

This call to saveDocumentState() was kind of important for not randomly
dropping form state, hence my timing in writing this patch :)

Description:
Lazily generate HistoryItem's serialized form state

Give HistoryItem access to the object that stores the list of form elements
with state, then generated the serialized state when state is requested.

BUG=

Please review this at https://codereview.chromium.org/239993011/

SVN Base: svn://svn.chromium.org/blink/trunk

Affected files (+91, -67 lines):
M Source/core/dom/Document.h
M Source/core/dom/Document.cpp
M Source/core/html/HTMLFormControlElementWithState.cpp
M Source/core/html/forms/FormController.h
M Source/core/html/forms/FormController.cpp
M Source/core/loader/FrameLoader.h
M Source/core/loader/FrameLoader.cpp
M Source/core/loader/HistoryItem.h
M Source/core/loader/HistoryItem.cpp
M Source/core/testing/Internals.cpp
M Source/web/WebFrameImpl.cpp
M Source/web/WebHistoryItem.cpp

[aba...@chromium.org]

lgtm




https://codereview.chromium.org/239993011/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/dom/Document.cpp#newcode1470
Source/core/dom/Document.cpp:1470:
m_frame->loader().currentItem()->setDocumentState(m_formController->formElementsState());
It seems slightly odd that getting the formController would store data
on the current history item as a side-effect... I guess the idea is
that if you don't have a formController then the DocumentState is
zero...

https://codereview.chromium.org/239993011/diff/1/Source/core/html/HTMLFormControlElementWithState.cpp
File Source/core/html/HTMLFormControlElementWithState.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/HTMLFormControlElementWithState.cpp#newcode80
Source/core/html/HTMLFormControlElementWithState.cpp:80: return
inDocument() && shouldAutocomplete();

On 2014/04/17 21:16:25, Nate Chapin wrote:
> Remove the active check because we might be generating the saved state
as the
> document going away. That check is 5+ years old and I can't find
evidence it's
> necessary.

Ok.

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.cpp
File Source/core/html/forms/FormController.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.cpp#newcode394
Source/core/html/forms/FormController.cpp:394:
RELEASE_ASSERT(m_formControls.contains(control));
Why RELEASE_ASSERT and not just ASSERT?

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.h
File Source/core/html/forms/FormController.h (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.h#newcode88
Source/core/html/forms/FormController.h:88: };
Should this class have its own header?

https://codereview.chromium.org/239993011/

[jap...@chromium.org]

https://codereview.chromium.org/239993011/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/dom/Document.cpp#newcode1470
Source/core/dom/Document.cpp:1470:
m_frame->loader().currentItem()->setDocumentState(m_formController->formElementsState());

On 2014/04/17 23:41:24, abarth wrote:
> It seems slightly odd that getting the formController would store data
on the
> current history item as a side-effect... I guess the idea is that if
you don't
> have a formController then the DocumentState is zero...

Right. I didn't see a cleaner way to hook this up. I guess maybe
FormController is cheap enough to construct that we could just
initialize it greedily?

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.cpp
File Source/core/html/forms/FormController.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.cpp#newcode394
Source/core/html/forms/FormController.cpp:394:
RELEASE_ASSERT(m_formControls.contains(control));

On 2014/04/17 23:41:24, abarth wrote:
> Why RELEASE_ASSERT and not just ASSERT?

I copy/pasted it without understanding the history :) Will investigate
before landing.

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.h
File Source/core/html/forms/FormController.h (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.h#newcode88
Source/core/html/forms/FormController.h:88: };

On 2014/04/17 23:41:24, abarth wrote:
> Should this class have its own header?

I was planning on that initially, but it was surprisingly hard to get
the dependencies right such that it compiled, and I gave up.

https://codereview.chromium.org/239993011/

[aba...@chromium.org]

On 2014/04/17 23:48:19, Nate Chapin wrote:
> https://codereview.chromium.org/239993011/diff/1/Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):


https://codereview.chromium.org/239993011/diff/1/Source/core/dom/Document.cpp#newcode1470
> Source/core/dom/Document.cpp:1470:

m_frame->loader().currentItem()->setDocumentState(m_formController->formElementsState());
> On 2014/04/17 23:41:24, abarth wrote:
> > It seems slightly odd that getting the formController would store data
> on
the
> > current history item as a side-effect... I guess the idea is that if
> you
> don't
> > have a formController then the DocumentState is zero...

> Right. I didn't see a cleaner way to hook this up. I guess maybe
FormController
> is cheap enough to construct that we could just initialize it greedily?

Your call.

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormController.h#newcode88
> Source/core/html/forms/FormController.h:88: };
> On 2014/04/17 23:41:24, abarth wrote:
> > Should this class have its own header?

> I was planning on that initially, but it was surprisingly hard to get the
> dependencies right such that it compiled, and I gave up.

Ok. :)

https://codereview.chromium.org/239993011/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jap...@chromium.org/239993011/20001


https://codereview.chromium.org/239993011/

[commi...@chromium.org]

Try jobs failed on following builders:
tryserver.blink on win_blink_rel

https://codereview.chromium.org/239993011/

[commi...@chromium.org]

[commi...@chromium.org]

Change committed as 171961

https://codereview.chromium.org/239993011/