Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(373)

Issue 1956893003: compositor-worker: Add CompositorProxyClient worker client of CompositorWorker. (Closed)

Created:
4 years, 7 months ago by flackr
Modified:
4 years, 7 months ago
CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, Eric Willigers, gavinp+loader_chromium.org, Nate Chapin, kenrb, loading-reviews_chromium.org, rjwright, rwlbuis, shans, sof, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

compositor-worker: Add CompositorProxyClient worker client of CompositorWorker. Adds the CompositorProxyClient worker to CompositorWorkers. In order to test calling animation frame callbacks a task is created to call them when an animation frame is requested. BUG=430155 TEST=virtual/threaded/fast/compositorworker/request-animation-frame.html Committed: https://crrev.com/de5eb38e0da6ac0ec80b51b77fc562334edb1c47 Cr-Commit-Position: refs/heads/master@{#396018}

Patch Set 1 #

Total comments: 5

Patch Set 2 : Comment and change name of trigger method. #

Total comments: 1

Patch Set 3 : Create TestCompositorProxyClient for CompositorWorkerThreadTest. #

Total comments: 5

Patch Set 4 : Plumb createCompositorProxyClient through WebFrameWidget. #

Total comments: 5

Patch Set 5 : Use local root frame and remove unneeded includes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+288 lines, -11 lines) Patch
A third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/request-animation-frame.html View 1 chunk +21 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/request-animation-frame-expected.txt View 1 chunk +12 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/request-animation-frame.js View 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/CompositorProxyClient.h View 1 1 chunk +39 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/CompositorProxyClient.cpp View 1 chunk +26 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/EmptyClients.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/page/ChromeClient.h View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp View 1 2 3 2 chunks +7 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.h View 3 chunks +5 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp View 1 2 3 3 chunks +12 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorkerMessagingProxy.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorkerMessagingProxy.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp View 1 2 3 3 chunks +14 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/ChromeClientImpl.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/ChromeClientImpl.cpp View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/web/CompositorProxyClientImpl.h View 1 1 chunk +45 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp View 1 2 3 1 chunk +62 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrameWidgetImpl.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewFrameWidget.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewFrameWidget.cpp View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.h View 1 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.cpp View 1 2 3 4 2 chunks +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/web.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrameWidget.h View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (15 generated)
flackr
Hey, this is split off from https://codereview.chromium.org/1895873006 to just add the CompositorProxyClient worker client. I ...
4 years, 7 months ago (2016-05-06 20:00:57 UTC) #2
haraken
https://codereview.chromium.org/1956893003/diff/1/third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp (right): https://codereview.chromium.org/1956893003/diff/1/third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp#newcode51 third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp:51: provideCompositorProxyClientTo(workerClients, document->frame()->chromeClient().createCompositorProxyClient()); As commented in the original CL (https://codereview.chromium.org/1895873006), ...
4 years, 7 months ago (2016-05-09 02:05:55 UTC) #4
kinuko
Worker client plumbing code looks sane to me. https://codereview.chromium.org/1956893003/diff/1/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp (right): https://codereview.chromium.org/1956893003/diff/1/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp#newcode60 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:60: thread()->postTask(BLINK_FROM_HERE, ...
4 years, 7 months ago (2016-05-09 02:41:27 UTC) #5
jbroman
lgtm (carried forward from the original CL) https://codereview.chromium.org/1956893003/diff/1/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp (right): https://codereview.chromium.org/1956893003/diff/1/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp#newcode59 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:59: // For ...
4 years, 7 months ago (2016-05-09 18:44:19 UTC) #6
flackr
https://codereview.chromium.org/1956893003/diff/1/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp (right): https://codereview.chromium.org/1956893003/diff/1/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp#newcode59 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:59: // For now, just post a task to call ...
4 years, 7 months ago (2016-05-10 20:56:13 UTC) #7
haraken
https://codereview.chromium.org/1956893003/diff/20001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1956893003/diff/20001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1078 third_party/WebKit/Source/web/ChromeClientImpl.cpp:1078: return m_webView->createCompositorProxyClient(); Would you help me understand why you ...
4 years, 7 months ago (2016-05-11 00:38:26 UTC) #8
kinuko
On 2016/05/11 00:38:26, haraken wrote: > https://codereview.chromium.org/1956893003/diff/20001/third_party/WebKit/Source/web/ChromeClientImpl.cpp > File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): > > https://codereview.chromium.org/1956893003/diff/20001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1078 > ...
4 years, 7 months ago (2016-05-11 02:57:49 UTC) #9
haraken
On 2016/05/11 02:57:49, kinuko wrote: > On 2016/05/11 00:38:26, haraken wrote: > > > https://codereview.chromium.org/1956893003/diff/20001/third_party/WebKit/Source/web/ChromeClientImpl.cpp ...
4 years, 7 months ago (2016-05-11 03:32:15 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956893003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956893003/20001
4 years, 7 months ago (2016-05-11 16:38:01 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/211778)
4 years, 7 months ago (2016-05-11 17:41:53 UTC) #17
flackr
I forgot to pull in the TestCompositorProxyClient for CompositorWorkerThreadTest, haraken does this still look good?
4 years, 7 months ago (2016-05-11 23:36:41 UTC) #18
kinuko
https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp#newcode69 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:69: void runAnimationFrameCallbacks() override {} We are not testing any ...
4 years, 7 months ago (2016-05-12 09:13:19 UTC) #19
flackr
https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp#newcode69 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:69: void runAnimationFrameCallbacks() override {} On 2016/05/12 at 09:13:19, kinuko ...
4 years, 7 months ago (2016-05-12 16:09:45 UTC) #20
dcheng
https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1078 third_party/WebKit/Source/web/ChromeClientImpl.cpp:1078: return m_webView->createCompositorProxyClient(); Why does this have to go through ...
4 years, 7 months ago (2016-05-12 16:56:15 UTC) #22
flackr
https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1078 third_party/WebKit/Source/web/ChromeClientImpl.cpp:1078: return m_webView->createCompositorProxyClient(); On 2016/05/12 at 16:56:15, dcheng wrote: > ...
4 years, 7 months ago (2016-05-12 17:00:51 UTC) #23
dcheng
https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1078 third_party/WebKit/Source/web/ChromeClientImpl.cpp:1078: return m_webView->createCompositorProxyClient(); On 2016/05/12 at 17:00:51, flackr wrote: > ...
4 years, 7 months ago (2016-05-12 17:09:29 UTC) #24
flackr
On 2016/05/12 at 17:09:29, dcheng wrote: > https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/web/ChromeClientImpl.cpp > File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): > > https://codereview.chromium.org/1956893003/diff/40001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1078 ...
4 years, 7 months ago (2016-05-19 22:02:29 UTC) #25
flackr
dcheng, ping. Can you take a look?
4 years, 7 months ago (2016-05-20 20:07:32 UTC) #26
dcheng
+kenrb as FYI https://codereview.chromium.org/1956893003/diff/60001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1956893003/diff/60001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1079 third_party/WebKit/Source/web/ChromeClientImpl.cpp:1079: return webFrame->frameWidget()->createCompositorProxyClient(); frameWidget() is only non-null ...
4 years, 7 months ago (2016-05-20 23:57:47 UTC) #27
flackr
https://codereview.chromium.org/1956893003/diff/60001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1956893003/diff/60001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1079 third_party/WebKit/Source/web/ChromeClientImpl.cpp:1079: return webFrame->frameWidget()->createCompositorProxyClient(); On 2016/05/20 at 23:57:47, dcheng wrote: > ...
4 years, 7 months ago (2016-05-25 17:47:41 UTC) #28
kenrb
https://codereview.chromium.org/1956893003/diff/60001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1956893003/diff/60001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1079 third_party/WebKit/Source/web/ChromeClientImpl.cpp:1079: return webFrame->frameWidget()->createCompositorProxyClient(); On 2016/05/25 17:47:40, flackr wrote: > On ...
4 years, 7 months ago (2016-05-25 18:02:17 UTC) #30
flackr
On 2016/05/25 at 18:02:17, kenrb wrote: > https://codereview.chromium.org/1956893003/diff/60001/third_party/WebKit/Source/web/ChromeClientImpl.cpp > File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): > > https://codereview.chromium.org/1956893003/diff/60001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode1079 ...
4 years, 7 months ago (2016-05-25 19:56:19 UTC) #31
dcheng
On 2016/05/25 at 19:56:19, flackr wrote: > On 2016/05/25 at 18:02:17, kenrb wrote: > > ...
4 years, 7 months ago (2016-05-25 20:09:51 UTC) #32
dcheng
Also, LGTM
4 years, 7 months ago (2016-05-25 20:10:14 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956893003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956893003/80001
4 years, 7 months ago (2016-05-25 20:12:15 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/117257)
4 years, 7 months ago (2016-05-25 20:35:47 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956893003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956893003/80001
4 years, 7 months ago (2016-05-25 21:35:21 UTC) #40
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 7 months ago (2016-05-25 22:46:22 UTC) #42
commit-bot: I haz the power
4 years, 7 months ago (2016-05-25 22:47:54 UTC) #44
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/de5eb38e0da6ac0ec80b51b77fc562334edb1c47
Cr-Commit-Position: refs/heads/master@{#396018}

Powered by Google App Engine
This is Rietveld 408576698