Bug 22177 - Need a notification when a form element changes
Summary: Need a notification when a form element changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brett Wilson (Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-11 09:19 PST by Brett Wilson (Google)
Modified: 2008-12-10 04:24 PST (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Brett Wilson (Google) 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 Brett Wilson (Google) 2008-11-11 12:33:45 PST
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.
Comment 2 Sam Weinig 2008-11-12 00:08:02 PST
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 @@
> +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 Brett Wilson (Google) 2008-11-13 14:10:33 PST
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.
Comment 4 Sam Weinig 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 Brett Wilson (Google) 2008-11-19 15:36:32 PST
What about checkboxes and radio buttons which have no editor?
Comment 6 Brett Wilson (Google) 2008-11-21 16:50:54 PST
Created attachment 25366 [details]
Patch v3

On IRC (I think Adele?) suggested using ChromeClient instead, which this patch does.
Comment 7 Dave Hyatt 2008-12-05 09:56:55 PST
Comment on attachment 25366 [details]
Patch v3

r=me
Comment 8 Brett Wilson (Google) 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 David Levin 2008-12-09 22:31:04 PST
Created attachment 25914 [details]
Fix windows build.

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

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