Bug 100736 - [EFL][WK2][AC] Avoid storing dirty rects in a Vector inside EwkViewImpl
Summary: [EFL][WK2][AC] Avoid storing dirty rects in a Vector inside EwkViewImpl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-30 01:59 PDT by Chris Dumez
Modified: 2012-10-31 11:02 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.81 KB, patch)
2012-10-30 02:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-10-30 01:59:26 PDT
http://trac.webkit.org/changeset/132483 is storing dirty rectangles in a Vector data member inside EwkView and then creates a Region from them in EwkViewImpl::displayTimerFired(). It would therefore be more optimal to keep the dirty rects as a Region instead of Vector to avoid iterating over the Vector to construct a Region from it.
Comment 1 Chris Dumez 2012-10-30 02:08:54 PDT
Created attachment 171402 [details]
Patch
Comment 2 Chris Dumez 2012-10-30 02:10:50 PDT
I have tried running in MiniBrowser:
http://trac.webkit.org/export/132877/trunk/Websites/webkit.org/blog-files/3d-transforms/poster-circle.html

I could not reproduce infinite loop problem and the animation seems to work properly.

I enabled AC using:
--- a/Tools/Scripts/webkitperl/FeatureList.pm
+++ b/Tools/Scripts/webkitperl/FeatureList.pm
@@ -392,7 +392,7 @@ my @features = (
       define => "ENABLE_TEXT_AUTOSIZING", default => 0, value => \$textAutosizingSupport },
 
     { option => "tiled-backing-store", desc => "Toggle Tiled Backing Store support",
-      define => "WTF_USE_TILED_BACKING_STORE", default => isQt(), value => \$tiledBackingStoreSupport },
+      define => "WTF_USE_TILED_BACKING_STORE", default => (isQt() || isEfl()), value => \$tiledBackingStoreSupport },
 
     { option => "touch-events", desc => "Toggle Touch Events support",
       define => "ENABLE_TOUCH_EVENTS", default => (isQt() || isBlackBerry() || isEfl()), value => \$touchEventsSupport },
Comment 3 Kenneth Rohde Christiansen 2012-10-30 02:14:04 PDT
Comment on attachment 171402 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:275
> +    OwnPtr<WebCore::Region> m_dirtyRegion;

Ah I didn't even know we had a region, but of course we do.
Comment 4 Chris Dumez 2012-10-30 14:10:01 PDT
Yael, would you please take a look before we commit this?
Comment 5 Yael 2012-10-31 09:49:40 PDT
(In reply to comment #4)
> Yael, would you please take a look before we commit this?

LGTM,

I am not sure why you did not observe a crash. I got the crash I posted in https://bugs.webkit.org/show_bug.cgi?id=100288#c0 very consistently.
Comment 6 Chris Dumez 2012-10-31 09:52:08 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Yael, would you please take a look before we commit this?
> 
> LGTM,
> 
> I am not sure why you did not observe a crash. I got the crash I posted in https://bugs.webkit.org/show_bug.cgi?id=100288#c0 very consistently.

To be clear, I did not experience the crash with my patch (and yours) applied so it is not surprising that I couldn't reproduce the patch. I merely wanted to make sure that my patch did not cause a regression (i.e. reintroduce the crash you had fixed).

Could someone please cq+ then?
Comment 7 WebKit Review Bot 2012-10-31 11:02:20 PDT
Comment on attachment 171402 [details]
Patch

Clearing flags on attachment: 171402

Committed r133046: <http://trac.webkit.org/changeset/133046>
Comment 8 WebKit Review Bot 2012-10-31 11:02:26 PDT
All reviewed patches have been landed.  Closing bug.