Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(21)

Issue 1245683004: Mandoline: Merge Surfaces and Views apps (Closed)

Created:
5 years, 5 months ago by Fady Samuel
Modified:
5 years, 4 months ago
Reviewers:
rjkroege, sadrul, jam, sky
CC:
chromium-reviews, kalyank, sadrul, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Mandoline: Merge Surfaces and Views apps Currently, the scroll position of the embedder can fall out of sync with the position of an OOPIF's content on screen. This CL moves towards resolving this issue. See https://docs.google.com/document/d/13ElYs_X_UvZYXOltafyExRs7N3WwtQMBVc1eDLY2dTU/edit The eventual goal is make a mojo::View a cc::Surface or at least a SurfaceClient of some sort such that when the compositor thread moves around some surfaces, they also shift over the position of the views. This CL merges surfaces, views and GL into one thread. TopLevelDisplayClient has a DirectOutputSurface that uses the SurfacesContextProvider as its ContextProvider. This means it talks to the GL driver directly rather than going through shared memory to hop to another thread first. BUG=492389 Committed: https://crrev.com/b8840eae687d6d8797d52e7228b727f1bc5167bd Cr-Commit-Position: refs/heads/master@{#344296}

Patch Set 1 #

Patch Set 2 : Doesn't hang #

Patch Set 3 : Remove DisplayFactory #

Patch Set 4 : Renders #

Patch Set 5 : Cleanup #

Patch Set 6 : More cleanup #

Patch Set 7 : More cleanups. Getting closer to review #

Patch Set 8 : Simplified surfaces plumbing with state object #

Patch Set 9 : Rebased and Works again! Yay! #

Patch Set 10 : Remove display.mojom #

Patch Set 11 : Remove ViewwportParameterListener. More cleanup to do #

Patch Set 12 : Lots of cleanup + Fixed shutdown #

Patch Set 13 : More cleanup #

Patch Set 14 : Remove context_provider.mojom #

Total comments: 57

Patch Set 15 : Rebased. Haven't addressed Rob's comments yet #

Patch Set 16 : Addressed Rob's comments #

Total comments: 12

Patch Set 17 : Addressed comment #

Patch Set 18 : Factor out GpuMemoryTracker #

Patch Set 19 : Factored out GpuMemoryBufferImpl...Hmm...seems like there's more refactoring to do #

Patch Set 20 : Cleanup GpuMemoryBufferImpl a bit #

Patch Set 21 : Rebased #

Patch Set 22 : Fix PDF Viewer rendering #

Patch Set 23 : Rebased #

Total comments: 16

Patch Set 24 : Rebased #

Patch Set 25 : Rebased #

Patch Set 26 : Rebased against SurfacesState #

Patch Set 27 : Rebased #

Patch Set 28 : Removed new GpuMemoryBufferImpl #

Patch Set 29 : August 18 Rebase #

Total comments: 16

Patch Set 30 : Addressed sky's comments #

Patch Set 31 : Rebased #

Patch Set 32 : Fixed bot issues (I hope) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+810 lines, -827 lines) Patch
M components/gpu/public/interfaces/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +0 lines, -2 lines 0 comments Download
M components/html_viewer/html_document.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -1 line 0 comments Download
M components/html_viewer/html_frame.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +1 line, -1 line 0 comments Download
M components/pdf_viewer/pdf_viewer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -1 line 0 comments Download
M components/view_manager/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 4 chunks +2 lines, -5 lines 0 comments Download
M components/view_manager/display_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 5 chunks +17 lines, -14 lines 0 comments Download
M components/view_manager/display_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 5 chunks +21 lines, -34 lines 0 comments Download
M components/view_manager/display_manager_factory.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M components/view_manager/gles2/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +4 lines, -1 line 0 comments Download
M components/view_manager/gles2/command_buffer_driver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 5 chunks +0 lines, -18 lines 0 comments Download
M components/view_manager/gles2/command_buffer_driver.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 4 chunks +1 line, -35 lines 0 comments Download
M components/view_manager/gles2/command_buffer_impl.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +0 lines, -6 lines 0 comments Download
M components/view_manager/gles2/command_buffer_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +0 lines, -15 lines 0 comments Download
A components/view_manager/gles2/command_buffer_local.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +101 lines, -0 lines 0 comments Download
A components/view_manager/gles2/command_buffer_local.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +264 lines, -0 lines 0 comments Download
A components/view_manager/gles2/command_buffer_local_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +21 lines, -0 lines 0 comments Download
M components/view_manager/gles2/gpu_impl.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -2 lines 0 comments Download
M components/view_manager/gles2/mojo_gpu_memory_buffer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +2 lines, -0 lines 0 comments Download
M components/view_manager/gles2/mojo_gpu_memory_buffer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +4 lines, -0 lines 0 comments Download
D components/view_manager/native_viewport/BUILD.gn View 1 1 chunk +0 lines, -24 lines 0 comments Download
D components/view_manager/native_viewport/DEPS View 1 1 chunk +0 lines, -10 lines 0 comments Download
M components/view_manager/native_viewport/onscreen_context_provider.h View 1 1 chunk +0 lines, -58 lines 0 comments Download
D components/view_manager/native_viewport/onscreen_context_provider.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -92 lines 0 comments Download
M components/view_manager/public/interfaces/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +0 lines, -3 lines 0 comments Download
D components/view_manager/public/interfaces/context_provider.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +0 lines, -28 lines 0 comments Download
D components/view_manager/public/interfaces/display.mojom View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -33 lines 0 comments Download
D components/view_manager/public/interfaces/viewport_parameter_listener.mojom View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -12 lines 0 comments Download
M components/view_manager/surfaces/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 3 chunks +8 lines, -25 lines 0 comments Download
D components/view_manager/surfaces/display_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +0 lines, -25 lines 0 comments Download
D components/view_manager/surfaces/display_factory_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +0 lines, -52 lines 0 comments Download
D components/view_manager/surfaces/display_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +0 lines, -86 lines 0 comments Download
D components/view_manager/surfaces/main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +0 lines, -12 lines 0 comments Download
M components/view_manager/surfaces/public/interfaces/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -1 line 0 comments Download
M components/view_manager/surfaces/surfaces_context_provider.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +35 lines, -9 lines 0 comments Download
M components/view_manager/surfaces/surfaces_context_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 3 chunks +71 lines, -24 lines 0 comments Download
A components/view_manager/surfaces/surfaces_context_provider_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +20 lines, -0 lines 0 comments Download
M components/view_manager/surfaces/surfaces_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +3 lines, -0 lines 0 comments Download
D components/view_manager/surfaces/surfaces_service_application.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +0 lines, -72 lines 0 comments Download
D components/view_manager/surfaces/surfaces_service_application.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +0 lines, -68 lines 0 comments Download
A + components/view_manager/surfaces/top_level_display_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 3 chunks +28 lines, -32 lines 0 comments Download
A components/view_manager/surfaces/top_level_display_client.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +120 lines, -0 lines 0 comments Download
M components/view_manager/view_manager_app.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 5 chunks +25 lines, -1 line 0 comments Download
M components/view_manager/view_manager_app.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 5 chunks +30 lines, -3 lines 0 comments Download
M components/view_manager/view_manager_root_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +14 lines, -4 lines 0 comments Download
M components/view_manager/view_manager_root_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +6 lines, -2 lines 0 comments Download
M components/view_manager/view_manager_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +5 lines, -2 lines 0 comments Download
M mandoline/app/core_services_initialization.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +0 lines, -2 lines 0 comments Download
M mandoline/services/core_services/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +0 lines, -1 line 0 comments Download
M mandoline/services/core_services/core_services_application_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +0 lines, -3 lines 0 comments Download
M mandoline/tab/frame_connection.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 3 chunks +1 line, -5 lines 0 comments Download
M mandoline/ui/aura/surface_binding.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M mojo/runner/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 39 (15 generated)
Fady Samuel
Let's chat about this when you get in, it's a fairly ginormous patch. Thanks!
5 years, 4 months ago (2015-08-05 16:48:52 UTC) #3
Fady Samuel
5 years, 4 months ago (2015-08-05 17:01:35 UTC) #5
rjkroege
On 2015/08/05 17:01:35, Fady Samuel wrote: Can you reformat the commit message to be 80c ...
5 years, 4 months ago (2015-08-06 00:15:32 UTC) #6
rjkroege
https://codereview.chromium.org/1245683004/diff/280001/components/view_manager/gles2/command_buffer_driver.h File components/view_manager/gles2/command_buffer_driver.h (left): https://codereview.chromium.org/1245683004/diff/280001/components/view_manager/gles2/command_buffer_driver.h#oldcode43 components/view_manager/gles2/command_buffer_driver.h:43: virtual void UpdateVSyncParameters(base::TimeTicks timebase, why did you take this ...
5 years, 4 months ago (2015-08-06 00:16:22 UTC) #7
Fady Samuel
PTAL https://codereview.chromium.org/1245683004/diff/280001/components/view_manager/gles2/command_buffer_driver.h File components/view_manager/gles2/command_buffer_driver.h (left): https://codereview.chromium.org/1245683004/diff/280001/components/view_manager/gles2/command_buffer_driver.h#oldcode43 components/view_manager/gles2/command_buffer_driver.h:43: virtual void UpdateVSyncParameters(base::TimeTicks timebase, On 2015/08/06 00:16:21, rjkroege ...
5 years, 4 months ago (2015-08-06 16:48:24 UTC) #8
rjkroege
https://codereview.chromium.org/1245683004/diff/280001/components/view_manager/gles2/command_buffer_impl.cc File components/view_manager/gles2/command_buffer_impl.cc (right): https://codereview.chromium.org/1245683004/diff/280001/components/view_manager/gles2/command_buffer_impl.cc#newcode1 components/view_manager/gles2/command_buffer_impl.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. ...
5 years, 4 months ago (2015-08-06 22:09:02 UTC) #9
rjkroege
can you add TODOs everywhere for what we plan changing? There's a lot of code ...
5 years, 4 months ago (2015-08-06 22:39:11 UTC) #10
rjkroege
and: please wrap the commit comment? On 2015/08/06 22:39:11, rjkroege wrote: > can you add ...
5 years, 4 months ago (2015-08-06 22:39:52 UTC) #11
Fady Samuel
PTAL https://codereview.chromium.org/1245683004/diff/250049/components/html_viewer/html_document.cc File components/html_viewer/html_document.cc (right): https://codereview.chromium.org/1245683004/diff/250049/components/html_viewer/html_document.cc#newcode307 components/html_viewer/html_document.cc:307: // TODO(jamesr): Should be mojo:gpu_service On 2015/08/06 22:39:11, ...
5 years, 4 months ago (2015-08-07 01:15:21 UTC) #12
rjkroege
LGTM with the provisos from our extensive discussion. https://codereview.chromium.org/1245683004/diff/450001/components/view_manager/surfaces/command_buffer_local.cc File components/view_manager/surfaces/command_buffer_local.cc (right): https://codereview.chromium.org/1245683004/diff/450001/components/view_manager/surfaces/command_buffer_local.cc#newcode46 components/view_manager/surfaces/command_buffer_local.cc:46: scoped_ptr<gfx::GpuMemoryBuffer> ...
5 years, 4 months ago (2015-08-13 17:45:38 UTC) #13
Fady Samuel
PTAL Rob. I think I've simplified this patch as much as I can. I'd like ...
5 years, 4 months ago (2015-08-17 15:38:05 UTC) #14
Fady Samuel
Passing along to sky@ for OWNER review.
5 years, 4 months ago (2015-08-17 23:51:35 UTC) #16
sky
https://codereview.chromium.org/1245683004/diff/570001/components/view_manager/display_manager.h File components/view_manager/display_manager.h (right): https://codereview.chromium.org/1245683004/diff/570001/components/view_manager/display_manager.h#newcode138 components/view_manager/display_manager.h:138: base::WeakPtrFactory<DefaultDisplayManager> weak_factory_; I think you can nuke this now. ...
5 years, 4 months ago (2015-08-18 17:42:26 UTC) #17
Fady Samuel
PTAL Scott! Thanks! https://codereview.chromium.org/1245683004/diff/570001/components/view_manager/display_manager.h File components/view_manager/display_manager.h (right): https://codereview.chromium.org/1245683004/diff/570001/components/view_manager/display_manager.h#newcode138 components/view_manager/display_manager.h:138: base::WeakPtrFactory<DefaultDisplayManager> weak_factory_; On 2015/08/18 17:42:26, sky ...
5 years, 4 months ago (2015-08-18 19:16:49 UTC) #18
sky
LGTM
5 years, 4 months ago (2015-08-18 20:48:29 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245683004/590001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245683004/590001
5 years, 4 months ago (2015-08-18 20:49:17 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_dbg/builds/104466) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, ...
5 years, 4 months ago (2015-08-18 20:52:45 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245683004/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245683004/610001
5 years, 4 months ago (2015-08-18 21:35:54 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/89676)
5 years, 4 months ago (2015-08-18 21:45:31 UTC) #31
Fady Samuel
+jam@ for pdf_viewer OWNER review.
5 years, 4 months ago (2015-08-18 21:55:15 UTC) #33
jam
On 2015/08/18 21:55:15, Fady Samuel wrote: > +jam@ for pdf_viewer OWNER review. lgtm
5 years, 4 months ago (2015-08-18 21:57:03 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245683004/630001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245683004/630001
5 years, 4 months ago (2015-08-19 18:23:27 UTC) #37
commit-bot: I haz the power
Committed patchset #32 (id:630001)
5 years, 4 months ago (2015-08-19 19:09:43 UTC) #38
commit-bot: I haz the power
5 years, 4 months ago (2015-08-19 19:11:05 UTC) #39
Message was sent while issue was closed.
Patchset 32 (id:??) landed as
https://crrev.com/b8840eae687d6d8797d52e7228b727f1bc5167bd
Cr-Commit-Position: refs/heads/master@{#344296}

Powered by Google App Engine
This is Rietveld 408576698