Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2131783002:
Refactored an entry struct into separate storage. (Closed)
|
Created:
4 years, 5 months ago by ojars Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, hush (inactive) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactored an entry struct into separate storage.
Now synchronous input handler proxies and output surfaces corresponding to some routing_id will be stored in separate maps.
Previously they were stored together in a struct.
Committed: https://crrev.com/999676d872ff3cbcb0c1e225b049be11b8db2ee1
Cr-Commit-Position: refs/heads/master@{#406172}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Implemented suggestions from patch set 1. #
Total comments: 14
Patch Set 3 : Implemented suggestions from patch set 2 #
Total comments: 6
Patch Set 4 : Implemented suggestions from the previous patch. #Patch Set 5 : Fixed a typo. #
Total comments: 1
Messages
Total messages: 16 (5 generated)
ojars@google.com changed reviewers: + boliu@chromium.org
https://codereview.chromium.org/2131783002/diff/1/content/renderer/android/sy... File content/renderer/android/synchronous_compositor_filter.cc (right): https://codereview.chromium.org/2131783002/diff/1/content/renderer/android/sy... content/renderer/android/synchronous_compositor_filter.cc:154: RemoveEntryIfNeeded(routing_id); actually gonna change the behavior here a bit.. This line can just be removed. OutputSurface lifetime is independent of Proxy, and Proxy can handle OutputSurface going away first. https://codereview.chromium.org/2131783002/diff/1/content/renderer/android/sy... content/renderer/android/synchronous_compositor_filter.cc:157: void SynchronousCompositorFilter::CheckIsReady(int routing_id) { This method does 3 things: 1) check if InputHandler is present 2) create Proxy 3) SetOutputSurface (if it's present) 1) should be moved to anywhere that calls this 2 and 3 should probably be factored out into separate methods, then callsite can determine which one if any should be called https://codereview.chromium.org/2131783002/diff/1/content/renderer/android/sy... content/renderer/android/synchronous_compositor_filter.cc:204: CheckIsReady(routing_id); example of comment on line 157, the check part here is redundant, since clearly we just got a InputHandler and can create the Proxy here https://codereview.chromium.org/2131783002/diff/1/content/renderer/android/sy... File content/renderer/android/synchronous_compositor_filter.h (right): https://codereview.chromium.org/2131783002/diff/1/content/renderer/android/sy... content/renderer/android/synchronous_compositor_filter.h:95: OutputSurfaceMap output_surface_map_; so the refactor doesn't go far enough yet, you just split the one map of struct of two members to two maps, which doesn't actually achieve much Other than corner cases, there should be a proxy iff we have a proxy. So other than corner cases, these maps should not be used at all. Both maps are actually still needed to deal with corner cases though. InputHandler map is needed only when we get an InputHandler before FilterReadyyOnCompositorThread OutputSurface map is needed for us getting an OutputSurface for a particular routing_id before InputHandler It would be great if we could rule out any of these corner cases, but I don't think we can.
cc hush as well https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... File content/renderer/android/synchronous_compositor_filter.cc (right): https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:121: if (entry_pair.second) { this should be a DCHECK https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:124: if (ContainsKey(output_surface_map_, routing_id)) { this actually does the loop up twice, use find/iterator and check == end() instead https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:142: DCHECK(!mapped_output_surface); Also should DCHECK output_surface_map_ does not contain an entry already before insert. https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:156: if (ContainsKey(output_surface_map_, routing_id)) ditto about double look up https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:174: DCHECK(ContainsKey(sync_compositor_map_, routing_id)); this is redundant with the DCHECK(proxy) below, remove this https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:194: if (ContainsKey(output_surface_map_, routing_id)) { ditto about double look up https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:210: UnregisterObjects(routing_id); remove the UnregistereObjects an just inline it here, there's only one caller now
Implemented suggestions from patch set 2. https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... File content/renderer/android/synchronous_compositor_filter.cc (right): https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:121: if (entry_pair.second) { On 2016/07/11 15:58:49, boliu wrote: > this should be a DCHECK Done. https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:124: if (ContainsKey(output_surface_map_, routing_id)) { On 2016/07/11 15:58:49, boliu wrote: > this actually does the loop up twice, use find/iterator and check == end() > instead Done. https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:142: DCHECK(!mapped_output_surface); On 2016/07/11 15:58:48, boliu wrote: > Also should DCHECK output_surface_map_ does not contain an entry already before > insert. Done. https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:156: if (ContainsKey(output_surface_map_, routing_id)) On 2016/07/11 15:58:48, boliu wrote: > ditto about double look up Done. https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:174: DCHECK(ContainsKey(sync_compositor_map_, routing_id)); On 2016/07/11 15:58:48, boliu wrote: > this is redundant with the DCHECK(proxy) below, remove this Done. https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:194: if (ContainsKey(output_surface_map_, routing_id)) { On 2016/07/11 15:58:48, boliu wrote: > ditto about double look up Done. https://codereview.chromium.org/2131783002/diff/20001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:210: UnregisterObjects(routing_id); On 2016/07/11 15:58:49, boliu wrote: > remove the UnregistereObjects an just inline it here, there's only one caller > now Done.
https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... File content/renderer/android/synchronous_compositor_filter.cc (right): https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:140: auto entry = output_surface_map_.find(routing_id); you can put this find inside the dcheck, which avoids an extra look up in release builds where dcheck is compiled out https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... File content/renderer/android/synchronous_compositor_filter.h (right): https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.h:100: SynchronousInputHandlerProxyMap synchronous_input_handler_proxy_map_; add comment here, this is only used for before FilterReadyOnCompositorThread https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.h:101: OutputSurfaceMap output_surface_map_; add comment this is only used if input_handler_proxy has not been registered
Did the suggested changes. https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... File content/renderer/android/synchronous_compositor_filter.cc (right): https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.cc:140: auto entry = output_surface_map_.find(routing_id); On 2016/07/17 01:57:02, boliu wrote: > you can put this find inside the dcheck, which avoids an extra look up in > release builds where dcheck is compiled out Done. https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... File content/renderer/android/synchronous_compositor_filter.h (right): https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.h:100: SynchronousInputHandlerProxyMap synchronous_input_handler_proxy_map_; On 2016/07/17 01:57:02, boliu wrote: > add comment here, this is only used for before FilterReadyOnCompositorThread Done. https://codereview.chromium.org/2131783002/diff/40001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.h:101: OutputSurfaceMap output_surface_map_; On 2016/07/17 01:57:02, boliu wrote: > add comment this is only used if input_handler_proxy has not been registered Done.
lgtm
The CQ bit was checked by ojars@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2131783002/#ps80001 (title: "Fixed a typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactored an entry struct into separate storage. Now synchronous input handler proxies and output surfaces corresponding to some routing_id will be stored in separate maps. Previously they were stored together in a struct. ========== to ========== Refactored an entry struct into separate storage. Now synchronous input handler proxies and output surfaces corresponding to some routing_id will be stored in separate maps. Previously they were stored together in a struct. Committed: https://crrev.com/999676d872ff3cbcb0c1e225b049be11b8db2ee1 Cr-Commit-Position: refs/heads/master@{#406172} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/999676d872ff3cbcb0c1e225b049be11b8db2ee1 Cr-Commit-Position: refs/heads/master@{#406172}
Message was sent while issue was closed.
hush@chromium.org changed reviewers: + hush@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2131783002/diff/80001/content/renderer/androi... File content/renderer/android/synchronous_compositor_filter.h (left): https://codereview.chromium.org/2131783002/diff/80001/content/renderer/androi... content/renderer/android/synchronous_compositor_filter.h:72: void CheckIsReady(int routing_id); Sorry for being late, but I think we can delete CheckIsReady here. It seems you can declare a method in .h file, and never implement it in .cc file. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
