With the recently added cache partitioning patch, subresources are properly getting separated into the relevant partitions, but iframe MainResources are not currently being partitioned.
Created attachment 190626 [details] Patch
Comment on attachment 190626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > 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? > Source/WebCore/loader/FrameLoader.cpp:1200 > + request.setCachePartition(m_frame->tree()->top()->document()->securityOrigin()->cachePartition()); m_frame->tree()->top()->document() <-- I'm not sure that's idiomatically the best way to find the topDocument. Maybe it doesn't matter now that we don't have that disconnected frame stuff.
(In reply to comment #2) > (From update of attachment 190626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > > > 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. > > Source/WebCore/loader/FrameLoader.cpp:1200 > > + request.setCachePartition(m_frame->tree()->top()->document()->securityOrigin()->cachePartition()); > > m_frame->tree()->top()->document() <-- I'm not sure that's idiomatically the best way to find the topDocument. Maybe it doesn't matter now that we don't have that disconnected frame stuff. I was under the impression that Document::topDocument() did that, but it doesn't seem to. Did that change at some point? I'm all for a better idiom, but I didn't know of one.
Frame::document() is only ever zero while we're initializing the frame. The concept of a disconnected frame is gone from the codebase, which is probably why you don't see it in topDocument.
Comment on attachment 190626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > LayoutTests/http/tests/cache/partitioned-cache-iframe-expected.txt:1 > +CONSOLE MESSAGE: line 18: FAIL Could you please make this test display produce more positive when it passes?
produce *something* more positive
Comment on attachment 190626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review >> LayoutTests/http/tests/cache/partitioned-cache-iframe-expected.txt:1 >> +CONSOLE MESSAGE: line 18: FAIL > > Could you please make this test display produce more positive when it passes? Or rather, it's not clear to me why the test is expected to fail now - ChangeLog does not explain this.
Perhaps I should change it to say "The cache is not partitioned" instead of "FAIL", as that's what's going on here. This test does not pass, per se, in any shipping ports at the moment, hence why it says "FAIL" right now.
Comment on attachment 190626 [details] Patch 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()); }
(In reply to comment #9) > (From update of attachment 190626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > > r=me Plus the changes that Alexey already mentioned (that you said you had already made locally).
Comment on attachment 190626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review >>>> 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()); > } If we added functions like that they should be in FrameTree, not on Frame itself. frame->tree()->isTop() and/or frame->tree()->isSubframe()
(In reply to comment #11) > (From update of attachment 190626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > > >>>> 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()); > > } > > If we added functions like that they should be in FrameTree, not on Frame itself. > > frame->tree()->isTop() > > and/or > > frame->tree()->isSubframe() This idiom appears to also be used in MainResourceLoader::willSendRequest, so it should likely be done in a follow up patch (although it holds onto frame->tree()->top() and then uses it later in the function in much the same way I could be doing in this patch--it only saves one call to top()).
<rdar://problem/13342678>
Committed r144694: <http://trac.webkit.org/changeset/144694>