Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763623002:
[ui/ws] Acknowledge BeginFrames in FrameGenerator + add tests. (Closed)
|
Created:
3 years, 9 months ago by Eric Seckler Modified:
3 years, 9 months ago CC:
chromium-reviews, rjkroege, brianderson, Sami, enne (OOO) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ui/ws] Acknowledge BeginFrames in FrameGenerator + add tests.
This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks
BUG=697086, 646774
Review-Url: https://codereview.chromium.org/2763623002
Cr-Commit-Position: refs/heads/master@{#458707}
Committed: https://chromium.googlesource.com/chromium/src/+/d086be303574fcf89fe19de16307b2a582dd3362
Patch Set 1 #
Total comments: 2
Patch Set 2 : rename LastBeginFrameAckFromFrameGenerator #Patch Set 3 : sync #
Total comments: 8
Patch Set 4 : address comments #
Messages
Total messages: 33 (25 generated)
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ui/ws] Acknowledge BeginFrames to source in FrameGenerator + add tests. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774 ========== to ========== [ui/ws] Acknowledge BeginFrames in FrameGenerator + add tests. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eseckler@chromium.org changed reviewers: + fsamuel@chromium.org
Off to Fady for a first look :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2763623002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2763623002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:178: const cc::BeginFrameAck& LastBeginFrameAckFromFrameGenerator() { nit: FromFrameGenerator is a bit redundant given this is a framegenerator unittest.
The CQ bit was checked by eseckler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2763623002/#ps40001 (title: "rename LastBeginFrameAckFromFrameGenerator")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by eseckler@chromium.org
eseckler@chromium.org changed reviewers: + msw@chromium.org
Thanks! +msw@ for owners. https://codereview.chromium.org/2763623002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2763623002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:178: const cc::BeginFrameAck& LastBeginFrameAckFromFrameGenerator() { On 2017/03/20 18:33:13, Fady Samuel wrote: > nit: FromFrameGenerator is a bit redundant given this is a framegenerator > unittest. Done.
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rubber stamp lgtm, I'm relying on Fady's review competence here. https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:103: begin_frame_args.sequence_number, 0, false); I'm unfamiliar, is it safe to mark this same sequence number as "positively acknowledged (confirmed)" if we go through the early return just below? https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:87: cc::BeginFrameAck current_begin_frame_ack_; what's the difference between last and current? https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:236: EXPECT_EQ(cc::BeginFrameAck(0, 2, 2, 0, true), LastBeginFrameAck()); why not use |expected_ack|? https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:257: // When visible again, frame is produced. nit: 'a frame'?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:103: begin_frame_args.sequence_number, 0, false); On 2017/03/21 17:38:57, msw wrote: > I'm unfamiliar, is it safe to mark this same sequence number as "positively > acknowledged (confirmed)" if we go through the early return just below? At the moment, we don't intend to use the latest_confirmed_sequence_number for FrameGenerator anywhere. That said, it should be valid to set it to the acknowledged BeginFrame's sequence number for two reasons: 1) If the window is invisible, we don't have updates to produce so can safely confirm. 2) If the args are MISSED, we didn't have updates when the BeginFrame was originally issued and thus it's fine to confirm it and include the updates in the next one. https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:87: cc::BeginFrameAck current_begin_frame_ack_; On 2017/03/21 17:38:57, msw wrote: > what's the difference between last and current? This is args vs. ack. The ack is set and used (in GenerateCompositorFrame) while the current BeginFrame is being processed (thus "current ack"). The args are stored to implement the LastUsedBeginFrameArgs() interface of BeginFrameObserver and are intended to be valid after the BeginFrame was completed (thus "last args"). https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:236: EXPECT_EQ(cc::BeginFrameAck(0, 2, 2, 0, true), LastBeginFrameAck()); On 2017/03/21 17:38:58, msw wrote: > why not use |expected_ack|? Done. https://codereview.chromium.org/2763623002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:257: // When visible again, frame is produced. On 2017/03/21 17:38:58, msw wrote: > nit: 'a frame'? Done.
The CQ bit was checked by eseckler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2763623002/#ps80001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490174958627490,
"parent_rev": "58af2e5f92c3c5706d7aba46169001c618684169", "commit_rev":
"d086be303574fcf89fe19de16307b2a582dd3362"}
Message was sent while issue was closed.
Description was changed from ========== [ui/ws] Acknowledge BeginFrames in FrameGenerator + add tests. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774 ========== to ========== [ui/ws] Acknowledge BeginFrames in FrameGenerator + add tests. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774 Review-Url: https://codereview.chromium.org/2763623002 Cr-Commit-Position: refs/heads/master@{#458707} Committed: https://chromium.googlesource.com/chromium/src/+/d086be303574fcf89fe19de16307... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d086be303574fcf89fe19de16307... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
