[webkit-reviews] review denied: [Bug 91076] [CSS Regions] Add the Region interface and the getRegions() API call : [Attachment 152991] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Andrei Bucur <abucur at adobe.com>'s
request for review:
Bug 91076: [CSS Regions] Add the Region interface and the getRegions() API call
https://bugs.webkit.org/show_bug.cgi?id=91076

Attachment 152991: Patch
https://bugs.webkit.org/attachment.cgi?id=152991&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
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?