[webkit-reviews] review granted: [Bug 41155] In FrameLoader, m_URL is not set before calling client dispatchDidCommitLoad for cached pages : [Attachment 67832] Final patch to be committed

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dinu Jacob <dinu.jacob at Nokia.com>'s
request for review:
Bug 41155: In FrameLoader, m_URL is not set before calling client
dispatchDidCommitLoad for cached pages
https://bugs.webkit.org/show_bug.cgi?id=41155

Attachment 67832: Final patch to be committed
https://bugs.webkit.org/attachment.cgi?id=67832&action=review

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

>>> WebCore/ChangeLog:11
>>> +	     Only qt
>> 
>> What does this cryptic change log line mean? The patch is a change to code
that is shared by all platforms. Do you mean to say something like: “This
change is harmless for all other platforms and helpful for Qt.”?
> 
> I replaced the line that asked for tests to indicate that tests were written
specifically for Qt. Should I just remove this line?

Yes.

>>> WebCore/loader/FrameLoader.cpp:1852
>>> +	     // For non-cached HTML pages, these methods are called in
receivedFirstData().
>> 
>> This comment, which you moved here with the code, is now confusing. In the
old code, it was referring to a line of code just two lines up where
receivedFirstData is called. In this new code, it’s hard to understand what
that means exactly.
> 
> I will remove this comment

Thanks.