[webkit-reviews] review denied: [Bug 22177] Need a notification when a form element changes : [Attachment 25050] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Brett Wilson (Google)
<brettw at chromium.org>'s request for review:
Bug 22177: Need a notification when a form element changes
https://bugs.webkit.org/show_bug.cgi?id=22177

Attachment 25050: Patch
https://bugs.webkit.org/attachment.cgi?id=25050&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
> 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 at 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.