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.
Created attachment 25050 [details]
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.
Comment on attachment 25050 [details]
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 75c2c38..0a62beb 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,22 @@
> +2008-11-11 Brett Wilson <firstname.lastname@example.org>
> + 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.
Created attachment 25136 [details]
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.
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.
What about checkboxes and radio buttons which have no editor?
Created attachment 25366 [details]
On IRC (I think Adele?) suggested using ChromeClient instead, which this patch does.
Comment on attachment 25366 [details]
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.
Created attachment 25914 [details]
Fix windows build.
I also took the opportunity to fix my ChangeLog comment for a previous check-in.
Created attachment 25915 [details]
Fix windows build.
I also fixed a comment in the ChangeLog for a recent checkin of mine.
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.
Created attachment 25916 [details]
Update build fix patch.
Build fix committed revision 39168. There were tabs in ChangeLog.