Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118653005:
Android: Remove some dead code (Closed)
|
Created:
4 years, 5 months ago by no sievers Modified:
4 years, 5 months ago Reviewers:
danakj CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: Remove some dead code
And collapse a function in preparing for moving out and sharing
the code that handles Surfaces (SubmitCompositorFrame()).
BUG=624666
Committed: https://crrev.com/e0835e17db7ecd698cac1e87eeba474f6210e496
Cr-Commit-Position: refs/heads/master@{#403580}
Patch Set 1 #Patch Set 2 : Android: Remove some dead code #
Total comments: 9
Patch Set 3 : comments #
Total comments: 5
Patch Set 4 : Android: Remove some dead code #
Messages
Total messages: 18 (6 generated)
Description was changed from ========== Android: Remove some dead code And collapse a function in preparing for moving out and sharing the code that handles Surfaces (SubmitCompositorFrame()). BUG=624666 ========== to ========== Android: Remove some dead code And collapse a function in preparing for moving out and sharing the code that handles Surfaces (SubmitCompositorFrame()). BUG=624666 ==========
sievers@chromium.org changed reviewers: + danakj@chromium.org
Dana, ptal.
https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:328: if (CompositorImpl::GetSurfaceManager()) should this be a DCHECK or go away? https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:433: scoped_refptr<cc::Layer> RenderWidgetHostViewAndroid::CreateSurfaceLayer() perhaps this return type can be SurfaceLayer? https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:435: scoped_refptr<cc::Layer> delegated_layer; this variable can go https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1035: DCHECK(CompositorImpl::GetSurfaceManager()); Then does this add value if we DCHECK in constructor? https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:345: scoped_refptr<cc::Layer> layer_; this could be SurfaceLayer
https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:328: if (CompositorImpl::GetSurfaceManager()) On 2016/07/01 23:15:37, danakj wrote: > should this be a DCHECK or go away? I removed it in ps1 but then remembered there is WebView :) https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:433: scoped_refptr<cc::Layer> RenderWidgetHostViewAndroid::CreateSurfaceLayer() On 2016/07/01 23:15:37, danakj wrote: > perhaps this return type can be SurfaceLayer? Done. https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:435: scoped_refptr<cc::Layer> delegated_layer; On 2016/07/01 23:15:37, danakj wrote: > this variable can go Done. https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2118653005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:345: scoped_refptr<cc::Layer> layer_; On 2016/07/01 23:15:37, danakj wrote: > this could be SurfaceLayer Actually I wanted to pull out the Surface-related stuff (like a DelegatedFrameHost) and then have something else that wraps the layer but doesn't need to know about the type (kinda like an aura window).
https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1048: cc::CompositorFrameMetadata metadata = frame.metadata.Clone(); this is cloned here just to move it later. can you move it, or just leave it and move it from the frame later? https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:240: static scoped_refptr<cc::SurfaceLayer> CreateSurfaceLayer( doesnt need to be part of the class either then i guess, can just live in .cc
LGTM after that
https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:240: static scoped_refptr<cc::SurfaceLayer> CreateSurfaceLayer( On 2016/07/01 23:39:40, danakj wrote: > doesnt need to be part of the class either then i guess, can just live in .cc Done.
The CQ bit was checked by sievers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2118653005/#ps60001 (title: "Android: Remove some dead code")
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.
Description was changed from ========== Android: Remove some dead code And collapse a function in preparing for moving out and sharing the code that handles Surfaces (SubmitCompositorFrame()). BUG=624666 ========== to ========== Android: Remove some dead code And collapse a function in preparing for moving out and sharing the code that handles Surfaces (SubmitCompositorFrame()). BUG=624666 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Android: Remove some dead code And collapse a function in preparing for moving out and sharing the code that handles Surfaces (SubmitCompositorFrame()). BUG=624666 ========== to ========== Android: Remove some dead code And collapse a function in preparing for moving out and sharing the code that handles Surfaces (SubmitCompositorFrame()). BUG=624666 Committed: https://crrev.com/e0835e17db7ecd698cac1e87eeba474f6210e496 Cr-Commit-Position: refs/heads/master@{#403580} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e0835e17db7ecd698cac1e87eeba474f6210e496 Cr-Commit-Position: refs/heads/master@{#403580}
Message was sent while issue was closed.
https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1048: cc::CompositorFrameMetadata metadata = frame.metadata.Clone(); On 2016/07/01 23:39:40, danakj wrote: > this is cloned here just to move it later. can you move it, or just leave it and > move it from the frame later? Oh, you left this copying when it doesn't need to tho FYI.
Message was sent while issue was closed.
https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1048: cc::CompositorFrameMetadata metadata = frame.metadata.Clone(); On 2016/07/02 01:09:56, danakj wrote: > On 2016/07/01 23:39:40, danakj wrote: > > this is cloned here just to move it later. can you move it, or just leave it > and > > move it from the frame later? > > Oh, you left this copying when it doesn't need to tho FYI. Hmm but that's as before since OnFrameMetadataUpdated() happens below afterwards. While before that SurfaceFactory::SubmitCompositorFrame() wants the frame by value.
Message was sent while issue was closed.
On Thu, Jul 7, 2016 at 3:25 PM, <sievers@chromium.org> wrote: > > > https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_android.cc > (right): > > > https://codereview.chromium.org/2118653005/diff/40001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_android.cc:1048: > cc::CompositorFrameMetadata metadata = frame.metadata.Clone(); > On 2016/07/02 01:09:56, danakj wrote: > > On 2016/07/01 23:39:40, danakj wrote: > > > this is cloned here just to move it later. can you move it, or just > leave it > > and > > > move it from the frame later? > > > > Oh, you left this copying when it doesn't need to tho FYI. > > Hmm but that's as before since OnFrameMetadataUpdated() happens below > afterwards. > While before that SurfaceFactory::SubmitCompositorFrame() wants the > frame by value. > Ohh yes. Got it. > > https://codereview.chromium.org/2118653005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
