[chromium] Create a LayerChromium layerTreeAsText. Add HUD to LayerRendererChromium that draws compositor FPS and, optionally, the layer tree.
Created attachment 82907 [details] Patch
Attachment 82907 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7918939
Comment on attachment 82907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82907&action=review I think this would make a great debugging tool. I would like to see it though leverage more of the code that's already in place for ContentLayers for rendering the debug info. > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:565 > + ts << "drawsContent: " << drawsContent() << "\n"; It would be very helpful to also include: 1. layer bounds 2. Render surface info > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791 > + if (!m_hudTexture) I'm not particularly fond of the amount of code that's duplicated between this method and ContentLayerChromium. Is it possible to create new layer type (maybe derived from ContentLayerChromium) that's used for displaying the debug information? It would take a bit of refactoring of the ContentLayerChromium to account for the lack of an m_owner but I think it would be worthwhile. Also mind you that this will need to be timed appropriately with: https://bugs.webkit.org/show_bug.cgi?id=54315 > Source/WebKit/chromium/src/WebViewImpl.cpp:2324 > + m_layerRenderer->setHudEnabled(m_page->settings()->showDebugBorders()); I would prefer activating the HUD with a different flag. The debug borders can be distracting but I can see developer wanting to see just the FPS meter.
(In reply to comment #3) > I think this would make a great debugging tool. I would like to see it though leverage more of the code that's already in place for ContentLayers for rendering the debug info. I'm 100% with you, I hate this duplication. The approach of using ContentLayer is 100% my end goal. I actually have another CL in flight that removes m_owner from ContentLayer to enable this ... I'll upload that shortly so you can see. I created this HUD to help me debug a bug in that change.. So, my proposal is, land this. Once this lands, I'll land my m_owner code and move the drawing into a CCPainterInerface and the uglies will "go away." > It would be very helpful to also include: > 1. layer bounds > 2. Render surface info Totally. > Also mind you that this will need to be timed appropriately with: https://bugs.webkit.org/show_bug.cgi?id=54315 Talked with JamesR about this last night. I think we were thinking to land land his update change, then my m_owner change. Mod a few minor edits, this HUD stuff can go either before or after the update change. > I would prefer activating the HUD with a different flag. It feels clunky that we have to go through the page's Settings object to get this setting in from Chromium. Is there a better way? Page Settings have a showRepaintCounter, I could use that and plumb it up to Chromium. Or should I make something new?
(In reply to comment #4) > (In reply to comment #3) > > I think this would make a great debugging tool. I would like to see it though leverage more of the code that's already in place for ContentLayers for rendering the debug info. > I'm 100% with you, I hate this duplication. The approach of using ContentLayer is 100% my end goal. I actually have another CL in flight that removes m_owner from ContentLayer to enable this ... I'll upload that shortly so you can see. I created this HUD to help me debug a bug in that change.. That's great to hear! > > So, my proposal is, land this. Once this lands, I'll land my m_owner code and move the drawing into a CCPainterInerface and the uglies will "go away." Unless there's a reason to rush this change in, why not make the necessary m_owner changes and then check-in the HUD? I think I'd prefer a single clean check-in. > > > It would be very helpful to also include: > > 1. layer bounds > > 2. Render surface info > Totally. > > > Also mind you that this will need to be timed appropriately with: https://bugs.webkit.org/show_bug.cgi?id=54315 > Talked with JamesR about this last night. I think we were thinking to land land his update change, then my m_owner change. Mod a few minor edits, this HUD stuff can go either before or after the update change. > > > I would prefer activating the HUD with a different flag. > It feels clunky that we have to go through the page's Settings object to get this setting in from Chromium. Is there a better way? Page Settings have a showRepaintCounter, I could use that and plumb it up to Chromium. Or should I make something new? Since this is a chrome-only flag, it can be added to WebSettingsImpl. See this change for an example: https://bugs.webkit.org/attachment.cgi?id=82803
> Unless there's a reason to rush this change in, why not make the necessary m_owner changes and then check-in the HUD? I think I'd prefer a single clean check-in. Didn't mean to imply that I was rushing! I was approaching this from "changes for debugging" and a "changes for threading" perspective. It sounds like you feel that these should be done at the same time so I'll go do that.
Comment on attachment 82907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82907&action=review R- because this will won't submit > Source/WebCore/ChangeLog:10 > + No new tests. (OOPS!) There's a presubmit check that will barf on this - you have to remove this line and (preferably) replace it with some text describing the patch + why it doesn't need a test.
(In reply to comment #6) > > Unless there's a reason to rush this change in, why not make the necessary m_owner changes and then check-in the HUD? I think I'd prefer a single clean check-in. > > Didn't mean to imply that I was rushing! I was approaching this from "changes for debugging" and a "changes for threading" perspective. It sounds like you feel that these should be done at the same time so I'll go do that. Keep the m_owner changes and HUD as separate patches, please :)
Created attachment 83374 [details] Patch
Thinking this through, I think its best to leave the HUD drawing code completely independent of ContentLayer's update code, even though it duplicates some code. Two reasons, based on discussions with jamesr: 1. Given the purpose of this system as a debugging aide, it seems better to have this system work even when, for example, content layer drawing is broken. 2. The refactoring that makes a clean path for GraphicsContext creation is going to while in coming, due to other things in the pipe. (In reply to comment #9) > Created an attachment (id=83374) [details] > Patch
Created attachment 83529 [details] Patch
Comment on attachment 83529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83529&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:363 > + m_presentTimeHistory[m_currentFrameNumber%2] = currentTime(); needs spaces around % > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:897 > + if (m_showRepaintCounter) I believe that the repaint counter in WebKit is somewhat different as it keeps a repaint count per layer (which is quite useful). Is having a global repaint counter for the page useful?
(In reply to comment #10) > Thinking this through, I think its best to leave the HUD drawing code completely independent of ContentLayer's update code, even though it duplicates some code. Two reasons, based on discussions with jamesr: > 1. Given the purpose of this system as a debugging aide, it seems better to have this system work even when, for example, content layer drawing is broken. > 2. The refactoring that makes a clean path for GraphicsContext creation is going to while in coming, due to other things in the pipe. Ok, I see the rationale behind #1 but I think the excessive code duplication outweighs the benefits here. We could create a new class that will serve as a base class for both ContentLayerChromium and some InfoLayerChromium (serving the HUD and any other debug display needs we have) to isolate ContentLayerChromium failures from HUD failure. If that doesn't work, I think at a minimum the code for creating the GraphicsContext for the HUD and drawing the textured quad for it should be moved in a more generic helper class outside the LayerRendererChromium. There's a lot of code there which makes an already large source file even larger. > > > (In reply to comment #9) > > Created an attachment (id=83374) [details] [details] > > Patch
The specific duplication is the setup code for a GraphicsContext. Everything else is HUD specific. Enne is working on a path that will make a PlatformCanvas abstraction that makes this cleaner. When that lands, I will use it. (In reply to comment #13) > (In reply to comment #10) > > Thinking this through, I think its best to leave the HUD drawing code completely independent of ContentLayer's update code, even though it duplicates some code. Two reasons, based on discussions with jamesr: > > 1. Given the purpose of this system as a debugging aide, it seems better to have this system work even when, for example, content layer drawing is broken. > > 2. The refactoring that makes a clean path for GraphicsContext creation is going to while in coming, due to other things in the pipe. > > Ok, I see the rationale behind #1 but I think the excessive code duplication outweighs the benefits here. We could create a new class that will serve as a base class for both ContentLayerChromium and some InfoLayerChromium (serving the HUD and any other debug display needs we have) to isolate ContentLayerChromium failures from HUD failure. > > If that doesn't work, I think at a minimum the code for creating the GraphicsContext for the HUD and drawing the textured quad for it should be moved in a more generic helper class outside the LayerRendererChromium. There's a lot of code there which makes an already large source file even larger. > > > > > > > > (In reply to comment #9) > > > Created an attachment (id=83374) [details] [details] [details] > > > Patch (In reply to comment #13) > (In reply to comment #10) > > Thinking this through, I think its best to leave the HUD drawing code completely independent of ContentLayer's update code, even though it duplicates some code. Two reasons, based on discussions with jamesr: > > 1. Given the purpose of this system as a debugging aide, it seems better to have this system work even when, for example, content layer drawing is broken. > > 2. The refactoring that makes a clean path for GraphicsContext creation is going to while in coming, due to other things in the pipe. > > Ok, I see the rationale behind #1 but I think the excessive code duplication outweighs the benefits here. We could create a new class that will serve as a base class for both ContentLayerChromium and some InfoLayerChromium (serving the HUD and any other debug display needs we have) to isolate ContentLayerChromium failures from HUD failure. > > If that doesn't work, I think at a minimum the code for creating the GraphicsContext for the HUD and drawing the textured quad for it should be moved in a more generic helper class outside the LayerRendererChromium. There's a lot of code there which makes an already large source file even larger. > > > > > > > > (In reply to comment #9) > > > Created an attachment (id=83374) [details] [details] [details] > > > Patch
Created attachment 83751 [details] Patch
Attachment 83751 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7985622
Comment on attachment 83751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83751&action=review Left some nitpicks. I'm not sure how the debugging bits are going to get set - will that go through Settings or some other process? > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:16 > +/* > + * Copyright (C) 2010 Google Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are > + * met: > + * > + * * Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * * Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following disclaimer > + * in the documentation and/or other materials provided with the > + * distribution. > + * * Neither the name of Google Inc. nor the names of its > + * contributors may be used to endorse or promote products derived from > + * this software without specific prior written permission. it's 2011 now, and this is technically the wrong license header format (should be 2-clause). Take a look at Source/WebCore/LICENSE-APPLE and replace the "Apple Inc." in the first line with "Google Inc." to get the most correct incantation. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:79 > + hudSize.setWidth(std::min(2048, m_layerRenderer->rootVisibleRect().width())); > + hudSize.setHeight(std::min(2048, m_layerRenderer->rootVisibleRect().height())); nit: webkit style is to have a using declaration at the top of the .cpp and just use min() > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:87 > + // render pixels into the texture nit: WebKit comments start with a capital letter and end with a period. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:90 > +#if PLATFORM(SKIA) this block gets a lot simpler with enne's PlatformCanvas thingy > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:47 > + CCHeadsUpDisplay(LayerRendererChromium* owner); nit: explicit > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:70 > + double m_presentTimeHistory[2]; // Units are in seconds. nit: include the unit in the type name so it's clearer without having to look at the header. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:73 > + bool m_showFPSCounter : 1; > + bool m_showPlatformLayerTree : 1; no real need to bitpack these, so just leave out the : 1 > Source/WebKit/chromium/public/WebSettings.h:101 > + virtual void setShowFPSCounter(bool) = 0; > + virtual bool showFPSCounter() const = 0; > + virtual void setShowPlatformLayerTree(bool) = 0; > + virtual bool showPlatformLayerTree() const = 0; I'm not sure who/what sets these guys, but if you only plan to manipulate these from within WebViewImpl, then you only need to put the bits on WebSettingsImpl and not WebSettings. WebSettings is supposed to match WebCore::Settings directly - if you have chromium-specific stuff it's better add those on WebSettingsImpl since they don't map to Settings. In WebViewImpl the m_webSettings object is of type OwnPtr<WebSettingsImpl>.
> I'm not sure who/what sets these guys, but if you only plan to manipulate these from within WebViewImpl.... The plan is to have --show-fps-counter and --show-composited-layer-tree as well as related about:flags... http://codereview.chromium.org/6581004/ This forces me to extend WebSettings as well.
Created attachment 84599 [details] Patch
Comment on attachment 84599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84599&action=review Lots more nitpicks. Overall it looks fine, though. I promise that all these nitpicks will make your future WebKit patching life easier :) > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:55 > + > + Unnecessary whitespace here > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:128 > + m_headsUpDisplay.clear(); // explicitly destroy the HUD before the TextureManager dies Caps, period > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:129 > + CCHeadsUpDisplay* getHeadsUpDisplay() { return m_headsUpDisplay.get(); } nit: in webkit getters don't have "get" in the name, so this should be headsUpDisplay(); > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:39 > +#if PLATFORM(SKIA) nit: should be USE(SKIA) nowadays. I don't know why this was changed, tbqh. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:143 > + // draw fps Caps, period > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:158 > + // draw layer tree, if enabled Caps, period > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:40 > +class CCHeadsUpDisplay { > +public: > + explicit CCHeadsUpDisplay(LayerRendererChromium* owner); nit: Since this is single-ownership, add WTF_MAKE_NONCOPYABLE here (like in PlatformCanvas), add a static PassOwnPtr<CCHeadsUpDisplay> create(), and make the constructor private so you have to use the create() function. That way the type system will help make sure that you don't leak these guys accidentally - you have to either assign it to an OwnPtr<> or call .leakPtr() explicitly. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:55 > + void drawHudContents(GraphicsContext* ctx, const IntSize& hudSize); nit: don't name the "ctx" parameter name here.
Created attachment 84653 [details] Patch
Comment on attachment 84653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84653&action=review Looks fine. Two very minor nits - I'm fine with landing as is or going one more. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:114 > +void CCHeadsUpDisplay::drawHudContents(GraphicsContext* ctx, const IntSize& hudSize) > +{ > + FontDescription medFontDesc; nit: WK prefers full words (context, medium) over abbr. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:60 > + CCHeadsUpDisplay(LayerRendererChromium* owner); nit: still needs explicit
Created attachment 84656 [details] Patch
Comment on attachment 84656 [details] Patch Clearing flags on attachment: 84656 Committed r80327: <http://trac.webkit.org/changeset/80327>
All reviewed patches have been landed. Closing bug.