Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
317823002:
Implement NativeViewHostAura::InstallClip. (Closed)
|
Created:
6 years, 6 months ago by calamity Modified:
6 years, 6 months ago Reviewers:
sky CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement NativeViewHostAura::InstallClip.
Despite the NOTIMPLEMENTED comment saying that this method is only
needed for performance, it causes bugs with aura windows in side
Bubbles which need to clip to their parent view rather than the parent
window.
This CL is based on https://codereview.chromium.org/30993004/ but strips
out the gravity code and simplifies the windowing code.
Since the window hierarchy of NPAPI plugins has changed due to this
patch, it was also necessary to modify WebContentsViewAura to reflect
this change.
BUG=377378
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279051
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279316
Patch Set 1 : #
Total comments: 20
Patch Set 2 : fix tests, address comments #Patch Set 3 : avoid a window reparent on destruction #Patch Set 4 : fix test crashes #
Total comments: 4
Patch Set 5 : address comments #Patch Set 6 : fix crashes and test failures #Patch Set 7 : fix last two tests #Patch Set 8 : rebase, reimplement fast resize #Patch Set 9 : add more tests #
Total comments: 5
Patch Set 10 : address a comment #Patch Set 11 : add kHostWindowKey property #Patch Set 12 : remove stray whitespace #
Total comments: 5
Patch Set 13 : address comments #
Total comments: 1
Patch Set 14 : remove test #Patch Set 15 : rebase #Patch Set 16 : clean up better #Messages
Total messages: 29 (0 generated)
+sky for ui/views OWNERS and reviewer of previous iteration. The reason for revert (crbug.com/311895) has been resolved.
https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:18: : host_(host), installed_clip_(false), clipping_window_(NULL) { nit: one param per line. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:29: RemoveClippingWindow(); Are you sure this is necessary here? In particular I'm questioning whether you need to reparent the nativeview. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:51: clipping_window_.Hide(); Why is this in an else? Shouldn't you always do it? https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:72: RemoveClippingWindow(); Seems weird to reparent the native_view to the widget and then remove it in the next line. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:81: clipping_window_.layer()->SetMasksToBounds(true); Can't you always set mask to bounds true? https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:31: }; DISALLOW_... https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_unittest.cc (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_unittest.cc:139: // One notification is generated from inserting the clipping window into the I think we can avoid this if you don't call Widget::ReparentView for the clipping_window. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_wrapper.h (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_wrapper.h:42: // space of the gfx::NativeView, so if this method is called from ShowWidget gfx::NativeView is overloaded here. I think you mean Widget.
Also fixed the tests. The clipping window needs to be sized properly to correctly bubble events. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:18: : host_(host), installed_clip_(false), clipping_window_(NULL) { On 2014/06/05 15:49:01, sky wrote: > nit: one param per line. Done. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:29: RemoveClippingWindow(); On 2014/06/05 15:49:01, sky wrote: > Are you sure this is necessary here? In particular I'm questioning whether you > need to reparent the nativeview. Removing this crashes NativeViewHostAuraTest.HostViewPropertyKey... https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:51: clipping_window_.Hide(); On 2014/06/05 15:49:01, sky wrote: > Why is this in an else? Shouldn't you always do it? RemoveClippingView actually takes care of this. Fixed. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:72: RemoveClippingWindow(); On 2014/06/05 15:49:01, sky wrote: > Seems weird to reparent the native_view to the widget and then remove it in the > next line. Switched the order. RemoveClippingWindow() won't reparent after the native view is removed from its parent. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:81: clipping_window_.layer()->SetMasksToBounds(true); On 2014/06/05 15:49:01, sky wrote: > Can't you always set mask to bounds true? It's set to false so that the native view can draw anywhere within the widget when a clip isn't installed. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:31: }; On 2014/06/05 15:49:01, sky wrote: > DISALLOW_... Done. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_unittest.cc (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_unittest.cc:139: // One notification is generated from inserting the clipping window into the On 2014/06/05 15:49:01, sky wrote: > I think we can avoid this if you don't call Widget::ReparentView for the > clipping_window. I made this change and it only works for some of these blocks. This one in particular gets a notification from the native view getting reparented into the clipping window. Removed what I could. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_wrapper.h (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_wrapper.h:42: // space of the gfx::NativeView, so if this method is called from ShowWidget On 2014/06/05 15:49:01, sky wrote: > gfx::NativeView is overloaded here. I think you mean Widget. Done.
https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:29: RemoveClippingWindow(); On 2014/06/06 08:13:44, calamity wrote: > On 2014/06/05 15:49:01, sky wrote: > > Are you sure this is necessary here? In particular I'm questioning whether you > > need to reparent the nativeview. > > Removing this crashes NativeViewHostAuraTest.HostViewPropertyKey... What does the trace look like? https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:81: clipping_window_.layer()->SetMasksToBounds(true); On 2014/06/06 08:13:44, calamity wrote: > On 2014/06/05 15:49:01, sky wrote: > > Can't you always set mask to bounds true? > > It's set to false so that the native view can draw anywhere within the widget > when a clip isn't installed. Isn't the only time the size of the clipping_window and native_view differ when InstallClip is invoked? That's why I think you can always set this mask to bounds to true.
I had to add a NULL check in NativeWidgetAura to stop the print preview browser tests from crashing... I'm not entirely sure what's happening there. It looks the crash happens on browser shutdown as the windows get destroyed.. The crash doesn't happen when doing similar things in an actual chrome instance (opening the print preview dialog and then closing chrome). https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:29: RemoveClippingWindow(); On 2014/06/06 15:53:57, sky wrote: > On 2014/06/06 08:13:44, calamity wrote: > > On 2014/06/05 15:49:01, sky wrote: > > > Are you sure this is necessary here? In particular I'm questioning whether > you > > > need to reparent the nativeview. > > > > Removing this crashes NativeViewHostAuraTest.HostViewPropertyKey... > > What does the trace look like? Evidently we need to remove the native view from the clipping window at least or else destroying the clipping window destroys the native view. Removed this line in patchset #3 and replaced it with code that doesn't cause a reparent. https://codereview.chromium.org/317823002/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:81: clipping_window_.layer()->SetMasksToBounds(true); On 2014/06/06 15:53:57, sky wrote: > On 2014/06/06 08:13:44, calamity wrote: > > On 2014/06/05 15:49:01, sky wrote: > > > Can't you always set mask to bounds true? > > > > It's set to false so that the native view can draw anywhere within the widget > > when a clip isn't installed. > > Isn't the only time the size of the clipping_window and native_view differ when > InstallClip is invoked? That's why I think you can always set this mask to > bounds to true. Oops, this comment was outdated. Yeah. Since I set the bounds correctly in patch #2, this is now always SetMasksToBounds(true).
FYI Traces. Without this patch, it runs successfully and hits via: views.dll!views::NativeWidgetAura::OnWindowFocused(aura::Window * gained_focus, aura::Window * lost_focus) Line 925 wm.dll!wm::FocusController::SetFocusedWindow(aura::Window * window) Line 266 + 0x19 bytes wm.dll!wm::FocusController::WindowLostFocusFromDispositionChange(aura::Window * window, aura::Window * next) Line 358 wm.dll!wm::FocusController::OnWindowDestroying(aura::Window * window) Line 201 aura.dll!aura::Window::~Window() Line 226 + 0x4f bytes views.dll!aura::Window::`scalar deleting destructor'() + 0x1a bytes views.dll!views::NativeWidgetAura::CloseNow() Line 466 + 0x26 bytes With the patch, it crashes with: views.dll!views::NativeWidgetAura::OnWindowFocused(aura::Window * gained_focus, aura::Window * lost_focus) Line 925 wm.dll!wm::FocusController::SetFocusedWindow(aura::Window * window) Line 266 + 0x19 bytes wm.dll!wm::FocusController::WindowLostFocusFromDispositionChange(aura::Window * window, aura::Window * next) Line 358 wm.dll!wm::FocusController::OnWindowHierarchyChanged(const aura::WindowObserver::HierarchyChangeParams & params) Line 221 aura.dll!aura::Window::NotifyWindowHierarchyChangeAtReceiver(const aura::WindowObserver::HierarchyChangeParams & params) Line 1248 + 0x51 bytes aura.dll!aura::Window::NotifyWindowHierarchyChangeDown(const aura::WindowObserver::HierarchyChangeParams & params) Line 1224 aura.dll!aura::Window::NotifyWindowHierarchyChange(const aura::WindowObserver::HierarchyChangeParams & params) Line 1206 aura.dll!aura::Window::RemoveChild(aura::Window * child) Line 563 aura.dll!aura::Window::~Window() Line 254 views.dll!aura::Window::`scalar deleting destructor'() + 0x1a bytes views.dll!views::NativeWidgetAura::CloseNow() Line 466 + 0x26 bytes
https://codereview.chromium.org/317823002/diff/100001/ui/views/controls/nativ... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/317823002/diff/100001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_aura.cc:78: installed_clip_ = true; Is it possible to nuke installed_clip_ and use clip_rect_? If necessary, make clip_rect_ a scoped_ptr. https://codereview.chromium.org/317823002/diff/100001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_aura.cc:92: InstallClip(x, y, w, h); Doesn't InstallClip expect coordinates in the views coordinate space but x/y/w/h is in the coordinates of the widget?
Had to change WebContentsViewAura due to its assumption that the web contents window is a direct child of its host. Also set kHostViewKey on the clipping window to have it get reordered to the right z-order. https://codereview.chromium.org/317823002/diff/100001/ui/views/controls/nativ... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/317823002/diff/100001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_aura.cc:78: installed_clip_ = true; On 2014/06/10 16:11:06, sky wrote: > Is it possible to nuke installed_clip_ and use clip_rect_? If necessary, make > clip_rect_ a scoped_ptr. Done. https://codereview.chromium.org/317823002/diff/100001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_aura.cc:92: InstallClip(x, y, w, h); On 2014/06/10 16:11:06, sky wrote: > Doesn't InstallClip expect coordinates in the views coordinate space but x/y/w/h > is in the coordinates of the widget? Hmm. Yeah. I think that this is actually completely wrong at the moment since it uses w and h below which I believe is against the whole point of fast resize. I'll just leave this as a TODO as this patch is getting a bit too unwieldy.
On Wed, Jun 11, 2014 at 2:50 AM, <calamity@chromium.org> wrote: > Had to change WebContentsViewAura due to its assumption that the web > contents > window is a direct child of its host. > > Also set kHostViewKey on the clipping window to have it get reordered to > the > right z-order. > > > > https://codereview.chromium.org/317823002/diff/100001/ui/ > views/controls/native/native_view_host_aura.cc > File ui/views/controls/native/native_view_host_aura.cc (right): > > https://codereview.chromium.org/317823002/diff/100001/ui/ > views/controls/native/native_view_host_aura.cc#newcode78 > ui/views/controls/native/native_view_host_aura.cc:78: installed_clip_ = > true; > On 2014/06/10 16:11:06, sky wrote: > >> Is it possible to nuke installed_clip_ and use clip_rect_? If >> > necessary, make > >> clip_rect_ a scoped_ptr. >> > > Done. > > > https://codereview.chromium.org/317823002/diff/100001/ui/ > views/controls/native/native_view_host_aura.cc#newcode92 > ui/views/controls/native/native_view_host_aura.cc:92: InstallClip(x, y, > w, h); > On 2014/06/10 16:11:06, sky wrote: > >> Doesn't InstallClip expect coordinates in the views coordinate space >> > but x/y/w/h > >> is in the coordinates of the widget? >> > > Hmm. Yeah. I think that this is actually completely wrong at the moment > since it uses w and h below which I believe is against the whole point > of fast resize. I'll just leave this as a TODO as this patch is getting > a bit too unwieldy. > My worry is that you've partially implemented things and without completely fixing it we may get things wrong. So, either fix it now or make InstallClip do nothing, which should disable your code paths. > > https://codereview.chromium.org/317823002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/06/11 16:22:55, sky wrote: > On Wed, Jun 11, 2014 at 2:50 AM, <mailto:calamity@chromium.org> wrote: > > > Had to change WebContentsViewAura due to its assumption that the web > > contents > > window is a direct child of its host. > > > > Also set kHostViewKey on the clipping window to have it get reordered to > > the > > right z-order. > > > > > > > > https://codereview.chromium.org/317823002/diff/100001/ui/ > > views/controls/native/native_view_host_aura.cc > > File ui/views/controls/native/native_view_host_aura.cc (right): > > > > https://codereview.chromium.org/317823002/diff/100001/ui/ > > views/controls/native/native_view_host_aura.cc#newcode78 > > ui/views/controls/native/native_view_host_aura.cc:78: installed_clip_ = > > true; > > On 2014/06/10 16:11:06, sky wrote: > > > >> Is it possible to nuke installed_clip_ and use clip_rect_? If > >> > > necessary, make > > > >> clip_rect_ a scoped_ptr. > >> > > > > Done. > > > > > > https://codereview.chromium.org/317823002/diff/100001/ui/ > > views/controls/native/native_view_host_aura.cc#newcode92 > > ui/views/controls/native/native_view_host_aura.cc:92: InstallClip(x, y, > > w, h); > > On 2014/06/10 16:11:06, sky wrote: > > > >> Doesn't InstallClip expect coordinates in the views coordinate space > >> > > but x/y/w/h > > > >> is in the coordinates of the widget? > >> > > > > Hmm. Yeah. I think that this is actually completely wrong at the moment > > since it uses w and h below which I believe is against the whole point > > of fast resize. I'll just leave this as a TODO as this patch is getting > > a bit too unwieldy. > > > > My worry is that you've partially implemented things and without completely > fixing it we may get things wrong. So, either fix it now or make > InstallClip do nothing, which should disable your code paths. > > > > > https://codereview.chromium.org/317823002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Isn't fast resize orthogonal to InstallClip stuff? Making InstallClip do nothing unfixes the bug that this patch is trying to fix.
Wed, Jun 11, 2014 at 4:29 PM, <calamity@chromium.org> wrote: > On 2014/06/11 16:22:55, sky wrote: > > On Wed, Jun 11, 2014 at 2:50 AM, <mailto:calamity@chromium.org> wrote: >> > > > Had to change WebContentsViewAura due to its assumption that the web >> > contents >> > window is a direct child of its host. >> > >> > Also set kHostViewKey on the clipping window to have it get reordered to >> > the >> > right z-order. >> > >> > >> > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ >> > views/controls/native/native_view_host_aura.cc >> > File ui/views/controls/native/native_view_host_aura.cc (right): >> > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ >> > views/controls/native/native_view_host_aura.cc#newcode78 >> > ui/views/controls/native/native_view_host_aura.cc:78: installed_clip_ = >> > true; >> > On 2014/06/10 16:11:06, sky wrote: >> > >> >> Is it possible to nuke installed_clip_ and use clip_rect_? If >> >> >> > necessary, make >> > >> >> clip_rect_ a scoped_ptr. >> >> >> > >> > Done. >> > >> > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ >> > views/controls/native/native_view_host_aura.cc#newcode92 >> > ui/views/controls/native/native_view_host_aura.cc:92: InstallClip(x, y, >> > w, h); >> > On 2014/06/10 16:11:06, sky wrote: >> > >> >> Doesn't InstallClip expect coordinates in the views coordinate space >> >> >> > but x/y/w/h >> > >> >> is in the coordinates of the widget? >> >> >> > >> > Hmm. Yeah. I think that this is actually completely wrong at the moment >> > since it uses w and h below which I believe is against the whole point >> > of fast resize. I'll just leave this as a TODO as this patch is getting >> > a bit too unwieldy. >> > >> > > My worry is that you've partially implemented things and without >> completely >> fixing it we may get things wrong. So, either fix it now or make >> InstallClip do nothing, which should disable your code paths. >> > > > >> > https://codereview.chromium.org/317823002/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > Isn't fast resize orthogonal to InstallClip stuff? Making InstallClip do > nothing > unfixes the > bug that this patch is trying to fix. > I couldn't entirely convince myself of that after looking at the code. The main use case of fast resize is showing/hiding the bookmark bar. Is that working ok after this patch? -Scott > > https://codereview.chromium.org/317823002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/06/12 15:38:05, sky wrote: > Wed, Jun 11, 2014 at 4:29 PM, <mailto:calamity@chromium.org> wrote: > > > On 2014/06/11 16:22:55, sky wrote: > > > > On Wed, Jun 11, 2014 at 2:50 AM, <mailto:calamity@chromium.org> wrote: > >> > > > > > Had to change WebContentsViewAura due to its assumption that the web > >> > contents > >> > window is a direct child of its host. > >> > > >> > Also set kHostViewKey on the clipping window to have it get reordered to > >> > the > >> > right z-order. > >> > > >> > > >> > > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ > >> > views/controls/native/native_view_host_aura.cc > >> > File ui/views/controls/native/native_view_host_aura.cc (right): > >> > > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ > >> > views/controls/native/native_view_host_aura.cc#newcode78 > >> > ui/views/controls/native/native_view_host_aura.cc:78: installed_clip_ = > >> > true; > >> > On 2014/06/10 16:11:06, sky wrote: > >> > > >> >> Is it possible to nuke installed_clip_ and use clip_rect_? If > >> >> > >> > necessary, make > >> > > >> >> clip_rect_ a scoped_ptr. > >> >> > >> > > >> > Done. > >> > > >> > > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ > >> > views/controls/native/native_view_host_aura.cc#newcode92 > >> > ui/views/controls/native/native_view_host_aura.cc:92: InstallClip(x, y, > >> > w, h); > >> > On 2014/06/10 16:11:06, sky wrote: > >> > > >> >> Doesn't InstallClip expect coordinates in the views coordinate space > >> >> > >> > but x/y/w/h > >> > > >> >> is in the coordinates of the widget? > >> >> > >> > > >> > Hmm. Yeah. I think that this is actually completely wrong at the moment > >> > since it uses w and h below which I believe is against the whole point > >> > of fast resize. I'll just leave this as a TODO as this patch is getting > >> > a bit too unwieldy. > >> > > >> > > > > My worry is that you've partially implemented things and without > >> completely > >> fixing it we may get things wrong. So, either fix it now or make > >> InstallClip do nothing, which should disable your code paths. > >> > > > > > > >> > https://codereview.chromium.org/317823002/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > Isn't fast resize orthogonal to InstallClip stuff? Making InstallClip do > > nothing > > unfixes the > > bug that this patch is trying to fix. > > > > I couldn't entirely convince myself of that after looking at the code. The > main use case of fast resize is showing/hiding the bookmark bar. Is that > working ok after this patch? > > -Scott > > > > > > https://codereview.chromium.org/317823002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Yeah. I checked the old patch and the fast resize was definitely wrong (the webcontents would flash white during the bookmark bar animation). Removing the fast resize support behaves the same as without this whole patch (the webcontents resizes on every animation frame). I added another implementation of fast resize. It's up to you if you want it here, as a separate patch or just hold off on it completely. The behavior with this fast resize patchset is that the bottom of the webcontents flashes once at the end of the bookmark bar hide animation whereas without it, the webcontents animates during the whole bar animation.
On 2014/06/13 06:34:16, calamity wrote: > On 2014/06/12 15:38:05, sky wrote: > > Wed, Jun 11, 2014 at 4:29 PM, <mailto:calamity@chromium.org> wrote: > > > > > On 2014/06/11 16:22:55, sky wrote: > > > > > > On Wed, Jun 11, 2014 at 2:50 AM, <mailto:calamity@chromium.org> wrote: > > >> > > > > > > > Had to change WebContentsViewAura due to its assumption that the web > > >> > contents > > >> > window is a direct child of its host. > > >> > > > >> > Also set kHostViewKey on the clipping window to have it get reordered to > > >> > the > > >> > right z-order. > > >> > > > >> > > > >> > > > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ > > >> > views/controls/native/native_view_host_aura.cc > > >> > File ui/views/controls/native/native_view_host_aura.cc (right): > > >> > > > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ > > >> > views/controls/native/native_view_host_aura.cc#newcode78 > > >> > ui/views/controls/native/native_view_host_aura.cc:78: installed_clip_ = > > >> > true; > > >> > On 2014/06/10 16:11:06, sky wrote: > > >> > > > >> >> Is it possible to nuke installed_clip_ and use clip_rect_? If > > >> >> > > >> > necessary, make > > >> > > > >> >> clip_rect_ a scoped_ptr. > > >> >> > > >> > > > >> > Done. > > >> > > > >> > > > >> > https://codereview.chromium.org/317823002/diff/100001/ui/ > > >> > views/controls/native/native_view_host_aura.cc#newcode92 > > >> > ui/views/controls/native/native_view_host_aura.cc:92: InstallClip(x, y, > > >> > w, h); > > >> > On 2014/06/10 16:11:06, sky wrote: > > >> > > > >> >> Doesn't InstallClip expect coordinates in the views coordinate space > > >> >> > > >> > but x/y/w/h > > >> > > > >> >> is in the coordinates of the widget? > > >> >> > > >> > > > >> > Hmm. Yeah. I think that this is actually completely wrong at the moment > > >> > since it uses w and h below which I believe is against the whole point > > >> > of fast resize. I'll just leave this as a TODO as this patch is getting > > >> > a bit too unwieldy. > > >> > > > >> > > > > > > My worry is that you've partially implemented things and without > > >> completely > > >> fixing it we may get things wrong. So, either fix it now or make > > >> InstallClip do nothing, which should disable your code paths. > > >> > > > > > > > > > >> > https://codereview.chromium.org/317823002/ > > >> > > > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > >> > > > email > > > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > >> > > > > > > Isn't fast resize orthogonal to InstallClip stuff? Making InstallClip do > > > nothing > > > unfixes the > > > bug that this patch is trying to fix. > > > > > > > I couldn't entirely convince myself of that after looking at the code. The > > main use case of fast resize is showing/hiding the bookmark bar. Is that > > working ok after this patch? > > > > -Scott > > > > > > > > > > https://codereview.chromium.org/317823002/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Yeah. I checked the old patch and the fast resize was definitely wrong (the > webcontents would flash white during the bookmark bar animation). Removing the > fast resize support behaves the same as without this whole patch (the > webcontents resizes on every animation frame). > > I added another implementation of fast resize. It's up to you if you want it > here, as a separate patch or just hold off on it completely. The behavior with > this fast resize patchset is that the bottom of the webcontents flashes once at > the end of the bookmark bar hide animation whereas without it, the webcontents > animates during the whole bar animation. Also added a couple of tests to clearly illustrate the fast resize and clipping behavior.
https://codereview.chromium.org/317823002/diff/300001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/317823002/diff/300001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:564: if (parent) That a different parent exists is an implementation detail of NativeViewHost. If that changes this code will break and we may now know. We need a better way to be insulated from changes in NativeViewHost. Maybe a method on NativeViewHost? https://codereview.chromium.org/317823002/diff/300001/ui/views/controls/nativ... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/317823002/diff/300001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_aura.cc:143: clipping_window_.SetProperty(views::kHostViewKey, Can't you set this in the constructor once and not set/clear each time?
https://codereview.chromium.org/317823002/diff/300001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/317823002/diff/300001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:564: if (parent) On 2014/06/16 17:11:03, sky wrote: > That a different parent exists is an implementation detail of NativeViewHost. If > that changes this code will break and we may now know. We need a better way to > be insulated from changes in NativeViewHost. Maybe a method on NativeViewHost? Since this operates at the window level, I don't think there's a way to ask the relevant NativeViewHost about this. Maybe a static method that checks if the window name matches the clipping window's name? Another option is to static_cast the NativeViewHost out of the window's kHostViewKey and use host->GetWidget()->GetNativeView(). https://codereview.chromium.org/317823002/diff/300001/ui/views/controls/nativ... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/317823002/diff/300001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_aura.cc:143: clipping_window_.SetProperty(views::kHostViewKey, On 2014/06/16 17:11:03, sky wrote: > Can't you set this in the constructor once and not set/clear each time? Done.
https://codereview.chromium.org/317823002/diff/300001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/317823002/diff/300001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:564: if (parent) On 2014/06/17 05:01:12, calamity wrote: > On 2014/06/16 17:11:03, sky wrote: > > That a different parent exists is an implementation detail of NativeViewHost. > If > > that changes this code will break and we may now know. We need a better way to > > be insulated from changes in NativeViewHost. Maybe a method on NativeViewHost? > > Since this operates at the window level, I don't think there's a way to ask the > relevant NativeViewHost about this. Maybe a static method that checks if the > window name matches the clipping window's name? > > Another option is to static_cast the NativeViewHost out of the window's > kHostViewKey and use host->GetWidget()->GetNativeView(). Neither of these options work since this code shouldn't know about views. Also, since this code isn't necessarily used with views assuming views is being used is equally wrong. Also, isn't the code in UpdateConstrainedWindow going to incorrectly consider the clipping window? I think we're going to need some way to tag (similar to kHostViewKey) which parent is used but is exposed in a place this code can access.
On 2014/06/17 16:25:25, sky wrote: > https://codereview.chromium.org/317823002/diff/300001/content/browser/web_con... > File content/browser/web_contents/web_contents_view_aura.cc (right): > > https://codereview.chromium.org/317823002/diff/300001/content/browser/web_con... > content/browser/web_contents/web_contents_view_aura.cc:564: if (parent) > On 2014/06/17 05:01:12, calamity wrote: > > On 2014/06/16 17:11:03, sky wrote: > > > That a different parent exists is an implementation detail of > NativeViewHost. > > If > > > that changes this code will break and we may now know. We need a better way > to > > > be insulated from changes in NativeViewHost. Maybe a method on > NativeViewHost? > > > > Since this operates at the window level, I don't think there's a way to ask > the > > relevant NativeViewHost about this. Maybe a static method that checks if the > > window name matches the clipping window's name? > > > > Another option is to static_cast the NativeViewHost out of the window's > > kHostViewKey and use host->GetWidget()->GetNativeView(). > > Neither of these options work since this code shouldn't know about views. Also, > since this code isn't necessarily used with views assuming views is being used > is equally wrong. Also, isn't the code in UpdateConstrainedWindow going to > incorrectly consider the clipping window? Can you elaborate on the UpdateConstrainedWindow issue? The current patch uses host_window_->Contains() vs window == view_->window() in order to account for the clipping window. > I think we're going to need some way to tag (similar to kHostViewKey) which > parent is used but is exposed in a place this code can access. Okay. Added a kHostWindowKey to aura_constants.h. Let me know if there's somewhere better to put it. https://codereview.chromium.org/317823002/diff/380001/ui/views/controls/nativ... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/317823002/diff/380001/ui/views/controls/nativ... ui/views/controls/native/native_view_host_aura_unittest.cc:123: EXPECT_TRUE(clipping_window()->GetProperty(views::kHostViewKey)); This change is necessary due to the clipping window getting its kHostViewKey property set at construction time.
On Wed, Jun 18, 2014 at 5:14 PM, <calamity@chromium.org> wrote: > On 2014/06/17 16:25:25, sky wrote: > > https://codereview.chromium.org/317823002/diff/300001/ > content/browser/web_contents/web_contents_view_aura.cc > >> File content/browser/web_contents/web_contents_view_aura.cc (right): >> > > > https://codereview.chromium.org/317823002/diff/300001/ > content/browser/web_contents/web_contents_view_aura.cc#newcode564 > >> content/browser/web_contents/web_contents_view_aura.cc:564: if (parent) >> On 2014/06/17 05:01:12, calamity wrote: >> > On 2014/06/16 17:11:03, sky wrote: >> > > That a different parent exists is an implementation detail of >> NativeViewHost. >> > If >> > > that changes this code will break and we may now know. We need a >> better >> > way > >> to >> > > be insulated from changes in NativeViewHost. Maybe a method on >> NativeViewHost? >> > >> > Since this operates at the window level, I don't think there's a way to >> ask >> the >> > relevant NativeViewHost about this. Maybe a static method that checks >> if the >> > window name matches the clipping window's name? >> > >> > Another option is to static_cast the NativeViewHost out of the window's >> > kHostViewKey and use host->GetWidget()->GetNativeView(). >> > > Neither of these options work since this code shouldn't know about views. >> > Also, > >> since this code isn't necessarily used with views assuming views is being >> used >> is equally wrong. Also, isn't the code in UpdateConstrainedWindow going to >> incorrectly consider the clipping window? >> > > Can you elaborate on the UpdateConstrainedWindow issue? The current patch > uses > host_window_->Contains() vs window == view_->window() in order to account > for > the clipping window. Ah, I missed that. My mistake. > > > I think we're going to need some way to tag (similar to kHostViewKey) >> which >> parent is used but is exposed in a place this code can access. >> > > Okay. Added a kHostWindowKey to aura_constants.h. Let me know if there's > somewhere better to put it. > > > https://codereview.chromium.org/317823002/diff/380001/ui/ > views/controls/native/native_view_host_aura_unittest.cc > File ui/views/controls/native/native_view_host_aura_unittest.cc (right): > > https://codereview.chromium.org/317823002/diff/380001/ui/ > views/controls/native/native_view_host_aura_unittest.cc#newcode123 > ui/views/controls/native/native_view_host_aura_unittest.cc:123: > EXPECT_TRUE(clipping_window()->GetProperty(views::kHostViewKey)); > This change is necessary due to the clipping window getting its > kHostViewKey property set at construction time. > > https://codereview.chromium.org/317823002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Can you write tests for the WebContentsViewAura side? https://codereview.chromium.org/317823002/diff/380001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/317823002/diff/380001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:562: window->GetProperty(aura::client::kHostWindowKey); It's not far to assume the only place using this code is setting kHostWindowKey. In other words if kHostWindowKey is not set you should fallback to what the code was doing before. https://codereview.chromium.org/317823002/diff/380001/ui/aura/client/aura_con... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/317823002/diff/380001/ui/aura/client/aura_con... ui/aura/client/aura_constants.h:41: // A property key to store the host window of a window. This description is too generic, be more specific.
I added a test that checks the property is set on a WebContentsViewAura. I'm not really sure how to test the WebContentsViewAura side much further. The changes made only really affect NPAPI plugins which are tested by the PrintPreviewTest, FindInPageTest and ChromePluginTest that I broke when doing the original clipping change. https://codereview.chromium.org/317823002/diff/380001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/317823002/diff/380001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:562: window->GetProperty(aura::client::kHostWindowKey); On 2014/06/19 15:15:01, sky wrote: > It's not far to assume the only place using this code is setting kHostWindowKey. > In other words if kHostWindowKey is not set you should fallback to what the code > was doing before. Done. https://codereview.chromium.org/317823002/diff/380001/ui/aura/client/aura_con... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/317823002/diff/380001/ui/aura/client/aura_con... ui/aura/client/aura_constants.h:41: // A property key to store the host window of a window. On 2014/06/19 15:15:01, sky wrote: > This description is too generic, be more specific. Updated.
LGTM https://codereview.chromium.org/317823002/diff/400001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right): https://codereview.chromium.org/317823002/diff/400001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura_browsertest.cc:691: // Verify that hiding a parent of the renderer will hide the content too. I don't think this test provides much value, go ahead and nuke it. To really test the code you're changing requires it to be refactored into a standalone test. If we have coverage through other tests, no need to write another.
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/317823002/440001
Message was sent while issue was closed.
Change committed as 279051
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/339663003/ by jackhou@chromium.org. The reason for reverting is: Breaking ASan LSan: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/317823002/460001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 279316 |
