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

Issue 491783003: cc: Resource list for headup display (Closed)

Created:
6 years, 4 months ago by JungJik
Modified:
6 years, 3 months ago
Reviewers:
danakj
CC:
chromium-reviews, cc-bugs_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

cc: Resource list for headup display Current HeadUpDisplayLayerImpl allocates one resource and reuse it continuously. This patch is trying to allocate two resources and swap one of them which is available. so HeadUpDisplayLayerImpl does not need free the resource while the layer is alive. Bug=None Committed: https://crrev.com/695d3e36e0735a5848562a3a4bb50a9c081ab2bd Cr-Commit-Position: refs/heads/master@{#292690}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 13

Patch Set 4 : the patch with ScopedPtrVector #

Total comments: 5

Patch Set 5 : remove unnecessary run in common case. #

Total comments: 5

Patch Set 6 : #

Total comments: 1

Patch Set 7 : #

Total comments: 2

Patch Set 8 : rename to ResourceSizeIsEqualTo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -22 lines) Patch
M cc/layers/heads_up_display_layer_impl.h View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments Download
M cc/layers/heads_up_display_layer_impl.cc View 1 2 3 4 5 6 7 5 chunks +47 lines, -21 lines 0 comments Download

Messages

Total messages: 24 (1 generated)
JungJik
Could you review this patch?
6 years, 4 months ago (2014-08-25 11:51:01 UTC) #1
danakj
Thanks for the patch! https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/40001/cc/layers/heads_up_display_layer_impl.cc#newcode90 cc/layers/heads_up_display_layer_impl.cc:90: if (!resource_provider->InUseByConsumer((*iter)->id())) how come this ...
6 years, 4 months ago (2014-08-25 14:51:21 UTC) #2
JungJik
thanks for reviewing the patch and all comments. I will file new patch soon. BTW, ...
6 years, 4 months ago (2014-08-26 11:16:35 UTC) #3
danakj
On Tue, Aug 26, 2014 at 7:16 AM, <jungjik.lee@samsung.com> wrote: > thanks for reviewing the ...
6 years, 3 months ago (2014-08-26 14:12:15 UTC) #4
JungJik
On 2014/08/26 14:12:15, danakj wrote: > On Tue, Aug 26, 2014 at 7:16 AM, <mailto:jungjik.lee@samsung.com> ...
6 years, 3 months ago (2014-08-27 06:13:02 UTC) #5
danakj
Looks better thanks https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_display_layer_impl.cc#newcode109 cc/layers/heads_up_display_layer_impl.cc:109: to_be_kept.push_back(resources_.take(it)); i feel like the common ...
6 years, 3 months ago (2014-08-27 13:25:01 UTC) #6
JungJik
https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_display_layer_impl.cc#newcode109 cc/layers/heads_up_display_layer_impl.cc:109: to_be_kept.push_back(resources_.take(it)); On 2014/08/27 13:25:01, danakj wrote: > i feel ...
6 years, 3 months ago (2014-08-28 15:55:38 UTC) #7
danakj
On 2014/08/28 15:55:38, JungJik wrote: > https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_display_layer_impl.cc > File cc/layers/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_display_layer_impl.cc#newcode109 > ...
6 years, 3 months ago (2014-08-28 16:44:24 UTC) #8
JungJik
On 2014/08/28 16:44:24, danakj wrote: > On 2014/08/28 15:55:38, JungJik wrote: > > > https://codereview.chromium.org/491783003/diff/60001/cc/layers/heads_up_display_layer_impl.cc ...
6 years, 3 months ago (2014-08-28 17:17:54 UTC) #9
danakj
On Thu, Aug 28, 2014 at 1:17 PM, <jungjik.lee@samsung.com> wrote: > On 2014/08/28 16:44:24, danakj ...
6 years, 3 months ago (2014-08-28 17:21:13 UTC) #10
JungJik
On 2014/08/28 17:21:13, danakj wrote: > On Thu, Aug 28, 2014 at 1:17 PM, <mailto:jungjik.lee@samsung.com> ...
6 years, 3 months ago (2014-08-29 15:01:48 UTC) #11
danakj
On Fri, Aug 29, 2014 at 11:01 AM, <jungjik.lee@samsung.com> wrote: > On 2014/08/28 17:21:13, danakj ...
6 years, 3 months ago (2014-08-29 15:15:01 UTC) #12
JungJik
On 2014/08/29 15:15:01, danakj wrote: > On Fri, Aug 29, 2014 at 11:01 AM, <mailto:jungjik.lee@samsung.com> ...
6 years, 3 months ago (2014-08-29 15:57:43 UTC) #13
danakj
https://codereview.chromium.org/491783003/diff/80001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/80001/cc/layers/heads_up_display_layer_impl.cc#newcode53 cc/layers/heads_up_display_layer_impl.cc:53: class CompareResourceSize { move this down above the function ...
6 years, 3 months ago (2014-08-29 16:42:47 UTC) #14
JungJik
Please take a look again :)
6 years, 3 months ago (2014-08-29 17:34:25 UTC) #15
danakj
https://codereview.chromium.org/491783003/diff/100001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/100001/cc/layers/heads_up_display_layer_impl.cc#newcode116 cc/layers/heads_up_display_layer_impl.cc:116: ScopedPtrVector<ScopedResource>::iterator it_erase = resources_.end(); why set it to end() ...
6 years, 3 months ago (2014-08-29 17:38:25 UTC) #16
JungJik
On 2014/08/29 17:38:25, danakj wrote: > https://codereview.chromium.org/491783003/diff/100001/cc/layers/heads_up_display_layer_impl.cc > File cc/layers/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/491783003/diff/100001/cc/layers/heads_up_display_layer_impl.cc#newcode116 > ...
6 years, 3 months ago (2014-08-29 17:44:49 UTC) #17
danakj
LGTM w/ 1 name change https://codereview.chromium.org/491783003/diff/120001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/120001/cc/layers/heads_up_display_layer_impl.cc#newcode104 cc/layers/heads_up_display_layer_impl.cc:104: explicit CompareResourceSize(const gfx::Size& size_) ...
6 years, 3 months ago (2014-08-29 17:47:17 UTC) #18
JungJik
https://codereview.chromium.org/491783003/diff/120001/cc/layers/heads_up_display_layer_impl.cc File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/491783003/diff/120001/cc/layers/heads_up_display_layer_impl.cc#newcode104 cc/layers/heads_up_display_layer_impl.cc:104: explicit CompareResourceSize(const gfx::Size& size_) : compare_size_(size_) {} On 2014/08/29 ...
6 years, 3 months ago (2014-08-29 18:10:18 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/491783003/140001
6 years, 3 months ago (2014-08-29 18:22:36 UTC) #21
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_compile_dbg on tryserver.chromium.mac ...
6 years, 3 months ago (2014-08-29 19:21:07 UTC) #22
commit-bot: I haz the power
Committed patchset #8 (id:140001) as 3ae0e8598a8de76f8540a706c4f34f51789b2fdf
6 years, 3 months ago (2014-08-29 22:26:53 UTC) #23
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 03:10:47 UTC) #24
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/695d3e36e0735a5848562a3a4bb50a9c081ab2bd
Cr-Commit-Position: refs/heads/master@{#292690}

Powered by Google App Engine
This is Rietveld 408576698