Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1410153003:
Browser frame for Aura Android. (Closed)
DescriptionImplement initial version of browser frame for Aura Android.
This implementation assumes that we have only one root window.
We will need to change ChromeBrowserMainExtraPartsViews::ToolkitInitialized() to call SetHost() to make this CL work.
BUG=550423
Committed: https://crrev.com/2b42825136a8202715d4df8b623689f1c8e713b3
Cr-Commit-Position: refs/heads/master@{#357623}
Patch Set 1 #Patch Set 2 : Removed unrelated changes + formatting. #
Total comments: 22
Patch Set 3 : Addressed feedback from patch set 2 partially. Going to look into the remaining items. #Patch Set 4 : #Patch Set 5 : Try to fix analyze errors in some try bots. #Patch Set 6 : Used static SetHost() to access WTH from browser_frame_android. #
Total comments: 10
Patch Set 7 : Addressed feedback from patch set 6. #
Total comments: 13
Patch Set 8 : Addressed feedback fro patch set 7. #
Total comments: 4
Patch Set 9 : Addressed mfomitchev's feedback. #
Depends on Patchset: Messages
Total messages: 33 (9 generated)
moshayedi@chromium.org changed reviewers: + mfomitchev@chromium.org, sievers@chromium.org
https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_android.cc:8: #include "chrome/browser/ui/browser_finder.h" nit: some headers look unused https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_android.h (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_android.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/native_browser_frame_factory_android.cc (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/native_browser_frame_factory_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/1410153003/diff/20001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2212: 'browser/ui/views/frame/native_browser_frame.h', nit: might as well move native_browser_frame_factory_android.cc over here too for consistency, and revert gn changes https://codereview.chromium.org/1410153003/diff/20001/components/copresence/p... File components/copresence/proto/enums.proto (right): https://codereview.chromium.org/1410153003/diff/20001/components/copresence/p... components/copresence/proto/enums.proto:8: ANDROIDOS = 3; why this change? https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.cc:29: static WindowTreeHostAndroid* instance = nullptr; this class is not a singleton, there will be multiple, so this doesn't work
https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/10/28 20:04:51, sievers wrote: > nit: 2015 Done. https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_android.cc:8: #include "chrome/browser/ui/browser_finder.h" On 2015/10/28 20:04:51, sievers wrote: > nit: some headers look unused Done. Need to double-check when we are able compile things. https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_android.h (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_android.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/10/28 20:04:51, sievers wrote: > nit: 2015 Done. https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/native_browser_frame_factory_android.cc (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/native_browser_frame_factory_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/10/28 20:04:51, sievers wrote: > nit: 2015 Done. https://codereview.chromium.org/1410153003/diff/20001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2212: 'browser/ui/views/frame/native_browser_frame.h', On 2015/10/28 20:04:51, sievers wrote: > nit: might as well move native_browser_frame_factory_android.cc over here too > for consistency, and revert gn changes Done. https://codereview.chromium.org/1410153003/diff/20001/components/copresence/p... File components/copresence/proto/enums.proto (right): https://codereview.chromium.org/1410153003/diff/20001/components/copresence/p... components/copresence/proto/enums.proto:8: ANDROIDOS = 3; On 2015/10/28 20:04:51, sievers wrote: > why this change? When we compile for android, g++ gets the flag "-DANDROID", and then pre-processor removes every ANDROID in the code and we get some compile errors. This change was to avoid this situation. https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.cc:29: static WindowTreeHostAndroid* instance = nullptr; On 2015/10/28 20:04:51, sievers wrote: > this class is not a singleton, there will be multiple, so this doesn't work Going to look into this.
https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:432: sources += [ "//chrome/browser/ui/views/frame/native_browser_frame_factory_android.cc" ] All the other native_browser_frame files are in chrome_browser_ui_views_non_mac_sources in chrome_browser_ui.gypi. We could put this file there as well, but then we'd need to exclude it in a bunch of different places. Instead, I suggest we add it where we add chrome_browser_ui_views_non_mac_sources in this file (line 238). https://codereview.chromium.org/1410153003/diff/20001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2212: 'browser/ui/views/frame/native_browser_frame.h', On 2015/10/28 20:04:51, sievers wrote: > nit: might as well move native_browser_frame_factory_android.cc over here too > for consistency, and revert gn changes Note we'd have to exclude it in 4 different places instead of adding it in one if we do that (see my comment in BUILD.gn). https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.cc:29: static WindowTreeHostAndroid* instance = nullptr; @sievers: added you to https://codereview.chromium.org/1408373005/ https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_platform.h (right): https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.h:27: static WindowTreeHostPlatform* GetHost(); biao is already in the process of landing this specific change (in a slightly different way) - see https://codereview.chromium.org/1408373005/. Can you rebase on top of that and exclude it from your CL?
mfomitchev@chromium.org changed reviewers: + bshe@chromium.org
+bshe as the original author of most of these changes.
https://codereview.chromium.org/1410153003/diff/20001/components/copresence/p... File components/copresence/proto/enums.proto (right): https://codereview.chromium.org/1410153003/diff/20001/components/copresence/p... components/copresence/proto/enums.proto:8: ANDROIDOS = 3; On 2015/10/29 15:27:02, Hadi wrote: > On 2015/10/28 20:04:51, sievers wrote: > > why this change? > > When we compile for android, g++ gets the flag "-DANDROID", and then > pre-processor removes every ANDROID in the code and we get some compile errors. > This change was to avoid this situation. It would be nice to land this change separately in a different CL. It doesnt seem to directly relate to other changes in this CL. https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_platform.h (right): https://codereview.chromium.org/1410153003/diff/20001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.h:27: static WindowTreeHostPlatform* GetHost(); On 2015/10/29 15:53:44, mfomitchev wrote: > biao is already in the process of landing this specific change (in a slightly > different way) - see https://codereview.chromium.org/1408373005/. Can you rebase > on top of that and exclude it from your CL? looks like we dont want to add the GetHost function? I have closed the CL without upstream.
> looks like we dont want to add the GetHost function? I have closed the CL > without upstream. Yes. I've discussed this with Sadrul offline, and here's what he suggested: 1. Create a static SetHost function in BrowserFrameAndroid. Assume that it will be called before the browser frame is created. Use that host in BrowserFrameAndroid::GetWidgetParams() (add a DCHECK there ensuring it is non-null). 2. Call that function from ChromeBrowserMainExtraPartsViews::ToolkitInitialized(). Also move WTH/WMTestHelper initialization there from ChromeBrowserMainPartsAndroid::PreProfileInit(). (We will stop using the actual WMTestHelper class, but that's a separate topic). In this CL we only need to be concerned with (1).
https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/1410153003/diff/20001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:432: sources += [ "//chrome/browser/ui/views/frame/native_browser_frame_factory_android.cc" ] On 2015/10/29 15:53:44, mfomitchev wrote: > All the other native_browser_frame files are in > chrome_browser_ui_views_non_mac_sources in chrome_browser_ui.gypi. We could put > this file there as well, but then we'd need to exclude it in a bunch of > different places. Instead, I suggest we add it where we add > chrome_browser_ui_views_non_mac_sources in this file (line 238). Done. Can you please double check if I got what you meant right? https://codereview.chromium.org/1410153003/diff/20001/components/copresence/p... File components/copresence/proto/enums.proto (right): https://codereview.chromium.org/1410153003/diff/20001/components/copresence/p... components/copresence/proto/enums.proto:8: ANDROIDOS = 3; On 2015/10/30 16:29:10, bshe wrote: > On 2015/10/29 15:27:02, Hadi wrote: > > On 2015/10/28 20:04:51, sievers wrote: > > > why this change? > > > > When we compile for android, g++ gets the flag "-DANDROID", and then > > pre-processor removes every ANDROID in the code and we get some compile > errors. > > This change was to avoid this situation. > > It would be nice to land this change separately in a different CL. It doesnt > seem to directly relate to other > changes in this CL. Moved this to 1415533018.
On 2015/10/30 16:45:41, mfomitchev wrote: > > looks like we dont want to add the GetHost function? I have closed the CL > > without upstream. > > Yes. I've discussed this with Sadrul offline, and here's what he suggested: > > 1. Create a static SetHost function in BrowserFrameAndroid. Assume that it will > be called before the browser frame is created. Use that host in > BrowserFrameAndroid::GetWidgetParams() (add a DCHECK there ensuring it is > non-null). > 2. Call that function from > ChromeBrowserMainExtraPartsViews::ToolkitInitialized(). Also move > WTH/WMTestHelper initialization there from > ChromeBrowserMainPartsAndroid::PreProfileInit(). (We will stop using the actual > WMTestHelper class, but that's a separate topic). > > In this CL we only need to be concerned with (1). Made the changes for (1) in this CL.
Description was changed from ========== Browser frame for Aura Android. BUG=NONE ========== to ========== Browser frame for Aura Android. BUG=550423 ==========
https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:247: if (is_android && !use_aura) { I think you can just use if (!use_aura) here. All files end with _android will not be included in android builds. https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:21: static aura::WindowTreeHostPlatform* host = nullptr; This doesnt need to be static I believe. https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:29: Add comment "// static" before SetHost so it is easier to read.
https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:247: if (is_android && !use_aura) { Actually it looks like we can just remove this, since there's already a check for views in line 220 (and we don't plan to have non-Aura toolkit_views on Android). https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:21: static aura::WindowTreeHostPlatform* host = nullptr; no need for static since you are using anonymous namespace https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:31: aura::WindowTreeHostPlatform* window_tree_host) { nit: call this "host" so it the function signature fits on one line? :)
https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:247: if (is_android && !use_aura) { On 2015/11/02 16:07:22, mfomitchev wrote: > Actually it looks like we can just remove this, since there's already a check > for views in line 220 (and we don't plan to have non-Aura toolkit_views on > Android). Done. https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:21: static aura::WindowTreeHostPlatform* host = nullptr; On 2015/11/02 16:07:22, mfomitchev wrote: > no need for static since you are using anonymous namespace Done. https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:29: On 2015/11/02 16:05:13, bshe wrote: > Add comment "// static" before SetHost so it is easier to read. Done. https://codereview.chromium.org/1410153003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:31: aura::WindowTreeHostPlatform* window_tree_host) { On 2015/11/02 16:07:22, mfomitchev wrote: > nit: call this "host" so it the function signature fits on one line? :) We already had the name "host", so I named this one window_tree_host. I also renamed the function declaration in the .h file to be consistent.
LGTM
moshayedi@chromium.org changed reviewers: + msw@chromium.org
msw@ can you please provide OWNERS review for this patch?
Please expand the CL description with a simple overview of what this CL does, and what's left to be done for the basics (eg. calling SetHost). https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:12: // A stripped version of BrowserFrameAsh. May need to add things back later. nit: move comment to header? https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:21: aura::WindowTreeHostPlatform* host = nullptr; nit: rename |global_host| or |g_host|; add a comment regarding the reasoning behind the global pattern. https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:48: views::NativeWidgetAura::OnWindowTargetVisibilityChanged(visible); nit: remove this override if it justs calls the base class impl. https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:51: bool BrowserFrameAndroid::ShouldSaveWindowPlacement() const { Match function def order to header decl order and fix override section comments. (eg. this NativeBrowserFrame impl should follow GetMinimizeButtonOffset) https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:63: DCHECK(host != nullptr); nit: DCHECK(host) should suffice https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.h (right): https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.h:32: static void SetHost(aura::WindowTreeHostPlatform* window_tree_host); nit: add an explanatory comment
https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:12: // A stripped version of BrowserFrameAsh. May need to add things back later. On 2015/11/02 21:40:01, msw wrote: > nit: move comment to header? We should probably get rid of this comment altogether.
https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:12: // A stripped version of BrowserFrameAsh. May need to add things back later. On 2015/11/02 23:00:17, mfomitchev wrote: > On 2015/11/02 21:40:01, msw wrote: > > nit: move comment to header? > > We should probably get rid of this comment altogether. Done. https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:21: aura::WindowTreeHostPlatform* host = nullptr; On 2015/11/02 21:40:01, msw wrote: > nit: rename |global_host| or |g_host|; add a comment regarding the reasoning > behind the global pattern. Added a comment for this and SetHost() after discussing it with mfomitchev. Can you please double-check? https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:48: views::NativeWidgetAura::OnWindowTargetVisibilityChanged(visible); On 2015/11/02 21:40:01, msw wrote: > nit: remove this override if it justs calls the base class impl. Done. https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:51: bool BrowserFrameAndroid::ShouldSaveWindowPlacement() const { On 2015/11/02 21:40:01, msw wrote: > Match function def order to header decl order and fix override section comments. > (eg. this NativeBrowserFrame impl should follow GetMinimizeButtonOffset) Done. https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:63: DCHECK(host != nullptr); On 2015/11/02 21:40:01, msw wrote: > nit: DCHECK(host) should suffice Done. https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.h (right): https://codereview.chromium.org/1410153003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.h:32: static void SetHost(aura::WindowTreeHostPlatform* window_tree_host); On 2015/11/02 21:40:01, msw wrote: > nit: add an explanatory comment Added a comment for this and |g_host| after discussing it with mfomitchev. Can you please double-check?
lgtm with a repetition of my earlier request: Please expand the CL description with a simple overview of what this CL does, and what's left to be done for the basics (eg. calling SetHost).
https://codereview.chromium.org/1410153003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:21: // TODO(moshayedi): This is temporary until we have multi-window support. reference the crbug.com/551055 here https://codereview.chromium.org/1410153003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.h (right): https://codereview.chromium.org/1410153003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.h:32: // Must be called before the browser frame is created. Used to provide a I suggest we get rid of "Used to provide a...". As we discussed, this is an implementation detail, which doesn't belong in the API description.
Description was changed from ========== Browser frame for Aura Android. BUG=550423 ========== to ========== Implement initial version of browser frame for Aura Android. This implementation assumes that we have only one root window. When we have multi-window support, we'll need to change this implementation. We will need to change ChromeBrowserMainExtraPartsViews::ToolkitInitialized() to call SetHost() to make this CL work. We also need to move WTH/WMTestHelper initialization there from ChromeBrowserMainPartsAndroid::PreProfileInit(). BUG=550423 ==========
Description was changed from ========== Implement initial version of browser frame for Aura Android. This implementation assumes that we have only one root window. When we have multi-window support, we'll need to change this implementation. We will need to change ChromeBrowserMainExtraPartsViews::ToolkitInitialized() to call SetHost() to make this CL work. We also need to move WTH/WMTestHelper initialization there from ChromeBrowserMainPartsAndroid::PreProfileInit(). BUG=550423 ========== to ========== Implement initial version of browser frame for Aura Android which is a stripped version of BrowserFrameAsh. This implementation assumes that we have only one root window. When we have multi-window support, we'll need to change this implementation. We will need to change ChromeBrowserMainExtraPartsViews::ToolkitInitialized() to call SetHost() to make this CL work. We also need to move WTH/WMTestHelper initialization there from ChromeBrowserMainPartsAndroid::PreProfileInit(). BUG=550423 ==========
https://codereview.chromium.org/1410153003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.cc (right): https://codereview.chromium.org/1410153003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.cc:21: // TODO(moshayedi): This is temporary until we have multi-window support. On 2015/11/03 19:26:10, mfomitchev wrote: > reference the crbug.com/551055 here Done. https://codereview.chromium.org/1410153003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_android.h (right): https://codereview.chromium.org/1410153003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_android.h:32: // Must be called before the browser frame is created. Used to provide a On 2015/11/03 19:26:10, mfomitchev wrote: > I suggest we get rid of "Used to provide a...". As we discussed, this is an > implementation detail, which doesn't belong in the API description. Done. Sorry for missing this.
On 2015/11/03 19:22:28, msw wrote: > lgtm with a repetition of my earlier request: > Please expand the CL description with a simple overview of what this CL does, > and what's left to be done for the basics (eg. calling SetHost). Done. Sorry for missing the earlier feedback.
Description was changed from ========== Implement initial version of browser frame for Aura Android which is a stripped version of BrowserFrameAsh. This implementation assumes that we have only one root window. When we have multi-window support, we'll need to change this implementation. We will need to change ChromeBrowserMainExtraPartsViews::ToolkitInitialized() to call SetHost() to make this CL work. We also need to move WTH/WMTestHelper initialization there from ChromeBrowserMainPartsAndroid::PreProfileInit(). BUG=550423 ========== to ========== Implement initial version of browser frame for Aura Android. This implementation assumes that we have only one root window. We will need to change ChromeBrowserMainExtraPartsViews::ToolkitInitialized() to call SetHost() to make this CL work. BUG=550423 ==========
The CQ bit was checked by moshayedi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1410153003/#ps160001 (title: "Addressed mfomitchev's feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410153003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410153003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2b42825136a8202715d4df8b623689f1c8e713b3 Cr-Commit-Position: refs/heads/master@{#357623} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
