Bug 22177 - Need a notification when a form element changes
: Need a notification when a form element changes
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-11-11 09:19 PST by
Modified: 2008-12-10 04:24 PST (History)


Attachments
Patch (7.31 KB, patch)
2008-11-11 12:33 PST, Brett Wilson (Google)
sam: review-
Review Patch | Details | Formatted Diff | Diff
Patch v2 (13.02 KB, patch)
2008-11-13 14:10 PST, Brett Wilson (Google)
no flags Review Patch | Details | Formatted Diff | Diff
Patch v3 (12.18 KB, patch)
2008-11-21 16:50 PST, Brett Wilson (Google)
hyatt: review+
Review Patch | Details | Formatted Diff | Diff
Fix windows build. (1.51 KB, patch)
2008-12-09 22:31 PST, David Levin
no flags Review Patch | Details | Formatted Diff | Diff
Fix windows build. (1.51 KB, patch)
2008-12-09 22:41 PST, David Levin
ap: review-
Review Patch | Details | Formatted Diff | Diff
Update build fix patch. (1.84 KB, patch)
2008-12-10 00:28 PST, David Levin
ap: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-11-11 09:19:27 PST
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.
------- Comment #1 From 2008-11-11 12:33:45 PST -------
Created an attachment (id=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.
------- Comment #2 From 2008-11-12 00:08:02 PST -------
(From update of 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  <brettw@chromium.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.
------- Comment #3 From 2008-11-13 14:10:33 PST -------
Created an attachment (id=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.
------- Comment #4 From 2008-11-19 15:34:39 PST -------
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.
------- Comment #5 From 2008-11-19 15:36:32 PST -------
What about checkboxes and radio buttons which have no editor?
------- Comment #6 From 2008-11-21 16:50:54 PST -------
Created an attachment (id=25366) [details]
Patch v3

On IRC (I think Adele?) suggested using ChromeClient instead, which this patch does.
------- Comment #7 From 2008-12-05 09:56:55 PST -------
(From update of attachment 25366 [details])
r=me
------- Comment #8 From 2008-12-09 16:03:50 PST -------
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.
------- Comment #9 From 2008-12-09 22:31:04 PST -------
Created an attachment (id=25914) [details]
Fix windows build.

I also took the opportunity to fix my ChangeLog comment for a previous check-in.
------- Comment #10 From 2008-12-09 22:41:21 PST -------
Created an attachment (id=25915) [details]
Fix windows build.

I also fixed a comment in the ChangeLog for a recent checkin of mine.
------- Comment #11 From 2008-12-09 23:46:07 PST -------
(From update of attachment 25915 [details])
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.
------- Comment #12 From 2008-12-10 00:28:54 PST -------
Created an attachment (id=25916) [details]
Update build fix patch.
------- Comment #13 From 2008-12-10 04:24:13 PST -------
Build fix committed revision 39168. There were tabs in ChangeLog.