Bug 94472

Created attachment 159431 [details] Patch

Comment on attachment 159431 [details] Patch Attachment 159431 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13544375 New failing tests: fast/regions/region-style-text-shadow.html

Created attachment 159441 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 159431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159431&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:165 > + childStyle = flowThread->renderStyleForLine(this); Won't this break :first-letter or :first-line in region as you don't pass in isFirstLineStyle()?

I am no expert in line box layout so CC'ing people who know this code better in case they have some comment.

Comment on attachment 159431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159431&action=review r- (Comments below) > Source/WebCore/rendering/InlineFlowBox.cpp:166 > RenderStyle* childStyle = child->renderer()->style(isFirstLineStyle()); > + if (renderer()->inRenderFlowThread()) { > + RenderFlowThread* flowThread = renderer()->enclosingRenderFlowThread(); > + childStyle = flowThread->renderStyleForLine(this); > + } I would bundle all of this together into a styleInRegion() function on InlineBox. Then the code here would just read: RenderStyle* childStyle = child->styleInRegion(child->region(), isFirstLineStyle()); Technically if the first line style bit is properly set on the child box, you don't even need to pass in isFirstLineStyle(). In other words, let's separate fetching the style for a specific region from the function that figures out what region the child is in. I'll explain why below. > Source/WebCore/rendering/InlineFlowBox.cpp:832 > RenderStyle* style = textBox->renderer()->style(isFirstLineStyle()); > > + if (renderer()->inRenderFlowThread()) { > + RenderFlowThread* flowThread = renderer()->enclosingRenderFlowThread(); > + style = flowThread->renderStyleForLine(this); > + } Same here. All of this could just be inside InlineBox helpers, since you just did the exact same thing as above. > Source/WebCore/rendering/RenderFlowThread.cpp:405 > + LayoutUnit offsetFromLogicalTop = toRenderBlock(renderer)->offsetFromLogicalTopOfFirstPage(); > + if (flowBox->prevLineBox()) > + offsetFromLogicalTop += flowBox->prevLineBox()->logicalTop(); This is not correct. You're treating the first line as though it started right at the very top of the RenderBlock, but there could be border/padding/floats, etc., any # of things that push even the first line down. Why aren't you just adding in your own logicalTop? I don't understand why the previous line box has to be involved at all. Also, this is very fragile code, since it only works during layout. We don't know the offsetFromLogicalTopOfFirstPage at paint time for example (we'll have the RenderRegion in the painting info in that case). This is why I'd like to see styleInRegion() and region() as two different helpers on InlineBox. Then at paint time, you'll be able to still ask for the region-specific style and just pass in the RenderRegion from the painting info (since you won't be able to rely on offsetFromLogicalTopOfFirstPage). > Source/WebCore/rendering/RenderFlowThread.cpp:409 > + return region->computeStyleInRegion(renderer).get(); Where is first line styling taken into account? It looks like it's being dropped on the floor.

Created attachment 160697 [details] Patch

Comment on attachment 160697 [details] Patch Attachment 160697 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13622078 New failing tests: fast/regions/region-style-text-shadow.html

Created attachment 160703 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 160697 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160697&action=review Looking good! Just a few suggestions. > Source/WebCore/rendering/InlineBox.cpp:257 > +RenderStyle* InlineBox::styleInRegion(RenderRegion* region, bool isFirstLine) You don't need the second argument. The style bit has been set on the box itself at all the call sites I see you using this function. So you can just use the member on InlineBox. > Source/WebCore/rendering/InlineBox.cpp:268 > +RenderRegion* InlineBox::region() Let's rename this to "regionDuringLayout()" to make it clear you can't call it at other times. > Source/WebCore/rendering/InlineBox.cpp:274 > + return flowThread->renderRegionForLine(renderer()->containingBlock()->offsetFromLogicalTopOfFirstPage()); You should add in the logicalHeight() of the containing block, so that you're at least at the bottom of the previous line. > Source/WebCore/rendering/InlineBox.h:103 > + RenderRegion* region(); regionDuringLayout() rename. Also let's add a comment that indicates that this assumes the box has not been positioned yet, and that it will use the block's current logical height (similar to what floats do).

Let's get some layout tests where the text-shadow is on a later line in the block and not just at the top.

Created attachment 161028 [details] Patch

Comment on attachment 161028 [details] Patch Attachment 161028 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13651576 New failing tests: fast/regions/region-style-text-shadow.html

Created attachment 161098 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 161028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161028&action=review > LayoutTests/fast/regions/region-style-text-shadow-expected.html:10 > + #p1 { text-shadow: 340px 100px #008000; position: absolute; top: 10px; } Why are you manually positioning every element absolutely? It really makes the test a lot harder to read/interpret > LayoutTests/fast/regions/region-style-text-shadow.html:14 > + #region1 { -webkit-flow-from: flow1; position: absolute; top: 10px; } Is there a particular reason why you manually specify the position of the regions? I would like the tests as clear and clutter free as possible :) > LayoutTests/fast/regions/region-style-text-shadow.html:59 > + <div id="article1"> For the sake of maintainability / readability, I would rather have one text at the bottom of the file explaining the expected output (E.g. "No red shadow should be visible") and then each "assertion" to better explain what exactly is it testing rather than having "Text shadow styled in region: <color>" over and over again :) > LayoutTests/fast/regions/region-style-text-shadow.html:87 > + <div id="article5"> Aren't article5 and article6 testing the same thing? (The only difference that I see is in the element used inside the <article> tag. Why do you think they would behave differently?

Created attachment 161247 [details] Patch

Comment on attachment 161247 [details] Patch r=me

Comment on attachment 161247 [details] Patch Rejecting attachment 161247 [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: ineTextBox.o Source/WebCore/rendering/InlineBox.cpp: In member function 'WebCore::RenderRegion* WebCore::InlineBox::regionDuringLayout()': Source/WebCore/rendering/InlineBox.cpp:278: error: 'class WebCore::RenderFlowThread' has no member named 'renderRegionForLine' CXX(target) out/Release/obj.target/webcore_rendering/Source/WebCore/rendering/LayoutState.o make: *** [out/Release/obj.target/webcore_rendering/Source/WebCore/rendering/InlineBox.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/13682416

Created attachment 161437 [details] Patch for landing

Comment on attachment 161437 [details] Patch for landing Attachment 161437 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13682635

(In reply to comment #20) > (From update of attachment 161437 [details]) > Attachment 161437 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/13682635 This is false alarm. Please ignore it. Sorry for noise.

Comment on attachment 161437 [details] Patch for landing Clearing flags on attachment: 161437 Committed r127131: <http://trac.webkit.org/changeset/127131>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 95463

Here is a crash report running valgrind on Chromium linux: valgrind out/Debug/DumpRenderTree --no-timeout LayoutTests/fast/regions/bottom-overflow-out-of-first-region.html ==7220== Memcheck, a memory error detector ==7220== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al. ==7220== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info ==7220== Command: out/Debug/DumpRenderTree --no-timeout LayoutTests/fast/regions/bottom-overflow-out-of-first-region.html ==7220== ==7220== Warning: set address range perms: large range [0x3815cfafb000, 0x3815efafb000) (noaccess) ==7220== Invalid read of size 2 ==7220== at 0x520E46: WebCore::Font::letterSpacing() const (Font.h:117) ==7220== by 0x1620D2B: WebCore::RenderStyle::letterSpacing() const (RenderStyle.cpp:1210) ==7220== by 0x142AFDF: WebCore::InlineFlowBox::addToLine(WebCore::InlineBox*) (InlineFlowBox.cpp:163) ==7220== by 0x1495818: WebCore::RenderBlock::createLineBoxes(WebCore::RenderObject*, WebCore::LineInfo const&, WebCore::InlineBox*) (RenderBlockLineLayout.cpp:487) ==7220== by 0x1495BDD: WebCore::RenderBlock::constructLine(WebCore::BidiRunList<WebCore::BidiRun>&, WebCore::LineInfo const&) (RenderBlockLineLayout.cpp:556) ==7220== by 0x14983E6: WebCore::RenderBlock::createLineBoxesFromBidiRuns(WebCore::BidiRunList<WebCore::BidiRun>&, WebCore::InlineIterator const&, WebCore::LineInfo&, WebCore::VerticalPositionCache&, WebCore::BidiRun*) (RenderBlockLineLayout.cpp:1078) ==7220== by 0x149950B: WebCore::RenderBlock::layoutRunsAndFloatsInRange(WebCore::LineLayoutState&, WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>&, WebCore::InlineIterator const&, WebCore::BidiStatus const&, unsigned int) (RenderBlockLineLayout.cpp:1370) ==7220== by 0x1498AB8: WebCore::RenderBlock::layoutRunsAndFloats(WebCore::LineLayoutState&, bool) (RenderBlockLineLayout.cpp:1271) ==7220== by 0x149AF2D: WebCore::RenderBlock::layoutInlineChildren(bool, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) (RenderBlockLineLayout.cpp:1600) ==7220== by 0x144A99D: WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) (RenderBlock.cpp:1531) ==7220== by 0x1449ED7: WebCore::RenderBlock::layout() (RenderBlock.cpp:1374) ==7220== by 0x144FAA1: WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) (RenderBlock.cpp:2452) ==7220== Address 0x363636363636369e is not stack'd, malloc'd or (recently) free'd ==7220== [7220:7220:1477808315938:ERROR:process_util_posix.cc(143)] Received signal 11 base::debug::StackTrace::StackTrace() [0x1419e1a] base::(anonymous namespace)::StackDumpSignalHandler() [0x13c68b5] 0xccccaf0 WebCore::Font::letterSpacing() [0x520e46] WebCore::RenderStyle::letterSpacing() [0x1620d2c] WebCore::InlineFlowBox::addToLine() [0x142afe0] WebCore::RenderBlock::createLineBoxes() [0x1495819] WebCore::RenderBlock::constructLine() [0x1495bde] WebCore::RenderBlock::createLineBoxesFromBidiRuns() [0x14983e7] WebCore::RenderBlock::layoutRunsAndFloatsInRange() [0x149950c] WebCore::RenderBlock::layoutRunsAndFloats() [0x1498ab9] WebCore::RenderBlock::layoutInlineChildren() [0x149af2e] WebCore::RenderBlock::layoutBlock() [0x144a99e] WebCore::RenderBlock::layout() [0x1449ed8] WebCore::RenderBlock::layoutBlockChild() [0x144faa2] WebCore::RenderBlock::layoutBlockChildren() [0x144f619] WebCore::RenderBlock::layoutBlock() [0x144a9bf] WebCore::RenderBlock::layout() [0x1449ed8] WebCore::RenderBlock::layoutBlockChild() [0x144faa2] WebCore::RenderBlock::layoutBlockChildren() [0x144f619] WebCore::RenderBlock::layoutBlock() [0x144a9bf] WebCore::RenderBlock::layout() [0x1449ed8] WebCore::RenderBlock::layoutBlockChild() [0x144faa2] WebCore::RenderBlock::layoutBlockChildren() [0x144f619] WebCore::RenderBlock::layoutBlock() [0x144a9bf] WebCore::RenderBlock::layout() [0x1449ed8] WebCore::RenderBlock::layoutBlockChild() [0x144faa2] WebCore::RenderBlock::layoutBlockChildren() [0x144f619] WebCore::RenderBlock::layoutBlock() [0x144a9bf] WebCore::RenderBlock::layout() [0x1449ed8] WebCore::RenderFlowThread::layout() [0x1503a88] WebCore::RenderObject::layoutIfNeeded() [0x141e45b] WebCore::FlowThreadController::layoutRenderNamedFlowThreads() [0x141dfee] WebCore::RenderView::layout() [0x15f79ee] WebCore::FrameView::layout() [0x2493160] WebCore::FrameView::layoutTimerFired() [0x2496a23] WebCore::Timer<>::fired() [0x24a39ee] WebCore::ThreadTimers::sharedTimerFiredInternal() [0x63aa10] Here is the crash under gdb: Program received signal SIGSEGV, Segmentation fault. 0x0000000000520e46 in WebCore::Font::letterSpacing (this=0x363636363636366e) at ../../Source/WebCore/platform/graphics/Font.h:117 117 short letterSpacing() const { return m_letterSpacing; } (gdb) f 1 #1 0x0000000001620d2c in WebCore::RenderStyle::letterSpacing (this=0x7fffeb36dd80) at ../../Source/WebCore/rendering/style/RenderStyle.cpp:1210 1210 int RenderStyle::letterSpacing() const { return inherited->font.letterSpacing(); }

This caused also a crash that can be seen in https://bugs.webkit.org/show_bug.cgi?id=95583

Created attachment 163610 [details] Patch

Comment on attachment 163610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163610&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:163 > - RenderStyle* childStyle = child->renderer()->style(isFirstLineStyle()); > + RefPtr<RenderStyle> childStyle = child->styleInRegion(child->regionDuringLayout()); Let's follow this through to its logical conclusion, which is that all style access during layout that needs to get a style that can vary from region to region is going to have to be put in a refptr just to keep it from going away? Ick. If someone repeatedly asks for style, they're going to cause it to be computed again and again. It seems obvious that we have to change the architecture here for what to do with region styling during layout. My opinion is that the styles should be cached and updated once they are accessed once and they should stick around as long as the region lives (and as long as the renderer that the style is applying to lives). If you had a cache like that it could work for both layout and painting. Computing the styles and then tossing them for painting is not exactly a great approach either, since you do way more work when painting than you need to. I think ultimately there's going to need to be a method like styleAtOffset that layout / painting use to fetch the appropriate region-specific style for an object. If the style doesn't exist, then it could be created at that time. I have no problem with a first-cut cache destroying all render styles at the end of every layout (similar to what painting does today). If you want to take that approach initially of doing something kind of like what painting does, I'd be cool with that.

Created attachment 164572 [details] Patch

Comment on attachment 164572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164572&action=review > Source/WebCore/ChangeLog:17 > + (WebCore::InlineBox::styleInRegion): I would like to see a comment describing what this function is doing. > Source/WebCore/ChangeLog:19 > + (WebCore::InlineBox::regionDuringLayout): Same here with the mention that this function should be used only during layout. > Source/WebCore/ChangeLog:44 > + value and caches it. Since this function is added in this patch, i would change the comment to something like: "This function exposes region styling information to be available not only at paint time, but also at layout time". > Source/WebCore/rendering/InlineBox.cpp:272 > + // This assumes that the box has not been positioned yet, so it uses the containing block's current logical height to get the region. Is there a way to test this assumption? > Source/WebCore/rendering/InlineBox.h:103 > + RenderRegion* regionDuringLayout(); These 2 methods can be made const. > Source/WebCore/rendering/RenderRegion.cpp:-466 > - ASSERT(!object->isAnonymous()); An explanation in the changelog about this assertion removal would be nice. > Source/WebCore/rendering/RenderRegion.cpp:463 > + RenderStyle* defaultParentStyle = object->parent() && !object->node()->inNamedFlow()? ensureRegionStyleForObject(object->parent()):0; I would also add a line in the changelog about the computation (and caching) of parent style in region. > LayoutTests/fast/regions/region-style-text-shadow-expected.html:10 > + #p1 { text-shadow: 340px 100px #008000; position: absolute; top: 10px; } In this test, you have 2 text-shadow styles that you are using. I would create 2 classes for them and annotate the elements with them.

Created attachment 164736 [details] Patch

Created attachment 171384 [details] Patch 8 Bring the patch up-to-date.

Attachment 171384 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/RenderRegion.cpp:513: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 171387 [details] Patch 9

Comment on attachment 171387 [details] Patch 9 r=me

Created attachment 172282 [details] Patch for landing

Comment on attachment 172282 [details] Patch for landing Rejecting attachment 172282 [details] from commit-queue. New failing tests: fast/regions/region-style-inline-background-color.html Full output: http://queues.webkit.org/results/14712957

Created attachment 173226 [details] Patch for landing

I have build chromium port with the patch applied on Linux and the failing test passed. I have also built Chromium on Linux with the patch and run the test manually without noticing a problem. I decided to try to apply the patch once again. I wish we would have a way to see the errors in such cases.

Comment on attachment 173226 [details] Patch for landing Rejecting attachment 173226 [details] from commit-queue. New failing tests: fast/regions/region-style-inline-background-color.html Full output: http://queues.webkit.org/results/14781227

Created attachment 173608 [details] Patch for landing

Comment on attachment 173608 [details] Patch for landing Clearing flags on attachment: 173608 Committed r134205: <http://trac.webkit.org/changeset/134205>

All reviewed patches have been landed. Closing bug.

After http://trac.webkit.org/changeset/134205, fast/regions/region-style-inline-background-color.html and fast/regions/webkit-flow-inlines-dynamic.html fail on Qt (the second one fails on GTK and EFL, too). You can find details at https://bugs.webkit.org/show_bug.cgi?id=101933

Re-opened since this is blocked by bug 102090

CSS Regions were removed in Bug 174978.