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

Issue 11189037: toggle FPS counter in compositor (Closed)

Created:
5 years, 1 month ago by egraether
Modified:
5 years ago
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 #

Messages

Total messages: 31 (0 generated)
brianderson
Looking good. Let me know when you fix the crash when enabling the fps counter ...
5 years, 1 month 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()) { ...
5 years, 1 month 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 ...
5 years, 1 month 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 > ...
5 years, 1 month 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 ...
5 years, 1 month ago (2012-10-19 18:22:38 UTC) #5
nduca
> > The crash on the welcome page was fixed by adding a check for ...
5 years, 1 month 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 ...
5 years, 1 month 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 ...
5 years, 1 month 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> ...
5 years, 1 month 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 ...
5 years, 1 month ago (2012-10-23 01:24:00 UTC) #10
danakj
Some comments for you. This looks cool, thanks for taking this on. > I changed ...
5 years, 1 month 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 ...
5 years, 1 month ago (2012-10-23 23:53:59 UTC) #12
enne (OOO)
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: ...
5 years, 1 month 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 ...
5 years, 1 month ago (2012-10-24 23:41:13 UTC) #14
enne (OOO)
(In the future, would you mind responding to code comments with more comments in code, ...
5 years, 1 month 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 ...
5 years, 1 month ago (2012-10-25 02:01:11 UTC) #16
enne (OOO)
The code to enable/disable the fps counter at runtime looks good, but I have some ...
5 years, 1 month 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 ...
5 years, 1 month ago (2012-10-25 17:08:28 UTC) #18
enne (OOO)
Ah, I see. I misread what was going in in ui/compositor/compositor.cc. The --show-fps-counter flag will ...
5 years 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 ...
5 years ago (2012-10-30 00:03:57 UTC) #20
enne (OOO)
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 ...
5 years 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 ...
5 years 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 ...
5 years 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 ...
5 years ago (2012-10-30 20:14:04 UTC) #24
enne (OOO)
lgtm
5 years ago (2012-10-30 20:26:06 UTC) #25
nduca
lgtm
5 years 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
5 years 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
5 years 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
5 years 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
5 years ago (2012-11-06 17:16:45 UTC) #30
commit-bot: I haz the power
5 years ago (2012-11-06 18:28:25 UTC) #31
Change committed as 166233

Powered by Google App Engine
This is Rietveld efc10ee0f