[webkit-reviews] review granted: [Bug 111022] Cache partitioning does not affect iframe MainResources : [Attachment 190626] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Jeffrey Pfau
<jeffrey at endrift.com>'s request for review:
Bug 111022: Cache partitioning does not affect iframe MainResources
https://bugs.webkit.org/show_bug.cgi?id=111022

Attachment 190626: Patch
https://bugs.webkit.org/attachment.cgi?id=190626&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review


r=me

>>> Source/WebCore/loader/FrameLoader.cpp:1199
>>> +	 if (m_frame->tree()->top() != m_frame &&
m_frame->tree()->top()->document())
>> 
>> How can m_frame->tree()->top()->document() be zero?
> 
> I've hit cases where a frame's document was 0, but I wasn't sure when exactly
those were. Maybe it's only for subframes? That would render that clause
useless. I just wanted to make sure I didn't accidentally run into null pointer
derefs.

This code would read clearer if "m_frame->tree()->top() != m_frame" were
extracted into a method called isSubFrame():

bool Frame::isSubFrame()
{
    return tree()->top() != self;
}

Not necessary for this patch, but please consider a follow-up patch.  (Or
implement the opposite:  Frame::isTopFrame().)

--

Also, it would be nice not to call "m_frame->tree()->top()->document()" twice,
so maybe you could do this:

    if (m_frame->isSubFrame()) {
	if (Document *topDocument = m_frame->tree()->top()->document())
	   
request.setCachePartition(topDocument->securityOrigin()->cachePartition());
    }