Issue 331103002: aw: Remove legacy rendering path

Issue 331103002: aw: Remove legacy rendering path (Closed)

Created:
6 years, 6 months ago by boliu

Modified:
6 years, 6 months ago

Reviewers:
hush (inactive), mkosiba (inactive), no sievers

CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

aw: Remove legacy rendering path Remove locks, post tasks, and complexity used to support legacy rendering path. BUG=344087 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277907

Patch Set 3 : remove post tasks #

Total comments: 2

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+101 lines, -606 lines)			Patch
M	android_webview/android_webview.gyp	View		2 chunks	+0 lines, -5 lines	0 comments	Download
M	android_webview/browser/aw_browser_main_parts.cc	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	android_webview/browser/browser_view_renderer.h	View	1	6 chunks	+5 lines, -12 lines	0 comments	Download
M	android_webview/browser/browser_view_renderer.cc	View	1 2	26 chunks	+62 lines, -208 lines	2 comments	Download
M	android_webview/browser/hardware_renderer.h	View	1	2 chunks	+4 lines, -8 lines	0 comments	Download
M	android_webview/browser/hardware_renderer.cc	View	1	1 chunk	+1 line, -2 lines	0 comments	Download
D	android_webview/browser/hardware_renderer_interface.h	View		1 chunk	+0 lines, -26 lines	0 comments	Download
D	android_webview/browser/hardware_renderer_legacy.h	View		1 chunk	+0 lines, -46 lines	0 comments	Download
D	android_webview/browser/hardware_renderer_legacy.cc	View		1 chunk	+0 lines, -106 lines	0 comments	Download
M	android_webview/browser/shared_renderer_state.h	View		5 chunks	+1 line, -25 lines	0 comments	Download
M	android_webview/browser/shared_renderer_state.cc	View		4 chunks	+0 lines, -43 lines	0 comments	Download
D	android_webview/common/aw_switches.h	View		1 chunk	+0 lines, -16 lines	0 comments	Download
D	android_webview/common/aw_switches.cc	View		1 chunk	+0 lines, -19 lines	0 comments	Download
M	android_webview/lib/main/aw_main_delegate.cc	View		2 chunks	+0 lines, -6 lines	0 comments	Download
M	android_webview/native/aw_contents.h	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	android_webview/native/aw_contents.cc	View	1	2 chunks	+3 lines, -16 lines	0 comments	Download
M	content/browser/android/in_process/synchronous_compositor_factory_impl.h	View	1	2 chunks	+1 line, -6 lines	0 comments	Download
M	content/browser/android/in_process/synchronous_compositor_factory_impl.cc	View	1	4 chunks	+7 lines, -17 lines	0 comments	Download
M	content/browser/android/in_process/synchronous_compositor_impl.h	View	1	1 chunk	+2 lines, -4 lines	0 comments	Download
M	content/browser/android/in_process/synchronous_compositor_impl.cc	View	1 2	3 chunks	+5 lines, -16 lines	0 comments	Download
M	content/browser/android/in_process/synchronous_compositor_output_surface.h	View	1	1 chunk	+1 line, -2 lines	0 comments	Download
M	content/browser/android/in_process/synchronous_compositor_output_surface.cc	View	1	4 chunks	+3 lines, -12 lines	0 comments	Download
M	content/public/browser/android/synchronous_compositor.h	View	1	3 chunks	+4 lines, -8 lines	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

hush (inactive)

https://codereview.chromium.org/331103002/diff/1/android_webview/browser/browser_view_renderer.cc File android_webview/browser/browser_view_renderer.cc (left): https://codereview.chromium.org/331103002/diff/1/android_webview/browser/browser_view_renderer.cc#oldcode297 android_webview/browser/browser_view_renderer.cc:297: DidComposite(!result->clip_contains_visible_rect); after you delete this line, DidComposite will always ...

6 years, 6 months ago (2014-06-16 21:41:50 UTC) #2

boliu

All good catches. Anything else I missed? :) https://codereview.chromium.org/331103002/diff/1/android_webview/browser/browser_view_renderer.cc File android_webview/browser/browser_view_renderer.cc (left): https://codereview.chromium.org/331103002/diff/1/android_webview/browser/browser_view_renderer.cc#oldcode297 android_webview/browser/browser_view_renderer.cc:297: DidComposite(!result->clip_contains_visible_rect); ...

6 years, 6 months ago (2014-06-16 22:43:27 UTC) #3

All good catches. Anything else I missed? :)

https://codereview.chromium.org/331103002/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (left):

https://codereview.chromium.org/331103002/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:297:
DidComposite(!result->clip_contains_visible_rect);
On 2014/06/16 21:41:50, hush wrote:
> after you delete this line, DidComposite will always be called with "false".
> 
> what are the circumstances that you force invalidation?

Good catch. Removed the arg to DidComposite

Force means we don't care if the compositor wants to invalidate, we invalidate
anyway. After this, it's only used for ClearView.

https://codereview.chromium.org/331103002/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/331103002/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:265: hardware_enabled_ =
compositor_->InitializeHwDraw(NULL);
On 2014/06/16 21:41:50, hush wrote:
> what does it mean to initialize hw draw with a NULL GLSurface?

Good catch again, removed the GLSurface path.

Normally creating a context (client side) will create its own GLSurface on the
service side. Webview had to punch throw all the layers to insert our own
GLSurface due to our own requirement, but that's no longer needed, at least the
bits through SyncCompositor bit.

The context in HardwareRenderer still uses it, but it creates the context
directly now.

https://codereview.chromium.org/331103002/diff/1/android_webview/browser/hard...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/331103002/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:127: bool
HardwareRenderer::DrawGL(bool stencil_enabled,
On 2014/06/16 21:41:50, hush wrote:
> what is this? why is it not used?

Ehh...I think I dropped this accidentally.

So when functor is called, frameworks gives us a clip, which is a rectangle. But
if the view is rotated in 3d, the "clip" isn't a rectangle, in which case
framework should set the stencil properly before calling us, and we tell the
compositor to use the existing stencil. It's an edge case that's clearly been
broken accidentally for a few months and no one noticed...

Right now, we should do it in the parent compositor now, which is here. This
means plumbing in SyncCompositor can be removed again :)

Going to fix this first separately before landing this.

boliu

+sievers for content sync compositor stuff removing code lalala~

6 years, 6 months ago (2014-06-16 23:37:28 UTC) #5

no sievers

lgtm https://codereview.chromium.org/331103002/diff/20001/content/browser/android/in_process/synchronous_compositor_factory_impl.cc File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/331103002/diff/20001/content/browser/android/in_process/synchronous_compositor_factory_impl.cc#newcode68 content/browser/android/in_process/synchronous_compositor_factory_impl.cc:68: NULL /* surface */, Can we remove the ...

6 years, 6 months ago (2014-06-17 00:32:35 UTC) #6

boliu

https://codereview.chromium.org/331103002/diff/20001/content/browser/android/in_process/synchronous_compositor_factory_impl.cc File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/331103002/diff/20001/content/browser/android/in_process/synchronous_compositor_factory_impl.cc#newcode68 content/browser/android/in_process/synchronous_compositor_factory_impl.cc:68: NULL /* surface */, On 2014/06/17 00:32:35, sievers wrote: ...

6 years, 6 months ago (2014-06-17 00:44:39 UTC) #7

boliu

Hey, Martin is owner of content/public/browser/android now. Stampity stamp?

6 years, 6 months ago (2014-06-17 00:48:14 UTC) #8

boliu

removed some more stuff, locks and PostTasks that are no longer needed

6 years, 6 months ago (2014-06-17 17:07:04 UTC) #10

hush (inactive)

https://codereview.chromium.org/331103002/diff/40001/android_webview/browser/browser_view_renderer.cc File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/331103002/diff/40001/android_webview/browser/browser_view_renderer.cc#newcode296 android_webview/browser/browser_view_renderer.cc:296: base::Bind(&BrowserViewRenderer::DidDrawDelegated, i see the only place where DidDrawDelegated is ...

6 years, 6 months ago (2014-06-17 17:45:54 UTC) #11

boliu

6 years, 6 months ago (2014-06-17 17:47:23 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/boliu@chromium.org/331103002/40001

6 years, 6 months ago (2014-06-17 18:17:26 UTC) #14

Message was sent while issue was closed.

Change committed as 277907

Expand Messages | Collapse Messages