[webkit-reviews] review denied: [Bug 100557] Implement HTMLFormElement#requestAutocomplete and associated events : [Attachment 171979] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Dan Beam <dbeam at chromium.org>'s
request for review:
Bug 100557: Implement HTMLFormElement#requestAutocomplete and associated events
https://bugs.webkit.org/show_bug.cgi?id=100557

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171979&action=review


C++ code looks good.

> Source/WebCore/html/HTMLFormElement.cpp:417
> +	   m_requestAutocompleteDelayTimer.startOneShot(0);

We had better have a comment why we delay event dispatching.

> Source/WebCore/html/HTMLFormElement.h:107
> +    void requestAutocompleteTimerFired(Timer<HTMLFormElement>*);

THis should be private.

> LayoutTests/ChangeLog:20
> +	   * fast/forms/request-autocomplete.html: Added.

Please rename it to form-request-autocomplete.html.

> LayoutTests/fast/forms/request-autocomplete.html:14
> +if (window.testRunner)
> +   testRunner.dumpAsText();
> +
> +function log(message)
> +{
> +    var console = document.getElementById('console');
> +    console.appendChild(document.createTextNode(message));
> +    console.appendChild(document.createElement('br'));
> +}
> +

Do not re-implement such utility function.  js-test-pre.js provides some.
  <script src="../js/resources/js-test-pre.js"></script>

and add <script src="../js/resources/js-test-post.js"></script> at the bottom.

> LayoutTests/fast/forms/request-autocomplete.html:21
> +	   log('FAILED: no requestAutocomplete function');

Use testFailed() provided by js-test-pre.js.

> LayoutTests/fast/forms/request-autocomplete.html:34
> +	   if (/onautocomplete/.test(field))
> +		log('FAILED: enumerable form attribute found on
HTMLFormElement: ' + field);

Wrong indentation.
testFailed().

> LayoutTests/fast/forms/request-autocomplete.html:37
> +    log(enumerated ? 'PASSED: no enumerable properties on HTMLFormElement' :
'FAILED: to enumerate HTMLFormElement properties');

Use testPassed() and testFailed().

>>> LayoutTests/fast/forms/request-autocomplete.html:64
>>> +	 if (numErrors > numErrorsExpected)
>> 
>> I had an idea to let doneTesting() wait 2s for other errors - is this a
good/bad idea?	right now we'll hit notifyDone() and we wouldn't know if
there's too many errors assuming the test halts immediately.
> 
> If you always wait for 2s, this test will take a minimum of 2s to run, no
matter what.  That's not so good.  It's almost always better to wait on some
specific event.

Don't wait for 2s.

> LayoutTests/fast/forms/request-autocomplete.html:74
> +    log('PASSED: got expected number of error events (' + numErrorsExpected
+ ')');
> +    if (window.notifyDone)
> +	   notifyDone();

Use testPassed and finishJSTest(), and add jsTestIsAsync = true at the top
level.

> LayoutTests/fast/forms/request-autocomplete.html:86
> +</html>

No </body>