Closed Bug 737851 Opened 8 years ago Closed 6 years ago

Generate form state key at page load instead of page unload

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mounir, Assigned: almasry.mina)

References

Details

Attachments

(2 files, 8 obsolete files)

Given that elements created after page load will unlikely exist at page reload, we could probably just generate the form state key at page load so we would make sure to be able to find the same elements on reload in most cases. For the edge cases, we could also generate a key at page unload if the element doesn't have a key yet.

Boris, that's your idea originally, do you confirm it's worth trying it?
I'm adding a few bugs in blocking list. There might be others though.
Blocks: 311507, 673646
> Boris, that's your idea originally, do you confirm it's worth trying it?

Well, _I_ clearly think it's a good idea... ;)

Bug 660549 has the relevant discussion.  Bug 660549 comment 5 talks about what WebKit does here.
Louis, I think the patch you wrote in bug 660549 would be more appropriate in this bug.
Does exactly what Boris suggested: generates the state-key at loading time, stores it, and reuses it when saving. Following Mounir's recommendations, it's now done in a new class instead of bloating nsHTMLGenericFormElement.

There's a few things I am not sure about: first, if we should silently generate the key at save-time if it failed at load time, as Boris suggested above. In that case, it doesn't need to appear in the interface, it's really a lazy variable (in this case I could remove explicit calls to GenerateStateKey from child classes; I didn't want to do too much magic).

Also, I'm wondering wether we should generate the state key even if mInhibitRestoration is set: is it permanent, or can its state change over time ? (and maybe mInhibitStateRestoration could even be moved to the new class and tested there).

Also, I must admit I am still a little puzzled by these NS_FORWARD_NSIDOMNODE and such. For example, nsHTMLInputElement forwards nsHTMLGenericFormElement, which was its parent. Now that there is nsHTMLGenericFormElementWithState between the two, should the forwarding be updated ? Any explanation of the consequences would be welcome :)
Attachment #608397 - Flags: review?(bzbarsky)
Comment on attachment 608397 [details] [diff] [review]
This is a new version of the patch submitted to bug 660549

Review of attachment 608397 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if we should generate the key when the document fires the load event to take into account elements being moved/added during the document load? Also, if we generate a key at unload when not present, can't we end up having two elements with the same key if they have moved during the document life? (I don't know the details of key generations so I don't know if we have a mechanism against this)
> I wonder if we should generate the key when the document fires the load event 

Basically, we should generate the key when we would be restoring state.  That's the point at which we would use the key, so that's the point when it should be generated.

Do we wait until after onload to restore state?  I don't think we do.
(In reply to Boris Zbarsky (:bz) from comment #6)
> Do we wait until after onload to restore state?  I don't think we do.

Indeed.
Blocks: 677430
I've written some tests for nsHTMLGenericFormElementWithState, will submit a patch as soon as they go smoothly in the reftests (I guess I'll have to go through an iframe to get the page to reload correctly).

Two suspicious cases:
- dynamically generated inputs (document.createElement) aren't restored
- cloned inputs have the same key, so get restored to the same state. Maybe we should re-generate the key when cloning, that would probably solve the most common case but cause some more weirdness in others.

Anyway, in both cases, webkit exhibits the same behaviour (except that you need to forward/back, reload clears the form).
Attached patch As promised, some tests (obsolete) — Splinter Review
They pass with the patch, but also show the limit cases mentioned above (field not restored / restored twice when dynamically generating / cloning input fields); the ref can be updated when we make a decision on those.
Attachment #609735 - Flags: review?(mounir)
Comment on attachment 609735 [details] [diff] [review]
As promised, some tests

Review of attachment 609735 [details] [diff] [review]:
-----------------------------------------------------------------

I think it would be more appropriate to do a Mochitest. Given that it is related to forms, you should put it in content/html/content/test/forms/ there is already a test for save/restore there already (test_save_restore_radio_groups.html) so it shouldn't be too hard to adapt your test.
Attachment #609735 - Flags: review?(mounir)
Assignee: nobody → meta+reg
Attached patch Rewritten the test as Mochitest (obsolete) — Splinter Review
Attachment #609735 - Attachment is obsolete: true
Attachment #611156 - Flags: review?(mounir)
Attached patch Rewritten the test as Mochitest (obsolete) — Splinter Review
Oops, forgot to remove a debug line (local script src)
Attachment #611156 - Attachment is obsolete: true
Attachment #611156 - Flags: review?(mounir)
Attachment #611160 - Flags: review?(mounir)
Comment on attachment 611160 [details] [diff] [review]
Rewritten the test as Mochitest

Review of attachment 611160 [details] [diff] [review]:
-----------------------------------------------------------------

I prefer tests that clearly differentiate different parts because it's easier to have an overview and understand what is actually tested. However, it's not a big deal.

A note regarding the coding style: you should add more empty lines in your code, it would be nicer to read ;)

I did not r+ the test because there is one change I would like to see (the iframe). Otherwise, it looks good.

::: content/html/content/test/forms/test_restore_form_elements.html
@@ +7,5 @@
> +  <meta charset="utf-8">
> +  <title>Test for Bug 737851</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +<body onload="mainOnLoad();">

Could you use addLoadEvent() instead?

@@ +46,5 @@
> +<script type="application/javascript">
> +
> +/** Test for Bug 737851 **/
> +
> +/* We reload the page once to check persistence of elements */

Could you use an iframe instead of reloading the entire test?

@@ +128,5 @@
> +      SimpleTest.is(document.getElementById(id).value, fieldValues[id],
> +                    "Field "+id+" should be restored after reload");
> +    } else {
> +      SimpleTest.todo_is(document.getElementById(id).value, fieldValues[id],
> +                         "Field "+id+" isn't properly restored after reload");

Could you explain why do you need that? I mean, are there some failures?
Attachment #611160 - Flags: review?(mounir)
Please create new files for your new class; nsGenericElement.* is too big already. (Also: move it into the mozilla::dom namespace.)
Cleaned up according to Mounir's review. Thanks !
Attachment #611160 - Attachment is obsolete: true
Attachment #612871 - Flags: review?(mounir)
I'm sorry for the lag here; this is at the top of my list for Monday.
Comment on attachment 612871 [details] [diff] [review]
Mochitests for this bug (and the one it blocks)

Review of attachment 612871 [details] [diff] [review]:
-----------------------------------------------------------------

I'm afraid I will have to ask you for another patch.

::: content/html/content/test/forms/inner_restore_form_elements.html
@@ +62,5 @@
> +      inputE2.setAttribute("id","e2");
> +      testdiv.appendChild(inputE2);
> +    }
> +
> +    addLoadEvent(shuffle);

That could have been done in the outer frame.

::: content/html/content/test/forms/test_restore_form_elements.html
@@ +21,5 @@
> +
> +<div id="content"><!-- style="display: none">-->
> +
> +  <iframe id="frame" width="800px" height="600px"
> +          src="inner_restore_form_elements.html">

I would highly prefer a data url instead of a new file. It's way harder to read a test in multiple files.

@@ +38,5 @@
> +
> +var frameElement = document.getElementById("frame");
> +var frame = frameElement.contentWindow;
> +
> +frameElement.addEventListener("load", frameOnLoad1);

You could do:
addLoadEvent(function() {
  frameElement.addEventListener("load", frameOnLoad2);
  fill();
  frame.location.assign(frame.location.href + '#');
  frame.location.reload();
});

That would be safer IMO.

@@ +84,5 @@
> +/* Simulate user input by entering the values */
> +
> +function fill() {
> +  for (id in fieldValues) {
> +    frame.document.getElementById(id).value += fieldValues[id];

+= or =? Why do you need the + here?

@@ +94,5 @@
> +/* Check that all the fields are as they have been entered */
> +
> +function checkAllFields() {
> +  SimpleTest.isnot(frame.document.URL.indexOf("#"), -1,
> +                   "Document should be reloaded once (self-check)");

I would prefer something like:
if (frame.document.URL.indexOf("#") == -1) {
  ok(false, "Something went wrong: the document wasn't reloaded!");
}

So you don't pollute the tests results if everything was okay.

@@ +98,5 @@
> +                   "Document should be reloaded once (self-check)");
> +
> +  for (id in fieldValues) {
> +    var fieldValue = frame.document.getElementById(id).value;
> +    if (typeof(changedFields[id]) === 'undefined') {

if (changedFields[id] === undefined) {
would be equivalent AFAIK (maybe not with some strict)

@@ +103,5 @@
> +      SimpleTest.is(fieldValue, fieldValues[id],
> +                    "Field "+id+" should be restored after reload");
> +    } else {
> +      SimpleTest.is(fieldValue, changedFields[id],
> +                    "Field "+id+" normally gets a different value after reload");

Please, don't use |SimpleTest.| for is(), ok(), isnot(), etc.
Attachment #612871 - Flags: review?(mounir)
Attachment #612871 - Attachment is obsolete: true
Attachment #613943 - Flags: review?
Comment on attachment 613943 [details] [diff] [review]
Mochitest, cleaned up according to review

Review of attachment 613943 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Though, I would prefer to have the file named "test_save_restore_form_controls.html" for consistency with "test_save_restore_radio_groups.html".
Attachment #613943 - Flags: review? → review+
Boris: ping ?

I am particularly interested in whether the NS_FORWARD_NSIDOMNODE should be changed ; and we should agree on the results (I made the edge cases visible in the test).
Thanks!
Forwarding to the direct superclass is the right thing to do.

I'm still thinking through the rest of the patch; trying to figure out whether the key generation algorithm we're using here is really what we want.  It might be that it is, or at least that changing it could be a separate bug, but as long as we're here....
Comment on attachment 608397 [details] [diff] [review]
This is a new version of the patch submitted to bug 660549

I'm sorry it took me so long to get to this.  :(

I think overall this is fine, but we should definitely file a followup bug on reevaluating our state key generation to see whether we can do simpler keys in the new setup.  I'm somewhat worried that generating state keys the way we do right now will slow down pageload.  Note that before this patch we have an optimization to not do key generation on pageload if there's no layout history (in other words, for normal page loads). That's the aRead boolean in GetLayoutHistoryAndKey.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp	Thu Mar 22 18:47:33 2012 +0100

>+nsGenericHTMLFormElementWithState::GenerateStateKey()
>+  // Get the pres shell

There's no presshell here.  Just nix this comment, please.

>+  nsCOMPtr<nsIDocument> doc = GetDocument();

I think this can just be an nsIDocument*

>+  nsCAutoString stateKey;
>+  nsresult rv = nsContentUtils::GenerateStateKey(this, doc,
>+                                                 nsIStatefulFrame::eNoID,
>+                                                 stateKey);
>+  if (NS_FAILED(rv)) {
>+    // mStateKey unchanged (void)
>+    return rv;
>+  }

Would it make sense to generate into mStateKey directly and just SetIsVoid() in the rare failure case?

>+++ b/content/html/content/src/nsGenericHTMLElement.h	Thu Mar 22 18:47:33 2012 +0100
>+  /* Used to store the key to that element in the session. Is void until
>+     InitStateKey has been used */

s/InitStateKey/GenerateStateKey/

>+++ b/content/html/content/src/nsHTMLButtonElement.cpp	Thu Mar 22 18:47:33 2012 +0100
> nsHTMLButtonElement::DoneCreatingElement()
>+  nsresult rv = GenerateStateKey();
>+
>+  if (!mInhibitStateRestoration && NS_SUCCEEDED(rv)) {
>+    RestoreFormControlState();

mInhibitStateRestoration should probably inhibit saving the state too.  So just don't generate a state key when mInhibitStateRestoration, please?

Similar for other elements.

We should probably SetVoid() the state key if we're adopted into a different document.

r- for the above nits, and because the NS_FORWARD bits need fixing, but I promise a much much faster review on the next iteration.

And please let me know if you can't work on this any more at this point.  Again, I'm really sorry this took so long.
Attachment #608397 - Flags: review?(bzbarsky) → review-
Sorry for the lag ; just a note to say I'm still on this, only I've also been moving, etc., and am still without a proper Internet connection. I'll get back to it very soon!
Louis, are you still working on this? Can I take this over?

And if so, I think the work to be done are the nits and NS_FORWARD, correct? Boris I think you never weighed in on what to do with the NS_FORWARD bits. Where should I start with that?
> Boris I think you never weighed in on what to do with the NS_FORWARD bits.

See comment 21.
Assignee: meta+reg → almasry.mina
Attached patch Patch (obsolete) — Splinter Review
So far I've done the things in Boris's last review. There are a few things I don't understand:

> We should probably SetVoid() the state key if we're adopted into a different document.

Sorry where does the adopting happen in the code?

Also, I've read comment #21, but I still don't understand what's needed. Can you please elaborate on what the problem with the NS_FORWARD bits is and where do I start with fixing it?

Aside from these things, and the tests is there anything else to be done? Can you point me to where I start with these too?

I have attached the patch I have so far, but I'm not asking for a review since the changes from the last one are minimal... but feel free to review of course.
Attachment #608397 - Attachment is obsolete: true
> Sorry where does the adopting happen in the code?

nsNodeUtils::CloneAndAdopt.  The adoption case is where the NodeInfoChanged() call happens and then the various event listener recompilation, etc.

> Can you please elaborate on what the problem with the NS_FORWARD bits is

Looks like we always just forward nsIDOMNode to nsINode, so there is no longer a problem for that forwarding.

But what needs to happen is that various things that reference the parent class need to reference the new parent class.  So any time nsGenericHTMLFormElement is referenced in HTMLButtonElement.cpp, for example should probably be referencing the new parent class instead.

> Aside from these things, and the tests is there anything else to be done? 

Probably nothing; I'll need to re-review the patch carefully at some point, though.
Attached patch patch with test (obsolete) — Splinter Review
Alright it took a bit of tweaking and testing but here is a patch that passes all the tests that mounir reviewed+

It's a long patch, but most of it is redirecting to the new superclass.
Attachment #613943 - Attachment is obsolete: true
Attachment #774430 - Attachment is obsolete: true
Attachment #776077 - Flags: review?(bzbarsky)
Mina, it'll probably take me a few days to get to this.
No problem :-)
Attachment #776077 - Attachment is patch: true
Attachment #776077 - Attachment mime type: text/x-patch → text/plain
Just a friendly reminder this waits for a review. Thanks!
Yes, I do remember that.  I just haven't had a chance to sit down to do a longish review... :(
No problem. Don't be sad :-)
Comment on attachment 776077 [details] [diff] [review]
patch with test

>+++ b/content/html/content/src/HTMLButtonElement.cpp
> HTMLButtonElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
>+  nsresult rv = nsGenericHTMLFormElementWithState::BindToTree(aDocument, aParent,
>                                                      aBindingParent,
>                                                      aCompileEventHandlers);

Please fix the indent.  Probably by putting a linebreak after the '=' and then lining up the arguments correctly.

> HTMLButtonElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+  return nsGenericHTMLFormElementWithState::BeforeSetAttr(aNameSpaceID, aName,
>                                                  aValue, aNotify);

Fix indent.

>@@ -489,29 +492,29 @@ HTMLButtonElement::AfterSetAttr(int32_t 
>+  return nsGenericHTMLFormElementWithState::AfterSetAttr(aNameSpaceID, aName,
>                                                 aValue, aNotify);

Ditto.

>+++ b/content/html/content/src/HTMLInputElement.cpp
> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(HTMLInputElement,
>-                                                  nsGenericHTMLFormElement)
>+                                                  nsGenericHTMLFormElementWithState)

The indent here was broken before too, but fix it while you're changing this.

> HTMLInputElement::DoneCreatingElement()
>+  nsresult rv = GenerateStateKey();

This is wasted work if mInhibitRestoration.  Please don't call this if mInhibitRestoration is true.

So perhaps something like:

  bool restoreCheckedState =
    !mInhibitRestoration && NS_SUCCEEDED(GenerateStateKey()) && RestoreFormControlState();

and take out the standalone GenerateStateKey call or some such.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLFormElementWithState::GetLayoutHistory(bool aRead)
>+  // Get the pres shell

No presshell anywhere here.  Please take that comment out.

>+++ b/content/html/content/src/nsGenericHTMLElement.h
>+class nsGenericHTMLFormElementWithState: public nsGenericHTMLFormElement

Space before ':', please.

Please add newlines between these lines:

>+  nsresult GetPrimaryPresState(nsPresState** aPresState);
>+    GetLayoutHistory(bool aRead);

and the following comments.

If you can figure out a way to test the "clear state key on adopt" case, that would be awesome, but I'm fine either way on that.

r=me with the above nits, and very sorry for the terrible lag...
Attachment #776077 - Flags: review?(bzbarsky) → review+
Changes made, but I also found that this patch is out of date for the current m-c, so I had to make some more changes. The only ones with any significance are in nsGenericFormElementWithState::GetPrimaryPresState, but the function still does the same thing.

Not sure if I should ask for another review. I'm assuming no, so here is a try push:

https://tbpl.mozilla.org/?tree=Try&rev=886a35063b60
Attachment #776077 - Attachment is obsolete: true
I'd actually appreciate an interdiff between those last two patches...
Attached patch InterdiffSplinter Review
Interdiff!
Attachment #784499 - Flags: review?(bzbarsky)
Comment on attachment 784499 [details] [diff] [review]
Interdiff

Thanks for the interdiff!

r=me
Attachment #784499 - Flags: review?(bzbarsky) → review+
My pleasure!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4173c694fb43
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Mina, I think it would be good to go trough the bugs marked as blocked by this one and see if this patch solved them and if not what should be done. Do you think you can find some cycles to do that?
> Do you think you can find some cycles to do that?

Will take a look :-)
Blocks: 910652
Marking qawanted for the issue from comment 42.
Keywords: qawanted, verifyme
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Duplicate of this bug: 680893
I'm dropping the qawanted request on this bug since it's well over a year old. Please re-add the qawanted keyword if the request is still valid.
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.