Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6)

Issue 11366094: cc: Create a Region class that wraps SkRegion, to replace use of WebCore::Region. (Closed)

Created:
8 years, 1 month ago by danakj
Modified:
8 years, 1 month ago
Reviewers:
enne (OOO)
CC:
chromium-reviews, cc-bugs_chromium.org, piman, backer
Visibility:
Public.

Description

cc: Create a Region class that wraps SkRegion, to replace use of WebCore::Region. We create a class in cc/ called Region which provides a gfx:: type-friendly API to SkRegion, and allows for easily swapping out region implementations in the future if required. During the process, I removed tests dependency on a "size()" method on Region, that used to give the number of rects in the Region's internal representation. Instead, we always create a Region in the tests from our expected rects, and compare the Regions directly. We use ToString() comparisons to get useful failure outputs, similar to the unit tests of other geometry types in ui/gfx. This uncovered a WTF::Vector holdout in the OcclusionTracker class, which is converted to a std::vector. Covered by existing tests; no change in behaviour. R=enne BUG=147395 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166617

Patch Set 1 #

Patch Set 2 : Move deg2rad etc to MathUtil #

Total comments: 3

Patch Set 3 : nits+Fix compositor bindings #

Patch Set 4 : Add missing algorithm header #

Patch Set 5 : More algorithm headers #

Patch Set 6 : Add unittests (that fail) #

Patch Set 7 : nits #

Patch Set 8 : More tests #

Patch Set 9 : for landing #

Patch Set 10 : export the nested class #

Patch Set 11 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+954 lines, -515 lines) Patch
M cc/DEPS View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download
M cc/cc.gyp View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
M cc/cc_tests.gyp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M cc/layer.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M cc/layer_impl.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M cc/layer_sorter.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M cc/layer_tiling_data.h View 1 chunk +1 line, -1 line 0 comments Download
M cc/layer_tree_host.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -1 line 0 comments Download
M cc/layer_tree_host_impl.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
M cc/layer_tree_host_impl_unittest.cc View 1 2 3 4 5 6 7 8 4 chunks +6 lines, -3 lines 0 comments Download
M cc/layer_tree_host_unittest.cc View 1 2 3 4 5 6 7 8 11 chunks +39 lines, -79 lines 0 comments Download
M cc/math_util.h View 1 1 chunk +8 lines, -0 lines 0 comments Download
M cc/math_util.cc View 1 2 2 chunks +4 lines, -7 lines 0 comments Download
M cc/occlusion_tracker.h View 3 chunks +3 lines, -3 lines 0 comments Download
M cc/occlusion_tracker.cc View 10 chunks +31 lines, -31 lines 0 comments Download
M cc/occlusion_tracker_unittest.cc View 50 chunks +158 lines, -250 lines 0 comments Download
M cc/quad_culler.cc View 1 chunk +0 lines, -1 line 0 comments Download
A cc/region.h View 1 2 3 4 5 6 7 8 9 1 chunk +125 lines, -0 lines 0 comments Download
A cc/region.cc View 1 2 3 4 5 6 7 1 chunk +109 lines, -0 lines 0 comments Download
A cc/region_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +420 lines, -0 lines 0 comments Download
M cc/render_surface_impl.cc View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
D cc/stubs/Region.h View 1 chunk +0 lines, -98 lines 0 comments Download
M cc/test/layer_test_common.cc View 1 chunk +1 line, -1 line 0 comments Download
M cc/test/occlusion_tracker_test_common.h View 2 chunks +4 lines, -5 lines 0 comments Download
M cc/test/tiled_layer_test_common.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M cc/tiled_layer.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -1 line 0 comments Download
M cc/tiled_layer_unittest.cc View 1 2 3 4 5 6 7 8 5 chunks +6 lines, -10 lines 0 comments Download
M cc/tree_synchronizer_unittest.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M webkit/compositor_bindings/web_layer_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +26 lines, -14 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
danakj
8 years, 1 month ago (2012-11-05 21:01:22 UTC) #1
danakj
I moved the Deg2Rad etc mathods to math_util.h from geometry.h, seems a better spot.
8 years, 1 month ago (2012-11-05 21:09:56 UTC) #2
danakj
https://codereview.chromium.org/11366094/diff/3001/cc/math_util.cc File cc/math_util.cc (right): https://codereview.chromium.org/11366094/diff/3001/cc/math_util.cc#newcode12 cc/math_util.cc:12: #include "cc/geometry.h" not needed now, will remove.
8 years, 1 month ago (2012-11-05 21:13:30 UTC) #3
enne (OOO)
https://codereview.chromium.org/11366094/diff/3001/cc/region.cc File cc/region.cc (right): https://codereview.chromium.org/11366094/diff/3001/cc/region.cc#newcode76 cc/region.cc:76: for (Iterator it(*this); it.has_rect(); it.next()) { Is there any ...
8 years, 1 month ago (2012-11-05 21:15:21 UTC) #4
danakj
https://codereview.chromium.org/11366094/diff/3001/cc/region.cc File cc/region.cc (right): https://codereview.chromium.org/11366094/diff/3001/cc/region.cc#newcode76 cc/region.cc:76: for (Iterator it(*this); it.has_rect(); it.next()) { On 2012/11/05 21:15:21, ...
8 years, 1 month ago (2012-11-05 21:18:26 UTC) #5
enne (OOO)
lgtm
8 years, 1 month ago (2012-11-05 21:18:43 UTC) #6
danakj
Thanks! I had missed a change in the web_layer_impl, which, incidentally, wants to use size(). ...
8 years, 1 month ago (2012-11-05 21:23:53 UTC) #7
enne (OOO)
lgtm
8 years, 1 month ago (2012-11-05 21:30:11 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/11366094/5003
8 years, 1 month ago (2012-11-05 21:32:03 UTC) #9
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
8 years, 1 month ago (2012-11-05 22:19:03 UTC) #10
tfarina
Dana have you abandoned this change?
8 years, 1 month ago (2012-11-07 00:18:03 UTC) #11
danakj
On 2012/11/07 00:18:03, tfarina wrote: > Dana have you abandoned this change? No, I am ...
8 years, 1 month ago (2012-11-07 00:35:28 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/11366094/4003
8 years, 1 month ago (2012-11-08 01:25:00 UTC) #13
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
8 years, 1 month ago (2012-11-08 01:37:00 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/11366094/4003
8 years, 1 month ago (2012-11-08 01:40:42 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/11366094/7020
8 years, 1 month ago (2012-11-08 01:46:13 UTC) #16
commit-bot: I haz the power
8 years, 1 month ago (2012-11-08 06:26:55 UTC) #17
Change committed as 166617

Powered by Google App Engine
This is Rietveld 408576698