Bug 94472 - [CSS Regions] Add support for text-shadow in region styling
Summary: [CSS Regions] Add support for text-shadow in region styling
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords: AdobeTracked
Depends on: 95463 101933 102090 102093
Blocks: 71487
  Show dependency treegraph
 
Reported: 2012-08-20 05:35 PDT by Andrei Onea
Modified: 2013-03-31 07:52 PDT (History)
17 users (show)

See Also:


Attachments
Patch (13.97 KB, patch)
2012-08-20 08:08 PDT, Andrei Onea
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (496.13 KB, application/zip)
2012-08-20 08:47 PDT, WebKit Review Bot
no flags Details
Patch (14.29 KB, patch)
2012-08-27 05:26 PDT, Andrei Onea
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (809.01 KB, application/zip)
2012-08-27 06:11 PDT, WebKit Review Bot
no flags Details
Patch (15.21 KB, patch)
2012-08-28 12:15 PDT, Andrei Onea
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (1.27 MB, application/zip)
2012-08-28 17:28 PDT, WebKit Review Bot
no flags Details
Patch (15.11 KB, patch)
2012-08-29 09:33 PDT, Andrei Onea
no flags Details | Formatted Diff | Diff
Patch for landing (15.12 KB, patch)
2012-08-30 03:44 PDT, Andrei Onea
no flags Details | Formatted Diff | Diff
Patch (16.09 KB, patch)
2012-09-12 05:53 PDT, Andrei Onea
no flags Details | Formatted Diff | Diff
Patch (22.02 KB, patch)
2012-09-18 09:10 PDT, Andrei Onea
no flags Details | Formatted Diff | Diff
Patch (22.71 KB, patch)
2012-09-19 07:39 PDT, Andrei Onea
no flags Details | Formatted Diff | Diff
Patch 8 (25.46 KB, patch)
2012-10-30 00:43 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 9 (25.58 KB, patch)
2012-10-30 00:57 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (26.21 KB, patch)
2012-11-05 00:31 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (26.25 KB, patch)
2012-11-09 00:33 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (26.30 KB, patch)
2012-11-12 03:39 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Onea 2012-08-20 05:35:39 PDT
The spec permits region styling to be applied for the text-shadow property. We need to also implement this in WebKit.
Comment 1 Andrei Onea 2012-08-20 08:08:30 PDT
Created attachment 159431 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-20 08:47:41 PDT
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
Comment 3 WebKit Review Bot 2012-08-20 08:47:45 PDT
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 4 Julien Chaffraix 2012-08-22 09:13:48 PDT
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()?
Comment 5 Julien Chaffraix 2012-08-22 09:14:53 PDT
I am no expert in line box layout so CC'ing people who know this code better in case they have some comment.
Comment 6 Dave Hyatt 2012-08-23 11:47:45 PDT
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.
Comment 7 Andrei Onea 2012-08-27 05:26:49 PDT
Created attachment 160697 [details]
Patch
Comment 8 WebKit Review Bot 2012-08-27 06:11:19 PDT
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
Comment 9 WebKit Review Bot 2012-08-27 06:11:23 PDT
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 10 Dave Hyatt 2012-08-27 12:35:47 PDT
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).
Comment 11 Dave Hyatt 2012-08-27 12:36:34 PDT
Let's get some layout tests where the text-shadow is on a later line in the block and not just at the top.
Comment 12 Andrei Onea 2012-08-28 12:15:06 PDT
Created attachment 161028 [details]
Patch
Comment 13 WebKit Review Bot 2012-08-28 17:28:15 PDT
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
Comment 14 WebKit Review Bot 2012-08-28 17:28:18 PDT
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 15 Mihai Balan 2012-08-29 06:36:45 PDT
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?
Comment 16 Andrei Onea 2012-08-29 09:33:01 PDT
Created attachment 161247 [details]
Patch
Comment 17 Dave Hyatt 2012-08-29 12:00:00 PDT
Comment on attachment 161247 [details]
Patch

r=me
Comment 18 WebKit Review Bot 2012-08-29 16:26:11 PDT
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
Comment 19 Andrei Onea 2012-08-30 03:44:44 PDT
Created attachment 161437 [details]
Patch for landing
Comment 20 EFL EWS Bot 2012-08-30 03:51:41 PDT
Comment on attachment 161437 [details]
Patch for landing

Attachment 161437 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13682635
Comment 21 EFL EWS Bot 2012-08-30 04:24:07 PDT
(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 22 WebKit Review Bot 2012-08-30 06:19:19 PDT
Comment on attachment 161437 [details]
Patch for landing

Clearing flags on attachment: 161437

Committed r127131: <http://trac.webkit.org/changeset/127131>
Comment 23 WebKit Review Bot 2012-08-30 06:19:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 WebKit Review Bot 2012-08-30 08:44:42 PDT
Re-opened since this is blocked by 95463
Comment 25 Julien Chaffraix 2012-08-30 09:35:21 PDT
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(); }
Comment 26 Mihnea Ovidenie 2012-08-31 11:05:12 PDT
This caused also a crash that can be seen in https://bugs.webkit.org/show_bug.cgi?id=95583
Comment 27 Andrei Onea 2012-09-12 05:53:40 PDT
Created attachment 163610 [details]
Patch
Comment 28 Dave Hyatt 2012-09-13 11:09:14 PDT
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.
Comment 29 Andrei Onea 2012-09-18 09:10:22 PDT
Created attachment 164572 [details]
Patch
Comment 30 Mihnea Ovidenie 2012-09-18 10:58:49 PDT
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.
Comment 31 Andrei Onea 2012-09-19 07:39:22 PDT
Created attachment 164736 [details]
Patch
Comment 32 Mihnea Ovidenie 2012-10-30 00:43:00 PDT
Created attachment 171384 [details]
Patch 8

Bring the patch up-to-date.
Comment 33 WebKit Review Bot 2012-10-30 00:44:50 PDT
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.
Comment 34 Mihnea Ovidenie 2012-10-30 00:57:36 PDT
Created attachment 171387 [details]
Patch 9
Comment 35 Dave Hyatt 2012-11-02 11:05:45 PDT
Comment on attachment 171387 [details]
Patch 9

r=me
Comment 36 Mihnea Ovidenie 2012-11-05 00:31:59 PST
Created attachment 172282 [details]
Patch for landing
Comment 37 WebKit Review Bot 2012-11-05 01:41:20 PST
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
Comment 38 Mihnea Ovidenie 2012-11-09 00:33:06 PST
Created attachment 173226 [details]
Patch for landing
Comment 39 Mihnea Ovidenie 2012-11-09 00:35:41 PST
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 40 WebKit Review Bot 2012-11-09 04:25:16 PST
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
Comment 41 Mihnea Ovidenie 2012-11-12 03:39:15 PST
Created attachment 173608 [details]
Patch for landing
Comment 42 WebKit Review Bot 2012-11-12 04:08:49 PST
Comment on attachment 173608 [details]
Patch for landing

Clearing flags on attachment: 173608

Committed r134205: <http://trac.webkit.org/changeset/134205>
Comment 43 WebKit Review Bot 2012-11-12 04:08:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 44 János Badics 2012-11-12 06:16:28 PST
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
Comment 45 WebKit Review Bot 2012-11-13 08:12:53 PST
Re-opened since this is blocked by bug 102090