This plumbs willCommit() and didComposite() from CCThreadProxy and CCSingleThreadProxy to InspectorInsturmentation. Note that we currently only account for tasks happening on the main renderer thread, so in case of threaded compositing this is limited to commit. See also bug 83842 for related inspector-side change.
Created attachment 137122 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 137122 [details] Patch Attachment 137122 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12405130
Comment on attachment 137122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137122&action=review Looks cool. A few thoughts but nothing major. :) > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:68 > + virtual void didComposite() = 0; didComposite -> didBeginFrame --- we have willBeginFrame, this is the closing of that. > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:284 > if (!m_layerTreeHost->updateLayers(updater)) Fyi, this updateLayers call is the bulk of the time between willBeginFrame/didBeginFrame. If painting is heavy, e.g. due to rounded corners or gradients etc, this will be big. But it includes singifiant costs more than just WebCore::*:paint(), for example texture upload. I'd call this compositedPaint, or paintLayers. Some percentage of it will be caught by the existing paint instrumentation, but not all. Do you want to hook this while you're at it? willPaintLayers/didPaintLayers would be a good name at least on the CCLayerTreeHostClient.
(In reply to comment #4) > Looks cool. A few thoughts but nothing major. :) > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:68 > > + virtual void didComposite() = 0; > > didComposite -> didBeginFrame --- we have willBeginFrame, this is the closing of that. Done. > > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:284 > > if (!m_layerTreeHost->updateLayers(updater)) > > Fyi, this updateLayers call is the bulk of the time between willBeginFrame/didBeginFrame. > > If painting is heavy, e.g. due to rounded corners or gradients etc, this will be big. But it includes singifiant costs more than just WebCore::*:paint(), for example texture upload. > > I'd call this compositedPaint, or paintLayers. Some percentage of it will be caught by the existing paint instrumentation, but not all. > > Do you want to hook this while you're at it? willPaintLayers/didPaintLayers would be a good name at least on the CCLayerTreeHostClient. Yup, I noticed that, it's just that I thought this should be a separate change (we're also likely want to tweak front-end for that).
Created attachment 142490 [details] Patch
Comment on attachment 142490 [details] Patch Attachment 142490 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12716625
ping :-)
Comment on attachment 142490 [details] Patch The approach looks good, but please add unit tests in CCLayerTreeHostTest.cpp.
Created attachment 143612 [details] Patch
(In reply to comment #9) > (From update of attachment 142490 [details]) > The approach looks good, but please add unit tests in CCLayerTreeHostTest.cpp. Fixed, though I decided we better test these on WebLayerTreeView, given it's a part of the public interface -- so adding WebLayerTreeViewTest.
Attachment 143612 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:28: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 143612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143612&action=review Looks good but be careful when landing not to compile-bust chromium. Safest is to land the chromium implementations of the new functions first, I think. > Source/Platform/chromium/public/WebLayerTreeViewClient.h:44 > + // Indicates that main thread tasks associated with frame rendering have completed. > + // Issued unconditionally, even if the context was lost in the process. > + virtual void didBeginFrame() = 0; There are implementations of WebLayerTreeViewClient outside the WebKit repository (like ui/compositor/compositor.h in chromium), so adding a new pure virtual means you have to land the chromium-side impls first or add some #ifdef guards nit: newline between the previous function and this comment block, please >> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:28 >> +#include <gmock/gmock.h> > > Alphabetical sorting problem. [build/include_order] [4] Just bulk select this whole #include block and run sort on it >> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:30 >> +#include "platform/WebLayer.h" > > Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Here the stylebot is just wrong. a sort of the whole block is right. > Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:33 > +#include "platform/WebLayerTreeView.h" > +#include "platform/WebLayerTreeViewClient.h" > +#include "platform/WebThread.h" nit: include this as <public/Web*.h>, please
Comment on attachment 143612 [details] Patch Attachment 143612 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12769302
Committed r118513: <http://trac.webkit.org/changeset/118513>
(In reply to comment #13) > (From update of attachment 143612 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143612&action=review > > Looks good but be careful when landing not to compile-bust chromium. Safest is to land the chromium implementations of the new functions first, I think. Thanks! I think we just can have an empty default implementation of these methods rather than making them pure virtual. I made them pure originally to see the usages better (and I missed the one in ui/compositor/compositor.h that you pointed out as it only appears to be compiled for Chromium OS). Given that these are just notifications, I think we don't have to force clients to implement them. > nit: newline between the previous function and this comment block, please Fixed. > >> Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:28 > >> +#include <gmock/gmock.h> > > > > Alphabetical sorting problem. [build/include_order] [4] > > Just bulk select this whole #include block and run sort on it Sorry for that, I actually had the local version that check-webkit-style was happy with. I forgot to commit it locally before using webkit-patch upload -g HEAD to skip the inspector part of the patch -- so it picked an earlier revision for upload, while running style check on top of (correct) locally-modified version. > > Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:33 > > +#include "platform/WebLayerTreeView.h" > > +#include "platform/WebLayerTreeViewClient.h" > > +#include "platform/WebThread.h" > > nit: include this as <public/Web*.h>, please Fixed. I've also fixed a somewhat bold way in which test timeouts used to be handled, so that timeout task is canceled once we're done with the test.