[webkit-reviews] review denied: [Bug 31718] Framework to show form validation message for invalid controls : [Attachment 48145] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 31718: Framework to show form validation message for invalid controls
https://bugs.webkit.org/show_bug.cgi?id=31718

Attachment 48145: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=48145&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    void mayShowValidationMessage();
> +    void mayHideValidationMessage();

A single update function would be better than these two.

> +    // A validation message for this element is shown.
> +    bool m_validationMessageShown : 1;

For boolean data members we normally use a name that fits the sentence "form
control element <xxx>".

But I think what you may need here is a copy of the message that was last sent
to the client, rather than just a boolean.

> +	   virtual void showFormValidationMessage(const Node*, const IntRect&,
FrameView*, const String&) { };
> +	   virtual void hideFormValidationMessage(const Node*) { };

Should be HTMLFormControlElement* instead of Node*. Also no good reason to use
const Node* since these objects are in a tree and const pointers are not all
that useful.

No semicolons after "{ }".

I don’t understand the value of the IntRect and FrameView arguments. What
rectangle? What guarantees that a form element is rectangular, given that it
can be transformed arbitrarily? Why can’t the caller get things like the
rectangle and view from the form control element using the usual APIs instead
of having them explicitly passed? Maybe this is best for the current usage in
Chromium but I don’t think it’s quite right. I would leave all that complexity
out of the chrome client hookup.

Further, if you pass a rectangle and frame view, then who is responsible for
calling with a new rectangle if the page changes? It's clearly wrong to send a
rectangle for one moment in time without providing for tracking over time, and
it's not useful to supply the FrameView since it can easily be derived.

If the validation interface is going to be shown overlaying the web page, I’d
like to see more support for it in WebKit itself. It’s unpleasant to have to
redo this feature for each platform separately. It’s good to have flexibility
but ideally we would share as much common code as necessary.

I’m not sure it makes sense to have separate hide/show calls for this -- this
should instead just be a single set function. All the logic would end up
simpler and we can use either a null or empty string to mean "hide".

Is the client guaranteed to receive a call for a form element if it’s removed
from the document? What about when a document moves out of the view or back
into the view during navigation? I think that sending the call at destructor
time is clearly bad, because these are reference-counted times and the timing
of the call to the destructor is unpredictable. More thought needs to go into
this aspect of the design.

How can we test this? Can you hook this up to DumpRenderTree? I don’t think we
should land the ChromeClient level alone without the upper level hookup that
allows testing.

What prevents the same validation message from being repeatedly sent to the
client?

There's a lot to think about here.