[Webkit-unassigned] [Bug 88665] Favicon URL list sent with favicon updated message contains out of date icon URLs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88665


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #149857|review?                     |review-
               Flag|                            |




--- Comment #65 from Kent Tamura <tkent at chromium.org>  2012-06-27 21:31:48 PST ---
(From update of attachment 149857)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.