[Webkit-unassigned] [Bug 91076] [CSS Regions] Add the Region interface and the getRegions() API call

[bugzilla-daemon at webkit.org]

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


Andreas Kling <kling at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #152991|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #2 from Andreas Kling <kling at webkit.org>  2012-07-18 06:12:11 PST ---
(From update of attachment 152991)
View in context: https://bugs.webkit.org/attachment.cgi?id=152991&action=review

Looks good functionality-wise, but there are a number of overly generic variable and function names that need fixing.

> Source/WebCore/css/CSSRegion.cpp:2
> + * Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved.

2012!

> Source/WebCore/css/CSSRegion.cpp:46
> +String CSSRegion::flowFrom(ExceptionCode& e) const

Style nit: We typically use "ec" for the ExceptionCode variables.

> Source/WebCore/css/CSSRegion.cpp:59
> +const AtomicString& CSSRegion::regionOverset(ExceptionCode& e) const

Style nit: We typically use "ec" for the ExceptionCode variables.

> Source/WebCore/css/CSSRegion.cpp:87
> +    default:
> +        break;
> +    }
> +
> +    return undefinedState;

We could replace the "default:" with "case RenderRegion::RegionUndefined:" here, that way the compiler will give us a warning if more values are ever added to RegionState without also adding code here.

> Source/WebCore/css/CSSRegion.h:2
> + * Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved.

2012!

> Source/WebCore/css/CSSRegion.h:62
> +    RenderRegion* m_region;

We need a better name for this variable. m_renderer perhaps?

> Source/WebCore/rendering/RenderRegion.h:59
> +    CSSRegion* ensureRegionObject();

This function needs a better name. ensureRegionCSSOMWrapper()?

> Source/WebCore/rendering/RenderRegion.h:148
> +    RefPtr<CSSRegion> m_region;

This variable needs a better name. m_regionCSSOMWrapper?

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