Issue 1411503005: Aura on Android: content/browser

Issue 1411503005: Aura on Android: content/browser (Closed)

Created:
5 years, 2 months ago by mfomitchev

Modified:
5 years, 1 month ago

Reviewers:
dsinclair, dmazzoni, no sievers, shatch

CC:
chromium-reviews, yusukes+watch_chromium.org, zea+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, sievers+watch_chromium.org, dmazzoni+watch_chromium.org, tracing+reviews_chromium.org, tim+watch_chromium.org, nona+watch_chromium.org, aboxhall+watch_chromium.org, jam, jbauman+watch_chromium.org, pvalenzuela+watch_chromium.org, je_julie, darin-cc_chromium.org, jkarlin+watch_chromium.org, devtools-reviews_chromium.org, kalyank, creis+watch_chromium.org, yuzo+watch_chromium.org, feature-media-reviews_chromium.org, piman+watch_chromium.org, plundblad+watch_chromium.org, wfh+watch_chromium.org, maxbogue+watch_chromium.org, nektar+watch_chromium.org, yurys, plaree+watch_chromium.org, dtseng+watch_chromium.org, danakj+watch_chromium.org, James Su, pfeldman

Base URL:
https://chromium.googlesource.com/chromium/src.git@auraclank_upstream_select_file

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Aura on Android: content/browser Changes necessary to get content/browser, content/public/browser, and content/child GN targets to build. BUG=507792 Committed: https://crrev.com/841f818387cab380b6b21158ad94c9b256bb9c32 Cr-Commit-Position: refs/heads/master@{#358496}

Patch Set 1 #

Total comments: 52

Patch Set 2 : Getting rid of BrowserStartupController changes #

Total comments: 9

Patch Set 3 : Some cleanup #

Patch Set 4 : Rebase and changes from crrev.com/1418663003 #

Patch Set 5 : Addressing some feedback. Fixes for content/child and content/public/browser targets. #

Total comments: 19

Patch Set 6 : Addressing remaining feedback, cleaning up build files. #

Total comments: 16

Patch Set 7 : Addressing final comments, Mac GN build fix. #

Total comments: 1

Patch Set 8 : Rebase #

Patch Set 9 : One more rebase #

Created: 5 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+190 lines, -119 lines)			Patch
M	components/tracing/trace_config_file.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	components/tracing/trace_config_file.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/app/android/library_loader_hooks.cc	View	1 2 3 4 5 6 7	2 chunks	+5 lines, -0 lines	0 comments	Download
M	content/browser/BUILD.gn	View	1 2 3 4 5 6 7	3 chunks	+13 lines, -5 lines	0 comments	Download
M	content/browser/accessibility/browser_accessibility_manager.cc	View	1 2 3 4 5 6 7 8	1 chunk	+5 lines, -0 lines	0 comments	Download
M	content/browser/android/browser_jni_registrar.cc	View	1 2 3 4 5 6 7	6 chunks	+23 lines, -18 lines	0 comments	Download
M	content/browser/android/child_process_launcher_android.cc	View	1 2 3 4	2 chunks	+4 lines, -0 lines	0 comments	Download
M	content/browser/android/content_startup_flags.cc	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -0 lines	0 comments	Download
M	content/browser/android/content_video_view.cc	View	1 2 3 4 5	3 chunks	+13 lines, -4 lines	0 comments	Download
M	content/browser/browser_main_loop.cc	View	1 2 3 4 5 6 7	5 chunks	+6 lines, -4 lines	0 comments	Download
M	content/browser/browser_main_runner.cc	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	content/browser/compositor/gpu_process_transport_factory.cc	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
M	content/browser/compositor/surface_utils.cc	View		3 chunks	+3 lines, -3 lines	0 comments	Download
M	content/browser/devtools/render_frame_devtools_agent_host.cc	View	1 2 3 4	1 chunk	+2 lines, -1 line	0 comments	Download
M	content/browser/frame_host/render_widget_host_view_child_frame.cc	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download
M	content/browser/power_save_blocker_impl.cc	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
M	content/browser/renderer_host/render_process_host_impl.cc	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -1 line	0 comments	Download
M	content/browser/renderer_host/render_widget_host_view_aura.cc	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -0 lines	0 comments	Download
M	content/browser/wake_lock/wake_lock_service_context.cc	View	1 2 3 4	1 chunk	+2 lines, -1 line	0 comments	Download
M	content/browser/web_contents/web_contents_impl.h	View	1 2 3 4 5 6 7	5 chunks	+6 lines, -5 lines	0 comments	Download
M	content/browser/web_contents/web_contents_impl.cc	View	1 2 3 4 5 6 7 8	6 chunks	+12 lines, -8 lines	0 comments	Download
M	content/child/BUILD.gn	View	1 2 3 4 5 6 7	3 chunks	+5 lines, -7 lines	0 comments	Download
M	content/content_browser.gypi	View	1 2 3 4 5 6 7	14 chunks	+64 lines, -57 lines	0 comments	Download
M	content/public/browser/native_web_keyboard_event.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	content/public/browser/web_contents.h	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 50 (20 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

mfomitchev

First stab at upstreaming content changes. I will be cleaning this up.

5 years, 2 months ago (2015-10-22 20:10:17 UTC) #2

no sievers

https://codereview.chromium.org/1411503005/diff/1/content/browser/accessibility/browser_accessibility_manager.cc File content/browser/accessibility/browser_accessibility_manager.cc (right): https://codereview.chromium.org/1411503005/diff/1/content/browser/accessibility/browser_accessibility_manager.cc#newcode78 content/browser/accessibility/browser_accessibility_manager.cc:78: return nullptr; ifdef with android && aura and TODO ...

5 years, 2 months ago (2015-10-22 20:55:41 UTC) #3

no sievers

https://codereview.chromium.org/1411503005/diff/20001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/1411503005/diff/20001/content/browser/BUILD.gn#newcode372 content/browser/BUILD.gn:372: "renderer_host/begin_frame_observer_proxy.cc", you can just move this into the else-path ...

5 years, 2 months ago (2015-10-22 21:11:59 UTC) #4

no sievers

I landed some minor cleanups, which simplify your exclusion list: https://codereview.chromium.org/1419123002/ (no need to worry ...

5 years, 2 months ago (2015-10-23 18:13:20 UTC) #7

mfomitchev

5 years, 1 month ago (2015-11-02 21:51:16 UTC) #8

mfomitchev

I will break up this CL - probably factor out accessibility changes and maybe some ...

5 years, 1 month ago (2015-11-02 22:10:43 UTC) #9

no sievers

Regarding splitting it up - feel free but I think it's also gotten pretty manageable ...

5 years, 1 month ago (2015-11-02 22:27:15 UTC) #10

mfomitchev

@sievers: You've previously suggested we remove !USE_AURA ifdef here: https://codereview.chromium.org/1411503005/diff/1/content/browser/browser_main_loop.cc#newcode1166 Are you okay with leaving ...

5 years, 1 month ago (2015-11-02 22:56:32 UTC) #11

no sievers

On 2015/11/02 22:56:32, mfomitchev wrote: > @sievers: You've previously suggested we remove !USE_AURA ifdef here: ...

5 years, 1 month ago (2015-11-02 23:02:18 UTC) #12

mfomitchev

Getting close. Added a couple of comments through the last revision with questions, @sievers - ...

5 years, 1 month ago (2015-11-03 22:23:20 UTC) #13

no sievers

lgtm https://codereview.chromium.org/1411503005/diff/80001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/1411503005/diff/80001/content/browser/BUILD.gn#newcode371 content/browser/BUILD.gn:371: "renderer_host/begin_frame_observer_proxy.cc", On 2015/11/03 22:23:19, mfomitchev wrote: > On ...

5 years, 1 month ago (2015-11-04 01:59:32 UTC) #15

lgtm

https://codereview.chromium.org/1411503005/diff/80001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/1411503005/diff/80001/content/browser/BUILD.g...
content/browser/BUILD.gn:371: "renderer_host/begin_frame_observer_proxy.cc",
On 2015/11/03 22:23:19, mfomitchev wrote:
> On 2015/11/02 22:27:15, sievers wrote:
> > you can just move this into the else-path below where we exclude
> > compositor_resize_lock_aura.cc and other stuff (line 509 right)
> 
> Hmm... currently it is only excluded on Android and iOS. If I move it, it will
> be excluded on all non-Aura platforms, e.g. Mac. BTW we should probably also

It's only used from RWHVAura, so I'd remove it below where we remove
render_widget_host_view_aura. I think it just happens to compile on Mac, because
Mac depends on ui::Compositor (which BeginFrameObserverProxy depends on).

You also have to exclude begin_frame_observer_proxy_unittest.cc, which you can
probably just do from where we remove render_wiget_host_view_aura_unittests.cc
> exclude the .h file, not just the .cc?

sounds good

https://codereview.chromium.org/1411503005/diff/100001/content/browser/androi...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/1411503005/diff/100001/content/browser/androi...
content/browser/android/content_startup_flags.cc:105:
switches::kEnableAndroidCompositorAnimationTimelines))
On 2015/11/03 22:23:20, mfomitchev wrote:
> Should we create a crbug for this? What's the right thing to do here - remove
> the switch on Aura Android?

ui::Compositor does the same thing in ui/compositor/layer.cc:

void Layer::InitializeUILayerSettings() {
  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
  if (command_line->HasSwitch(switches::kUIEnableCompositorAnimationTimelines))
    g_ui_layer_settings.Get().use_compositor_animation_timelines = true;
}

So this is fine. But if you want to avoid the ifdef and rather have it in some
non-aura .cc file somewhere, there is probably a simple way to do that, such as
removing SetLayerSettings and having a wrapper class for the
LazyInstance<cc::LayerSettings> that overrides this one setting based on the
cmdline in its constructor.

https://codereview.chromium.org/1411503005/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1411503005/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:976: #if
defined(OS_ANDROID) && !defined(USE_AURA)
On 2015/11/03 22:23:20, mfomitchev wrote:
> Should we create a crbug for this functionality for Aura Android?

If you want to (basically 'sync play services integration').

https://codereview.chromium.org/1411503005/diff/100001/content/browser/browse...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/1411503005/diff/100001/content/browser/browse...
content/browser/browser_main_runner.cc:267: #if defined(OS_ANDROID) &&
!defined(USE_AURA)
On 2015/11/03 22:23:20, mfomitchev wrote:
> @sievers: Do you know off the top of your head if we doing the right thing
with
> tracing here? (I haven't looked into why we are handing this differently on
> Android). If not - I guess we should create a crbug?

I think it's only for startup and shutdown tracing.
Probably ok to ignore for now and revisit as needed. I think the changes you
made are consistent for that stuff.

https://codereview.chromium.org/1411503005/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1411503005/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:855: void
OnFindMatchRectsReply(int version,
On 2015/11/03 22:23:20, mfomitchev wrote:
> @sievers: What is this used for exactly? Should we create a crbug?

find-in-page.

This will work differently with Aura and it won't need this code, so I think
it's exactly right to ifdef it out here for now.

https://codereview.chromium.org/1411503005/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:859: void OnOpenDateTimeDialog(
On 2015/11/03 22:23:20, mfomitchev wrote:
> @sievers: Should we create a separate bug for this? Or roll it into the popup
> windows bug (do we have one)?

same as above. this will go through a popup created from RWHVAura instead.

https://codereview.chromium.org/1411503005/diff/100001/content/content_browse...
File content/content_browser.gypi (right):

https://codereview.chromium.org/1411503005/diff/100001/content/content_browse...
content/content_browser.gypi:1613:
'browser/renderer_host/compositor_impl_android.cc',
you dropped browser/renderer_host/compositor_impl_android.h

https://codereview.chromium.org/1411503005/diff/100001/content/content_browse...
content/content_browser.gypi:2045: '<@(android_non_aura_browser_sources)',
nit: since it's trivial now to make gyp work for 'content', consider adding the
condition:

'conditions': [
  ['use_aura == 0', {
    'sources': [
    '<@(android_non_aura_browser_sources)',
  ],
}],

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411503005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411503005/100001

5 years, 1 month ago (2015-11-04 02:04:43 UTC) #17

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/25245)

5 years, 1 month ago (2015-11-04 02:35:41 UTC) #19

mfomitchev

https://codereview.chromium.org/1411503005/diff/100001/content/browser/android/content_startup_flags.cc File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/1411503005/diff/100001/content/browser/android/content_startup_flags.cc#newcode105 content/browser/android/content_startup_flags.cc:105: switches::kEnableAndroidCompositorAnimationTimelines)) On 2015/11/04 01:59:32, sievers wrote: > On 2015/11/03 ...

5 years, 1 month ago (2015-11-04 20:49:31 UTC) #20

https://codereview.chromium.org/1411503005/diff/100001/content/browser/androi...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/1411503005/diff/100001/content/browser/androi...
content/browser/android/content_startup_flags.cc:105:
switches::kEnableAndroidCompositorAnimationTimelines))
On 2015/11/04 01:59:32, sievers wrote:
> On 2015/11/03 22:23:20, mfomitchev wrote:
> > Should we create a crbug for this? What's the right thing to do here -
remove
> > the switch on Aura Android?
> 
> ui::Compositor does the same thing in ui/compositor/layer.cc:
> 
> void Layer::InitializeUILayerSettings() {
>   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
>   if
(command_line->HasSwitch(switches::kUIEnableCompositorAnimationTimelines))
>     g_ui_layer_settings.Get().use_compositor_animation_timelines = true;
> }
> 
> So this is fine. But if you want to avoid the ifdef and rather have it in some
> non-aura .cc file somewhere, there is probably a simple way to do that, such
as
> removing SetLayerSettings and having a wrapper class for the
> LazyInstance<cc::LayerSettings> that overrides this one setting based on the
> cmdline in its constructor.
> 
> 
> 

Acknowledged. Seems odd that the flags are different for Aura vs Android, but I
guess there's some reason for that.

https://codereview.chromium.org/1411503005/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1411503005/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:976: #if
defined(OS_ANDROID) && !defined(USE_AURA)
On 2015/11/04 01:59:32, sievers wrote:
> On 2015/11/03 22:23:20, mfomitchev wrote:
> > Should we create a crbug for this functionality for Aura Android?
> 
> If you want to (basically 'sync play services integration').

Done.

https://codereview.chromium.org/1411503005/diff/100001/content/content_browse...
File content/content_browser.gypi (right):

https://codereview.chromium.org/1411503005/diff/100001/content/content_browse...
content/content_browser.gypi:1613:
'browser/renderer_host/compositor_impl_android.cc',
On 2015/11/04 01:59:32, sievers wrote:
> you dropped browser/renderer_host/compositor_impl_android.h

Fixed, thanks. Good eye!

https://codereview.chromium.org/1411503005/diff/100001/content/content_browse...
content/content_browser.gypi:2045: '<@(android_non_aura_browser_sources)',
It needs a bit more than that, but done.

mfomitchev

dsinclair@chromium.org: Please review changes in components/tracing

5 years, 1 month ago (2015-11-04 20:49:49 UTC) #22

mfomitchev

+simonhatch@chromium.org for components/tracing (just a two ifdefs) since Dan is out and then travelling.

5 years, 1 month ago (2015-11-06 16:29:36 UTC) #24

dmazzoni

https://codereview.chromium.org/1411503005/diff/120001/content/browser/accessibility/accessibility_tree_formatter.cc File content/browser/accessibility/accessibility_tree_formatter.cc (right): https://codereview.chromium.org/1411503005/diff/120001/content/browser/accessibility/accessibility_tree_formatter.cc#newcode103 content/browser/accessibility/accessibility_tree_formatter.cc:103: (!defined(OS_ANDROID) || defined(USE_AURA)) && \ This has gotten too ...

5 years, 1 month ago (2015-11-06 16:44:36 UTC) #25

shatch

On 2015/11/06 16:29:36, mfomitchev wrote: > mailto:+simonhatch@chromium.org for components/tracing (just a two ifdefs) since Dan ...

5 years, 1 month ago (2015-11-06 18:17:13 UTC) #26

mfomitchev

Logged crbug.com/552474 to make sure we don't lose track of this.

5 years, 1 month ago (2015-11-06 18:33:27 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411503005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411503005/120001

5 years, 1 month ago (2015-11-06 18:35:05 UTC) #32

commit-bot: I haz the power

Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/91231) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, ...

5 years, 1 month ago (2015-11-06 18:37:42 UTC) #34

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411503005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411503005/140001

5 years, 1 month ago (2015-11-06 20:22:00 UTC) #37

commit-bot: I haz the power

Failed to apply patch for content/browser/accessibility/accessibility_tree_formatter.cc: While running git apply --index -3 -p1; error: patch ...

5 years, 1 month ago (2015-11-06 21:35:10 UTC) #39

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411503005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411503005/160001

5 years, 1 month ago (2015-11-06 21:52:35 UTC) #43

commit-bot: I haz the power

Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/92771)

5 years, 1 month ago (2015-11-07 00:21:36 UTC) #45

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411503005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411503005/160001

5 years, 1 month ago (2015-11-07 00:53:35 UTC) #47

commit-bot: I haz the power

Patchset 9 (id:??) landed as https://crrev.com/841f818387cab380b6b21158ad94c9b256bb9c32 Cr-Commit-Position: refs/heads/master@{#358496}

5 years, 1 month ago (2015-11-07 02:04:12 UTC) #49

mfomitchev

4 years, 11 months ago (2016-01-27 21:21:47 UTC) #50

Message was sent while issue was closed.

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1649483002/ by mfomitchev@chromium.org.

The reason for reverting is: Reverting, since Android Aura has been cancelled..

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages