Bug 89881

Summary: Split map* functions out of StyleResolver into a helper object
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, koivisto, macpherson, menard, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89879    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Fix Qt none

Description Eric Seidel (no email) 2012-06-25 08:34:34 PDT
Split map* functions out of StyleResolver into a helper object
Comment 1 Eric Seidel (no email) 2012-06-25 08:42:42 PDT
Created attachment 149297 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-06-25 08:45:39 PDT
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.
Comment 3 Eric Seidel (no email) 2012-06-25 09:01:49 PDT
Created attachment 149299 [details]
Patch
Comment 4 Eric Seidel (no email) 2012-06-25 09:12:31 PDT
Created attachment 149304 [details]
Patch
Comment 5 Early Warning System Bot 2012-06-25 09:19:33 PDT
Comment on attachment 149304 [details]
Patch

Attachment 149304 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13096162
Comment 6 Early Warning System Bot 2012-06-25 09:21:21 PDT
Comment on attachment 149304 [details]
Patch

Attachment 149304 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13102084
Comment 7 Eric Seidel (no email) 2012-06-25 09:26:20 PDT
Created attachment 149308 [details]
Fix Qt
Comment 8 Daniel Bates 2012-06-25 10:15:32 PDT
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".
Comment 9 WebKit Review Bot 2012-06-25 10:36:58 PDT
Comment on attachment 149308 [details]
Fix Qt

Clearing flags on attachment: 149308

Committed r121164: <http://trac.webkit.org/changeset/121164>
Comment 10 WebKit Review Bot 2012-06-25 10:37:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Antti Koivisto 2012-06-25 14:26:49 PDT
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.
Comment 12 Eric Seidel (no email) 2012-06-25 15:01:19 PDT
(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.
Comment 13 Eric Seidel (no email) 2012-06-25 22:16:14 PDT
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.