Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1796773003:
Implement NativeWidgetMac::ReorderNativeViews (Closed)
DescriptionImplement NativeWidgetMac::ReorderNativeViews.
NativeWidgetMac doesn't currently take into account native view order
when attaching NSViews in NativeViewHost and in view::View::ReorderChildView.
It is important when showing DevTools.
BUG=594567
Committed: https://crrev.com/852a2ec06f9aac8f1b048d241a5bb8f9c5559d13
Cr-Commit-Position: refs/heads/master@{#382259}
Patch Set 1 #Patch Set 2 : Implement NativeWidgetMac::ReorderNativeViews #
Total comments: 13
Patch Set 3 : Using NativeViewHost*->NSView* map. #
Total comments: 18
Patch Set 4 : Added tests. Review fixes. Fixed sorting. #Patch Set 5 : Added DISALLOW_COPY_AND_ASSIGN #
Total comments: 16
Patch Set 6 : Nits and xcode 7 compilation. #
Messages
Total messages: 25 (8 generated)
kirr@yandex-team.ru changed reviewers: + tapted@chromium.org
Please see my CL. Seems like it's better to unify test with aura platform. But I'm not sure that other code is fine :)
On 2016/03/14 16:03:26, kirr wrote:
> Please see my CL.
> Seems like it's better to unify test with aura platform.
> But I'm not sure that other code is fine :)
Yeah I think this needs to be aligned more with aura.
For example, NativeWidgetMac::ReorderNativeViews() should trigger a reordering
in case the order in the views::View hierarchy has changed "arbitrarily". To do
that, I think we need to store a WindowReordererMac (or SubviewReorderer?) on a
BridgedNativeWidget, which has a map of views::View* -> NSView* associations,
which is updated whenever NativeViewHostMac::Attach/DetachNativeView is called.
I don't think we need objc_setAssociatedObject in this case -- it's neater just
to maintain the map in SubviewReorderer.
There might not be much overlap with the existing aura WindowReorderer. I think
not even `GetOrderOfViewsWithLayers` would fit (i.e. with a
std::map<views::View, gfx::NativeView> for |hosts|).
I think we can do something like
using RankMap = std::map<NSView*, int>;
void RankNSViews(View* view, const std::map<View*, NSView*> &hosts, RankMap
*rank) {
auto it = hosts.find(view);
if (it != hosts.end())
rank->insert(it->second, rank->size())
for (int i = 0; i < view->child_count(); ++i)
RankNSViews(view->child_at(i), hosts, rank);
}
NSComparisonResult SubviewSorter(NSView* lhs, NSView* rhs, void* rank_as_void) {
DCHECK_NE(lhs, rhs);
const RankMap* rank = static_cast<const RankMap*>(rank_as_void);
auto left_rank = rank->find(lhs);
auto right_rank = rank->find(rhs);
bool left_found = left_rank != rank->end();
bool right_found = right_rank != rank->end();
if (left_found != right_found) {
// Sort unassociated views above associated views, same as Aura's
WindowReorderer::ReorderChildWindows().
return left_found ? NSOrderedAscending : NSOrderedDescending;
}
if (left_found)
return *left_rank < *right_rank ? NSOrderedAscending : NSOrderedDescending;
// If both are unassociated, sort by their memory location.
return lhs < rhs ? NSOrderedAscending : NSOrderedDescending;
}
SubviewReorderer::ReorderNativeViews(NSView* parent) {
RankMap rank;
RankNSViews(root_view, hosts_, &rank);
[parent sortSubviewsUsingFunction:&SubviewSorter context:&rank];
}
I've added ReorderNativeViews implementation. But it is not contains the View*->NSView* map and class like WindowOrderer. Make a map in NativeWidgetMac requires to change interface of views::Widget. But aura still use kHostViewKey. So platforms still be different. Also i think that using a map is a little wired, because it contains duplicated data (views::View hierarchy already contains that data, but doesn't have an interface to access for native views ). I don't like objc_setAssociatedObject too, but I think it would be better to add some method like views::View::GetAssociatedNativeView and walk all views. Does it makes sense? Also I didn't add WindowOrderer, because I'm not understand, what we want to observe there? If we only need to react on NativeViewHostMac::Attach/Detach, i think it's better to insert NSView in right order, than add NSView on top, and then sort. BTW Here is a call stack that cause blinking in old versions of Yandex Browser https://gist.github.com/kirr/9adf17bf7d8c9a3aa3e7 . So I'm afraid of sortSubviewsUsingFunction a little :)
On 2016/03/15 16:42:29, kirr wrote: > I've added ReorderNativeViews implementation. > But it is not contains the View*->NSView* map and class like WindowOrderer. > > Make a map in NativeWidgetMac requires to change interface of views::Widget. I said BridgedNativeWidget should have the map (not NativeWidgetMac). We can access BridgedContentView or BridgedNativeWidget directly. NativeViewHostMac already accesses BridgedContentView. A BridgedNativeWidget can obtained via a static method -- NativeWidgetMac::GetBridgeForNativeWindow > But > aura still use kHostViewKey. So platforms still be different. > Also i think that using a map is a little wired, because it contains duplicated > data (views::View hierarchy already contains that data, but doesn't have an > interface to access for native views ). I don't think it's weird. getAssociatedObject is little more than a map from NSObject* to *another* map of data_key -> data. > I don't like objc_setAssociatedObject too, but I think it would be better to add > some method like views::View::GetAssociatedNativeView and walk all views. > Does it makes sense? WindowReorderer::AssociationObserver has a std::set<aura:Window>. I don't think there's any issue with a map. > > Also I didn't add WindowOrderer, because I'm not understand, what we want to > observe there? Yeah it's fine to not have WindowOrderer if we can achieve it with non-member functions. > If we only need to react on NativeViewHostMac::Attach/Detach, i think it's > better to insert NSView in right order, than add NSView on top, and then sort. We should go with what's simplest / the least amount of code until there's a use case for something else, or we identify a glitch/performance bottleneck. View::AddChildViewAt(..) calls Widget->ReorderNativeViews, and that happens a whole bunch, so I don't think it's worth trying to avoid calling it. https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_mac_utils.mm (right): https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:70: auto left_it = std::find_if(hosts->begin(), hosts->end(), EqualToNSView(lhs)); This call is O(n) so it makes the sorting algorithm O(n*n*log(n)); My suggestion to first collect the "rank" as an integer would keep the algorithm at O(n*log(n)*log(n)) (or just O(n*log(n)) if a hashmap is used for |rank|). Was there a reason you didn't like that idea? (I think also neater/easier to read than using find_if and the extra functors.). Comparator functions typically need to scale well - the shouldn't take time proportional to the number of things being sorted. But I guess there also aren't expected to be particularly many associated views in a particular widget. If we are assuming that we can comment here that this makes the comparator slow, but it shouldn't matter. https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:77: if (left_found != right_found) { nit: doesn't need curlies https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:81: if (left_found) nit: needs curlies https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:95: void AttachNSViewRelatedToHost(NSView* native_view, NativeViewHost* host) { I don't think this function is necessary. Aura observes the kViewHostKey being added to the NativeView and then just does `reorderer_->ReorderChildWindows();`. We can do the same, but without an observer. (just do `[new_superview addSubview:native_view];` and call ReorderChildNSViews) In fact I'd go with something like void ReparentNSView(NSView* native_view, NativeViewHost* host) { DCHECK(native_view); // Mac's NativeViewHost has no support for hosting its own child widgets. // This check is probably overly restrictive, since the Widget containing the // NativeViewHost _is_ allowed child Widgets. However, we don't know yet // whether those child Widgets need to be distinguished from Widgets that code // might want to associate with the hosted NSView instead. { Widget::Widgets child_widgets; Widget::GetAllChildWidgets(native_view, &child_widgets); CHECK_GE(1u, child_widgets.size()); // 1 (itself) or 0 if detached. } if (!host) { BridgedContentView* old_superview = base::mac::ObjCCastStrict<BridgedContentView>( [native_view superview]); DCHECK(old_superview); [old_superview clearAssociationForView:host]; return; } BridgedContentView* new_superview = base::mac::ObjCCastStrict<BridgedContentView>( new_parent->GetNativeView()); DCHECK(new_superview); [new_superview setAssociationForView:host toNativeView:native_view]; } Then in @implementation BridgedContentView (after adding a std::map<View*, NSView*> associated_views member) - (void)setAssociationForView:(views::View*)view toNativeView(NSView*)native_view { DCHECK_EQ(0u, associated_views.count(view)); [self addSubview:native_view]; associated_views[view] = native_view; hostedView_->GetWidget()->ReorderNativeViews(); } - (void)clearAssociatedView:(views::View*)view { auto it = associated_views.find(view); DCHECK(it != associated_views.end()); [it->second removeFromSuperview] associated_views.erase(it); } https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:123: if (hosts != sorted_hosts) nit: need curlies (policy is if the body is multi lines [including comments] then it must have curlies. If the body is one line, curlies are optional [generally preferred not]. Note the lines that the _condition_ takes up isn't a factor -- you can have a multi-line `if (..)` condition without curlies) https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:124: [widget->GetNativeView() sortSubviewsUsingFunction:&SubviewSorter If we are worried about using this function in particular, you can do NSView* superview = [widget->GetNativeView() subviews] NSArray new_subviews = [superview sortedArrayUsingFunction:.. context:..]; [superview setSubviews:new_subviews]; But they should really be amounting to the same thing in AppKit. However, there is also sortedArrayWithOptions:usingComparator: which we get access to on NSArray - that would allow us to pass NSSortStable so that the unassociated NSViews that compare as NSOrderedSame do not get reordered. (We may actually need that.. but if it adds complexity we should wait until there is a need)
https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_mac_utils.mm (right): https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:70: auto left_it = std::find_if(hosts->begin(), hosts->end(), EqualToNSView(lhs)); I used vector of pairs as one container for all (against associative map + views order map); because it just looks simpler for me. Also it allows to check if views is already in correct order (hosts != sorted_hosts). But there is not a big difference. So I'll use a map if you want. BTW I've checked the performance with clang. Small vector is faster without optimizations and with -O2. Map is faster with -O1. https://gist.github.com/kirr/9480fc2e4836f27fd814
https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_mac_utils.mm (right): https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:70: auto left_it = std::find_if(hosts->begin(), hosts->end(), EqualToNSView(lhs)); On 2016/03/15 22:54:53, tapted wrote: > This call is O(n) so it makes the sorting algorithm O(n*n*log(n)); My suggestion > to first collect the "rank" as an integer would keep the algorithm at > O(n*log(n)*log(n)) (or just O(n*log(n)) if a hashmap is used for |rank|). Was > there a reason you didn't like that idea? (I think also neater/easier to read > than using find_if and the extra functors.). > > Comparator functions typically need to scale well - the shouldn't take time > proportional to the number of things being sorted. But I guess there also aren't > expected to be particularly many associated views in a particular widget. If we > are assuming that we can comment here that this makes the comparator slow, but > it shouldn't matter. Done. https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:77: if (left_found != right_found) { On 2016/03/15 22:54:53, tapted wrote: > nit: doesn't need curlies Done. https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:81: if (left_found) On 2016/03/15 22:54:53, tapted wrote: > nit: needs curlies Done. https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:95: void AttachNSViewRelatedToHost(NSView* native_view, NativeViewHost* host) { On 2016/03/15 22:54:53, tapted wrote: > I don't think this function is necessary. > > Aura observes the kViewHostKey being added to the NativeView and then just does > `reorderer_->ReorderChildWindows();`. We can do the same, but without an > observer. (just do `[new_superview addSubview:native_view];` and call > ReorderChildNSViews) > > In fact I'd go with something like > > > void ReparentNSView(NSView* native_view, NativeViewHost* host) { > DCHECK(native_view); > // Mac's NativeViewHost has no support for hosting its own child widgets. > // This check is probably overly restrictive, since the Widget containing the > // NativeViewHost _is_ allowed child Widgets. However, we don't know yet > // whether those child Widgets need to be distinguished from Widgets that code > // might want to associate with the hosted NSView instead. > { > Widget::Widgets child_widgets; > Widget::GetAllChildWidgets(native_view, &child_widgets); > CHECK_GE(1u, child_widgets.size()); // 1 (itself) or 0 if detached. > } > > if (!host) { > BridgedContentView* old_superview = > base::mac::ObjCCastStrict<BridgedContentView>( > [native_view superview]); > DCHECK(old_superview); > [old_superview clearAssociationForView:host]; > return; > } > > BridgedContentView* new_superview = > base::mac::ObjCCastStrict<BridgedContentView>( > new_parent->GetNativeView()); > DCHECK(new_superview); > [new_superview setAssociationForView:host toNativeView:native_view]; > } > > > Then in @implementation BridgedContentView (after adding a std::map<View*, > NSView*> associated_views member) > > - (void)setAssociationForView:(views::View*)view > toNativeView(NSView*)native_view { > DCHECK_EQ(0u, associated_views.count(view)); > [self addSubview:native_view]; > associated_views[view] = native_view; > hostedView_->GetWidget()->ReorderNativeViews(); > } > > - (void)clearAssociatedView:(views::View*)view { > auto it = associated_views.find(view); > DCHECK(it != associated_views.end()); > [it->second removeFromSuperview] > associated_views.erase(it); > } Done. https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:123: if (hosts != sorted_hosts) On 2016/03/15 22:54:53, tapted wrote: > nit: need curlies > > (policy is if the body is multi lines [including comments] then it must have > curlies. If the body is one line, curlies are optional [generally preferred > not]. Note the lines that the _condition_ takes up isn't a factor -- you can > have a multi-line `if (..)` condition without curlies) Done. Thanks. https://codereview.chromium.org/1796773003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:124: [widget->GetNativeView() sortSubviewsUsingFunction:&SubviewSorter On 2016/03/15 22:54:53, tapted wrote: > If we are worried about using this function in particular, you can do > > NSView* superview = [widget->GetNativeView() subviews] > NSArray new_subviews = [superview sortedArrayUsingFunction:.. context:..]; > [superview setSubviews:new_subviews]; > > But they should really be amounting to the same thing in AppKit. > > > However, there is also > > sortedArrayWithOptions:usingComparator: > > which we get access to on NSArray - that would allow us to pass NSSortStable so > that the unassociated NSViews that compare as NSOrderedSame do not get > reordered. (We may actually need that.. but if it adds complexity we should wait > until there is a need) Seems like it's better not worry about it until we have no case. https://codereview.chromium.org/1796773003/diff/40001/ui/views/controls/nativ... File ui/views/controls/native/native_view_host_mac.mm (right): https://codereview.chromium.org/1796773003/diff/40001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_mac.mm:70: if (bridge) I've also tried to use BridgedContentView for holding associative_views, and there was also problems with removing views in widget destructor. When using content view FocusTraversalTest fails because NativeViewHost is content view there.
Thanks! This looks pretty neat https://codereview.chromium.org/1796773003/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.h (right): https://codereview.chromium.org/1796773003/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. CL description: perhaps something like `Implement NativeWidgetMac::ReorderNativeViews` NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost... etc. https://codereview.chromium.org/1796773003/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.h:160: // Updates associated_views_ on NativeViewHost::Attach/Detach nit: .. |associated_views_| .. NativeViewHost::Attach()/Detach(). https://codereview.chromium.org/1796773003/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.h:163: void ReorderChildViews(); This should have a separate comment https://codereview.chromium.org/1796773003/diff/40001/ui/views/controls/nativ... File ui/views/controls/native/native_view_host_mac.mm (right): https://codereview.chromium.org/1796773003/diff/40001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_mac.mm:10: #include "ui/views/cocoa/bridged_native_widget.h" nit: import https://codereview.chromium.org/1796773003/diff/40001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_mac.mm:70: if (bridge) On 2016/03/16 21:43:45, kirr wrote: > I've also tried to use BridgedContentView for holding associative_views, and > there was also problems with removing views in widget destructor. > When using content view FocusTraversalTest fails because NativeViewHost is > content view there. Yeah that makes sense - BridgedNativeWidget is fine. The null check concerns me a little -- it doesn't surprise me that it's needed, I just wish I was more certain that there wasn't a subtle problem lurking there. I think it's OK for now - if there's problems we can try to figure out a way to obtain the BridgedNativeWidget, or find a way to ensure everything is detached. i.e. it would be "nice" BridgedNativeWidget could be destroyed without attachments, since it's a stronger guarantee that there are no lifetime issues. Does adding a DCHECK(GetWidget()->IsClosed()); work? That would be nice to have but, again, I wouldn't be surprised if that's only just *about* to get set. https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:229: gfx::NativeView GetView() const { return view.get(); } nit: these accessors can be in hacker_style -- that's more typical for inline stuff. i.e. GetView() -> view() https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:230: NativeViewHost* GetHost() const { return host.get(); } GetHost -> host() https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:233: base::scoped_nsobject<NSView> view; view->view_ https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:234: scoped_ptr<NativeViewHost> host; host->host_ https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:235: }; nit: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:237: void CheckViewsOrderEqual(NSArray* actual, NSArray* expected) { Comment why this can't just be isEqualToArray. (I'm assuming because there are `extra` views in there, but where do they come from?) https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1377: // Test that NativeViewHost::Attach/Detach method save NativeView z-order nit: Test that NativeViewHost::Attach()/Detach() method saves the NativeView z-order https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1381: const int native_views_count = 3; optional nit: kNativeViewCount https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1385: new NativeHostHolder([[NSView alloc] init], new NativeViewHost)); it would be nicer to move the initialization into NativeHostHolder -- we try not to pass ownership of objects via raw pointers. NativeHostHolder can just have a default constructor. https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1392: [NSArray arrayWithObjects:hosts[0]->GetView(), hosts[1]->GetView(), you can use an array literal, like @[ hosts[0]->view(), hosts[1]->view(), hosts[2]->view() ] https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1407: I think there should be a bit more coverage. There's maybe some ideas in window_reorderer_unittest.cc, but they seem overly complicated. Perhaps just something that populates a Widget::GetContentView() with the views in a NativeHostHolder using AddChildView/AddChildViewAt. Then call Widget::GetContentView::ReorderChildView and ensure the native subview order is preserved. Perhaps add a couple of unassociated native NSViews to ensure they stack at the top, too. Note scoped_ptr in NativeHostHolder could get in the way - you could either explicitly do View::DoRemoveChildView(..) to prevent deletion, or just ensure |hosts| is destroyed before the widget is closed -- that should delete all the views and auto-remove them from the hierarchy. https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/widget_... File ui/views/widget/widget_mac_utils.mm (right): https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/widget_... ui/views/widget/widget_mac_utils.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. Can this entire file just be moved into the anonymous namespace in brridged_native_widget? (except the typedef - that can go on BridgedNativeWidget -- and ReorderChildNSViews might as well just go on BridgedNativeWidget as a member function) (otherwise, it should go in ui/views/cocoa/ and.. probably be called reorder_child_nsviews.mm or something -- foo_utils.h files have a tendency to become a grab-bag of unrelated stuff that don't belong together)
Description was changed from ========== Take into account views order when attaching NSView in NativeViewHost. Otherwise last attached native view is always on top. View order is important when showing DevTools. BUG=594567 ========== to ========== NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 ==========
On 2016/03/17 08:18:59, tapted wrote: > CL description: perhaps something like `Implement > NativeWidgetMac::ReorderNativeViews` Done > https://codereview.chromium.org/1796773003/diff/40001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget.h:160: // Updates associated_views_ on > NativeViewHost::Attach/Detach > nit: .. |associated_views_| .. NativeViewHost::Attach()/Detach(). Done > https://codereview.chromium.org/1796773003/diff/40001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget.h:163: void ReorderChildViews(); > This should have a separate comment Done > https://codereview.chromium.org/1796773003/diff/40001/ui/views/controls/nativ... > ui/views/controls/native/native_view_host_mac.mm:10: #include > "ui/views/cocoa/bridged_native_widget.h" > nit: import Done > https://codereview.chromium.org/1796773003/diff/40001/ui/views/controls/nativ... > ui/views/controls/native/native_view_host_mac.mm:70: if (bridge) > Does adding a > > DCHECK(GetWidget()->IsClosed()); > > work? That would be nice to have but, again, I wouldn't be surprised if that's > only just *about* to get set. Unfortunately not. In tests widget is closing via Widget::CloseNow, that not set the flag. > https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:229: gfx::NativeView GetView() > stuff. i.e. GetView() -> view() > GetHost -> host() > view->view_ > host->host_ > nit: DISALLOW_COPY_AND_ASSIGN(..) Done > https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:237: void > CheckViewsOrderEqual(NSArray* actual, NSArray* expected) { > Comment why this can't just be isEqualToArray. (I'm assuming because there are > `extra` views in there, but where do they come from?) Replaced by isEqualToArray. Extra view - is composito_view_ in BridgedNativeWidgetMac. > https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:1377: // Test that > NativeViewHost::Attach/Detach method save NativeView z-order > nit: Test that NativeViewHost::Attach()/Detach() method saves the NativeView > z-order > optional nit: kNativeViewCount Done > https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:1385: new > NativeHostHolder([[NSView alloc] init], new NativeViewHost)); > it would be nicer to move the initialization into NativeHostHolder -- we try not > to pass ownership of objects via raw pointers. NativeHostHolder can just have a > default constructor. Done. Thanks. > https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:1392: [NSArray > arrayWithObjects:hosts[0]->GetView(), hosts[1]->GetView(), > you can use an array literal, like > @[ hosts[0]->view(), hosts[1]->view(), hosts[2]->view() ] Done. Thanks. > https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:1407: > I think there should be a bit more coverage. There's maybe some ideas in > window_reorderer_unittest.cc, but they seem overly complicated. Done. > https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/widget_... > File ui/views/widget/widget_mac_utils.mm (right): > > https://codereview.chromium.org/1796773003/diff/40001/ui/views/widget/widget_... > ui/views/widget/widget_mac_utils.mm:1: // Copyright 2016 The Chromium Authors. > All rights reserved. > Can this entire file just be moved into the anonymous namespace in > brridged_native_widget? (except the typedef - that can go on BridgedNativeWidget > -- and ReorderChildNSViews might as well just go on BridgedNativeWidget as a > member function) Done Also fixed sorting order and make append/removing native view independent of bridge existence.
looks good! After fixing the stuff below, it should be good to loop in an owner for NativeViewHostMac (probably sky), so you avoid another timezone round-trip (I'm in Sydney, but the other owners are in the US). https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.h (right): https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. CL description: git doesn't automatically put the summary field into the log, so it needs to be repeated in the description, with a blank line after. Also, the description should be wrapped at 72 cols https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.h:162: // Updates |associated_views_| on NativeViewHost::Attach()/Detach() nit: full stop at end. https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.h:165: // Sorts child NSViews accrording to NativeViewHosts order in views hierarchy. accrording -> according https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:938: // Set compositor view below Yep - this makes sense, but the comment should explain a bit more. E.g. // Unassociated NSViews should be ordered above associated ones. The exception // is the UI compositor's superview, which should always be on the very // bottom, so give it an explicit negative rank. https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:940: rank[compositor_superview_.get()] = -1; The .get() shouldn't be required -- scoped_nsobject auto-converts usually https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:941: [widget->GetNativeView() sortSubviewsUsingFunction:&SubviewSorter widget->GetNativeView() --> bridged_view_ https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... File ui/views/controls/native/native_view_host_mac.mm (right): https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_mac.mm:48: EnsureNativeViewHasNoChildWidgets(native_view_.get()); nit: .get() shouldn't be required https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_mac.mm:49: [host_->GetWidget()->GetNativeView() addSubview:native_view_.get()]; Yep - I like this change. But can you move this below the DCHECK(bridge) and do [bridge->ns_view() addSubview:native_view]; (the .get() shouldn't be required) https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_mac.mm:53: bridge->SetAssociationForView(host_, native_view_.get()); nit: no .get() https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_mac.mm:72: if (bridge) { nit: curlies not required https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:219: host_->set_owned_by_client(); oh neat - I forgot you could do this https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1369: void SetUp() override { nit: // testing::Test: https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1387: EXPECT_EQ(native_host_parent_->child_count(), kNativeViewCount); nit: EXPECT_EQ(kNativeViewCount, native_host_parent_->child_count()); (putting the "more constant" value on the left makes the logging more sensible) https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1405: private: It's actually fine to have protected members in test harnesses (there's an exception in the style guide -- just ensure the DISALLOW is still private). This would allow you do do hosts_[i]->view, which feels a lot neater than hosts()[i]->view https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:1462: [GetContentNativeView() addSubview:child_view.get()]; nit: no .get()
https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:278: NSComparisonResult SubviewSorter(id lhs, id rhs, void* rank_as_void) { Make sure you do a build with xcode 7 too. I had to change this method signature to NSComparisonResult SubviewSorter(__kindof NSView* lhs, __kindof NSView* rhs, void* rank_as_void) to avoid ../../ui/views/cocoa/bridged_native_widget.mm:941:54: error: incompatible pointer types sending 'NSComparisonResult (*)(id, id, void *)' to parameter of type 'NSComparisonResult (* _Nonnull)(__kindof NSView * _Nonnull, __kindof NSView * _Nonnull, void * _Nullable)' (aka 'NSComparisonResult (*)(__kindof NSView * _Nonnull, __kindof NSView * _Nonnull, void * _Nullable)') [-Werror,-Wincompatible-pointer-types] However, I think the bots are still xcode 6, and that probably doesn't have __kindof, so we might need some other way to fix this, or have a #if xcode < 7 check that #defines it to something empty. Sadly, if you break developer's compiles on xcode 7, that's probably enough to have the patch quickly reverted, even if no bots break.
Description was changed from ========== NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 ========== to ========== Implement NativeWidgetMac::ReorderNativeViews. NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 ==========
Description was changed from ========== Implement NativeWidgetMac::ReorderNativeViews. NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 ========== to ========== Implement NativeWidgetMac::ReorderNativeViews. NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 ==========
kirr@yandex-team.ru changed reviewers: + sky@chromium.org
Fixed nits. Use typedef for xcode 7 compilation.
I'm assuming Trent is doing a thorough review. Rubber stamp LGTM from me.
lgtm - thanks! Don't forget to publish out the CL drafts marking the last round of nits 'Done.'
> CL description: git doesn't automatically put the summary field into the log, so > it needs to be repeated in the description, with a blank line after. > > Also, the description should be wrapped at 72 cols Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget.h:162: // Updates |associated_views_| on > NativeViewHost::Attach()/Detach() > nit: full stop at end. Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget.h:165: // Sorts child NSViews accrording to > NativeViewHosts order in views hierarchy. > accrording -> according Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget.mm:938: // Set compositor view below > Yep - this makes sense, but the comment should explain a bit more. E.g. > > // Unassociated NSViews should be ordered above associated ones. The exception > // is the UI compositor's superview, which should always be on the very > // bottom, so give it an explicit negative rank. Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget.mm:940: rank[compositor_superview_.get()] = > -1; > The .get() shouldn't be required -- scoped_nsobject auto-converts usually Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget.mm:941: [widget->GetNativeView() > sortSubviewsUsingFunction:&SubviewSorter > widget->GetNativeView() --> bridged_view_ Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... > ui/views/controls/native/native_view_host_mac.mm:48: > EnsureNativeViewHasNoChildWidgets(native_view_.get()); > nit: .get() shouldn't be required Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... > ui/views/controls/native/native_view_host_mac.mm:49: > [host_->GetWidget()->GetNativeView() addSubview:native_view_.get()]; > Yep - I like this change. But can you move this below the DCHECK(bridge) and do > [bridge->ns_view() addSubview:native_view]; > (the .get() shouldn't be required) Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... > ui/views/controls/native/native_view_host_mac.mm:53: > bridge->SetAssociationForView(host_, native_view_.get()); > nit: no .get() Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/controls/nativ... > ui/views/controls/native/native_view_host_mac.mm:72: if (bridge) { > nit: curlies not required Done > https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:1369: void SetUp() override { > nit: > > // testing::Test: Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:1387: > EXPECT_EQ(native_host_parent_->child_count(), kNativeViewCount); > nit: EXPECT_EQ(kNativeViewCount, native_host_parent_->child_count()); > > (putting the "more constant" value on the left makes the logging more sensible) Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:1405: private: > It's actually fine to have protected members in test harnesses (there's an > exception in the style guide -- just ensure the DISALLOW is still private). This > would allow you do do hosts_[i]->view, which feels a lot neater than > hosts()[i]->view Done. > https://codereview.chromium.org/1796773003/diff/80001/ui/views/widget/native_... > ui/views/widget/native_widget_mac_unittest.mm:1462: [GetContentNativeView() > addSubview:child_view.get()]; > nit: no .get() Done.
The CQ bit was checked by kirr@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1796773003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1796773003/100001
Message was sent while issue was closed.
Description was changed from ========== Implement NativeWidgetMac::ReorderNativeViews. NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 ========== to ========== Implement NativeWidgetMac::ReorderNativeViews. NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implement NativeWidgetMac::ReorderNativeViews. NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 ========== to ========== Implement NativeWidgetMac::ReorderNativeViews. NativeWidgetMac doesn't currently take into account native view order when attaching NSViews in NativeViewHost and in view::View::ReorderChildView. It is important when showing DevTools. BUG=594567 Committed: https://crrev.com/852a2ec06f9aac8f1b048d241a5bb8f9c5559d13 Cr-Commit-Position: refs/heads/master@{#382259} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/852a2ec06f9aac8f1b048d241a5bb8f9c5559d13 Cr-Commit-Position: refs/heads/master@{#382259} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
