Bug 22177 – Need a notification when a form element changes

• Component: Forms
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

Chromium saves the current "state" of the page to the browser process and then to disk periodically. This allows us to know the form state of the page if the tab crashes and you press reload. It also enables browser crash recovery since the last open tabs were serialized to disk.

A very long time ago we had regular polling of the page. This was inefficient, so we forked HTMLInputElement to call a glue function when the element state changes. Because there could be a lot of form state changes, we implemented a delay where we implemented the glue function, so that calls from HTMLInputElement were very inexpensive.

We need an "official" way to do this rather than the hacky forked version of the file.

Looking at the various clients, it looks like adding an additional callback to FrameLoaderClient would be the best way to do this.

[Brett Wilson (Google)]

Created attachment 25050 [details]
Patch

I didn't get any comments on IRC about this change, and I'm not 100% sure the best way to do it. This is the simple way I proposed above. I just added a callback on the FrameLoaderClient that input elements and text areas call when their value changes. I would be interested in hearing better proposals for getting the same information.

[Sam Weinig]

Comment on attachment 25050 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 75c2c38..0a62beb 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,22 @@
> [phone]  Brett Wilson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        WARNING: NO TEST CASES ADDED OR CHANGED

Please explain what this patch does here.

>  #include "EventNames.h"
>  #include "File.h"
>  #include "FileList.h"
> +#include "FrameLoader.h"
> +#include "FrameLoaderClient.h"
>  #include "FocusController.h"
>  #include "FormDataList.h"
>  #include "Frame.h"
Please sort these.

> @@ -100,6 +102,16 @@ static int numCharactersInGraphemeClusters(StringImpl* s, int numGraphemeCluster
>      return textBreakCurrent(it);
>  }
>  
> +// Notifies the FrameLoaderClient (if any) corresponding to the frame of the
> +// given form control that the form state may have changed.
This comment is unnessery as the code really explains itself here.

> +static inline void notifyFormStateChanged(const HTMLInputElement *element)
The * is on the wrong side per our style guidelines.

> +static inline void notifyFormStateChanged(const HTMLTextAreaElement *element)
The * on the wrong side.

I don't think you are notifying for changes to <input type="file"> which is set through setFileListFromRenderer.  What about <select> elements.

> +        // Notification that page state such as form elements have changed.
> +        // This function will be called frequently, so handling should be very fast.
> +        virtual void pageStateDidChange() = 0;

What over state changes do you envision this notifying?  The name seems to indicate it would fire for all kinds of state changes (style, DOM, etc.)

You need to update the FrameLoaderClient for Qt, Gtk, and wx.

If this all about form state, why not use the DOM.  It already has an event model which exposes these things.

r- because I think the name needs work and you break the other ports.

[Brett Wilson (Google)]

Created attachment 25136 [details]
Patch v2

This fixes the style and ChangeLog problems. It adds support for file and select elements (thanks for noticing this). It adds the similar changes to the wx, qt, and gtk ports.

I renamed the calleback "formStateChanged" since it's really just form state. I originally called this page state because that's what we call it, because it also includes some things such as scroll position that we compute on the port side. This is irrelevant to this function, however, so the new name is better.

I didn't want to do a DOM hook for form elements for several reasons. It seems impractical to hook each one individually, and I definitely didn't want to install a global mutation handler for every page.

[Sam Weinig]

I am sorry I did not notice/remember this earlier, but we already have one mechanism for form state changes in the Editor client with methods like EditorClient::textDidChangeInTextField().  On the mac, this translates to callbacks on a WebFormDelegate.  If the existing methods are not enough, we should probably at least put your new method on the EditorClient for consistency.

[Brett Wilson (Google)]

What about checkboxes and radio buttons which have no editor?

[Brett Wilson (Google)]

Created attachment 25366 [details]
Patch v3

On IRC (I think Adele?) suggested using ChromeClient instead, which this patch does.

[Dave Hyatt]

Comment on attachment 25366 [details]
Patch v3

r=me

[Brett Wilson (Google)]

Fixed in r39152. (I updated the ChangeLogs from the patch here, a few of them still referenced FrameLoaderClient even after I moved it to the ChromeClient.

[David Levin]

Created attachment 25914 [details]
Fix windows build.

I also took the opportunity to fix my ChangeLog comment for a previous check-in.

[David Levin]

Created attachment 25915 [details]
Fix windows build.

I also fixed a comment in the ChangeLog for a recent checkin of mine.

[Alexey Proskuryakov]

Comment on attachment 25915 [details]
Fix windows build.

The notifyFormStateChanged(this) call clearly doesn't belong to HTMLInputElement::type(), I think that the patch was mis-applied when landing. Maybe this call is missing elsewhere as a result.

[David Levin]

Created attachment 25916 [details]
Update build fix patch.

[Alexey Proskuryakov]

Build fix committed revision 39168. There were tabs in ChangeLog.