Bug 85302

Created attachment 139662 [details] Patch

Comment on attachment 139662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139662&action=review > Source/WebCore/ChangeLog:20 > + were still to have this seamles styling. I've added additional testing for this case. typo: seamles > Source/WebCore/html/HTMLIFrameElement.cpp:120 > + return hasAttribute(seamlessAttr) && contentDocument() && contentDocument()->mayDisplaySeamlessWithParent(); Doubt it matters, but you might want to put the hasAttribute check last since it's slower than the other two null-checks just to be safe.

View in context: https://bugs.webkit.org/attachment.cgi?id=139662&action=review > Source/WebCore/dom/SecurityContext.h:70 > + bool mayDisplaySeamlessWithParent() const { return m_mayDisplaySeamlessWithParent; } You might also consider always returning false here until you're ready to rock and roll with seamless. > Source/WebCore/html/HTMLIFrameElement.idl:33 > + attribute [Reflect] boolean seamless; You might consider adding this attribute last since this is what folks will use to detect whether this feature is present.

Comment on attachment 139662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139662&action=review >> Source/WebCore/ChangeLog:20 >> + were still to have this seamles styling. I've added additional testing for this case. > > typo: seamles Fixed. >> Source/WebCore/html/HTMLIFrameElement.cpp:120 >> + return hasAttribute(seamlessAttr) && contentDocument() && contentDocument()->mayDisplaySeamlessWithParent(); > > Doubt it matters, but you might want to put the hasAttribute check last since it's slower than the other two null-checks just to be safe. OK, done.

(In reply to comment #3) > View in context: https://bugs.webkit.org/attachment.cgi?id=139662&action=review > > > Source/WebCore/dom/SecurityContext.h:70 > > + bool mayDisplaySeamlessWithParent() const { return m_mayDisplaySeamlessWithParent; } > > You might also consider always returning false here until you're ready to rock and roll with seamless. That would make testing impossible unfortunately. :) > > Source/WebCore/html/HTMLIFrameElement.idl:33 > > + attribute [Reflect] boolean seamless; > > You might consider adding this attribute last since this is what folks will use to detect whether this feature is present. Sure. I'll leave that out. It will make some tests fail, but won't affect the rest of the patches.

Comment on attachment 139662 [details] Patch Attachment 139662 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12588658 New failing tests: svg/in-html/by-reference.html

Created attachment 139675 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 139677 [details] Patch for landing

--- /tmp/layout-test-results/http/tests/security/sandboxed-iframe-modify-self-expected.txt +++ /tmp/layout-test-results/http/tests/security/sandboxed-iframe-modify-self-actual.txt @@ -1,3 +1,5 @@ +CONSOLE MESSAGE: Unsafe JavaScript attempt to initiate a navigation change for frame with URL http://127.0.0.1:8000/security/sandboxed-iframe-form-top.html from frame with URL http://127.0.0.1:8000/security/resources/sandboxed-iframe-form-top.html. + CONSOLE MESSAGE: Unsafe JavaScript attempt to access frame with URL http://127.0.0.1:8000/security/sandboxed-iframe-modify-self.html from frame with URL http://127.0.0.1:8000/security/resources/sandboxed-iframe-modify-self.html. Domains, protocols and ports must match. This is a "sanity" test case to verify that a sandboxed frame cannot break out of its sandbox by modifying its own sandbox attribute. Two attempts are made: Dr. Barth: does that ring any bells for you?

Maybe I just didn't update the http expected files before uploading the first patch. :) We'll see how the CQ handles my updated one.

Although that looks like it might be output bleed from the previous test?

Comment on attachment 139677 [details] Patch for landing Rejecting attachment 139677 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: sts/fast/frames/seamless/seamless-inherited-origin.html patching file LayoutTests/fast/frames/seamless/seamless-inline-expected.txt patching file LayoutTests/fast/frames/seamless/seamless-min-max-expected.txt patching file LayoutTests/fast/frames/seamless/seamless-sandbox-flag-expected.txt patching file LayoutTests/fast/frames/seamless/seamless-sandbox-flag.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12597516

Created attachment 139683 [details] Patch for landing

Comment on attachment 139683 [details] Patch for landing Attachment 139683 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12589664 New failing tests: svg/in-html/by-reference.html

Created attachment 139697 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Crazy. svg/in-html/by-reference.html uses "seamless". It doesn't want to though, so I'll remove the mention.

Created attachment 139698 [details] Patch for landing

Comment on attachment 139698 [details] Patch for landing Clearing flags on attachment: 139698 Committed r115773: <http://trac.webkit.org/changeset/115773>

All reviewed patches have been landed. Closing bug.

Comment on attachment 139698 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=139698&action=review > Source/WebCore/css/html.css:986 > +iframe:not([seamless]) { This just occurred to me... :not is super slow. I think :not of an attribute selector might be especially slow. Might want to make sure this doesn't regress performance on pages that do or don't have seamless iframes.

Antti might have thoughts on whether it's OK performance-wise to use iframe:not[seamless] in html.css.

The bots will have thoughts. :) Until then, I'm not worried.

iframes are not that frequent and :not is not that bad. Should be ok. (In reply to comment #22) > The bots will have thoughts. :) Until then, I'm not worried. That's not a good way to think about the performance implications of patches. The bot coverage is far from perfect.

Clearly we should make the bots better then. :) As Knuth said, "Premature optimization is the root of all evil." I refuse to be afraid. :) Thank you both for your thoughts.

(In reply to comment #24) > Clearly we should make the bots better then. :) > > As Knuth said, "Premature optimization is the root of all evil." I refuse to be afraid. :) > > Thank you both for your thoughts. There's being afraid and there's thinking about your code. Surely you wouldn't add an n-factorial algorithm in performance critical code that happens to not be covered by the bots (to take an extreme case). I don't think we can make a blanket rule that you don't need to think about performance if the bots don't cover that code. I'd be OK with saying this if your patch came with a performance test for the code in question.

(In reply to comment #25) > (In reply to comment #24) > > Clearly we should make the bots better then. :) > > > > As Knuth said, "Premature optimization is the root of all evil." I refuse to be afraid. :) > > > > Thank you both for your thoughts. > > There's being afraid and there's thinking about your code. And with that, you assume I don't think about my code. :) Surely you don't mean to sound that way. :) > I'd be OK with saying this if your patch came with a performance test for the code in question. And that, my friend, would be again, FUD, and premature optimization. Why should we assume that our existing perf tests do not cover this case? How about the 1000 other changes which we check in weekly, for which you or I or anitti didn't think about the perf implications? There is no way any of us are a better predictor than the actual tests themselves. :)

I look forward to discussing the more interesting aspects of seamless with you both. There are many tricky places in the code, but this patch (and the previous one where I only added tests!) is certainly not it. :)