[Webkit-unassigned] [Bug 94472] [CSSRegions]Add support for text-shadow in region styling

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94472





--- Comment #30 from Mihnea Ovidenie <mihnea at adobe.com>  2012-09-18 10:58:49 PST ---
(From update of attachment 164572)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.