Bug 79873 - [BlackBerry] Upstream LayerCompositingThread.{h, cpp}
Summary: [BlackBerry] Upstream LayerCompositingThread.{h, cpp}
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73119
  Show dependency treegraph
 
Reported: 2012-02-28 22:27 PST by Robin Cao
Modified: 2012-03-26 09:25 PDT (History)
7 users (show)

See Also:


Attachments
patch (38.27 KB, patch)
2012-02-29 01:42 PST, Robin Cao
no flags Details | Formatted Diff | Diff
patch v2 (37.88 KB, patch)
2012-03-01 01:08 PST, Robin Cao
no flags Details | Formatted Diff | Diff
patch-v3 (30.59 KB, patch)
2012-03-26 08:03 PDT, Robin Cao
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Cao 2012-02-28 22:27:26 PST
LayerCompositingThread is the counterpart of LayerWebKitThread, but most of its member functions need to be called from the compositing thread.
Comment 1 Robin Cao 2012-02-29 01:42:57 PST
Created attachment 129415 [details]
patch
Comment 2 Rob Buis 2012-02-29 11:08:50 PST
Comment on attachment 129415 [details]
patch

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

Not much wrong, but still some cleanup to do.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:297
> +        FloatQuad surfaceQuad = getTransformedRect(m_bounds, IntRect(IntPoint(), m_bounds), drawTransform);

Better use IntPoint::zero

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:487
> +    if (layer)

Can combine these two lines.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:679
> +        ASSERT_NOT_REACHED();

Can combine with AnimatedPropertyBackgroundColor case, they do the same.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:47
> +class SkBitmap;

Not used? Please clean it up and the includes above.
Comment 3 Robin Cao 2012-03-01 01:04:23 PST
Thanks for the review, all the comments will be addressed in a revised patch.
Comment 4 Robin Cao 2012-03-01 01:08:56 PST
Created attachment 129659 [details]
patch v2
Comment 5 Rob Buis 2012-03-05 07:16:36 PST
Comment on attachment 129659 [details]
patch v2

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

Looks good, can be improved some more.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:39
> +#include "HTMLCanvasElement.h"

Is this one actually used? Please check all includes in general.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:152
> +    result.setP2(drawTransform.mapPoint(FloatPoint(x, y+h)));

Watch spacing "y + h"

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:153
> +    result.setP3(drawTransform.mapPoint(FloatPoint(x+w, y+h)));

Ditto.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:162
> +static bool enableVideoClipping = false;

Can be moved into the function below, only used there.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:240
> +            m_mediaPlayer->paint(0, IntRect((int)(x+0.5), (int)(y+0.5), m_bounds.width(), m_bounds.height()));

Take care of the spacing please,  x+0.5 -> x + 0.5 etc.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:285
> +            // FIXME: is this going to happen?

Might require an ASSERT if we are quite sure it will not happen.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:563
> +// FIXME: Copy pasted from WebCore::KeyframeAnimation!!!

I am seeing this kind of FIXME a lot. Maybe we need a PR to track this?

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:57
> +class LayerMessage;

Not used?

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:155
> +    }

Can be put on one line like above : size_t numSublayers() const { return m_sublayers.size(); }
Comment 6 Robin Cao 2012-03-26 07:55:21 PDT
Thanks for the review, comments inlined.

(In reply to comment #5)
> (From update of attachment 129659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129659&action=review
> 
> Looks good, can be improved some more.
> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:39
> > +#include "HTMLCanvasElement.h"
> 
> Is this one actually used? Please check all includes in general.
> 
All unnecessary includes are removed.

> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:152
> > +    result.setP2(drawTransform.mapPoint(FloatPoint(x, y+h)));
> 
> Watch spacing "y + h"
> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:153
> > +    result.setP3(drawTransform.mapPoint(FloatPoint(x+w, y+h)));
> 
> Ditto.
> 
Fixed.

> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:162
> > +static bool enableVideoClipping = false;
> 
> Can be moved into the function below, only used there.
> 
Fixed.

> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:240
> > +            m_mediaPlayer->paint(0, IntRect((int)(x+0.5), (int)(y+0.5), m_bounds.width(), m_bounds.height()));
> 
> Take care of the spacing please,  x+0.5 -> x + 0.5 etc.
> 
Fixed.

> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:285
> > +            // FIXME: is this going to happen?
> 
> Might require an ASSERT if we are quite sure it will not happen.
> 
Replaced with ASSERT_NOT_REACHED();

> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:563
> > +// FIXME: Copy pasted from WebCore::KeyframeAnimation!!!
> 
> I am seeing this kind of FIXME a lot. Maybe we need a PR to track this?
> 
All layer animation related code have been moved to LayerAnimation.cpp.

> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:57
> > +class LayerMessage;
> 
> Not used?
> 
Removed.

> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:155
> > +    }
> 
> Can be put on one line like above : size_t numSublayers() const { return m_sublayers.size(); }
Done.
Comment 7 Robin Cao 2012-03-26 08:03:29 PDT
Created attachment 133813 [details]
patch-v3
Comment 8 Rob Buis 2012-03-26 08:21:40 PDT
Comment on attachment 133813 [details]
patch-v3

Looks good.
Comment 9 WebKit Review Bot 2012-03-26 09:24:59 PDT
Comment on attachment 133813 [details]
patch-v3

Clearing flags on attachment: 133813

Committed r112110: <http://trac.webkit.org/changeset/112110>
Comment 10 WebKit Review Bot 2012-03-26 09:25:05 PDT
All reviewed patches have been landed.  Closing bug.