Lazily generate HistoryItem's serialized form state

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=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171961

[Nate Chapin, 2014-04-17 21:16:25]

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

https://codereview.chromium.org/239993011/diff/1/Source/core/html/HTMLFormCon...
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/HistoryIt...
File Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/loader/HistoryIt...
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#...
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 :)

[abarth-chromium, 2014-04-17 23:41:24]

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...
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/HTMLFormCon...
File Source/core/html/HTMLFormControlElementWithState.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/HTMLFormCon...
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/FormC...
File Source/core/html/forms/FormController.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormC...
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/FormC...
File Source/core/html/forms/FormController.h (right):

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

[Nate Chapin, 2014-04-17 23:48:19]

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...
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/FormC...
File Source/core/html/forms/FormController.cpp (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormC...
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/FormC...
File Source/core/html/forms/FormController.h (right):

https://codereview.chromium.org/239993011/diff/1/Source/core/html/forms/FormC...
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.

[abarth-chromium, 2014-04-17 23:59:34]

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...
> 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/FormC...
> 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.  :)

[Nate Chapin, 2014-04-18 16:50:37]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-04-18 16:50:48]

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

[commit-bot: I haz the power, 2014-04-18 17:29:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 17:29:14]

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

[Nate Chapin, 2014-04-18 17:37:11]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-04-18 17:37:23]

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

[commit-bot: I haz the power, 2014-04-18 18:23:17]

Change committed as 171961