Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(31)

Issue 11189037: toggle FPS counter in compositor (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 7 months ago by egraether
Modified:
4 years, 6 months ago
Reviewers:
danakj, nduca, brianderson, enne
CC:
chromium-reviews, cc-bugs_chromium.org, darin-cc_chromium.org, brianderson, slavi
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

The FPS counter displays the framerate in the compositor's HUD layer. This change allows for toggling the FPS counter without using the '--show-fps-counter' command-line flag. WebKit side: https://bugs.webkit.org/show_bug.cgi?id=99660 BUG=154754 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166233

Patch Set 1 #

Total comments: 9

Patch Set 2 : added check for accelerated compositing, removed 'showFPSCounter' from settings, command-line flag … #

Total comments: 5

Patch Set 3 : plumbing 'showFPSCounter' through HUD, added 'createHUDLayerIfNeeded' #

Total comments: 5

Patch Set 4 : added DCHECKs and moved hasFontAtlas into HUDLayer #

Total comments: 6

Patch Set 5 : removed hasFontAtlas methods, put DCHECKs in HUDLayerImpl #

Total comments: 8

Patch Set 6 : restored showFPSCounter in WeblayerTreeView::Settings and removed fontAtlas DCHECKS for flags set i… #

Total comments: 2

Patch Set 7 : Rebase to 165943 #

Patch Set 8 : Rebase to 166046 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -21 lines) Patch
M cc/heads_up_display_layer.h View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M cc/heads_up_display_layer.cc View 1 2 3 4 5 6 7 3 chunks +11 lines, -5 lines 0 comments Download
M cc/heads_up_display_layer_impl.h View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M cc/heads_up_display_layer_impl.cc View 1 2 3 4 5 6 7 3 chunks +7 lines, -1 line 0 comments Download
M cc/layer_tree_host.h View 1 2 3 4 5 6 7 5 chunks +4 lines, -3 lines 0 comments Download
M cc/layer_tree_host.cc View 1 2 3 4 5 6 7 2 chunks +18 lines, -11 lines 0 comments Download
M webkit/compositor_bindings/web_layer_tree_view_impl.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M webkit/compositor_bindings/web_layer_tree_view_impl.cc View 1 2 3 4 5 6 7 3 chunks +8 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 31 (0 generated)
brianderson
Looking good. Let me know when you fix the crash when enabling the fps counter ...
4 years, 7 months ago (2012-10-18 00:13:21 UTC) #1
egraether
replies to Brian Anderson https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_impl.cc File cc/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_impl.cc#newcode149 cc/heads_up_display_layer_impl.cc:149: if (settings.showFPSCounter || layerTreeHostImpl()->showFPSCounter()) { ...
4 years, 7 months ago (2012-10-18 15:56:30 UTC) #2
danakj
https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_impl.cc File cc/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_impl.cc#newcode149 cc/heads_up_display_layer_impl.cc:149: if (settings.showFPSCounter || layerTreeHostImpl()->showFPSCounter()) { On 2012/10/18 15:56:30, egraether ...
4 years, 7 months ago (2012-10-18 16:45:07 UTC) #3
egraether
On 2012/10/18 16:45:07, danakj wrote: > https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_impl.cc > File cc/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_impl.cc#newcode149 > ...
4 years, 7 months ago (2012-10-19 18:15:23 UTC) #4
egraether
On 2012/10/18 00:13:21, Brian Anderson wrote: > Looking good. Let me know when you fix ...
4 years, 7 months ago (2012-10-19 18:22:38 UTC) #5
nduca
> > The crash on the welcome page was fixed by adding a check for ...
4 years, 7 months ago (2012-10-20 19:39:19 UTC) #6
nduca
Some more comments http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.cc File cc/layer_tree_host.cc (right): http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.cc#newcode283 cc/layer_tree_host.cc:283: m_proxy->setShowFPSCounter(show); so what happens if you ...
4 years, 7 months ago (2012-10-20 19:49:41 UTC) #7
danakj
On Sat, Oct 20, 2012 at 3:39 PM, <nduca@chromium.org> wrote: > >> The crash on ...
4 years, 7 months ago (2012-10-20 22:08:30 UTC) #8
egraether
On 2012/10/20 22:08:30, danakj wrote: > On Sat, Oct 20, 2012 at 3:39 PM, <mailto:nduca@chromium.org> ...
4 years, 7 months ago (2012-10-23 01:08:47 UTC) #9
egraether
On 2012/10/20 19:49:41, nduca wrote: > Some more comments > > http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.cc > File cc/layer_tree_host.cc ...
4 years, 7 months ago (2012-10-23 01:24:00 UTC) #10
danakj
Some comments for you. This looks cool, thanks for taking this on. > I changed ...
4 years, 7 months ago (2012-10-23 15:19:53 UTC) #11
egraether
On 2012/10/23 15:19:53, danakj wrote: > https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.cc#newcode281 > cc/layer_tree_host.cc:281: createHUDLayerIfNeeded(); > Can we DCHECK some ...
4 years, 7 months ago (2012-10-23 23:53:59 UTC) #12
enne
https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer.cc File cc/heads_up_display_layer.cc (right): https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer.cc#newcode51 cc/heads_up_display_layer.cc:51: return true; Maybe this should be hasFontAtlas()? https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer.cc#newcode68 cc/heads_up_display_layer.cc:68: ...
4 years, 7 months ago (2012-10-24 18:35:27 UTC) #13
egraether
On 2012/10/24 18:35:27, enne wrote: > https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer.cc#newcode51 > cc/heads_up_display_layer.cc:51: return true; > Maybe this should ...
4 years, 7 months ago (2012-10-24 23:41:13 UTC) #14
enne
(In the future, would you mind responding to code comments with more comments in code, ...
4 years, 7 months ago (2012-10-25 00:10:13 UTC) #15
egraether
I got convinced that removing hasFontAtlas again makes the code a lot cleaner. The WebViewImpl ...
4 years, 7 months ago (2012-10-25 02:01:11 UTC) #16
enne
The code to enable/disable the fps counter at runtime looks good, but I have some ...
4 years, 7 months ago (2012-10-25 17:01:23 UTC) #17
egraether
On 2012/10/25 17:01:23, enne wrote: > The code to enable/disable the fps counter at runtime ...
4 years, 7 months ago (2012-10-25 17:08:28 UTC) #18
enne
Ah, I see. I misread what was going in in ui/compositor/compositor.cc. The --show-fps-counter flag will ...
4 years, 7 months ago (2012-10-29 17:58:57 UTC) #19
egraether
http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc File ui/compositor/compositor.cc (left): http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc#oldcode150 ui/compositor/compositor.cc:150: settings.showFPSCounter = On 2012/10/29 17:58:57, enne wrote: > Won't ...
4 years, 7 months ago (2012-10-30 00:03:57 UTC) #20
enne
http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc File ui/compositor/compositor.cc (left): http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc#oldcode150 ui/compositor/compositor.cc:150: settings.showFPSCounter = On 2012/10/30 00:03:57, egraether wrote: > On ...
4 years, 6 months ago (2012-10-30 16:09:45 UTC) #21
egraether
http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc File ui/compositor/compositor.cc (left): http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc#oldcode150 ui/compositor/compositor.cc:150: settings.showFPSCounter = On 2012/10/30 16:09:45, enne wrote: > On ...
4 years, 6 months ago (2012-10-30 16:41:34 UTC) #22
danakj
On 2012/10/30 16:09:45, enne wrote: > It's for Aura builds, which has a separate compositor ...
4 years, 6 months ago (2012-10-30 16:53:07 UTC) #23
egraether
The setting of the '--ui-' flags in compositor.cc forced me to remove the DCHECKs for ...
4 years, 6 months ago (2012-10-30 20:14:04 UTC) #24
enne
lgtm
4 years, 6 months ago (2012-10-30 20:26:06 UTC) #25
nduca
lgtm
4 years, 6 months ago (2012-11-04 20:54:29 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/egraether@chromium.org/11189037/55011
4 years, 6 months ago (2012-11-05 22:36:00 UTC) #27
commit-bot: I haz the power
Retried try job too often for step(s) browser_tests
4 years, 6 months ago (2012-11-06 00:02:51 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/egraether@chromium.org/11189037/55011
4 years, 6 months ago (2012-11-06 01:03:14 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/egraether@chromium.org/11189037/55011
4 years, 6 months ago (2012-11-06 17:16:45 UTC) #30
commit-bot: I haz the power
4 years, 6 months ago (2012-11-06 18:28:25 UTC) #31
Change committed as 166233
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06