Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1864603003:
Remove redundant layer size check in RecordingSource. (Closed)
|
Created:
4 years, 8 months ago by chrishtr Modified:
4 years, 8 months ago Reviewers:
enne (OOO) CC:
cc-bugs_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove redundant layer size check in RecordingSource.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/7271c4e63ab82f8ff724182a71c71e0186c88b05
Cr-Commit-Position: refs/heads/master@{#388929}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Messages
Total messages: 22 (7 generated)
Description was changed from ========== none none BUG= ========== to ========== none none BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== none none BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Always paint contents if the size of the RecordingSource changed. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
chrishtr@chromium.org changed reviewers: + enne@chromium.org
Seems ok? I'd like to get this in for M51 to avoid any corner case bugs.
I don't think I understand. If the size of the layer has changed, then UpdateInvalidationForNewViewport should add some invalidations. If those invalidations are outside the recording, there's no need to update. When the layer size changes, has the PaintableRegion not changed?
On 2016/04/06 at 21:19:03, enne wrote: > I don't think I understand. If the size of the layer has changed, then UpdateInvalidationForNewViewport should add some invalidations. If those invalidations are outside the recording, there's no need to update. > > When the layer size changes, has the PaintableRegion not changed? It should have changed, yes, but it doesn't seem obviously guaranteed? e.g. in my testcase I could only trigger it with a fake misbehaving client. It may be that this case is impossible, but the code seems unnecessarily a little fragile.
On 2016/04/06 at 21:20:58, chrishtr wrote: > On 2016/04/06 at 21:19:03, enne wrote: > > I don't think I understand. If the size of the layer has changed, then UpdateInvalidationForNewViewport should add some invalidations. If those invalidations are outside the recording, there's no need to update. > > > > When the layer size changes, has the PaintableRegion not changed? > > It should have changed, yes, but it doesn't seem obviously guaranteed? e.g. in my testcase I could only trigger it with a fake misbehaving client. > It may be that this case is impossible, but the code seems unnecessarily a little fragile. The client says "here is the region that I've painted" and the resize includes areas that are outside of that. There's no invalidation needed and no update needed. If the client wants to paint more, it needs to tell the recording source that. Updating here without changing the paintable region would be a waste. The raster source will never raster anything that includes something outside the paintable region, as RasterSource::CoversRect will be false for anything that overlaps that new area. So, updating is a waste without changing the paintable region. And, if you change the paintable region, then the code behaves correctly.
On 2016/04/06 at 21:26:59, enne wrote: > On 2016/04/06 at 21:20:58, chrishtr wrote: > > On 2016/04/06 at 21:19:03, enne wrote: > > > I don't think I understand. If the size of the layer has changed, then UpdateInvalidationForNewViewport should add some invalidations. If those invalidations are outside the recording, there's no need to update. > > > > > > When the layer size changes, has the PaintableRegion not changed? > > > > It should have changed, yes, but it doesn't seem obviously guaranteed? e.g. in my testcase I could only trigger it with a fake misbehaving client. > > It may be that this case is impossible, but the code seems unnecessarily a little fragile. > > The client says "here is the region that I've painted" and the resize includes areas that are outside of that. There's no invalidation needed and no update needed. If the client wants to paint more, it needs to tell the recording source that. > > Updating here without changing the paintable region would be a waste. The raster source will never raster anything that includes something outside the paintable region, as RasterSource::CoversRect will be false for anything that overlaps that new area. > > So, updating is a waste without changing the paintable region. And, if you change the paintable region, then the code behaves correctly. Hmm, ok. Maybe we don't even need the size-change detection that sets updated=true on lines 140-143 then? There is a TODO listed there that honestly I don't fully remember the context of, but maybe it was this.
On 2016/04/06 at 21:36:31, chrishtr wrote: > Hmm, ok. Maybe we don't even need the size-change detection that sets updated=true on lines 140-143 then? > > There is a TODO listed there that honestly I don't fully remember the context of, but maybe it was this. Yeah. That size change detection looks superfluous to me.
Description was changed from ========== Always paint contents if the size of the RecordingSource changed. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Remove redundant layer size check in RecordingSource. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
On 2016/04/06 at 21:53:15, enne wrote: > On 2016/04/06 at 21:36:31, chrishtr wrote: > > > Hmm, ok. Maybe we don't even need the size-change detection that sets updated=true on lines 140-143 then? > > > > There is a TODO listed there that honestly I don't fully remember the context of, but maybe it was this. > > Yeah. That size change detection looks superfluous to me. Updated CL to just delete that then.
lgtm
On 2016/04/06 at 22:16:10, chrishtr wrote: > On 2016/04/06 at 21:53:15, enne wrote: > > On 2016/04/06 at 21:36:31, chrishtr wrote: > > > > > Hmm, ok. Maybe we don't even need the size-change detection that sets updated=true on lines 140-143 then? > > > > > > There is a TODO listed there that honestly I don't fully remember the context of, but maybe it was this. > > > > Yeah. That size change detection looks superfluous to me. > > Updated CL to just delete that then. ... and that caused a bunch of cc unittests to fail. Will fix, but not today.
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/1864603003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864603003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864603003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Apparently no more failing cc tests? O_o?
Message was sent while issue was closed.
On 2016/04/21 at 23:02:39, enne wrote: > Apparently no more failing cc tests? O_o? Oh - it was a silly error on my part. The code to set size_ on line 140 was still needed, I accidentally removed it.
Message was sent while issue was closed.
Description was changed from ========== Remove redundant layer size check in RecordingSource. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Remove redundant layer size check in RecordingSource. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/7271c4e63ab82f8ff724182a71c71e0186c88b05 Cr-Commit-Position: refs/heads/master@{#388929} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7271c4e63ab82f8ff724182a71c71e0186c88b05 Cr-Commit-Position: refs/heads/master@{#388929} |
