[webkit-reviews] review denied: [Bug 88665] Favicon URL list sent with favicon updated message contains out of date icon URLs : [Attachment 149857] Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Pete Williamson
<petewil at chromium.org>'s request for review:
Bug 88665: Favicon URL list sent with favicon updated message contains out of
date icon URLs
https://bugs.webkit.org/show_bug.cgi?id=88665

Attachment 149857: Patch with a proposed fix for the bug (with layout tests,
more recently synced, no extra files, better style, fixed CR comments)
https://bugs.webkit.org/attachment.cgi?id=149857&action=review

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


> LayoutTests/fast/dom/icon-url-change.html:12
> +function debugOutput(str) {
> +    text = document.createTextNode(str);
> +    debugDiv = document.getElementById('debugDiv');
> +    div = document.createElement ('div');
> +    div.appendChild(text);
> +    debugDiv.appendChild(div);
> +}

Please do not re-implement such function.  You had better use debug() in
fast/js/resources/js-test-pre.js.

> LayoutTests/fast/dom/icon-url-change.html:23
> +    for (var i = 0; i < links.length; ++i) {
> +	 var link = links[i];
> +	 if (link.type=="image/x-icon" && link.rel=="shortcut icon") {
> +	   docHead.removeChild(link);
> +	   break; // Assuming only one match at most.
> +	 }
> +    }

We usually use four-space indentation in JavaScript code.

> LayoutTests/fast/dom/icon-url-change.html:35
> +    iconURL =
document.getElementsByTagName("head")[0].getElementsByTagName("link")[0].href;

document.getElementsByTagName("link")[0].href is enough?

> LayoutTests/fast/dom/icon-url-change.html:59
> +	   debugOutput('PASS - URL list matches expected');
> +    else
> +	   debugOutput('FAIL - URL list does not match expected');

You had better use testPassed() and testFailed() defined in
fast/js/resources/js-test-pre.js

> LayoutTests/fast/dom/icon-url-list.html:14
> +function debugOutput(str) {
> +    text = document.createTextNode(str);
> +    debugDiv = document.getElementById('debugDiv');
> +    div = document.createElement ('div');
> +    div.appendChild(text);
> +    debugDiv.appendChild(div);
> +}

same comment as icon-url-change.html

> LayoutTests/fast/dom/icon-url-list.html:34
> +    for (var i = 0; i < links.length; ++i) {
> +	 var oldLink = links[i];
> +	 if (oldLink.type=="image/x-icon" && oldLink.rel=="shortcut icon") {
> +	   // if we find the child, replace it with the new node.
> +	//debugOutput('replacing ' + oldLink.href + ' with ' + newLink.href);
> +	   docHead.replaceChild(newLink, oldLink);
> +	   return; // Assuming only one match at most.
> +	 }
> +    }

ditto.
Also, please remove commented-out code.

> LayoutTests/fast/dom/icon-url-list.html:44
> +    iconURL =
document.getElementsByTagName("head")[0].getElementsByTagName("link")[0].href;

ditto.

> LayoutTests/fast/dom/icon-url-list.html:51
> +    iconURL =
document.getElementsByTagName("head")[0].getElementsByTagName("link")[0].href;

ditto.

> LayoutTests/fast/dom/icon-url-list.html:63
> +	   debugOutput('PASS - URL list matches expected');
> +    else
> +	   debugOutput('FAIL - URL list does not match expected');

ditto.