Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1403293003:
Introduce AndroidFocusRules and NativeWidgetAndroid (Closed)
DescriptionIntroduce AndroidFocusRules and NativeWidgetAndroid
NativeWidgetAndroidis very similar to NativeWidgetAura. It
owns a WindowTreeHost and FocusController which initialized
from AndroidFocusRules. Aura on Android should use this
native widget for top level windows. It should create and host
the Widget in a native Android window.
BUG=507792
Committed: https://crrev.com/31db6ee3532c748f40d2ae00514b5d91031d78e7
Cr-Commit-Position: refs/heads/master@{#360645}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Patch Set 5 : Add NOTIMPLEMENTED #
Total comments: 54
Patch Set 6 : #
Total comments: 34
Patch Set 7 : reviews #
Total comments: 18
Patch Set 8 : reviews #
Total comments: 8
Patch Set 9 : reviews #Patch Set 10 : reviews #Patch Set 11 : #
Total comments: 4
Patch Set 12 : reviews #
Total comments: 8
Patch Set 13 : reviews #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 58 (27 generated)
bshe@chromium.org changed reviewers: + sadrul@chromium.org
Hey Sadrul. Do you mind to take a look at this CL? It introduce AndroidFocusRules which will be used in aura android. Thanks! Biao
+ mfomitchev
https://codereview.chromium.org/1403293003/diff/20001/ui/wm/android/android_f... File ui/wm/android/android_focus_rules.cc (right): https://codereview.chromium.org/1403293003/diff/20001/ui/wm/android/android_f... ui/wm/android/android_focus_rules.cc:21: return false; return window->IsRootWindow(); https://codereview.chromium.org/1403293003/diff/20001/ui/wm/test/wm_test_help... File ui/wm/test/wm_test_helper.cc (right): https://codereview.chromium.org/1403293003/diff/20001/ui/wm/test/wm_test_help... ui/wm/test/wm_test_helper.cc:36: #endif Is there a reason to want to use the FocusController with platform-specific focus-rules in tests only on android, but not on other platforms?
Patchset #3 (id:40001) has been deleted
mfomitchev@chromium.org changed reviewers: + mfomitchev@chromium.org
https://codereview.chromium.org/1403293003/diff/60001/ui/wm/android/window_tr... File ui/wm/android/window_tree_host_manager.h (right): https://codereview.chromium.org/1403293003/diff/60001/ui/wm/android/window_tr... ui/wm/android/window_tree_host_manager.h:39: class WindowTreeHostManager : public aura::client::WindowTreeClient { Perhaps we should create a common base class for WMTestHelper and Android's WindowTreeHostManager?
Patchset #4 (id:80001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
Description was changed from ========== Introduce AndroidFocusRules For Aura on Android, a focus controller is needed, which requires a focus rule. Currently, it is only used in wm_test_helper.cc We will rename the file in the future and use it for Aura Android. BUG=507792 ========== to ========== Introduce AndroidFocusRules and NativeWidgetAndroid NativeWidgetAndroid is very similar to NativeWidgetMus. It owns a WindowTreeHost and FocusController which initialized from AndroidFocusRules. Aura on Android should use this native widget. BUG=507792 ==========
Description was changed from ========== Introduce AndroidFocusRules and NativeWidgetAndroid NativeWidgetAndroid is very similar to NativeWidgetMus. It owns a WindowTreeHost and FocusController which initialized from AndroidFocusRules. Aura on Android should use this native widget. BUG=507792 ========== to ========== Introduce AndroidFocusRules and NativeWidgetAndroid NativeWidgetAndroid is very similar to NativeWidgetMus. It owns a WindowTreeHost and FocusController which initialized from AndroidFocusRules. Aura on Android should use this native widget. BUG=507792 ==========
Patchset #4 (id:140001) has been deleted
This is in a better shape for review now. PTAL, thanks! According to offline discussion, it looks like we want to init a WTH when initialize a native widget(similar to NativeWidgetMus). https://codereview.chromium.org/1403293003/diff/20001/ui/wm/android/android_f... File ui/wm/android/android_focus_rules.cc (right): https://codereview.chromium.org/1403293003/diff/20001/ui/wm/android/android_f... ui/wm/android/android_focus_rules.cc:21: return false; On 2015/10/19 15:36:11, sadrul wrote: > return window->IsRootWindow(); Done. https://codereview.chromium.org/1403293003/diff/20001/ui/wm/test/wm_test_help... File ui/wm/test/wm_test_helper.cc (right): https://codereview.chromium.org/1403293003/diff/20001/ui/wm/test/wm_test_help... ui/wm/test/wm_test_helper.cc:36: #endif On 2015/10/19 15:36:11, sadrul wrote: > Is there a reason to want to use the FocusController with platform-specific > focus-rules in tests only on android, but not on other platforms? My understanding is that we might use this test helper in aura clank so it will be no longer only for test. For other platforms, keep it untouched seems safer?
Mostly nits. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... File ui/views/widget/android/android_focus_rules.h (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/android_focus_rules.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. I don't think we use (c) anymore? (too bad we don't have tooling for this :/) https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/android_focus_rules.h:20: // ::wm::BaseFocusRules: Just wm https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:49: NativeWidgetAndroid::~NativeWidgetAndroid() {} new line https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:115: return host_->window()->layer()->GetCompositor(); Just host_->compositor()? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:119: return host_->window() ? host_->window()->layer() : nullptr; When can this be null? Can this just be: return GetNativeWindow()->layer(); From a quick look, it doesn't look like host_->window() can be null, apart from during destruction. Most of the methods in this class shouldn't also normally be called during destruction. So it would be better to remove the null-checks (unless we can explain when it can be null during the call to that function). Instead of using host_->window() everywhere, it might make sense to just use GetNativeWindow() https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:240: delete host_->window(); Why not host_.reset() instead? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:335: host_->window()->layer()->SetOpacity(opacity / 255.0); GetLayer()->SetOpacity() https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.h (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.h:63: ui::WindowShowState* maximized) const override; |show_state| would make more sense, instead of |maximized|
https://codereview.chromium.org/1403293003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/1403293003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:396: new views::NativeWidgetAura(delegate); So we are using both NativeWidgetAndroid and NativeWidgetAura for Aura Clank? If so, perhaps NativeWidgetAndroid should really be called DesktopNativeWidgetAndroid? https://codereview.chromium.org/1403293003/diff/180001/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/views.gyp#new... ui/views/views.gyp:295: 'widget/android/android_focus_rules.cc', We modify GYP but not GN? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. It would be easier to review/use as a reference later if this showed as a diff with native_widget_aura.cc. Maybe play with --similarity to get it to show this way? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:55: // TODO(bshe): Get rid of the hard coded size. Uses screen size instead. Can you reference a crbug here? (Either use one of the existing ones or create a new one). https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:71: } What about screen position client? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:123: NOTIMPLEMENTED(); Is it tricky to use Aura's impl here? If so, can we TODO/crbug for this? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:142: NOTIMPLEMENTED(); Is it tricky to use Aura's impl here? If so, can we TODO/crbug for this? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:167: void NativeWidgetAndroid::CenterWindow(const gfx::Size& size) { Can you add one TODO referencing a crbug for all this window sizing/positioning functionality? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:178: NOTIMPLEMENTED(); Why not use Aura's impl? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:188: NOTIMPLEMENTED(); Why not use Aura's impl? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:356: NOTIMPLEMENTED(); Sorry, why is this NOTIMPLEMENTED()? Why not host->window()->SchedulePaintInRect(rect)? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:359: void NativeWidgetAndroid::SetCursor(gfx::NativeCursor cursor) { TODO/crbug for cursor? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:369: NOTIMPLEMENTED(); Why can't we use NativeWidgetAura's implementation here? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:373: NOTIMPLEMENTED(); TODO/crbug? maybe the same one we use for initializing the host to the right size? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:390: NOTIMPLEMENTED(); Why not use Aura's impl? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:395: NOTIMPLEMENTED(); ditto https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:400: NOTIMPLEMENTED(); ditto https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:418: NOTIMPLEMENTED(); ditto https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.h (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015
Patchset #6 (id:200001) has been deleted
https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:86: class AndroidDispatcherClient : public aura::client::DispatcherClient { We are trying to get rid of nested msg loops, so hopefully we won't need this. I guess It might be useful in the interim to get stuff to compile/run.. so perhaps add a TODO and reference the nested msg loop bug. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:168: DCHECK(params.parent || params.context); This seems wrong? This is top-level... https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:190: gfx::NativeView parent = params.parent; Not sure if this code is appropriate for top-level widget.. Perhaps we should use the code in DesktopNativeWidgetAura as a template here? https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:254: return NULL; previous impl was better IMHO https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:258: // There is only one frame type for aura. ditto https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:263: return false; ditto https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:269: GetWidget()->ThemeChanged(); We don't have themes.. maybe just leave NOTIMPLEMENTED since the frame-related method is not implemented? https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:290: NativeWidgetPrivate* native_widget = GetTopLevelNativeWidget(GetNativeView()); We are the top-level widget... use DNWA's impl? https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:295: return window_ ? window_->layer()->GetCompositor() : NULL; Sadrul suggested host_->compositor() https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:299: return window_ ? window_->layer() : NULL; See Sadrul's comment https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:307: DCHECK(drop_helper_.get() != NULL); We don't support DND, so NOTIMPLEMENTED() makes sense here https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:346: void NativeWidgetAndroid::CenterWindow(const gfx::Size& size) { See my previous comment - not sure this makes sense since this is not designed for top level. I think it makes sense to tackle this when we integrate with native functionality for this. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:399: // The interface specifies returning restored bounds, not current bounds. ditto https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:406: if (!window_) window_ can't be null? https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:416: // Aura doesn't have window icons. DesktopNativeWidgetAura has something for this, so NOTIMPLEMENTED() might make more sense? Not really sure https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:435: if (!window_) Same as CenterWindow and other top-level sizing/positioning https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:532: delete window_; see Sadrul's comment
Patchset #7 (id:240001) has been deleted
Patchset #7 (id:260001) has been deleted
Patchset #7 (id:280001) has been deleted
Patchset #7 (id:300001) has been deleted
Patchset #7 (id:320001) has been deleted
Patchset #7 (id:340001) has been deleted
Patchset #7 (id:360001) has been deleted
I think I have addressed all reviews. Some of the reviews might not be valid in the latest patch. Basically, what I did in the latest patch is: 1. create DesktopNativeWidgetAura based on NativeWidgetAura 2. Add WTH and various clients to DesktopNativeWidgetAura 3. Modify some functions (noticeably InitNativeWidget) to be more like the implementation in DesktopNativeWidgetAura when I see fit. 4. Removed a few implementation which do not make sense/or not yet support, added NOTIMPLEMENTED and TODO. PTAL. Thanks! https://codereview.chromium.org/1403293003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/1403293003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:396: new views::NativeWidgetAura(delegate); On 2015/11/04 23:21:13, mfomitchev wrote: > So we are using both NativeWidgetAndroid and NativeWidgetAura for Aura Clank? If > so, perhaps NativeWidgetAndroid should really be called > DesktopNativeWidgetAndroid? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/views.gyp#new... ui/views/views.gyp:295: 'widget/android/android_focus_rules.cc', On 2015/11/04 23:21:13, mfomitchev wrote: > We modify GYP but not GN? Looks like GN use the gyp value. So we only need to modify this file. https://code.google.com/p/chromium/codesearch/#chromium/src/ui/views/BUILD.gn... https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... File ui/views/widget/android/android_focus_rules.h (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/android_focus_rules.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/11/04 19:00:53, sadrul wrote: > I don't think we use (c) anymore? (too bad we don't have tooling for this :/) Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/android_focus_rules.h:20: // ::wm::BaseFocusRules: On 2015/11/04 19:00:53, sadrul wrote: > Just wm Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/11/04 23:21:14, mfomitchev wrote: > It would be easier to review/use as a reference later if this showed as a diff > with native_widget_aura.cc. Maybe play with --similarity to get it to show this > way? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:49: NativeWidgetAndroid::~NativeWidgetAndroid() {} On 2015/11/04 19:00:53, sadrul wrote: > new line Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:55: // TODO(bshe): Get rid of the hard coded size. Uses screen size instead. On 2015/11/04 23:21:13, mfomitchev wrote: > Can you reference a crbug here? (Either use one of the existing ones or create a > new one). Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:71: } On 2015/11/04 23:21:13, mfomitchev wrote: > What about screen position client? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:115: return host_->window()->layer()->GetCompositor(); On 2015/11/04 19:00:53, sadrul wrote: > Just host_->compositor()? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:119: return host_->window() ? host_->window()->layer() : nullptr; On 2015/11/04 19:00:53, sadrul wrote: > When can this be null? Can this just be: > > return GetNativeWindow()->layer(); > > From a quick look, it doesn't look like host_->window() can be null, apart from > during destruction. Most of the methods in this class shouldn't also normally be > called during destruction. So it would be better to remove the null-checks > (unless we can explain when it can be null during the call to that function). > > Instead of using host_->window() everywhere, it might make sense to just use > GetNativeWindow() Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:123: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > Is it tricky to use Aura's impl here? If so, can we TODO/crbug for this? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:142: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > Is it tricky to use Aura's impl here? If so, can we TODO/crbug for this? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:167: void NativeWidgetAndroid::CenterWindow(const gfx::Size& size) { On 2015/11/04 23:21:13, mfomitchev wrote: > Can you add one TODO referencing a crbug for all this window sizing/positioning > functionality? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:178: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > Why not use Aura's impl? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:188: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > Why not use Aura's impl? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:240: delete host_->window(); On 2015/11/04 19:00:53, sadrul wrote: > Why not host_.reset() instead? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:335: host_->window()->layer()->SetOpacity(opacity / 255.0); On 2015/11/04 19:00:53, sadrul wrote: > GetLayer()->SetOpacity() GetLayer() returns a const ref. Unless we want to do a cast, otherwise, use the normal layer() function is easier to read? https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:356: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > Sorry, why is this NOTIMPLEMENTED()? > Why not host->window()->SchedulePaintInRect(rect)? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:359: void NativeWidgetAndroid::SetCursor(gfx::NativeCursor cursor) { On 2015/11/04 23:21:13, mfomitchev wrote: > TODO/crbug for cursor? Used NativeWidgetAura implementation https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:369: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > Why can't we use NativeWidgetAura's implementation here? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:373: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > TODO/crbug? maybe the same one we use for initializing the host to the right > size? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:390: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > Why not use Aura's impl? Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:395: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > ditto Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:400: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > ditto Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:418: NOTIMPLEMENTED(); On 2015/11/04 23:21:13, mfomitchev wrote: > ditto Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.h (right): https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/11/04 23:21:14, mfomitchev wrote: > 2015 Done. https://codereview.chromium.org/1403293003/diff/180001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.h:63: ui::WindowShowState* maximized) const override; On 2015/11/04 19:00:53, sadrul wrote: > |show_state| would make more sense, instead of |maximized| Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:86: class AndroidDispatcherClient : public aura::client::DispatcherClient { On 2015/11/06 16:45:50, mfomitchev wrote: > We are trying to get rid of nested msg loops, so hopefully we won't need this. I > guess It might be useful in the interim to get stuff to compile/run.. so perhaps > add a TODO and reference the nested msg loop bug. Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:168: DCHECK(params.parent || params.context); On 2015/11/06 16:45:50, mfomitchev wrote: > This seems wrong? This is top-level... removed. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:190: gfx::NativeView parent = params.parent; On 2015/11/06 16:45:50, mfomitchev wrote: > Not sure if this code is appropriate for top-level widget.. Perhaps we should > use the code in DesktopNativeWidgetAura as a template here? Right. Anything related to parent is probably not relevant here. I am a little bit hesitate to use the code in DesktopNativeWidgetAura though. It looks like DNWA use host_->window() in a few places. But most of our code only knows about window_(correspond to content_window_ in DNWA). I worry there might be some surprises. To reduce risk, I think remove unrelated code but still based on NWA would be easier. WDYT? https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:254: return NULL; On 2015/11/06 16:45:50, mfomitchev wrote: > previous impl was better IMHO Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:258: // There is only one frame type for aura. On 2015/11/06 16:45:50, mfomitchev wrote: > ditto Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:263: return false; On 2015/11/06 16:45:51, mfomitchev wrote: > ditto Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:269: GetWidget()->ThemeChanged(); On 2015/11/06 16:45:51, mfomitchev wrote: > We don't have themes.. maybe just leave NOTIMPLEMENTED since the frame-related > method is not implemented? Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:290: NativeWidgetPrivate* native_widget = GetTopLevelNativeWidget(GetNativeView()); On 2015/11/06 16:45:51, mfomitchev wrote: > We are the top-level widget... use DNWA's impl? Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:295: return window_ ? window_->layer()->GetCompositor() : NULL; On 2015/11/06 16:45:51, mfomitchev wrote: > Sadrul suggested host_->compositor() Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:299: return window_ ? window_->layer() : NULL; On 2015/11/06 16:45:51, mfomitchev wrote: > See Sadrul's comment Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:307: DCHECK(drop_helper_.get() != NULL); On 2015/11/06 16:45:50, mfomitchev wrote: > We don't support DND, so NOTIMPLEMENTED() makes sense here Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:346: void NativeWidgetAndroid::CenterWindow(const gfx::Size& size) { On 2015/11/06 16:45:51, mfomitchev wrote: > See my previous comment - not sure this makes sense since this is not designed > for top level. I think it makes sense to tackle this when we integrate with > native functionality for this. Left a TODO and used NOTIMPLEMENTED for now. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:399: // The interface specifies returning restored bounds, not current bounds. On 2015/11/06 16:45:51, mfomitchev wrote: > ditto Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:406: if (!window_) On 2015/11/06 16:45:51, mfomitchev wrote: > window_ can't be null? Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:416: // Aura doesn't have window icons. On 2015/11/06 16:45:50, mfomitchev wrote: > DesktopNativeWidgetAura has something for this, so NOTIMPLEMENTED() might make > more sense? Not really sure Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:435: if (!window_) On 2015/11/06 16:45:50, mfomitchev wrote: > Same as CenterWindow and other top-level sizing/positioning Done. https://codereview.chromium.org/1403293003/diff/220001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:532: delete window_; On 2015/11/06 16:45:51, mfomitchev wrote: > see Sadrul's comment Done.
Description was changed from ========== Introduce AndroidFocusRules and NativeWidgetAndroid NativeWidgetAndroid is very similar to NativeWidgetMus. It owns a WindowTreeHost and FocusController which initialized from AndroidFocusRules. Aura on Android should use this native widget. BUG=507792 ========== to ========== Introduce AndroidFocusRules andDesktopNativeWidgetAndroid DesktopNativeWidgetAndroidis very similar to NativeWidgetAura. It owns a WindowTreeHost and FocusController which initialized from AndroidFocusRules. Aura on Android should use this native widget for top level windows BUG=507792 ==========
Looks pretty good, mostly minor comments. Maybe at some point we could look into sharing the duplicated code between the various native widgets.. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... File ui/views/widget/desktop_android/desktop_native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:52: // TODO(bshe): Most of the code is copied from DesktopNativeWidgetAura. There DesktopNativeWidgetAura or NativeWidgetAura? https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:137: // DesktopNativeWidgetAndroid, DesktopNativeWidgetAndroid implementation: This header doesn't make much sense to me. The previous header already clarified that this is a (public) implementation of DesktopNativeWidgetAndroid. Perhaps leave the old one or just get rid of it? https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:245: // TODO: Implement drag and drop. crbug.com/554029. TODO(bshe) https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:301: // TODO(bshe): Implement this. See crbug.com/554208. Does this window icon functionality really fall under "window positioning and moving"? If not, perhaps create a separate bug (if we actually need this)? https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:306: if (modal_type != ui::MODAL_TYPE_NONE) Perhaps this should just be NOTIMPLEMENTED()? Having a modal top-level window would presumably need native support? https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:327: host_->SetBounds(bounds); Presumably, we'd need to resize the SurfaceView to properly support this, but it's probably fine for now.. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:427: GetNativeWindow()->SetProperty(aura::client::kShowStateKey, Windows sizing functionality should be NOTIMPLEMENTED()? https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:524: const gfx::Vector2d& drag_offset, Perhaps just make this NOTIMPLEMENTED()? https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/native... File ui/views/widget/native_widget_aura.h (right): https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/native... ui/views/widget/native_widget_aura.h:44: // Called internally by NativeWidgetAura, NativeWidgetAndroid and NativeWidgetAura, DesktopNativeWidgetAura, and DesktopNativeWidgetAndroid
Patchset #8 (id:400001) has been deleted
https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... File ui/views/widget/desktop_android/desktop_native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:52: // TODO(bshe): Most of the code is copied from DesktopNativeWidgetAura. There On 2015/11/11 22:40:32, mfomitchev wrote: > DesktopNativeWidgetAura or NativeWidgetAura? Updated comment. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:137: // DesktopNativeWidgetAndroid, DesktopNativeWidgetAndroid implementation: On 2015/11/11 22:40:32, mfomitchev wrote: > This header doesn't make much sense to me. The previous header already clarified > that this is a (public) implementation of DesktopNativeWidgetAndroid. Perhaps > leave the old one or just get rid of it? Sorry. It is probably a search and replace error. This marks the beginning of NativeWidgetPrivate implementation. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:245: // TODO: Implement drag and drop. crbug.com/554029. On 2015/11/11 22:40:32, mfomitchev wrote: > TODO(bshe) Done. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:301: // TODO(bshe): Implement this. See crbug.com/554208. On 2015/11/11 22:40:32, mfomitchev wrote: > Does this window icon functionality really fall under "window positioning and > moving"? If not, perhaps create a separate bug (if we actually need this)? Done. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:306: if (modal_type != ui::MODAL_TYPE_NONE) On 2015/11/11 22:40:32, mfomitchev wrote: > Perhaps this should just be NOTIMPLEMENTED()? Having a modal top-level window > would presumably need native support? Done. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:327: host_->SetBounds(bounds); On 2015/11/11 22:40:33, mfomitchev wrote: > Presumably, we'd need to resize the SurfaceView to properly support this, but > it's probably fine for now.. Added a TODO. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:427: GetNativeWindow()->SetProperty(aura::client::kShowStateKey, On 2015/11/11 22:40:33, mfomitchev wrote: > Windows sizing functionality should be NOTIMPLEMENTED()? Done. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:524: const gfx::Vector2d& drag_offset, On 2015/11/11 22:40:32, mfomitchev wrote: > Perhaps just make this NOTIMPLEMENTED()? Done. https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/native... File ui/views/widget/native_widget_aura.h (right): https://codereview.chromium.org/1403293003/diff/380001/ui/views/widget/native... ui/views/widget/native_widget_aura.h:44: // Called internally by NativeWidgetAura, NativeWidgetAndroid and On 2015/11/11 22:40:33, mfomitchev wrote: > NativeWidgetAura, DesktopNativeWidgetAura, and DesktopNativeWidgetAndroid Done.
LGTM
bshe@chromium.org changed reviewers: + sky@chromium.org
sadrul@ do you mind to take another look? +sky for owners too
The more I see desktop_android, the more I tend to think that it should be just android (and NativeWidgetAndroid). I do not think adding desktop_ prefix here is useful or necessary. Maybe we can discuss offline if you folks disagree?
I agree with Sadrul on naming. I would also put this code in a ui/views/widget/android, not ui/views/widget/desktop_android. https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... File ui/views/widget/desktop_android/android_focus_rules.cc (right): https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... ui/views/widget/desktop_android/android_focus_rules.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. no (c). https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... File ui/views/widget/desktop_android/android_focus_rules.h (right): https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... ui/views/widget/desktop_android/android_focus_rules.h:14: class AndroidFocusRules : public wm::BaseFocusRules { We generally have the platform as the suffix, eng FocusRulesAndroid. https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... File ui/views/widget/desktop_android/desktop_native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:57: class DesktopNativeWidgetAndroidWindowTreeClient IMO I would name these things like WindowTreeClientImpl, but if you prefer a lot of typing go with what you have. That sia,d WindowTreeTreeClientAndroid would be better. https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:65: aura::client::SetWindowTreeClient(root_window_, NULL); NULL->nullptr
Patchset #9 (id:440001) has been deleted
Patchset #9 (id:460001) has been deleted
Changed name to NativeWidgeAndroid and addressed comments. PTAL, thanks! https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... File ui/views/widget/desktop_android/android_focus_rules.cc (right): https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... ui/views/widget/desktop_android/android_focus_rules.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/11/16 22:06:54, sky wrote: > no (c). Done. https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... File ui/views/widget/desktop_android/android_focus_rules.h (right): https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... ui/views/widget/desktop_android/android_focus_rules.h:14: class AndroidFocusRules : public wm::BaseFocusRules { On 2015/11/16 22:06:54, sky wrote: > We generally have the platform as the suffix, eng FocusRulesAndroid. I originally want to use suffix too. But later I found all other subclasses of BaseFocusRules do not use suffix. Examples are AshFocusRules, DesktopFocusRules and AppsFocusRules. So I decide to use android_focus_rules here to be consistent with others. However, only Android is a platform name here. So I can see the argument to use it as a suffix too. But before I make the change, given the name convention of other classes, just want to double check with you to see if you still prefer to use android as suffix. https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... File ui/views/widget/desktop_android/desktop_native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:57: class DesktopNativeWidgetAndroidWindowTreeClient On 2015/11/16 22:06:55, sky wrote: > IMO I would name these things like WindowTreeClientImpl, but if you prefer a lot > of typing go with what you have. That sia,d WindowTreeTreeClientAndroid would be > better. Done. https://codereview.chromium.org/1403293003/diff/420001/ui/views/widget/deskto... ui/views/widget/desktop_android/desktop_native_widget_android.cc:65: aura::client::SetWindowTreeClient(root_window_, NULL); On 2015/11/16 22:06:55, sky wrote: > NULL->nullptr Done.
Can you please update the title and the description of the CL?
Description was changed from ========== Introduce AndroidFocusRules andDesktopNativeWidgetAndroid DesktopNativeWidgetAndroidis very similar to NativeWidgetAura. It owns a WindowTreeHost and FocusController which initialized from AndroidFocusRules. Aura on Android should use this native widget for top level windows BUG=507792 ========== to ========== Introduce AndroidFocusRules and NativeWidgetAndroid NativeWidgetAndroidis very similar to NativeWidgetAura. It owns a WindowTreeHost and FocusController which initialized from AndroidFocusRules. Aura on Android should use this native widget for top level windows. It should create and host the Widget in a native Android window. BUG=507792 ==========
On 2015/11/17 17:02:41, mfomitchev wrote: > Can you please update the title and the description of the CL? done!
On 2015/11/17 17:11:31, bshe wrote: > On 2015/11/17 17:02:41, mfomitchev wrote: > > Can you please update the title and the description of the CL? > > done! friendly ping?
LGTM when you get it compiling. https://codereview.chromium.org/1403293003/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/1403293003/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:56: #include "ui/views/widget/desktop_android/desktop_native_widget_android.h" Fix this and 412. https://codereview.chromium.org/1403293003/diff/520001/ui/views/widget/native... File ui/views/widget/native_widget_aura.h (right): https://codereview.chromium.org/1403293003/diff/520001/ui/views/widget/native... ui/views/widget/native_widget_aura.h:45: // DesktopNativeWidgetAndroid to associate |native_widget| with |window|. NativeWidgetAndroid, or nuke all the names and say called internally by NativeWidget implementations.
Thanks! https://codereview.chromium.org/1403293003/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/1403293003/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:56: #include "ui/views/widget/desktop_android/desktop_native_widget_android.h" On 2015/11/18 19:11:06, sky wrote: > Fix this and 412. dooh. Sorry I missed this one. https://codereview.chromium.org/1403293003/diff/520001/ui/views/widget/native... File ui/views/widget/native_widget_aura.h (right): https://codereview.chromium.org/1403293003/diff/520001/ui/views/widget/native... ui/views/widget/native_widget_aura.h:45: // DesktopNativeWidgetAndroid to associate |native_widget| with |window|. On 2015/11/18 19:11:06, sky wrote: > NativeWidgetAndroid, or nuke all the names and say called internally by > NativeWidget implementations. Done.
https://codereview.chromium.org/1403293003/diff/540001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/1403293003/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:412: params->native_widget = new views::DesktopNativeWidgetAndroid(delegate); s/Desktop// https://codereview.chromium.org/1403293003/diff/540001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/540001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:81: // fiexed. *fixed https://codereview.chromium.org/1403293003/diff/540001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:87: base::MessagePumpDispatcher* dispatcher() { return dispatcher_; } This is not used. https://codereview.chromium.org/1403293003/diff/540001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:110: base::MessagePumpDispatcher* dispatcher_; It doesn't look like this is ever used. Remove?
Thanks for review! https://codereview.chromium.org/1403293003/diff/540001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/1403293003/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:412: params->native_widget = new views::DesktopNativeWidgetAndroid(delegate); On 2015/11/18 20:07:39, sadrul wrote: > s/Desktop// hah. I should really upstream everything asap to be able to compile chrome/browser/ui.. git gs DesktopNativeWidgetAndroid found nothing and I should have fixed the name for real this time. https://codereview.chromium.org/1403293003/diff/540001/ui/views/widget/androi... File ui/views/widget/android/native_widget_android.cc (right): https://codereview.chromium.org/1403293003/diff/540001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:81: // fiexed. On 2015/11/18 20:07:39, sadrul wrote: > *fixed Done. https://codereview.chromium.org/1403293003/diff/540001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:87: base::MessagePumpDispatcher* dispatcher() { return dispatcher_; } On 2015/11/18 20:07:39, sadrul wrote: > This is not used. removed. https://codereview.chromium.org/1403293003/diff/540001/ui/views/widget/androi... ui/views/widget/android/native_widget_android.cc:110: base::MessagePumpDispatcher* dispatcher_; On 2015/11/18 20:07:39, sadrul wrote: > It doesn't look like this is ever used. Remove? it looks like it is used at line 103. But line 103 doesnt seem like necessary. So removed.
lgtm
The CQ bit was checked by bshe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1403293003/#ps560001 (title: "reviews")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403293003/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403293003/560001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bshe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403293003/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403293003/560001
Message was sent while issue was closed.
Committed patchset #13 (id:560001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/31db6ee3532c748f40d2ae00514b5d91031d78e7 Cr-Commit-Position: refs/heads/master@{#360645}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:560001) has been created in https://codereview.chromium.org/1620463004/ by bshe@chromium.org. The reason for reverting is: Aura Android is no longer needed at this point.
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:560001) has been created in https://codereview.chromium.org/1639003002/ by mfomitchev@chromium.org. The reason for reverting is: Reverting the CL as the Android Aura project has been cancelled.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
