[webkit-reviews] review granted: [Bug 41165] FrameLoader cleanup : Remove m_URL member : [Attachment 65597] split patch - remove m_URL and have FrameLoader::url() call Document::url()

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 41165: FrameLoader cleanup : Remove m_URL member
https://bugs.webkit.org/show_bug.cgi?id=41165

Attachment 65597: split patch - remove m_URL and have FrameLoader::url() call
Document::url()
https://bugs.webkit.org/attachment.cgi?id=65597&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=65597&action=prettypatch

> WebCore/loader/FrameLoader.cpp:519
> +    m_workingURL = url;
> +    if (m_workingURL.protocolInHTTPFamily() &&
!m_workingURL.host().isEmpty() && m_workingURL.path().isEmpty())
> +	   m_workingURL.setPath("/");
It seems like m_URL no longer has this modification applied.  Does this code
actually do anything?  I would have expected KURL to normalize empty paths to
include "/"...

> WebCore/loader/FrameLoader.cpp:724
>	       if (!iconDatabase()->iconDataKnownForIconURL(urlString)) {
>		   LOG(IconDatabase, "Told not to load icon %s but icon data is
not yet available - registering for notification and requesting load from
disk", urlString.ascii().data());
>		   m_client->registerForIconNotification();
> -		   iconDatabase()->iconForPageURL(m_URL.string(), IntSize(0,
0));
> +		  
iconDatabase()->iconForPageURL(m_frame->document()->url().string(), IntSize(0,
0));
why not url() in all these places?

> WebCore/loader/FrameLoader.cpp:1829
> -    LOG(PageCache, "WebCoreLoading %s: About to commit provisional load from
previous URL '%s' to new URL '%s'",
m_frame->tree()->name().string().utf8().data(), m_URL.string().utf8().data(), 
> +    LOG(PageCache, "WebCoreLoading %s: About to commit provisional load from
previous URL '%s' to new URL '%s'",
m_frame->tree()->name().string().utf8().data(), 
> +	   m_frame->document() ?
m_frame->document()->url().string().utf8().data() : "", 
I presume you checked that document can actually be null here.

> WebCore/loader/FrameLoader.cpp:1870
> -    LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to
URL %s", m_frame->tree()->name().string().utf8().data(),
m_URL.string().utf8().data());
> +    LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to
URL %s", m_frame->tree()->name().string().utf8().data(),
> +	   m_frame->document() ?
m_frame->document()->url().string().utf8().data() : "");
and here.

> WebCore/loader/FrameLoader.cpp:2113
>      m_workingURL = url;
By the way, m_workingURL seems wrong too.  Shouldn't that state be held in the
DocumentLoader?

> LayoutTests/fast/dom/resources/a.html:9
> +    else if (window.layoutTestController)
> +	layoutTestController.notifyDone();
Indent issue?

> LayoutTests/ChangeLog:6
> +	   Update a test that busy-waits when it shouldn't.
> +	   https://bugs.webkit.org/show_bug.cgi?id=41165.
This doesn't appear to belong to this patch.