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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 477923004: Create native_viewport_service, don't build it into mojo_shell unless Android (Closed)

Created:
6 years, 4 months ago by DaveMoore
Modified:
5 years, 7 months ago
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org
Project:
chromium
Visibility:
Public.

Description

Create native_viewport_service, don't build it into mojo_shell unless Android BUG= Committed: https://crrev.com/80931e51c8085d8bd44077dc1cd96bc74794e609 Cr-Commit-Position: refs/heads/master@{#294780}

Patch Set 1 #

Patch Set 2 : Get the static build to work #

Total comments: 1

Patch Set 3 : Use InterfaceFactoryImpl<> #

Patch Set 4 : Remove workaround for thunks #

Total comments: 6

Patch Set 5 : Review nits #

Patch Set 6 : Fix build errors #

Patch Set 7 : Remove unnecessary export #

Patch Set 8 : Make the initialize gl test dynamic #

Patch Set 9 : gn fixes #

Patch Set 10 : Add NativeViewportConfig #

Patch Set 11 : Fix gn build #

Patch Set 12 : Disable ValidId on win, also unregister window classes on win #

Patch Set 13 : Resolve merge problem #

Patch Set 14 : Remove native_viewport from BUILD.gn, keeping only native_viewport:lib #

Patch Set 15 : rebase #

Patch Set 16 : Try to get gn build working again #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+215 lines, -126 lines) Patch
M mojo/aura/aura_init.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M mojo/mojo.gyp View 1 2 3 4 5 6 7 8 9 10 11 9 chunks +7 lines, -26 lines 0 comments Download
M mojo/mojo_examples.gypi View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +1 line, -5 lines 0 comments Download
M mojo/mojo_services.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 8 chunks +23 lines, -5 lines 0 comments Download
M mojo/services/html_viewer/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M mojo/services/html_viewer/html_viewer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +17 lines, -0 lines 1 comment Download
M mojo/services/native_viewport/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +14 lines, -9 lines 0 comments Download
M mojo/services/native_viewport/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
A mojo/services/native_viewport/main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +102 lines, -0 lines 0 comments Download
M mojo/services/public/cpp/view_manager/lib/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -3 lines 0 comments Download
M mojo/services/public/cpp/view_manager/lib/view_manager_test_suite.cc View 1 2 3 8 1 chunk +4 lines, -1 line 0 comments Download
M mojo/services/public/interfaces/native_viewport/native_viewport.mojom View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +7 lines, -0 lines 0 comments Download
M mojo/services/view_manager/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M mojo/services/view_manager/ids.h View 1 2 3 4 5 6 2 chunks +1 line, -2 lines 0 comments Download
M mojo/services/view_manager/view_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +11 lines, -7 lines 0 comments Download
M mojo/services/window_manager/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -2 lines 0 comments Download
M mojo/shell/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +1 line, -20 lines 0 comments Download
M mojo/shell/context.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +5 lines, -25 lines 0 comments Download
M mojo/shell/desktop/mojo_main.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +11 lines, -16 lines 0 comments Download
M mojo/shell/in_process_dynamic_service_runner.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 53 (12 generated)
DaveMoore
6 years, 4 months ago (2014-08-15 19:26:17 UTC) #1
DaveMoore
Get the static build to work
6 years, 4 months ago (2014-08-15 21:17:07 UTC) #2
Ben Goodger (Google)
lgtm https://codereview.chromium.org/477923004/diff/20001/mojo/services/native_viewport/main.cc File mojo/services/native_viewport/main.cc (right): https://codereview.chromium.org/477923004/diff/20001/mojo/services/native_viewport/main.cc#newcode30 mojo/services/native_viewport/main.cc:30: BindToRequest(new NativeViewportImpl, &request); Can you just use a ...
6 years, 4 months ago (2014-08-15 21:59:41 UTC) #3
DaveMoore
Use InterfaceFactoryImpl<>
6 years, 4 months ago (2014-08-15 22:10:20 UTC) #4
DaveMoore
The CQ bit was checked by davemoore@chromium.org
6 years, 4 months ago (2014-08-15 22:12:21 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davemoore@chromium.org/477923004/40001
6 years, 4 months ago (2014-08-15 22:14:15 UTC) #6
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win ...
6 years, 4 months ago (2014-08-16 01:35:32 UTC) #7
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-16 03:19:15 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel/builds/7663)
6 years, 4 months ago (2014-08-16 03:19:16 UTC) #9
DaveMoore
Remove workaround for thunks
6 years, 3 months ago (2014-09-02 22:16:44 UTC) #10
DaveMoore
Take another look after changes to get all configs to work, mostly around initialization. ViewManager ...
6 years, 3 months ago (2014-09-02 22:18:25 UTC) #11
Ben Goodger (Google)
https://codereview.chromium.org/477923004/diff/60001/mojo/aura/aura_init.cc File mojo/aura/aura_init.cc (right): https://codereview.chromium.org/477923004/diff/60001/mojo/aura/aura_init.cc#newcode14 mojo/aura/aura_init.cc:14: aura::Env::CreateInstance(false); awesome. what could go wrong?? https://codereview.chromium.org/477923004/diff/60001/mojo/services/html_viewer/html_viewer.cc File mojo/services/html_viewer/html_viewer.cc ...
6 years, 3 months ago (2014-09-02 22:24:16 UTC) #12
DaveMoore
https://codereview.chromium.org/477923004/diff/60001/mojo/aura/aura_init.cc File mojo/aura/aura_init.cc (right): https://codereview.chromium.org/477923004/diff/60001/mojo/aura/aura_init.cc#newcode14 mojo/aura/aura_init.cc:14: aura::Env::CreateInstance(false); On 2014/09/02 22:24:16, Ben Goodger (Google) wrote: > ...
6 years, 3 months ago (2014-09-02 22:39:06 UTC) #13
DaveMoore
Review nits
6 years, 3 months ago (2014-09-02 22:46:28 UTC) #14
DaveMoore
Fix build errors
6 years, 3 months ago (2014-09-02 23:22:55 UTC) #15
Ben Goodger (Google)
On 2014/09/02 23:22:55, DaveMoore wrote: > Fix build errors still lgtm
6 years, 3 months ago (2014-09-03 16:38:53 UTC) #16
DaveMoore
Remove unnecessary export
6 years, 3 months ago (2014-09-03 17:42:50 UTC) #17
DaveMoore
Make the initialize gl test dynamic
6 years, 3 months ago (2014-09-03 18:18:17 UTC) #18
DaveMoore
gn fixes
6 years, 3 months ago (2014-09-03 21:47:26 UTC) #19
DaveMoore
Add NativeViewportConfig
6 years, 3 months ago (2014-09-04 21:44:45 UTC) #20
DaveMoore
Fix gn build
6 years, 3 months ago (2014-09-04 22:20:13 UTC) #21
DaveMoore
Disable ValidId on win, also unregister window classes on win
6 years, 3 months ago (2014-09-12 20:18:16 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/477923004/220001
6 years, 3 months ago (2014-09-12 20:38:52 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_dbg_recipe/builds/4284) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/4133) chromium_presubmit ...
6 years, 3 months ago (2014-09-12 20:57:29 UTC) #26
DaveMoore
Resolve merge problem
6 years, 3 months ago (2014-09-12 22:04:42 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/477923004/240001
6 years, 3 months ago (2014-09-12 22:09:17 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_rel/builds/15176)
6 years, 3 months ago (2014-09-12 22:26:18 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/477923004/240001
6 years, 3 months ago (2014-09-13 03:23:58 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_rel/builds/15281)
6 years, 3 months ago (2014-09-13 03:34:44 UTC) #35
DaveMoore
Remove native_viewport from BUILD.gn, keeping only native_viewport:lib
6 years, 3 months ago (2014-09-13 15:21:48 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/477923004/260001
6 years, 3 months ago (2014-09-13 15:22:41 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_rel/builds/15333)
6 years, 3 months ago (2014-09-13 15:31:28 UTC) #40
DaveMoore
rebase
6 years, 3 months ago (2014-09-13 16:21:55 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/477923004/280001
6 years, 3 months ago (2014-09-13 16:22:22 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_rel/builds/15337)
6 years, 3 months ago (2014-09-13 16:30:04 UTC) #45
DaveMoore
Try to get gn build working again
6 years, 3 months ago (2014-09-15 02:04:00 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/477923004/300001
6 years, 3 months ago (2014-09-15 02:05:13 UTC) #48
commit-bot: I haz the power
Committed patchset #16 (id:300001) as 41f64253ea9c6dc325f2128031b56ec9045c6da9
6 years, 3 months ago (2014-09-15 03:03:40 UTC) #49
commit-bot: I haz the power
Patchset 16 (id:??) landed as https://crrev.com/80931e51c8085d8bd44077dc1cd96bc74794e609 Cr-Commit-Position: refs/heads/master@{#294780}
6 years, 3 months ago (2014-09-15 03:06:43 UTC) #50
DaveMoore
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/554163003/ by davemoore@chromium.org. ...
6 years, 3 months ago (2014-09-15 04:32:44 UTC) #51
Nico
5 years, 7 months ago (2015-05-02 00:45:51 UTC) #53
Message was sent while issue was closed. 
https://codereview.chromium.org/477923004/diff/300001/mojo/services/html_view...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/477923004/diff/300001/mojo/services/html_view...
mojo/services/html_viewer/html_viewer.cc:80:
ui::ResourceBundle::InitSharedInstanceWithPakPath(ui_test_pak_path);
I saw this today. It's pretty weird to load a test-only pak from non-testing
code. Was that intentional?

Powered by Google App Engine
This is Rietveld 408576698