Bug 114275 – [BlackBerry] Accelerated compositing debug rectangle incorrectly drawn for layers with surfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Arvid Nilsson]

PR 323746

[Arvid Nilsson]

Created attachment 197569 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 197569 [details]
Patch

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

Looks good.

> Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:598
> +    // If we're rendering to a surface, don't include debug border inside the surface

Finish comments with a period. ;-)

> Source/WebCore/platform/graphics/blackberry/LayerRendererSurface.cpp:67
> +FloatQuad LayerRendererSurface::getTransformedBounds() const

getFoo is typically used for methods that return the value as an out parameter. Sou you could either pass the FloatQuad as a reference and keep the name or rename it to transformedBounds(). I this case passing it as a reference we safe a copy, I think.

[Arvid Nilsson]

(In reply to comment #2)
> (From update of attachment 197569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197569&action=review
> 
> Looks good.
> 
> > Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:598
> > +    // If we're rendering to a surface, don't include debug border inside the surface
> 
> Finish comments with a period. ;-)
> 
> > Source/WebCore/platform/graphics/blackberry/LayerRendererSurface.cpp:67
> > +FloatQuad LayerRendererSurface::getTransformedBounds() const
> 
> getFoo is typically used for methods that return the value as an out parameter. Sou you could either pass the FloatQuad as a reference and keep the name or rename it to transformedBounds(). I this case passing it as a reference we safe a copy, I think.

I copied LayerCompositingThread::getTransformedBounds() which does return a reference and I guess thus deserves the "get" prefix. The LayerRendererSurface doesn't bother with caching the transformed bounds and can't return a reference. It's not worth caching them either, so let's just drop the "get" bit.

[Arvid Nilsson]

Created attachment 197572 [details]
Patch

[Arvid Nilsson]

I was under the impression that WebKit coding style guidelines sometimes allows using the "get" prefix when some more computation is required than just returning the value of a data member, for example when a hash map lookup or other extra work is needed. Maybe that's just something I imagined =)

[Arvid Nilsson]

(In reply to comment #5)
> I was under the impression that WebKit coding style guidelines sometimes allows using the "get" prefix when some more computation is required than just returning the value of a data member, for example when a hash map lookup or other extra work is needed. Maybe that's just something I imagined =)

That was just my imagination, the coding style guidelines only talks about using "get" when the method has out parameters.

[Arvid Nilsson]

Comment on attachment 197572 [details]
Patch

Thanks for the review, I should have a look at the code here and see what other getters should have their "get" prefix removed.

[WebKit Commit Bot]

Comment on attachment 197572 [details]
Patch

Clearing flags on attachment: 197572

Committed r148200: <http://trac.webkit.org/changeset/148200>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.