Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24299004:
Implement features in NativeViewHostAura for scroll end effect (Closed)
|
Created:
7 years, 3 months ago by rharrison Modified:
7 years, 2 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement features in NativeViewHostAura for scroll end effect
This CL enables the fast resize path on Aura by implementing
NativeViewHostAura::InstallClip. It also adds the concept of gravity to
the fast resize state to control the positioning of the clip relative
to the contents. Gravity is implemented for Aura only in this CL and
ignored in other implementations.
BUG=151356
BUG=282463
TEST=Confirmed the fast path and gravity works in conjunction with
scroll end effect CL. Added and updated unit tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228751
Patch Set 1 #
Total comments: 20
Patch Set 2 : Responded to sky's comments and added some testing #
Total comments: 12
Patch Set 3 : Responsed to sky's comments and finished tests #
Total comments: 16
Patch Set 4 : Iterated on a nother batch of comments from sky #
Total comments: 24
Patch Set 5 : Integrated xiyuan's CL and responded to comments #
Total comments: 10
Patch Set 6 : Fixed remaining concerns from sky and xiyuan #
Total comments: 4
Patch Set 7 : Addressed more comments from xiyuan #
Total comments: 6
Patch Set 8 : Fix nits from xiyuan #
Total comments: 13
Patch Set 9 : Cleaned up unit tests and responded to other comments from sky@ #Messages
Total messages: 33 (0 generated)
Moved the changes that I have made to NVHA into a seperate CL as requested.
On 2013/09/24 18:58:33, rharrison wrote: > Moved the changes that I have made to NVHA into a seperate CL as requested. I am still working on the tests for this CL.
https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... File ui/views/controls/native/native_view_host.h (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host.h:22: // the clip and the content should align, so the bottom and left sides of the left -> right https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host.h:38: enum FastResizeGravity { I'm not opposed to all of these, but do we really need them all? https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host.h:38: enum FastResizeGravity { Could you also make this a member of NativeViewHost. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.cc:78: clipping_layer_->Add(host_->native_view()->layer()); In general I don't like temporarily adding layers this. Could you instead add a View that has the clipping layer? Said view would always be present. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.cc:209: }; nit: no ; (same on 245 below). Also, remove the default switch so you get compile error. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.h:44: gfx::Point CalculateNewNativeViewOrigin(gfx::Rect input_rect, const gfx::Rect& on these (same for CalculateCLipOrigin). Also, add description for all members (eg GetWidthFactory, orig_bounds_...) https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.h:57: scoped_ptr<ui::Layer> clipping_layer_; If the life of clipping_layer is the same as NVHA, then no point in using scoped_ptr. Well, actually you could use it to avoid the header include, but style guide says no to that. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/webview/web... File ui/views/controls/webview/webview.cc (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/webview/web... ui/views/controls/webview/webview.cc:111: nit: only one newline. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/webview/web... File ui/views/controls/webview/webview.h (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/webview/web... ui/views/controls/webview/webview.h:26: enum FastResizeGravity; not needed
https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.cc:78: clipping_layer_->Add(host_->native_view()->layer()); On 2013/09/24 20:23:25, sky wrote: > In general I don't like temporarily adding layers this. Could you instead add a > View that has the clipping layer? Said view would always be present. The NativeView (aura::Window) attached to a NativeViewHost is parented to the aura::Window hosting the Widget. I don't think we have a good way of parenting an aura::Window to a View's layer (which makes it difficult to insert a View that clips a child Window). I believe this is why Ryan is dancing with the layers here. If we wanted to avoid dealing directly with layers, then an alternate approach could be to add a clipping Window to NativeViewHostAura. The clipping Window becomes a child of the Widget's Window, and the NativeView becomes a child of the clipping window. The NativeViewHostAura can then alter the clipping Window's bounds as needed.
https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.cc:78: clipping_layer_->Add(host_->native_view()->layer()); On 2013/09/25 21:27:23, sadrul wrote: > On 2013/09/24 20:23:25, sky wrote: > > In general I don't like temporarily adding layers this. Could you instead add > a > > View that has the clipping layer? Said view would always be present. > > The NativeView (aura::Window) attached to a NativeViewHost is parented to the > aura::Window hosting the Widget. I don't think we have a good way of parenting > an aura::Window to a View's layer (which makes it difficult to insert a View > that clips a child Window). I believe this is why Ryan is dancing with the > layers here. > > If we wanted to avoid dealing directly with layers, then an alternate approach > could be to add a clipping Window to NativeViewHostAura. The clipping Window > becomes a child of the Widget's Window, and the NativeView becomes a child of > the clipping window. The NativeViewHostAura can then alter the clipping Window's > bounds as needed. Good point. I like the clipping Window approach. That way we aren't dynamically adjusting things, which often times is error prone.
Switched over to using a clipping window and made other requested changes. I have also added some testing. I intend to add more, but wanted to give you the chance to see my new implementation while I work on it. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... File ui/views/controls/native/native_view_host.h (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host.h:22: // the clip and the content should align, so the bottom and left sides of the On 2013/09/24 20:23:25, sky wrote: > left -> right Done. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host.h:38: enum FastResizeGravity { On 2013/09/24 20:23:25, sky wrote: > I'm not opposed to all of these, but do we really need them all? The default is NW and I would need a southern one for the effect I am working on, but we don't need them all at the moment. I was following along with what Xlib does, so decided to do the 8 directions plus centre. I am peronally implementing them all since it seems a little weird to me to have just a few of the directions implemented, but not all of them. The additional directions don't require substantial or complex code to support them, just additional simple case statements. If there needed to be special handling or more involved code then I would be more concerned. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host.h:38: enum FastResizeGravity { On 2013/09/24 20:23:25, sky wrote: > Could you also make this a member of NativeViewHost. Done. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.cc:78: clipping_layer_->Add(host_->native_view()->layer()); On 2013/09/25 22:31:03, sky wrote: > On 2013/09/25 21:27:23, sadrul wrote: > > On 2013/09/24 20:23:25, sky wrote: > > > In general I don't like temporarily adding layers this. Could you instead > add > > a > > > View that has the clipping layer? Said view would always be present. > > > > The NativeView (aura::Window) attached to a NativeViewHost is parented to the > > aura::Window hosting the Widget. I don't think we have a good way of parenting > > an aura::Window to a View's layer (which makes it difficult to insert a View > > that clips a child Window). I believe this is why Ryan is dancing with the > > layers here. > > > > If we wanted to avoid dealing directly with layers, then an alternate approach > > could be to add a clipping Window to NativeViewHostAura. The clipping Window > > becomes a child of the Widget's Window, and the NativeView becomes a child of > > the clipping window. The NativeViewHostAura can then alter the clipping > Window's > > bounds as needed. > > Good point. I like the clipping Window approach. That way we aren't dynamically > adjusting things, which often times is error prone. Done. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.cc:209: }; On 2013/09/24 20:23:25, sky wrote: > nit: no ; (same on 245 below). Also, remove the default switch so you get > compile error. Done. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.h:44: gfx::Point CalculateNewNativeViewOrigin(gfx::Rect input_rect, On 2013/09/24 20:23:25, sky wrote: > const gfx::Rect& on these (same for CalculateCLipOrigin). > > Also, add description for all members (eg GetWidthFactory, orig_bounds_...) Done. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/native/nati... ui/views/controls/native/native_view_host_aura.h:57: scoped_ptr<ui::Layer> clipping_layer_; On 2013/09/24 20:23:25, sky wrote: > If the life of clipping_layer is the same as NVHA, then no point in using > scoped_ptr. Well, actually you could use it to avoid the header include, but > style guide says no to that. Done. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/webview/web... File ui/views/controls/webview/webview.cc (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/webview/web... ui/views/controls/webview/webview.cc:111: On 2013/09/24 20:23:25, sky wrote: > nit: only one newline. Done. https://codereview.chromium.org/24299004/diff/1/ui/views/controls/webview/web... File ui/views/controls/webview/webview.h (right): https://codereview.chromium.org/24299004/diff/1/ui/views/controls/webview/web... ui/views/controls/webview/webview.h:26: enum FastResizeGravity; On 2013/09/24 20:23:25, sky wrote: > not needed Done.
https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host.cc (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host.cc:74: ret_val = 0.0; There is no point in assigning to ret_val, break, return. Instead just return. Same comment in GetHeightScaleFactor. https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host.h (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host.h:118: Gravity fast_resize_gravity() { const https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:21: clipping_window_.SetTransparent(true); I suspect the ownership isn't right here. Create a test that creates a widget with ownership of NATIVE_WIDGET_OWNS_WIDGET (the default), add a NativeView with an aura::window to the widget, and then close the widget (widget->CloseNow()). https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:43: AddClippingWindow(); Replace this and NativeWillAttach with AttachNativeView, and move the call to ReparentWindow to here and NativeViewHostWin. https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_win.cc (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host_win.cc:101: // TODO: Implement support for gravity. Nuke this. This class should be dead once aura is the default. https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_win.h (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host_win.h:23: virtual void NativeViewAttached() { } style guide says no space between parens, eg {}
https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host.cc (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host.cc:74: ret_val = 0.0; On 2013/09/26 22:10:54, sky wrote: > There is no point in assigning to ret_val, break, return. Instead just return. > Same comment in GetHeightScaleFactor. Done. https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host.h (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host.h:118: Gravity fast_resize_gravity() { On 2013/09/26 22:10:54, sky wrote: > const Done. https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:21: clipping_window_.SetTransparent(true); On 2013/09/26 22:10:54, sky wrote: > I suspect the ownership isn't right here. Create a test that creates a widget > with ownership of NATIVE_WIDGET_OWNS_WIDGET (the default), add a NativeView with > an aura::window to the widget, and then close the widget (widget->CloseNow()). I have moved the ownership out of this class, so I think this has been resolved. https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:43: AddClippingWindow(); On 2013/09/26 22:10:54, sky wrote: > Replace this and NativeWillAttach with AttachNativeView, and move the call to > ReparentWindow to here and NativeViewHostWin. Done. https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_win.cc (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host_win.cc:101: // TODO: Implement support for gravity. On 2013/09/26 22:10:54, sky wrote: > Nuke this. This class should be dead once aura is the default. Done. https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_win.h (right): https://codereview.chromium.org/24299004/diff/13001/ui/views/controls/native/... ui/views/controls/native/native_view_host_win.h:23: virtual void NativeViewAttached() { } On 2013/09/26 22:10:54, sky wrote: > style guide says no space between parens, eg {} Done.
https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... File ui/views/controls/native/native_view_host.h (right): https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host.h:18: nit: remove newline. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:37: Widget::ReparentNativeView(host_->native_view(), Can't this be done using the clipping layer as the new parent and after AddclippingWindow? https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:40: Widget* widget = Widget::GetWidgetForNativeView(host_->native_view()); Can these three lines be kept in NativeViewHost? https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:44: host_->Layout(); How come Layout is invoked at the end here, but not in NativeViewHostWin? https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:150: if (window == clipping_window_) How will this ever happen since you don't add this as an observer of clipping_window_? I'm still worried that ownership isn't quite right here. What happens if clipping_window_ is deleted out from under you? Does the code deal with that? How about a test? This case could likely happen if someone deletes a Widget that contains a NativeViewHost. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:10: #include "base/memory/scoped_ptr.h" remove include. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:11: #include "ui/aura/window.h" Can you forward declare window? https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:13: #include "ui/compositor/layer.h" Is this needed?
PTAL https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... File ui/views/controls/native/native_view_host.h (right): https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host.h:18: On 2013/10/01 15:06:43, sky wrote: > nit: remove newline. Done. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:37: Widget::ReparentNativeView(host_->native_view(), On 2013/10/01 15:06:43, sky wrote: > Can't this be done using the clipping layer as the new parent and after > AddclippingWindow? I have moved all of the reparenting down into AddClippingWindow. Done https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:40: Widget* widget = Widget::GetWidgetForNativeView(host_->native_view()); On 2013/10/01 15:06:43, sky wrote: > Can these three lines be kept in NativeViewHost? Done. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:44: host_->Layout(); On 2013/10/01 15:06:43, sky wrote: > How come Layout is invoked at the end here, but not in NativeViewHostWin? I most likely moved it to the end, since the layout should be call after all of the hiearchy changes, but forgot to move it in _win. I will make sure they match. Done. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:150: if (window == clipping_window_) On 2013/10/01 15:06:43, sky wrote: > How will this ever happen since you don't add this as an observer of > clipping_window_? > > I'm still worried that ownership isn't quite right here. What happens if > clipping_window_ is deleted out from under you? Does the code deal with that? > How about a test? This case could likely happen if someone deletes a Widget that > contains a NativeViewHost. I have written the clipping_window_ ownership code again. Specifically it is a member of NVHA, but owned_by_parent is being set to false, so that its life time is tied to the NVHA, instead of their being contention with the hierarchy owning it. I think the ownership has been significantly simplified by doing this, but I have added a test that creates a widget and attaches a host to it. The widget is then deleted and it is confirmed that the host is deleted also. There is an implicit condition in the test that it completes that confirms nothing really bad is occurring in this case. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:10: #include "base/memory/scoped_ptr.h" On 2013/10/01 15:06:43, sky wrote: > remove include. Done. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:11: #include "ui/aura/window.h" On 2013/10/01 15:06:43, sky wrote: > Can you forward declare window? Changed code to have clipping_window_ owned by this class, so this is needed again. https://codereview.chromium.org/24299004/diff/22001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:13: #include "ui/compositor/layer.h" On 2013/10/01 15:06:43, sky wrote: > Is this needed? Done.
https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:83: gfx::Rect input_rect(x, y, w, h); nit: seems not used. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:86: orig_bounds_ = clipping_window_.layer()->bounds(); What if InstallClip is called while it has a clip? This could happen when we SetBoundsRect on NativeViewHost during an animation. Should we cache the value in ShowWidget call? https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:93: clipping_window_.layer()->SetBounds(clipping_bounds); host_ calls InstallClip with (x,y) in its own cooridates (relative the the origin of the hosted native view). Can we directly use it here? https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:121: if (host_->fast_resize()) { Is it possible to make it work for non fast resize path as well? https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:123: InstallClip(x, y, w, h); ShowWidget's (x,y) is in host_->GetWidget()'s coordinates. And InstallClip expect something relative to host_->native_view(). Think we need translation here.
https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:63: host_->GetWidget()->GetNativeView()) nit: I think you can fit this on one line. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:76: RemoveClippingWindow(); Why do you need anything more than removing the clipping window from its parent here? https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:85: if (!installed_clip_) { nit: no {} https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:112: clipping_window_.layer()->SetBounds(orig_bounds_); ShowWidget is invoked right after this. Can you wait to set the bounds until then? https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:147: if (window == host_->native_view()) Why the if and not the DCHECK? This only adds one observer. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:73: aura::Window clipping_window_; // non-owned non-owned doesn't make sense here.
I have created https://codereview.chromium.org/26516002/ based on this CL with clipping support on none-fast-resize path. Patch #1 is the CL here and #2 is the modifications I made.
Xiyuan, you should ping rharrison to see if he wants you to take it over. -Scott On Tue, Oct 8, 2013 at 9:45 AM, <xiyuan@chromium.org> wrote: > I have created https://codereview.chromium.org/26516002/ based on this CL > with > clipping support on none-fast-resize path. Patch #1 is the CL here and #2 is > the > modifications I made. > > https://codereview.chromium.org/24299004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/08 20:52:43, sky wrote: > Xiyuan, you should ping rharrison to see if he wants you to take it over. > > -Scott > > On Tue, Oct 8, 2013 at 9:45 AM, <mailto:xiyuan@chromium.org> wrote: > > I have created https://codereview.chromium.org/26516002/ based on this CL > > with > > clipping support on none-fast-resize path. Patch #1 is the CL here and #2 is > > the > > modifications I made. > > > > https://codereview.chromium.org/24299004/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org Sorry for the delay in responding. I was trying to refactor the CL so that it was simpler, but that failed and ended up wasting too much time :-/. Looking at it again, I like the approach that xiyuan has taken. I am currently working on integrating the other comments with it and making sure it works with the overscroll effect CL. I am CrOS sheriff today and tomorrow, so I am hoping to have a new version of the CL up on Thursday, since this weekend is Thanksgiving in Canada and I have relatives visiting for the long weekend. xiyuan, if this is blocking work other then the overscroll end effect feel free to take it over if you have the cycles, but otherwise I can continue working on it.
On 2013/10/08 21:35:10, rharrison wrote: > Sorry for the delay in responding. I was trying to refactor the CL so that it > was simpler, but that failed and ended up wasting too much time :-/. > > Looking at it again, I like the approach that xiyuan has taken. I am currently > working on integrating the other comments with it and making sure it works with > the overscroll effect CL. I am CrOS sheriff today and tomorrow, so I am hoping > to have a new version of the CL up on Thursday, since this weekend is > Thanksgiving in Canada and I have relatives visiting for the long weekend. > > xiyuan, if this is blocking work other then the overscroll end effect feel free > to take it over if you have the cycles, but otherwise I can continue working on > it. Thank you for implementing the feature. I put a WebContents inside the launcher bubble to serve as a start page. It does not look nice without the feature. But the start page work is far from finished. It's not blocking anything on my side. So take you time as you need. :)
https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:63: host_->GetWidget()->GetNativeView()) On 2013/10/02 20:49:00, sky wrote: > nit: I think you can fit this on one line. Done. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:76: RemoveClippingWindow(); On 2013/10/02 20:49:00, sky wrote: > Why do you need anything more than removing the clipping window from its parent > here? If we do that then the native will still be attached to the clipping window, which means the existence of the clipping window will leak outside of the class and leaves the native view's life time still tied to the NHVA, since the clipping window, its parent, is tied to the NVHA. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:83: gfx::Rect input_rect(x, y, w, h); On 2013/10/02 20:47:29, xiyuan wrote: > nit: seems not used. Done. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:85: if (!installed_clip_) { On 2013/10/02 20:49:00, sky wrote: > nit: no {} Done. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:86: orig_bounds_ = clipping_window_.layer()->bounds(); On 2013/10/02 20:47:29, xiyuan wrote: > What if InstallClip is called while it has a clip? This could happen when we > SetBoundsRect on NativeViewHost during an animation. Should we cache the value > in ShowWidget call? Done. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:93: clipping_window_.layer()->SetBounds(clipping_bounds); On 2013/10/02 20:47:29, xiyuan wrote: > host_ calls InstallClip with (x,y) in its own cooridates (relative the the > origin of the hosted native view). Can we directly use it here? You are correct, I have fixed this. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:112: clipping_window_.layer()->SetBounds(orig_bounds_); On 2013/10/02 20:49:00, sky wrote: > ShowWidget is invoked right after this. Can you wait to set the bounds until > then? I think you meant InstallClip maybe? I want to avoid having the UninstallClip method depend on a following call to InstallClip or other manipulations. Basically UninstallClip should undo everything InstallClip does. I think the UpdateClippingWindow from your version of the CL cleans this up nicely. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:121: if (host_->fast_resize()) { On 2013/10/02 20:47:29, xiyuan wrote: > Is it possible to make it work for non fast resize path as well? The fast v. non-fast resize paths have semantic/functional differences. Specificaly the fast path uses clipping, but never alerts the underlying native view that a resize has happened, where as the regular resize path is going to be updating the bounds of the native view. This has significant impact on how something like blink behaves, so I don't think using clipping on the fast resize path is appropriate or atleast out of the scope of this CL. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:123: InstallClip(x, y, w, h); On 2013/10/02 20:47:29, xiyuan wrote: > ShowWidget's (x,y) is in host_->GetWidget()'s coordinates. And InstallClip > expect something relative to host_->native_view(). Think we need translation > here. I agree, Done. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:147: if (window == host_->native_view()) On 2013/10/02 20:49:00, sky wrote: > Why the if and not the DCHECK? This only adds one observer. Done. https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:73: aura::Window clipping_window_; // non-owned On 2013/10/02 20:49:00, sky wrote: > non-owned doesn't make sense here. Done.
https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:102: UninstallClip(); This should go inside host_->fast_resize() because in non-fast-resize path, ShowWidget is called after InstallClip and this would incorrectly clear the clip. https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:104: gfx::Rect input_bounds(x, y, w, h); Where is this used? https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:105: InstallClip(x - clipping_window_.bounds().origin().x(), Should we use orig_bounds_ to convert |x|, |y| into host_'s local coordinates? https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:165: orig_bounds_ = bounds; Which coordinates is |orig_bounds_| using? This looks not right. https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:84: gfx::Rect clip_rect_; It would be good to document this and |orig_bounds_| are in which coordinate system. Are they in the host_->GetWidget() coordinates or in the host_'s local coordinates.
https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:76: RemoveClippingWindow(); On 2013/10/09 17:42:20, rharrison wrote: > On 2013/10/02 20:49:00, sky wrote: > > Why do you need anything more than removing the clipping window from its > parent > > here? > > If we do that then the native will still be attached to the clipping window, > which means the existence of the clipping window will leak outside of the class > and leaves the native view's life time still tied to the NHVA, since the > clipping window, its parent, is tied to the NVHA. You're right. My question should be, can we just remove the clipping window and keep the old code on old 63-64? https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:112: clipping_window_.layer()->SetBounds(orig_bounds_); On 2013/10/09 17:42:20, rharrison wrote: > On 2013/10/02 20:49:00, sky wrote: > > ShowWidget is invoked right after this. Can you wait to set the bounds until > > then? > > I think you meant InstallClip maybe? I want to avoid having the UninstallClip > method depend on a following call to InstallClip or other manipulations. > Basically UninstallClip should undo everything InstallClip does. I think the > UpdateClippingWindow from your version of the CL cleans this up nicely. Isn't there already this dependency though? NativeViewHost implementations can assume a particular call order, otherwise things are going to break.
On 2013/10/09 20:21:37, sky wrote: > https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... > File ui/views/controls/native/native_view_host_aura.cc (right): > > https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... > ui/views/controls/native/native_view_host_aura.cc:76: RemoveClippingWindow(); > On 2013/10/09 17:42:20, rharrison wrote: > > On 2013/10/02 20:49:00, sky wrote: > > > Why do you need anything more than removing the clipping window from its > > parent > > > here? > > > > If we do that then the native will still be attached to the clipping window, > > which means the existence of the clipping window will leak outside of the > class > > and leaves the native view's life time still tied to the NHVA, since the > > clipping window, its parent, is tied to the NVHA. > > You're right. My question should be, can we just remove the clipping window and > keep the old code on old 63-64? > I have taken another look at this, we can remove the clipping window and reset the native bounds and keep the existing code. This will be in the next revision I upload. > https://codereview.chromium.org/24299004/diff/45001/ui/views/controls/native/... > ui/views/controls/native/native_view_host_aura.cc:112: > clipping_window_.layer()->SetBounds(orig_bounds_); > On 2013/10/09 17:42:20, rharrison wrote: > > On 2013/10/02 20:49:00, sky wrote: > > > ShowWidget is invoked right after this. Can you wait to set the bounds until > > > then? > > > > I think you meant InstallClip maybe? I want to avoid having the UninstallClip > > method depend on a following call to InstallClip or other manipulations. > > Basically UninstallClip should undo everything InstallClip does. I think the > > UpdateClippingWindow from your version of the CL cleans this up nicely. > > Isn't there already this dependency though? NativeViewHost implementations can > assume a particular call order, otherwise things are going to break. ok, I have removed the code adjusting the bounds here and added a comment to native_view_host_wrapper.h making this dependency explicit. This will be in the next CL I upload. I think my concern was more
PTAL Disregard "I think my concern was more" from my previous message, I apparently was typing in the wrong window :-/ https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:102: UninstallClip(); On 2013/10/09 18:31:47, xiyuan wrote: > This should go inside host_->fast_resize() because in non-fast-resize path, > ShowWidget is called after InstallClip and this would incorrectly clear the > clip. Done. https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:104: gfx::Rect input_bounds(x, y, w, h); On 2013/10/09 18:31:47, xiyuan wrote: > Where is this used? Was part of some debug logging I had, thanks for catching this. https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:105: InstallClip(x - clipping_window_.bounds().origin().x(), On 2013/10/09 18:31:47, xiyuan wrote: > Should we use orig_bounds_ to convert |x|, |y| into host_'s local coordinates? Done. https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:165: orig_bounds_ = bounds; On 2013/10/09 18:31:47, xiyuan wrote: > Which coordinates is |orig_bounds_| using? This looks not right. orig_bounds_ is in the coordinate space of host_->GetWidget(), which is going to become the parent ... oh I see the problem now... yeah that is wrong, it should be with the original origin, not (0,0). https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/79001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:84: gfx::Rect clip_rect_; On 2013/10/09 18:31:47, xiyuan wrote: > It would be good to document this and |orig_bounds_| are in which coordinate > system. Are they in the host_->GetWidget() coordinates or in the host_'s local > coordinates. Done.
Mostly good. https://codereview.chromium.org/24299004/diff/88001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/88001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:108: orig_bounds_ = gfx::Rect(x, y, w, h); Since clip_rect_ is in host_->GetWidget() coordinates, we need to move it here as well. Otherwise, we are clipping at wrong position. Think we need to do clip_rect_.Offset(x - orig_bounds_.x(), y - orig_bounds_.y()); before we change |orig_bounds_| Another way would be changing |clip_rect_| to use local coordinates relative to orig_bounds_. https://codereview.chromium.org/24299004/diff/88001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/88001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:67: // relative it. nit: relative to it.
https://codereview.chromium.org/24299004/diff/88001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/88001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.cc:108: orig_bounds_ = gfx::Rect(x, y, w, h); On 2013/10/10 17:26:25, xiyuan wrote: > Since clip_rect_ is in host_->GetWidget() coordinates, we need to move it here > as well. Otherwise, we are clipping at wrong position. > > Think we need to do > clip_rect_.Offset(x - orig_bounds_.x(), y - orig_bounds_.y()); > before we change |orig_bounds_| > > Another way would be changing |clip_rect_| to use local coordinates relative to > orig_bounds_. Done. https://codereview.chromium.org/24299004/diff/88001/ui/views/controls/native/... File ui/views/controls/native/native_view_host_aura.h (right): https://codereview.chromium.org/24299004/diff/88001/ui/views/controls/native/... ui/views/controls/native/native_view_host_aura.h:67: // relative it. On 2013/10/10 17:26:25, xiyuan wrote: > nit: relative to it. Done.
Cool. LGTM Thank you for implementing the feature. https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:25: clipping_window_.layer()->SetMasksToBounds(false); nit: 2-space indent https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:160: host_->GetWidget()->GetNativeView()); nit: wrap with {} since it's more than one line. https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... File ui/views/controls/native/native_view_host_unittest.cc (right): https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... ui/views/controls/native/native_view_host_unittest.cc:12: #include "base/debug/stack_trace.h" nit: seems not used
https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:25: clipping_window_.layer()->SetMasksToBounds(false); On 2013/10/10 18:00:26, xiyuan wrote: > nit: 2-space indent Done. https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:160: host_->GetWidget()->GetNativeView()); On 2013/10/10 18:00:26, xiyuan wrote: > nit: wrap with {} since it's more than one line. Done. https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... File ui/views/controls/native/native_view_host_unittest.cc (right): https://codereview.chromium.org/24299004/diff/119001/ui/views/controls/native... ui/views/controls/native/native_view_host_unittest.cc:12: #include "base/debug/stack_trace.h" On 2013/10/10 18:00:26, xiyuan wrote: > nit: seems not used Done.
https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:45: void NativeViewHostAura::NativeViewDetaching(bool destroyed) { The assumption is that if destroyed is true, you shouldn't need to do anything to native_view->host() (it's been destroyed). Did you verify this properly cleans up in that case? In that case seems like you should remove the clipping window, but not touch native_view->host() https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:142: gfx::Point NativeViewHostAura::CalculateNativeViewOrigin( Seems like this should be promoted to NativeView. https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:166: bounds.set_origin(gfx::Point(0,0)); nit: 0, 0 https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:19: class DestroyedCount { Why have this in a separate object rather than part of NativeViewHostTesting? https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:161: EXPECT_EQ(gfx::Point(0, 0), Comparing strinsg (ToString()) on all points (and rects) results in much better error strings on failure. https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... File ui/views/controls/native/native_view_host_wrapper.h (right): https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_wrapper.h:50: // following call to ShowWedget should occur after calling this method to ShowWidget
https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:45: void NativeViewHostAura::NativeViewDetaching(bool destroyed) { On 2013/10/10 20:25:32, sky wrote: > The assumption is that if destroyed is true, you shouldn't need to do anything > to native_view->host() (it's been destroyed). Did you verify this properly > cleans up in that case? In that case seems like you should remove the clipping > window, but not touch native_view->host() Done. https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:142: gfx::Point NativeViewHostAura::CalculateNativeViewOrigin( On 2013/10/10 20:25:32, sky wrote: > Seems like this should be promoted to NativeView. gfx::NativeView is just a typedef wrapper aroung aura::Window* on Aura. Should I move this method to aura::Window or leave it here? https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:166: bounds.set_origin(gfx::Point(0,0)); On 2013/10/10 20:25:32, sky wrote: > nit: 0, 0 Done. https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:19: class DestroyedCount { On 2013/10/10 20:25:32, sky wrote: > Why have this in a separate object rather than part of NativeViewHostTesting? No good reason, Done. https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:161: EXPECT_EQ(gfx::Point(0, 0), On 2013/10/10 20:25:32, sky wrote: > Comparing strinsg (ToString()) on all points (and rects) results in much better > error strings on failure. Done. https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... File ui/views/controls/native/native_view_host_wrapper.h (right): https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_wrapper.h:50: // following call to ShowWedget should occur after calling this method to On 2013/10/10 20:25:32, sky wrote: > ShowWidget Done.
LGTM https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/24299004/diff/126001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:142: gfx::Point NativeViewHostAura::CalculateNativeViewOrigin( On 2013/10/13 13:54:57, rharrison wrote: > On 2013/10/10 20:25:32, sky wrote: > > Seems like this should be promoted to NativeView. > > gfx::NativeView is just a typedef wrapper aroung aura::Window* on Aura. Should I > move this method to aura::Window or leave it here? Sorry, I meant NativeViewHost. But go ahead and leave it here. NativeViewHostAura and NativeViewHost can be merged when we drop classic windows.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/24299004/132001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
On 2013/10/15 18:10:34, I haz the power (commit-bot) wrote: > Retried try job too often on win7_aura for step(s) browser_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&... I don't think this failure is me. I would have expected to see a crash/out right failure instead of a hang. Going to try again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/24299004/132001
Message was sent while issue was closed.
Change committed as 228751 |
