Bug 89881 – Split map* functions out of StyleResolver into a helper object

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Split map* functions out of StyleResolver into a helper object

[Eric Seidel (no email)]

Created attachment 149297 [details]
Patch

[Eric Seidel (no email)]

I'm not wedded to the naming here.

Basically this is a StyleBuilder helper object, which currently needs a StyleResolver pointer, because StyleResolver is the god-object of style resolve and has the current-resolve cache pointers we need.  Once those are split off into a separate object, this object wont have anything to do with StyleResolver and may even be completely static.

[Eric Seidel (no email)]

Created attachment 149299 [details]
Patch

[Eric Seidel (no email)]

Created attachment 149304 [details]
Patch

[Early Warning System Bot]

Comment on attachment 149304 [details]
Patch

Attachment 149304 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13096162

[Early Warning System Bot]

Comment on attachment 149304 [details]
Patch

Attachment 149304 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13102084

[Eric Seidel (no email)]

Created attachment 149308 [details]
Fix Qt

[Daniel Bates]

Comment on attachment 149308 [details]
Fix Qt

View in context: https://bugs.webkit.org/attachment.cgi?id=149308&action=review

> Source/WebCore/css/CSSToStyleMap.cpp:84
> +    case CSSValueFixed:
> +        layer->setAttachment(FixedBackgroundAttachment);
> +        break;
> +    case CSSValueScroll:
> +        layer->setAttachment(ScrollBackgroundAttachment);
> +        break;
> +    case CSSValueLocal:
> +        layer->setAttachment(LocalBackgroundAttachment);
> +        break;

I know that you're moving code. The compiler will probably already do this, we should substitute "return" for "break" in each of these case statements since we fall off the end of the function anyway after exiting the switch block. Also, I tend to find that a switch block that is all returns or all breaks tends to read better.

> Source/WebCore/css/CSSToStyleMap.cpp:266
> +    float zoomFactor = style()->effectiveZoom();
> +
> +    CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +    Length length;
> +    if (primitiveValue->isLength())
> +        length = primitiveValue->computeLength<Length>(style(), rootElementStyle(), zoomFactor);
> +    else if (primitiveValue->isPercentage())
> +        length = Length(primitiveValue->getDoubleValue(), Percent);
> +    else if (primitiveValue->isCalculatedPercentageWithLength())
> +        length = Length(primitiveValue->cssCalcValue()->toCalcValue(style(), rootElementStyle(), zoomFactor));
> +    else if (primitiveValue->isViewportPercentageLength())
> +        length = primitiveValue->viewportPercentageLength();
> +    else
> +        return;

This code is identical to code in CSSToStyleMap::mapFillXPosition. We consider sharing such code in a follow up patch.

> Source/WebCore/css/CSSToStyleMap.cpp:572
> +    float zoom = useSVGZoomRules() ? 1.0f : style()->effectiveZoom();

Nit: It's sufficient to use 1 here instead of 1.0f.

> Source/WebCore/css/CSSToStyleMap.cpp:581
> +        box.m_top = Length(slices->top()->getIntValue(), Relative);

LengthBox::m_{top, bottom, left, right} are all public instance variables. We may want to consider renaming them {top, bottom, left, right} in a follow up patch since LengthBox provides no encapsulation.

> Source/WebCore/css/CSSToStyleMap.h:86
> +}

Nit: We usually add an inline comment here of the form "// namespace WebCore"

> Source/WebCore/css/CSSToStyleMap.h:88
> +#endif

Nit: We usually add an inline comment here of the form "// CSSToStyleMap_h".

[WebKit Review Bot]

Comment on attachment 149308 [details]
Fix Qt

Clearing flags on attachment: 149308

Committed r121164: <http://trac.webkit.org/changeset/121164>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

CSSToStyleMap name does not follow our naming pattern for style resolver related classes (which is to use Style* prefix). It sounds like a hash map of some sort of (hash) map but apparently it is not. Is it just a random type to hang functions? Is that a good factoring?

Please cc me for any future patches that touch StyleResolver.

[Eric Seidel (no email)]

(In reply to comment #11)
> CSSToStyleMap name does not follow our naming pattern for style resolver related classes (which is to use Style* prefix). It sounds like a hash map of some sort of (hash) map but apparently it is not. Is it just a random type to hang functions? Is that a good factoring?
> 
> Please cc me for any future patches that touch StyleResolver.

Thank you for your thoughts.  I CC'd you on the meta bug, hoping for your commentary on this, and any other refactorings.  I'm glad you found your way here.

As mentioned in comment #2, I'm not at all wedded to this naming.  If you'd like to propose a change, I'm happy to make it.

Thank you again for your thoughts.

[Eric Seidel (no email)]

I have other responsibilities and may not be able to come back to the next stages of my desired refactoring of StyleResolver very soon.

As I would not like my work to be perceived as simply "drive-by", please let me (or sherrifbot) know and I can roll this patch out. :)

Otherwise I will assume silence to mean you agree this is an improvement and we'll leave this as is.  Thanks again for your thoughts.