Bug 86608

Summary:

Add seamless layout code (and pass all the remaining seamless tests)

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

New Bugs

Assignee:

Eric Seidel (no email) <eric>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cmarcelo, dbates, dglazkov, hyatt, inferno, jamesr, jchaffraix, koivisto, macpherson, menard, ojan, tony, webkit.review.bot, zalan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

87363

Bug Blocks:

45950

Attachments:

Description	Flags
Patch	webkit-ews: commit-queue-
Fix release builds and simplify RenderIFrame::layout	none
Archive of layout-test-results from ec2-cr-linux-04	none
Updated per emil's suggestions	none
Archive of layout-test-results from ec2-cr-linux-04	none
Information about the crash	none
now with less crash	none
now with more splelnig fixes	none
Fixed per Ojan's review	none
Archive of layout-test-results from ec2-cq-02	none
Archive of layout-test-results from ec2-cr-linux-02	none
Patch for landing	none
Patch for landing	none

Eric Seidel (no email)

Reported 2012-05-16 04:37:37 PDT

Add seamless layout code (and pass all the remaining seamless tests)

Attachments
Patch (30.61 KB, patch) 2012-05-16 05:08 PDT, Eric Seidel (no email)	webkit-ews: commit-queue-	Details Formatted Diff Diff
Fix release builds and simplify RenderIFrame::layout (30.68 KB, patch) 2012-05-16 05:40 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (595.81 KB, application/zip) 2012-05-16 10:03 PDT, WebKit Review Bot	no flags	Details
Updated per emil's suggestions (32.18 KB, patch) 2012-05-16 17:31 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (1008.91 KB, application/zip) 2012-05-16 18:27 PDT, WebKit Review Bot	no flags	Details
Information about the crash (10.82 KB, text/plain) 2012-05-17 01:45 PDT, Eric Seidel (no email)	no flags	Details
now with less crash (32.43 KB, patch) 2012-05-17 02:38 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
now with more splelnig fixes (32.74 KB, patch) 2012-05-17 14:16 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Fixed per Ojan's review (32.95 KB, patch) 2012-05-21 16:27 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cq-02 (696.30 KB, application/zip) 2012-05-21 23:03 PDT, WebKit Review Bot	no flags	Details
Archive of layout-test-results from ec2-cr-linux-02 (488.86 KB, application/zip) 2012-05-22 01:35 PDT, WebKit Review Bot	no flags	Details
Patch for landing (31.40 KB, patch) 2012-05-23 16:38 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch for landing (31.40 KB, patch) 2012-05-23 16:41 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2012-05-16 05:08:00 PDT

Created attachment 142225 [details] Patch

Eric Seidel (no email)

Comment 2 2012-05-16 05:10:07 PDT

This is the real interesting part of seamless. I'm very interested in hearing from other layout experts on if they agree with my implementation approach here.

Eric Seidel (no email)

Comment 3 2012-05-16 05:10:37 PDT

I should note that I also filed a w3c bug when preparing this patch: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17076 about how best to handle scrollbars, etc.

Eric Seidel (no email)

Comment 4 2012-05-16 05:17:54 PDT

Comment on attachment 142225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142225&action=review > LayoutTests/fast/frames/seamless/resources/two-inline-blocks.html:6 > +/* FIXME: This vertical-align should not be needed, but without it the contentHeight() > +of the document is reported as 4px too large, likely due to implied line decent? */ > +div { vertical-align: top; } This is kinda an odd bug. Without this the FrameView::contentHeight() reports 4px larger than I expected. It's possible there is a better way to work around this. the <html> element itself does not grow to include this presumed descent. I guess contentHeight() includes this implicit line "overflow" for descent?

Eric Seidel (no email)

Comment 5 2012-05-16 05:22:53 PDT

Comment on attachment 142225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142225&action=review > Source/WebCore/rendering/RenderIFrame.cpp:186 > + // So we set our height to min(max-height, UINT_MAX) to avoid generating unecessary > + // vertical scrollbars then we'll shrink the height after child layout. > + LayoutUnit maxHeight = MAX_LAYOUT_UNIT; > + if (!style()->logicalMaxHeight().isUndefined()) > + maxHeight = std::min<LayoutUnit>(maxHeight, style()->logicalMaxHeight().value()); > + setLogicalHeight(maxHeight); If folks have better suggestions as to how to get this behavior, I'd very much like to hear them. It's possible I could use the supressScrollbars() functionality, but that seems tied to firstLayout and I seem skeptical that it's the right approach. > Source/WebCore/rendering/RenderIFrame.cpp:190 > + FrameView* childFrameView = static_cast<FrameView*>(widget()); > + childFrameView->layout(); This is likely not needed. The layout actually happens inside updateWidgetPosition(). :) But I'm matching the RenderFrameBase::layoutFrameFlattening path here. Note that I don't have any of the computePrefWidths code here like frame-flattening does, since those calls are made as part of computeLogicalWidth() as per normal layout. > Source/WebCore/rendering/RenderIFrame.cpp:198 > + computeLogicalHeight(); It may be cleaner to split out a separate layoutAsSeamless() function, similar to what layoutWithFlattening has done. We also might consider just splitting seamless frames into a wholly separate render object.

Early Warning System Bot

Comment 6 2012-05-16 05:22:59 PDT

Comment on attachment 142225 [details] Patch Attachment 142225 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12715272

Early Warning System Bot

Comment 7 2012-05-16 05:27:41 PDT

Comment on attachment 142225 [details] Patch Attachment 142225 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12709410

Eric Seidel (no email)

Comment 8 2012-05-16 05:33:51 PDT

Comment on attachment 142225 [details] Patch Uploading a new patch to fix release builds.

Eric Seidel (no email)

Comment 9 2012-05-16 05:40:43 PDT

Created attachment 142230 [details] Fix release builds and simplify RenderIFrame::layout

WebKit Review Bot

Comment 10 2012-05-16 10:03:34 PDT

Comment on attachment 142230 [details] Fix release builds and simplify RenderIFrame::layout Attachment 142230 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12717257 New failing tests: http/tests/security/javascriptURL/javascriptURL-in-new-iframe.html http/tests/security/javascriptURL/xss-ALLOWED-from-javascript-url-to-javscript-url.html http/tests/security/seamless/seamless-cross-origin.html http/tests/security/seamless/seamless-sandbox-srcdoc.html http/tests/security/javascriptURL/xss-ALLOWED-to-javascript-url-from-javscript-url.html fast/frames/iframe-option-crash.xhtml http/tests/security/javascriptURL/xss-ALLOWED-from-javascript-url-sub-frame-2-level.html http/tests/appcache/credential-url.html fast/frames/javascript-url-for-deleted-frame.html fast/frames/adopt-from-created-document.html http/tests/appcache/different-scheme.html http/tests/security/javascriptURL/xss-ALLOWED-to-javascript-url-sub-frame-2-level.html plugins/iframe-shims.html

WebKit Review Bot

Comment 11 2012-05-16 10:03:39 PDT

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

Emil A Eklund

Comment 12 2012-05-16 10:27:00 PDT

Comment on attachment 142230 [details] Fix release builds and simplify RenderIFrame::layout View in context: https://bugs.webkit.org/attachment.cgi?id=142230&action=review > Source/WebCore/rendering/RenderIFrame.cpp:59 > + LayoutUnit border = borderTop() + borderBottom(); borderTop and borderBottom both return ints so no need to change this. > Source/WebCore/rendering/RenderIFrame.cpp:128 > + // FIXME: Is this always a valid cast? What about plugins? How about adding an assert instead of the FIXME? ASSERT(widget()->isFrameView()); > Source/WebCore/rendering/RenderIFrame.cpp:168 > + // So we set our height to min(max-height, UINT_MAX) to avoid generating unecessary s/UNIT_MAX/MAX_LAYOUT_UNIT/ > Source/WebCore/rendering/RenderIFrame.cpp:188 > + ASSERT(!childRoot->firstChild() || !childRoot->firstChild()->firstChild() || !childRoot->firstChild()->firstChild()->needsLayout()); This line probably needs an explanation.

Eric Seidel (no email)

Comment 13 2012-05-16 17:31:15 PDT

Created attachment 142378 [details] Updated per emil's suggestions

Eric Seidel (no email)

Comment 14 2012-05-16 17:32:19 PDT

I'm still diagnosing the XSS null pointer crashes. I'm not sure what I'm tripping over yet.

Eric Seidel (no email)

Comment 15 2012-05-16 17:32:50 PDT

Comment on attachment 142378 [details] Updated per emil's suggestions Due to the XSS null pointer crash, this won't be ready for commit yet, but certainly could have another round of review from layout experts!

WebKit Review Bot

Comment 16 2012-05-16 18:27:45 PDT

Comment on attachment 142378 [details] Updated per emil's suggestions Attachment 142378 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12716418 New failing tests: http/tests/security/javascriptURL/javascriptURL-in-new-iframe.html fast/frames/iframe-option-crash.xhtml fast/frames/javascript-url-for-deleted-frame.html http/tests/security/javascriptURL/xss-ALLOWED-to-javascript-url-from-javscript-url.html http/tests/security/javascriptURL/xss-ALLOWED-from-javascript-url-to-javscript-url.html http/tests/security/javascriptURL/xss-ALLOWED-from-javascript-url-sub-frame-2-level.html http/tests/appcache/credential-url.html fast/frames/adopt-from-created-document.html plugins/iframe-shims.html http/tests/security/javascriptURL/xss-ALLOWED-to-javascript-url-sub-frame-2-level.html http/tests/appcache/different-scheme.html

WebKit Review Bot

Comment 17 2012-05-16 18:27:51 PDT

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

Eric Seidel (no email)

Comment 18 2012-05-17 01:45:11 PDT

Created attachment 142442 [details] Information about the crash

Eric Seidel (no email)

Comment 19 2012-05-17 01:47:57 PDT

I think the crash comes because setDocument() does not expect the attach() to end up recusing through and resolving style on the parent document, which finds that it doesn't know if it can access the child document at this exact moment, as it doesn't yet have a security origin?

Eric Seidel (no email)

Comment 20 2012-05-17 01:53:11 PDT

I wonder if we could trigger this crash another way. What seems to be happening is that we're recalculating style on the parent document in between the time that the child document has been created and when it gets its security origin. Unclear if Frame::setDocument really wants to be attaching the document at this time. Stopping that would stop the crash. I could also remove the "resolve style on our parent before resolving on ourself" logic for now. It may not be necessary. I also could add a null check to either SecurityOrigin::canAccess or JSDOMWindowBase::allowsAccessFromPrivate, both of which could be reasonable.

Eric Seidel (no email)

Comment 21 2012-05-17 02:38:31 PDT

Created attachment 142445 [details] now with less crash

Eric Seidel (no email)

Comment 22 2012-05-17 02:56:15 PDT

Comment on attachment 142445 [details] now with less crash View in context: https://bugs.webkit.org/attachment.cgi?id=142445&action=review > Source/WebCore/rendering/RenderIFrame.cpp:197 > + RenderPart::computeLogicalWidth(); > + RenderPart::computeLogicalHeight(); These don't need to be explicit calls to RenderPart::, but I left them the way they were. > Source/WebCore/rendering/RenderIFrame.cpp:-120 > - RenderPart::layout(); RenderPart::layout() doesn't actually do anything. :) So I removed this useless call.

Daniel Bates

Comment 23 2012-05-17 13:30:49 PDT

Comment on attachment 142445 [details] now with less crash View in context: https://bugs.webkit.org/attachment.cgi?id=142445&action=review I briefly looked over this patch and noticed some issues. I didn't verify the correctness of the layout logic. > Source/WebCore/ChangeLog:14 > + - Seamless support shrink-wrap negotation when the iframe is inline. Nit: negotation => negotiation (Is it a negotiation or an algorithm?) Nit: This sentence doesn't read well. Maybe we need to make some words plural? like support? > Source/WebCore/dom/Document.cpp:1726 > + // hits a null-derefence due to security code always assuming the document has a SecurityOrigin. Nit: null-derefence => null-dereference > Source/WebCore/rendering/RenderIFrame.cpp:129 > + ASSERT(widget()->isFrameView()); We need to check that widget() is non-null here since the rest of this function assumes widget() can return a null pointer. > Source/WebCore/rendering/RenderIFrame.cpp:169 > + // So we set our height to min(max-height, MAX_LAYOUT_UNIT) to avoid generating unecessary Nit: unecessary => unnecessary > Source/WebCore/rendering/RenderIFrame.cpp:179 > + FrameView* childFrameView = static_cast<FrameView*>(widget()); Are we guaranteed that childFrameView will be non-null? Notice that you check that childFrameView is non-null on line 186. > Source/WebCore/rendering/RenderIFrame.cpp:186 > + RenderView* childRoot = childFrameView ? static_cast<RenderView*>(childFrameView->frame()->contentRenderer()) : 0; See remark on line 179. > Source/WebCore/rendering/RenderIFrame.cpp:201 > return; > + } else if (isSeamless()) { Nit: This should be a "if" statement instead of an "else if" because of the early return above.

Eric Seidel (no email)

Comment 24 2012-05-17 14:11:17 PDT

(In reply to comment #23) > (From update of attachment 142445 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142445&action=review > > Source/WebCore/rendering/RenderIFrame.cpp:201 > > return; > > + } else if (isSeamless()) { > > Nit: This should be a "if" statement instead of an "else if" because of the early return above. I've left this as-is, as the early-return is wrong. I just don't have tests to prove its wrong. :)

Eric Seidel (no email)

Comment 25 2012-05-17 14:16:09 PDT

Created attachment 142553 [details] now with more splelnig fixes

Ojan Vafai

Comment 26 2012-05-18 00:38:26 PDT

Comment on attachment 142553 [details] now with more splelnig fixes View in context: https://bugs.webkit.org/attachment.cgi?id=142553&action=review I'm almost comfortable r+'ing this. Mainly, I'd like to see the vertical writing mode tests for the scary bit in layoutSeamlessly. You really should try to get Hyatt to take a quick look at this patch if you can. If nothing else, just have him verify that the code in layoutSeamlessly is sane, that's the only part that I'm wary of since it's doing something quite different from normal layout methods. > Source/WebCore/page/FrameView.cpp:2928 > + if (iframeRenderer->flattenFrame() || iframeRenderer->isSeamless()) Doesn't this change behavior for frame flattening if frameFlatteningEnabled is false? i.e. we will frame flatten even though the setting was false. I think you need to check !m_frame->settings() || !m_frame->settings()->frameFlatteningEnabled() here as well. > Source/WebCore/rendering/RenderIFrame.cpp:174 > + // Normally we would set our height to 0 before laying out our kids. > + // However RenderView (the root of the child document's rendering tree) > + // assumes its container is of a fixed size, not dynamically grown to it. > + // So we set our height to min(max-height, MAX_LAYOUT_UNIT) to avoid generating unnecessary > + // vertical scrollbars then we'll shrink the height after child layout. > + LayoutUnit maxHeight = MAX_LAYOUT_UNIT; > + if (!style()->logicalMaxHeight().isUndefined()) > + maxHeight = std::min<LayoutUnit>(maxHeight, style()->logicalMaxHeight().value()); > + setLogicalHeight(maxHeight); This scares me, e.g., I highly doubt it does the right thing with vertical writing mode. You could add some vertical writing mode tests to verify. Specifically, write tests with enough content to wrap into multiple lines *and* overflow horizontally out of their container. Make sure they render the same as a vertical writing mode test that uses a div. In fact, you could write these as reftests! :) > Source/WebCore/rendering/RenderIFrame.cpp:178 > + // Replaced elements do not respect padding, so we just add border to the child's height. I'm on the fence as to whether seamless iframes should respect padding. I think they probably should. It's not a big deal either way. Can you just add a FIXME for now? > Source/WebCore/rendering/RenderIFrame.cpp:204 > + // Note we do not return so as to share the layer and overflow updates below. Pet peeve nit: s/Note we/We/ :) > Source/WebCore/rendering/RenderIFrame.cpp:207 > + // No kids to layout as a replaced element. Grammar nit: No kids to layout as *this is* a replaced element.

Ojan Vafai

Comment 27 2012-05-18 00:40:48 PDT

Example vertical writing mode case to test: http://plexode.com/eval3/#ht=%3Cdiv%20style%3D%22width%3A%2050px%3B%20height%3A%20100px%3B%20border%3A%203px%20solid%22%3E%0A%3Cdiv%20style%3D%22-webkit-writing-mode%3Avertical-lr%3B%20background-color%3A%20red%22%3ELorem%20ipsum%20dolor%20and%20some%20other%20latin...%3C%2Fdiv%3E%0A%3C%2Fdiv%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

Eric Seidel (no email)

Comment 28 2012-05-18 15:04:07 PDT

Comment on attachment 142553 [details] now with more splelnig fixes View in context: https://bugs.webkit.org/attachment.cgi?id=142553&action=review >> Source/WebCore/page/FrameView.cpp:2928 >> + if (iframeRenderer->flattenFrame() || iframeRenderer->isSeamless()) > > Doesn't this change behavior for frame flattening if frameFlatteningEnabled is false? i.e. we will frame flatten even though the setting was false. I think you need to check !m_frame->settings() || !m_frame->settings()->frameFlatteningEnabled() here as well. RenderIframe::flattenFrame() already checks the setting. But I'm happy to add a comment to that effect? If we were flattening iframes when we shouldn't a zillion tests would fail. :) >> Source/WebCore/rendering/RenderIFrame.cpp:174 >> + setLogicalHeight(maxHeight); > > This scares me, e.g., I highly doubt it does the right thing with vertical writing mode. You could add some vertical writing mode tests to verify. Specifically, write tests with enough content to wrap into multiple lines *and* overflow horizontally out of their container. Make sure they render the same as a vertical writing mode test that uses a div. In fact, you could write these as reftests! :) I'm not sure what you mean. The fact that I'm setting logical height instead of height? Or do you mean how I'm tryign to avoid scrollbars but only doing so in teh height direction? >> Source/WebCore/rendering/RenderIFrame.cpp:178 >> + // Replaced elements do not respect padding, so we just add border to the child's height. > > I'm on the fence as to whether seamless iframes should respect padding. I think they probably should. It's not a big deal either way. Can you just add a FIXME for now? Sure. Done.

Ojan Vafai

Comment 29 2012-05-18 15:21:05 PDT

Comment on attachment 142553 [details] now with more splelnig fixes View in context: https://bugs.webkit.org/attachment.cgi?id=142553&action=review >>> Source/WebCore/page/FrameView.cpp:2928 >>> + if (iframeRenderer->flattenFrame() || iframeRenderer->isSeamless()) >> >> Doesn't this change behavior for frame flattening if frameFlatteningEnabled is false? i.e. we will frame flatten even though the setting was false. I think you need to check !m_frame->settings() || !m_frame->settings()->frameFlatteningEnabled() here as well. > > RenderIframe::flattenFrame() already checks the setting. But I'm happy to add a comment to that effect? If we were flattening iframes when we shouldn't a zillion tests would fail. :) oic. That's fine either way then. Are there really many tests for frame flattening? :) >>> Source/WebCore/rendering/RenderIFrame.cpp:174 >>> + setLogicalHeight(maxHeight); >> >> This scares me, e.g., I highly doubt it does the right thing with vertical writing mode. You could add some vertical writing mode tests to verify. Specifically, write tests with enough content to wrap into multiple lines *and* overflow horizontally out of their container. Make sure they render the same as a vertical writing mode test that uses a div. In fact, you could write these as reftests! :) > > I'm not sure what you mean. The fact that I'm setting logical height instead of height? Or do you mean how I'm tryign to avoid scrollbars but only doing so in teh height direction? Setting logicalHeight is definitely correct. The fact that you're setting height to non-zero is what concerns me. Here's a case I'm worried we would do the wrong thing with text wrapping: http://plexode.com/eval3/#ht=%3Cdiv%20style%3D%22-webkit-writing-mode%3A%20vertical-lr%3B%20outline%3A%202px%20solid%20blue%3B%20width%3A%20100px%22%3E%0A%20%20%20%20%3Cdiv%20style%3D%22-webkit-writing-mode%3A%20horizontal-tb%3B%20background-color%3A%20red%22%3E%0A%20%20%20%20%20%20%20%20foo%20bar%20baz%20foo%0A%20%20%20%20%3C%2Fdiv%3E%0A%3C%2Fdiv%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1 I'm worried that with the above code we'll not wrap the text at all, where we should be wrapping at 100px. A test case would help appease my worries, but it really would be good to get Hyatt's signoff as well. I don't think we need to block committing this patch on Hyatt review if you add a test-case like the one above, but at least put in a FIXME that this might do the wrong thing until we can have Hyatt verify it's sanity.

Eric Seidel (no email)

Comment 30 2012-05-20 12:02:44 PDT

(In reply to comment #29) > (From update of attachment 142553 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142553&action=review > Setting logicalHeight is definitely correct. The fact that you're setting height to non-zero is what concerns me. Here's a case I'm worried we would do the wrong thing with text wrapping: > > http://plexode.com/eval3/#ht=%3Cdiv%20style%3D%22-webkit-writing-mode%3A%20vertical-lr%3B%20outline%3A%202px%20solid%20blue%3B%20width%3A%20100px%22%3E%0A%20%20%20%20%3Cdiv%20style%3D%22-webkit-writing-mode%3A%20horizontal-tb%3B%20background-color%3A%20red%22%3E%0A%20%20%20%20%20%20%20%20foo%20bar%20baz%20foo%0A%20%20%20%20%3C%2Fdiv%3E%0A%3C%2Fdiv%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1 > > I'm worried that with the above code we'll not wrap the text at all, where we should be wrapping at 100px. A test case would help appease my worries, but it really would be good to get Hyatt's signoff as well. I don't think we need to block committing this patch on Hyatt review if you add a test-case like the one above, but at least put in a FIXME that this might do the wrong thing until we can have Hyatt verify it's sanity. That test case displays differently on TOT vs. shipping Safari. The outer div has 0 height on TOT/Chrome, where as it has 100% height on shiping Safari.

Ojan Vafai

Comment 31 2012-05-21 16:13:16 PDT

(In reply to comment #30) > (In reply to comment #29) > > (From update of attachment 142553 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=142553&action=review > > Setting logicalHeight is definitely correct. The fact that you're setting height to non-zero is what concerns me. Here's a case I'm worried we would do the wrong thing with text wrapping: > > > > http://plexode.com/eval3/#ht=%3Cdiv%20style%3D%22-webkit-writing-mode%3A%20vertical-lr%3B%20outline%3A%202px%20solid%20blue%3B%20width%3A%20100px%22%3E%0A%20%20%20%20%3Cdiv%20style%3D%22-webkit-writing-mode%3A%20horizontal-tb%3B%20background-color%3A%20red%22%3E%0A%20%20%20%20%20%20%20%20foo%20bar%20baz%20foo%0A%20%20%20%20%3C%2Fdiv%3E%0A%3C%2Fdiv%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1 > > > > I'm worried that with the above code we'll not wrap the text at all, where we should be wrapping at 100px. A test case would help appease my worries, but it really would be good to get Hyatt's signoff as well. I don't think we need to block committing this patch on Hyatt review if you add a test-case like the one above, but at least put in a FIXME that this might do the wrong thing until we can have Hyatt verify it's sanity. > > That test case displays differently on TOT vs. shipping Safari. The outer div has 0 height on TOT/Chrome, where as it has 100% height on shiping Safari. Yup, the behavior change was intentional from http://trac.webkit.org/changeset/97654.

Eric Seidel (no email)

Comment 32 2012-05-21 16:27:09 PDT

Created attachment 143128 [details] Fixed per Ojan's review

Eric Seidel (no email)

Comment 33 2012-05-21 16:29:26 PDT

After discussion with Ojan over IRC, we'll look at possible writing mode bugs after landing the initial implementation.

Ojan Vafai

Comment 34 2012-05-21 16:31:54 PDT

Comment on attachment 143128 [details] Fixed per Ojan's review View in context: https://bugs.webkit.org/attachment.cgi?id=143128&action=review > Source/WebCore/rendering/RenderIFrame.cpp:214 > m_overflow.clear(); > addVisualEffectOverflow(); Side comment: I'm not sure what overflow iframes can ever have. This code is suspicious to me. Not that it should affect seamless in any way.

Ojan Vafai

Comment 35 2012-05-21 17:10:33 PDT

Tony might also have thoughts on what might break by setting logicalHeight to max instead of 0.

WebKit Review Bot

Comment 36 2012-05-21 23:03:21 PDT

Comment on attachment 143128 [details] Fixed per Ojan's review Rejecting attachment 143128 [details] from commit-queue. New failing tests: fast/frames/seamless/seamless-inline.html fast/frames/seamless/seamless-float.html Full output: http://queues.webkit.org/results/12744466

WebKit Review Bot

Comment 37 2012-05-21 23:03:31 PDT

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

WebKit Review Bot

Comment 38 2012-05-22 01:35:08 PDT

Comment on attachment 143128 [details] Fixed per Ojan's review Attachment 143128 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12742538 New failing tests: fast/frames/seamless/seamless-inline.html fast/frames/seamless/seamless-float.html

WebKit Review Bot

Comment 39 2012-05-22 01:35:13 PDT

Created attachment 143225 [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

Tony Chang

Comment 40 2012-05-22 10:20:39 PDT

(In reply to comment #35) > Tony might also have thoughts on what might break by setting logicalHeight to max instead of 0. I read the comment in the code, but I don't understand the problem that vertical scrollbars cause. Setting the height to max smells funny to me. There might be another way to accomplish the same goal. Have you asked Hyatt?

Eric Seidel (no email)

Comment 41 2012-05-22 11:35:10 PDT

(In reply to comment #40) > (In reply to comment #35) > > Tony might also have thoughts on what might break by setting logicalHeight to max instead of 0. > > I read the comment in the code, but I don't understand the problem that vertical scrollbars cause. Setting the height to max smells funny to me. There might be another way to accomplish the same goal. Have you asked Hyatt? Hyatt has been CC'd on every seamless change. Ojan has asked him specifically about this function over IRC. I've not seen any response. Certainly his (and anyone elses!) feedback about other ways to avoid scrollbars while still setting the height to 0 at the start are most welcome! I considered using the m_firstLayout flag, but that didn't seem quite right. I could manually disable vertical scrollbars during dthe first layout, but that also didn't seem right. So this was the solution I settled on for now. :)

Eric Seidel (no email)

Comment 42 2012-05-22 12:16:49 PDT

I don't understand. Those tests do not fail for me locally on Mac or CrMac builds. :(

Adam Barth

Comment 43 2012-05-22 14:28:09 PDT

(In reply to comment #42) > I don't understand. Those tests do not fail for me locally on Mac or CrMac builds. :( You might want to try with mock scrollbars turned on. That might help roll the dice w.r.t. scroll backs.

Eric Seidel (no email)

Comment 44 2012-05-23 16:38:21 PDT

Created attachment 143675 [details] Patch for landing

Eric Seidel (no email)

Comment 45 2012-05-23 16:41:37 PDT

Created attachment 143676 [details] Patch for landing

WebKit Review Bot

Comment 46 2012-05-23 18:06:40 PDT

Comment on attachment 143676 [details] Patch for landing Clearing flags on attachment: 143676 Committed r118291: <http://trac.webkit.org/changeset/118291>

WebKit Review Bot

Comment 47 2012-05-23 18:06:49 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.