Attachment 149563 Details for Bug 88665 – Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

[patch] Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

patch0.out (text/plain), 16.92 KB, created by Pete Williamson on 2012-06-26 11:02:22 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Pete Williamson

Created: 2012-06-26 11:02:22 PDT

Size: 16.92 KB

patch

obsolete

>diff --git a/ChangeLog b/ChangeLog
>index 72a7486..74e30c5 100644
>--- a/ChangeLog
>+++ b/ChangeLog
>@@ -1,3 +1,12 @@
>+2012-06-26  Pete Williamson  <petewil@google.com>
>+
>+        Expose an export for the iconUrl list so Internals can use it
>+        https://bugs.webkit.org/show_bug.cgi?id=88665
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * Source/autotools/symbols.filter: expose an export for iconURLs
>+
> 2012-06-25  Simon Hausmann  <simon.hausmann@nokia.com>
> 
>         [Qt] Make it possible to build WebKit without QtWidgets
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 597c00c..d6afed4 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,18 @@
>+2012-06-26  Pete Williamson  <petewil@google.com>
>+
>+        Add some new unit tests to test the favicon changing dynamically
>+        https://bugs.webkit.org/show_bug.cgi?id=88665
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/dom/icon-url-change-expected.txt: Added.
>+        * fast/dom/icon-url-change.html: Added a new test for changing the favicon dynamically
>+        * fast/dom/icon-url-list-expected.txt: Added.
>+        * fast/dom/icon-url-list.html: Added a new test for multiple favicons in the HTML header
>+        * fast/dom/icon-url-property-expected.txt: update unit test expectations
>+        * fast/dom/icon-url-property.html: update and enable existing favicon test
>+        * platform/chromium/TestExpectations: reenable the url-property test
>+
> 2012-06-26  Allan Sandfeld Jensen  <allan.jensen@nokia.com>
> 
>         [Qt] Baseline missing for 3D transforms tests.
>diff --git a/LayoutTests/fast/dom/icon-url-property-expected.txt b/LayoutTests/fast/dom/icon-url-property-expected.txt
>index 13ba05f..0c0ffce 100644
>--- a/LayoutTests/fast/dom/icon-url-property-expected.txt
>+++ b/LayoutTests/fast/dom/icon-url-property-expected.txt
>@@ -1,4 +1,4 @@
>-main frame - didChangeIcons
> Original iconURL is: http://test.com/oldfavicon.ico
>-Setting new icon URL to: http://test.com/newfavion.ico
>-New iconURL is: http://test.com/newfavion.ico
>+Setting new icon URL to: http://test.com/newfavicon.ico
>+New iconURL is: http://test.com/newfavicon.ico
>+PASS - URL list matches expected
>diff --git a/LayoutTests/fast/dom/icon-url-property.html b/LayoutTests/fast/dom/icon-url-property.html
>index eb51d7d..5fa13f5 100644
>--- a/LayoutTests/fast/dom/icon-url-property.html
>+++ b/LayoutTests/fast/dom/icon-url-property.html
>@@ -31,16 +31,24 @@ function setFavIcon(iconURL) {
> function runTests() {
>     if (window.testRunner) {
>         testRunner.dumpAsText();
>-        testRunner.dumpIconChanges();
>     }
>     iconURL = document.getElementsByTagName("head")[0].getElementsByTagName("link")[0].href;
>     debugOutput ('Original iconURL is: ' + iconURL);
>-    newURL = 'http://test.com/newfavion.ico';
>+    newURL = 'http://test.com/newfavicon.ico';
>     debugOutput ('Setting new icon URL to: ' + newURL);
>     setFavIcon(newURL);
>     iconURL = document.getElementsByTagName("head")[0].getElementsByTagName("link")[0].href;
> 
>     debugOutput ('New iconURL is: ' + iconURL);
>+
>+    // check that the URL list in the document is as we expect
>+    var expectedURLs = "http://test.com/newfavicon.ico";
>+    var iconURLs = window.internals.iconURLs(document);
>+    if (expectedURLs == iconURLs[0])
>+        debugOutput('PASS - URL list matches expected');
>+    else
>+        debugOutput('FAIL - URL list does not match expected');
>+
> }
> 
> </script>
>diff --git a/LayoutTests/platform/chromium/TestExpectations b/LayoutTests/platform/chromium/TestExpectations
>index c213869..5e96155 100644
>--- a/LayoutTests/platform/chromium/TestExpectations
>+++ b/LayoutTests/platform/chromium/TestExpectations
>@@ -1885,8 +1885,6 @@ BUGWK71758 : fast/url/port.html = TEXT PASS
> // // Started failing at r58152
> // BUGWK38038 : fast/url/file-http-base.html = TEXT
> 
>-BUGWK33812 SKIP : fast/dom/icon-url-property.html = PASS
>-
> // New layoutTestController function added at r57993
> BUGCR42696 : http/tests/xmlhttprequest/cross-origin-authorization-with-embedder.html = TIMEOUT
> 
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 85e4278..4df8c43 100755
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,43 @@
>+2012-06-26  Pete Williamson  <petewil@google.com>
>+
>+        Changed the behavior of iconURLs to always recalculate the list.
>+        https://bugs.webkit.org/show_bug.cgi?id=88665
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        As it turns out, it can contain stale URLs in the case that some script
>+        manipulates the DOM, which breaks scripts trying to reset the favicon
>+        URL. Also added a method in Internals to allow tests to get the list of
>+        icon
>+
>+        Tests: fast/dom/icon-url-change.html
>+               fast/dom/icon-url-list.html
>+
>+        * WebCore.exp.in: export Document::iconURLs on the mac for the Internals class
>+        * dom/Document.cpp:
>+        (WebCore::Document::iconURLs): Changed the method to recalculate the iconURL list every time
>+        * dom/Document.h:
>+        (Document): removed the addIconURL method which is no longer used
>+        * html/HTMLLinkElement.cpp:
>+        (WebCore::HTMLLinkElement::process):
>+        (WebCore::HTMLLinkElement::iconType): exposed the icon type with an accessor
>+        (WebCore):
>+        (WebCore::HTMLLinkElement::iconSizes): exposed the icon sizes with an accessor
>+        * html/HTMLLinkElement.h:
>+        (HTMLLinkElement): declared the icon type and size accessors
>+        * loader/LinkLoader.cpp:
>+        (WebCore::LinkLoader::loadLink): removed parameters that are no longer used
>+        * loader/LinkLoader.h:
>+        (LinkLoader): removed the call to the addIconURL method which is no longer used
>+        * loader/icon/IconController.cpp:
>+        (WebCore::IconController::urlsForTypes): added comment
>+        * testing/Internals.cpp:
>+        (WebCore::Internals::iconURLs): made a method to be used by unit tests for inspecting the icon URL list
>+        (WebCore):
>+        * testing/Internals.h:
>+        (Internals): declared the method for unit testing the icon URL list
>+        * testing/Internals.idl: exported the Document::iconURLs function
>+
> 2012-06-26  Alexei Filippov  <alexeif@chromium.org>
> 
>         Web Inspector: Native memory snapshots crash in debug mode.
>diff --git a/Source/WebCore/WebCore.exp.in b/Source/WebCore/WebCore.exp.in
>index abddfb7..30a0b71 100644
>--- a/Source/WebCore/WebCore.exp.in
>+++ b/Source/WebCore/WebCore.exp.in
>@@ -832,6 +832,7 @@ __ZN7WebCore8Document26pageSizeAndMarginsInPixelsEiRNS_7IntSizeERiS3_S3_S3_
> __ZN7WebCore8Document27removeMediaCanStartListenerEPNS_21MediaCanStartListenerE
> __ZN7WebCore8Document36updateLayoutIgnorePendingStylesheetsEv
> __ZN7WebCore8Document4headEv
>+__ZN7WebCore8Document8iconURLsEv
> __ZN7WebCore8FormData6createEPKvm
> __ZN7WebCore8FormDataD1Ev
> __ZN7WebCore8Gradient12addColorStopEfRKNS_5ColorE
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index 4dca64b..8ca55a2 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -4830,25 +4830,31 @@ PassRefPtr<XPathResult> Document::evaluate(const String& expression,
>     return m_xpathEvaluator->evaluate(expression, contextNode, resolver, type, result, ec);
> }
> 
>-const Vector<IconURL>& Document::iconURLs() const
>+const Vector<IconURL>& Document::iconURLs()
> {
>-    return m_iconURLs;
>-}
>+    static const char* const iconMIMEType = "image/x-icon";
> 
>-void Document::addIconURL(const String& url, const String& mimeType, const String& sizes, IconType iconType)
>-{
>-    if (url.isEmpty())
>-        return;
>+    m_iconURLs.clear();
> 
>-    // FIXME - <rdar://problem/4727645> - At some point in the future, we might actually honor the "mimeType"
>-    IconURL newURL(KURL(ParsedURLString, url), sizes, mimeType, iconType);
>-    m_iconURLs.append(newURL);
>+    // Include any icons where type = link, rel = "shortcut icon".
>+    HTMLCollection* children = head()->children();
>+    unsigned int length = children->length();
>+    for (unsigned int i = 0; i < length; ++i) {
>+        Node* child = children->item(i);
>+        if (!child->hasTagName(linkTag))
>+            continue;
>+        HTMLLinkElement* linkElement = static_cast<HTMLLinkElement*>(child);
>+        if (!equalIgnoringCase(linkElement->type(), iconMIMEType) || !(linkElement->iconType() == Favicon))
>+            continue;
>+        if (linkElement->href().isEmpty())
>+            continue;
> 
>-    if (Frame* f = frame()) {
>-        IconURL iconURL = f->loader()->icon()->iconURL(iconType);
>-        if (iconURL == newURL)
>-            f->loader()->didChangeIcons(iconType);
>+        // Put it at the front to ensure that icons seen later take precedence as required by the spec.
>+        IconURL newURL(linkElement->href(), linkElement->iconSizes(), linkElement->type(), linkElement->iconType());
>+        m_iconURLs.prepend(newURL);
>     }
>+
>+    return m_iconURLs;
> }
> 
> void Document::setUseSecureKeyboardEntryWhenActive(bool usesSecureKeyboard)
>diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
>index 68885fb..bfa17aa 100644
>--- a/Source/WebCore/dom/Document.h
>+++ b/Source/WebCore/dom/Document.h
>@@ -928,8 +928,7 @@ public:
>     
>     void setHasNodesWithPlaceholderStyle() { m_hasNodesWithPlaceholderStyle = true; }
> 
>-    const Vector<IconURL>& iconURLs() const;
>-    void addIconURL(const String& url, const String& mimeType, const String& size, IconType);
>+    const Vector<IconURL>& iconURLs();
> 
>     void setUseSecureKeyboardEntryWhenActive(bool);
>     bool useSecureKeyboardEntryWhenActive() const;
>diff --git a/Source/WebCore/html/HTMLLinkElement.cpp b/Source/WebCore/html/HTMLLinkElement.cpp
>index f378de5..d7bbf48 100644
>--- a/Source/WebCore/html/HTMLLinkElement.cpp
>+++ b/Source/WebCore/html/HTMLLinkElement.cpp
>@@ -181,7 +181,7 @@ void HTMLLinkElement::process()
> 
>     String type = m_type.lower();
> 
>-    if (!m_linkLoader.loadLink(m_relAttribute, type, m_sizes->toString(), m_url, document()))
>+    if (!m_linkLoader.loadLink(m_relAttribute, m_url, document()))
>         return;
> 
>     bool acceptIfTypeContainsTextCSS = document()->page() && document()->page()->settings() && document()->page()->settings()->treatsAnyTextCSSLinkAsStylesheet();
>@@ -418,6 +418,16 @@ String HTMLLinkElement::type() const
>     return getAttribute(typeAttr);
> }
> 
>+IconType HTMLLinkElement::iconType() const
>+{
>+    return m_relAttribute.m_iconType;
>+}
>+
>+String HTMLLinkElement::iconSizes() const
>+{
>+    return m_sizes->toString();
>+}
>+
> void HTMLLinkElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls) const
> {
>     HTMLElement::addSubresourceAttributeURLs(urls);
>diff --git a/Source/WebCore/html/HTMLLinkElement.h b/Source/WebCore/html/HTMLLinkElement.h
>index bb69e12..e330e73 100644
>--- a/Source/WebCore/html/HTMLLinkElement.h
>+++ b/Source/WebCore/html/HTMLLinkElement.h
>@@ -55,6 +55,11 @@ public:
> 
>     String type() const;
> 
>+    IconType iconType() const;
>+
>+    // the icon size string as parsed from the HTML attribute
>+    String iconSizes() const;
>+
>     CSSStyleSheet* sheet() const { return m_sheet.get(); }
> 
>     bool styleSheetIsLoading() const;
>diff --git a/Source/WebCore/loader/LinkLoader.cpp b/Source/WebCore/loader/LinkLoader.cpp
>index af25a86..9fba52e 100644
>--- a/Source/WebCore/loader/LinkLoader.cpp
>+++ b/Source/WebCore/loader/LinkLoader.cpp
>@@ -89,14 +89,12 @@ void LinkLoader::notifyFinished(CachedResource* resource)
>     m_cachedLinkResource = 0;
> }
> 
>-bool LinkLoader::loadLink(const LinkRelAttribute& relAttribute, const String& type,
>-                          const String& sizes, const KURL& href, Document* document)
>+bool LinkLoader::loadLink(const LinkRelAttribute& relAttribute, const KURL& href, Document* document)
> {
>     // We'll record this URL per document, even if we later only use it in top level frames
>     if (relAttribute.m_iconType != InvalidIcon && href.isValid() && !href.isEmpty()) {
>         if (!m_client->shouldLoadLink()) 
>             return false;
>-        document->addIconURL(href.string(), type, sizes, relAttribute.m_iconType);
>     }
> 
>     if (relAttribute.m_isDNSPrefetch) {
>diff --git a/Source/WebCore/loader/LinkLoader.h b/Source/WebCore/loader/LinkLoader.h
>index 58b29a7..0b81319 100644
>--- a/Source/WebCore/loader/LinkLoader.h
>+++ b/Source/WebCore/loader/LinkLoader.h
>@@ -56,7 +56,7 @@ public:
>     virtual void notifyFinished(CachedResource*);
>     
>     void released();
>-    bool loadLink(const LinkRelAttribute&, const String& type, const String& sizes, const KURL&, Document*);
>+    bool loadLink(const LinkRelAttribute&, const KURL&, Document*);
> 
> private:
>     void linkLoadTimerFired(Timer<LinkLoader>*);
>diff --git a/Source/WebCore/loader/icon/IconController.cpp b/Source/WebCore/loader/icon/IconController.cpp
>index b4a72c6..95829be 100644
>--- a/Source/WebCore/loader/icon/IconController.cpp
>+++ b/Source/WebCore/loader/icon/IconController.cpp
>@@ -86,6 +86,7 @@ IconURLs IconController::urlsForTypes(int iconTypes)
>     if (m_frame->tree() && m_frame->tree()->parent())
>         return iconURLs;
>         
>+    // Start by adding the favicon from the current document if we have one, if not, use the default favicon.
>     if (iconTypes & Favicon && !appendToIconURLs(Favicon, &iconURLs))
>         iconURLs.append(defaultURL(Favicon));
> 
>diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index 44ef4d5..8f4ac0a 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -1130,6 +1130,18 @@ String Internals::counterValue(Element* element)
>     return counterValueForElement(element);
> }
> 
>+PassRefPtr<DOMStringList> Internals::iconURLs(Document* document) const
>+{
>+    Vector<IconURL> iconURLs = document->iconURLs();
>+    RefPtr<DOMStringList> stringList = DOMStringList::create();
>+
>+    Vector<IconURL>::const_iterator iter(iconURLs.begin());
>+    for (; iter != iconURLs.end(); ++iter)
>+        stringList->append(iter->m_iconURL.string());
>+
>+    return stringList.release();
>+}
>+
> #if ENABLE(FULLSCREEN_API)
> void Internals::webkitWillEnterFullScreenForElement(Document* document, Element* element)
> {
>diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
>index cac206b..7c90484 100644
>--- a/Source/WebCore/testing/Internals.h
>+++ b/Source/WebCore/testing/Internals.h
>@@ -183,6 +183,7 @@ public:
> #endif
> 
>     String counterValue(Element*);
>+    PassRefPtr<DOMStringList> iconURLs(Document*) const;
> 
> #if ENABLE(FULLSCREEN_API)
>     void webkitWillEnterFullScreenForElement(Document*, Element*);
>diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
>index b3a249f..ce0a4a4 100644
>--- a/Source/WebCore/testing/Internals.idl
>+++ b/Source/WebCore/testing/Internals.idl
>@@ -157,6 +157,7 @@ module window {
>         [Conditional=INSPECTOR] sequence<String> consoleMessageArgumentCounts(in Document document);
> 
>         DOMString counterValue(in Element element);
>+        DOMString[] iconURLs(in Document document);
> 
> #if defined(ENABLE_FULLSCREEN_API) && ENABLE_FULLSCREEN_API
>         void webkitWillEnterFullScreenForElement(in Document document, in Element element);
>diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
>index 067b988..b78888e 100644
>--- a/Source/WebKit2/ChangeLog
>+++ b/Source/WebKit2/ChangeLog
>@@ -1,3 +1,12 @@
>+2012-06-26  Pete Williamson  <petewil@google.com>
>+
>+        Export the iconURL list to make it available to the Internals class for testing
>+        https://bugs.webkit.org/show_bug.cgi?id=88665
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * win/WebKit2.def: export the DocumentL::iconURLs function
>+
> 2012-06-26  Christophe Dumez  <christophe.dumez@intel.com>
> 
>         [WK2][GTK] Uninitialized variable in TextCheckerGtk.cpp
>diff --git a/Source/WebKit2/win/WebKit2.def b/Source/WebKit2/win/WebKit2.def
>index ed03bcc..3f7ba2d 100644
>--- a/Source/WebKit2/win/WebKit2.def
>+++ b/Source/WebKit2/win/WebKit2.def
>@@ -270,3 +270,4 @@ EXPORTS
>         ?jsStringSlowCase@WebCore@@YA?AVJSValue@JSC@@PAVExecState@3@AAV?$HashMap@PAVStringImpl@WTF@@V?$Weak@VJSString@JSC@@@JSC@@U?$PtrHash@PAVStringImpl@WTF@@@2@U?$HashTraits@PAVStringImpl@WTF@@@2@U?$HashTraits@V?$Weak@VJSString@JSC@@@JSC@@@2@@WTF@@PAVStringImpl@6@@Z        
>         ?registerURLSchemeAsBypassingContentSecurityPolicy@SchemeRegistry@WebCore@@SAXABVString@WTF@@@Z
>         ?removeURLSchemeRegisteredAsBypassingContentSecurityPolicy@SchemeRegistry@WebCore@@SAXABVString@WTF@@@Z
>+        ?iconURLs@Document@WebCore@@QAEABV?$Vector@UIconURL@WebCore@@$0A@@WTF@@XZ
>\ No newline at end of file
>diff --git a/Source/autotools/symbols.filter b/Source/autotools/symbols.filter
>index 4470def..7d63b2c 100644
>--- a/Source/autotools/symbols.filter
>+++ b/Source/autotools/symbols.filter
>@@ -37,6 +37,7 @@ _ZN7WebCore4toJSEPN3JSC9ExecStateEPNS_17JSDOMGlobalObjectEPNS_5RangeE;
> _ZN7WebCore5Range6createEN3WTF10PassRefPtrINS_8DocumentEEENS2_INS_4NodeEEEiS6_i;
> _ZN7WebCore5RangeD1Ev;
> _ZN7WebCore8Document36updateLayoutIgnorePendingStylesheetsEv;
>+_ZN7WebCore8Document8iconURLsEv;
> _ZN7WebCore9HTMLNames8inputTagE;
> _ZN7WebCore9HTMLNames11textareaTagE;
> _ZN7WebCore10JSDocument10putVirtualEPN3JSC9ExecStateERKNS1_10IdentifierENS1_7JSValueERNS1_15PutPropertySlotE;

Actions: View | Formatted Diff | Diff

Attachments on bug 88665: 146608 | 147382 | 147447 | 147452 | 147602 | 147604 | 147610 | 147678 | 148447 | 148598 | 148601 | 148697 | 148845 | 148889 | 149055 | 149070 | 149074 | 149097 | 149142 | 149159 | 149312 | 149320 | 149331 | 149353 | 149563 | 149766 | 149838 | 149850 | 149851 | 149857 | 149972 | 150199 | 151330 | 152323