Summary: | Web Inspector: add timeline instrumentation for frame events | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Kosyakov <caseq> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, apavlov, bweinstein, dglazkov, fishd, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Andrey Kosyakov
2012-03-02 01:45:45 PST
Created attachment 129844 [details]
Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. Comment on attachment 129844 [details] Patch Attachment 129844 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11776932 New failing tests: inspector/timeline/timeline-enum-stability.html Comment on attachment 129844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129844&action=review > Source/WebKit/chromium/ChangeLog:20 > + * public/WebWidget.h: > + (WebWidget): > + (WebKit::WebWidget::willBeginUpdate): > + (WebKit::WebWidget::didEndUpdate): > + * src/WebDevToolsAgentImpl.cpp: > + (WebKit::WebDevToolsAgentImpl::willComposeLayers): > + (WebKit): > + (WebKit::WebDevToolsAgentImpl::didComposeLayers): > + (WebKit::WebDevToolsAgentImpl::willBeginFrameUpdate): > + (WebKit::WebDevToolsAgentImpl::didEndFrameUpdate): Please file bugs for the other WebKit ports to hook this up. Created attachment 129892 [details]
Patch
Comment on attachment 129892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129892&action=review > Source/WebCore/inspector/InspectorController.cpp:214 > +void InspectorController::willBeginFrameUpdate() We use will*/did* notation for non-atomic actions, we use did* notation for atomic actions. Should either be didBeginFrame() / didEndFrame() or willProcessFrame / didProcessFrame. > Source/WebCore/inspector/InspectorController.h:83 > + void willBeginFrameUpdate(); I would move it into the instrumentation. > Source/WebCore/inspector/InspectorController.h:85 > + void willBeginCompositing(); What does Begin / End compositing mean? Can it contain nested paints? If yes, when and why? Also, seems unrelated to the change. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:64 > +static const char LayersCompositing[] = "LayersCompositing"; This will make test fail. > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:792 > + var lastFrameTime = records[0] && records[0].startTime; records.length && ... ? > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:828 > + for (; currentFrame < lastIndex && currentRecord < records.length; ++currentRecord) { Could you extract method? > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:-856 > - var rightIndex = rightOffset + barWidth >= this.element.clientWidth ? null : Math.ceil((rightOffset - offset0 - 2) / barWidth * this._recordsPerBar); What is 2 ? > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:909 > + endTime: rightOffset + 3 > windowSpan ? null : this._barTimes[lastBar].endTime What is 3? > Source/WebKit/chromium/public/WebWidget.h:83 > + virtual void willBeginUpdate() { } 1) "update" looks to generic, WebWidget may be updated in various ways 2) We should emphasize the instrumenting nature of these methods. debugWillBeginUpdate instrumentBeginFrame() > Source/WebKit/chromium/public/WebWidget.h:84 > + virtual void didEndUpdate() { } nuke this? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:234 > +void WebDevToolsAgentImpl::willBeginFrameUpdate() revert? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.h:72 > + virtual void willBeginFrameUpdate(); revert? > Source/WebKit/chromium/src/WebDevToolsAgentPrivate.h:46 > + virtual void willBeginFrameUpdate() = 0; revert? > Source/WebKit/chromium/src/WebViewImpl.cpp:1271 > + if (m_devToolsAgent) InspectorInstrumentation::didBeginFrame(m_page.get()) > Source/WebKit/chromium/src/WebViewImpl.cpp:1421 > + if (m_devToolsAgent) Separate change please. Created attachment 129932 [details]
Patch
Comment on attachment 129932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129932&action=review > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:283 > + return this._windowEndTime || this._presentationModel.maximumRecordTime(); !!this._windowEndTime ? > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:809 > + var scale = (this.element.clientHeight - 4) / boundarySpan; Please use const paddingTop = 4; I don't think we should wait for Darin's approval for the WebView::instrumentBeginFrame() and it can be landed safely. Darin, if you have any concerns about the changes to the API, please let us know and we will fix it. Committed r109724: <http://trac.webkit.org/changeset/109724> |