Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
397293005:
Fix positioning of children of NativeViewHostAura (Closed)
|
Created:
6 years, 5 months ago by Evan Stade Modified:
6 years, 5 months ago CC:
chromium-reviews, benquan, tfarina, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix positioning of children of NativeViewHostAura
regressed in r279316
BUG=391316
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285168
Patch Set 1 #Patch Set 2 : . #
Total comments: 2
Patch Set 3 : fix code, add test #
Total comments: 5
Patch Set 4 : another test #
Total comments: 3
Patch Set 5 : sky comes up with a one line fix #
Total comments: 4
Patch Set 6 : delete test #
Total comments: 4
Patch Set 7 : add comment, revert errant change #Patch Set 8 : rebase #
Messages
Total messages: 25 (0 generated)
How about test coverage? https://codereview.chromium.org/397293005/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:23: class OffsettingLayoutManager : public aura::LayoutManager { Seems heavyweight to create this rather than converting on 136.
https://codereview.chromium.org/397293005/diff/20001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/20001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:23: class OffsettingLayoutManager : public aura::LayoutManager { On 2014/07/17 21:46:03, sky wrote: > Seems heavyweight to create this rather than converting on 136. yes, but converting on 136 doesn't work for the autofill popup
I noticed the code didn't work quite right; I had to adjust it somewhat. Added a test as well. https://codereview.chromium.org/397293005/diff/40001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/40001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:37: if (child->type() == ui::wm::WINDOW_TYPE_POPUP) this may look hokey, but compare to https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...
https://codereview.chromium.org/397293005/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc (right): https://codereview.chromium.org/397293005/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc:135: IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, CorrectBoundsTest) { Could you create a more localized test for your change? There are already unit tests for nativeviewhost. https://codereview.chromium.org/397293005/diff/40001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/40001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:38: bounds.Offset(-child->parent()->GetBoundsInScreen().OffsetFromOrigin()); How does the autofill code end up using NativeViewHost?
https://codereview.chromium.org/397293005/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc (right): https://codereview.chromium.org/397293005/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc:135: IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, CorrectBoundsTest) { On 2014/07/18 15:33:49, sky wrote: > Could you create a more localized test for your change? There are already unit > tests for nativeviewhost. Done. https://codereview.chromium.org/397293005/diff/40001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/40001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:38: bounds.Offset(-child->parent()->GetBoundsInScreen().OffsetFromOrigin()); On 2014/07/18 15:33:49, sky wrote: > How does the autofill code end up using NativeViewHost? the web contents native view is the parent of the autofill popup. The clipping window ends up being the parent that controls the positioning of the autofill popup.
Incase my IM doesn't go through: "I'm still missing something. If the parent of the autofillpopup is the webcontentsview wouldn't the webcontentsview be responsible for setting the bounds of the autofillpopup? What would help is to see stack traces for how the bounds of the autofillpopup was previously set and how its being set now." -Scott On Fri, Jul 18, 2014 at 11:22 AM, <estade@chromium.org> wrote: > > https://codereview.chromium.org/397293005/diff/40001/chrome/browser/ui/views/... > File > chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc > (right): > > https://codereview.chromium.org/397293005/diff/40001/chrome/browser/ui/views/... > chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc:135: > IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, CorrectBoundsTest) { > On 2014/07/18 15:33:49, sky wrote: >> >> Could you create a more localized test for your change? There are > > already unit >> >> tests for nativeviewhost. > > > Done. > > > https://codereview.chromium.org/397293005/diff/40001/ui/views/controls/native... > File ui/views/controls/native/native_view_host_aura.cc (right): > > https://codereview.chromium.org/397293005/diff/40001/ui/views/controls/native... > ui/views/controls/native/native_view_host_aura.cc:38: > bounds.Offset(-child->parent()->GetBoundsInScreen().OffsetFromOrigin()); > On 2014/07/18 15:33:49, sky wrote: >> >> How does the autofill code end up using NativeViewHost? > > > the web contents native view is the parent of the autofill popup. The > clipping window ends up being the parent that controls the positioning > of the autofill popup. > > https://codereview.chromium.org/397293005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
clipping_window_.SetType() does the trick it seems.
ping
I was waiting for you to verify where the particular problem was. Was it RootWindowController? On Mon, Jul 21, 2014 at 5:51 PM, <estade@chromium.org> wrote: > ping > > https://codereview.chromium.org/397293005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/22 04:13:42, sky wrote: > I was waiting for you to verify where the particular problem was. Was > it RootWindowController? > > On Mon, Jul 21, 2014 at 5:51 PM, <mailto:estade@chromium.org> wrote: > > ping > > > > https://codereview.chromium.org/397293005/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. yes, if I change RootWindowController::GetContainerForWindow from [1] to [2] then it also fixes this bug (who knows what else breaks though). [1] aura::Window* RootWindowController::GetContainerForWindow( aura::Window* window) { aura::Window* container = window->parent(); while (container && container->type() != ui::wm::WINDOW_TYPE_UNKNOWN) container = container->parent(); return container; } [2] aura::Window* RootWindowController::GetContainerForWindow( aura::Window* window) { aura::Window* container = window->parent(); while (container && container->parent()) container = container->parent(); return container; }
LGTM - thanks for verifying where the early out was happening. https://codereview.chromium.org/397293005/diff/50001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/50001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:37: if (child->type() == ui::wm::WINDOW_TYPE_POPUP) Ok, I get it now. But you need a comment as to why this is necessary. https://codereview.chromium.org/397293005/diff/70001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/397293005/diff/70001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:337: params.parent = clipping_window(); parent should be the native view here, right?
https://codereview.chromium.org/397293005/diff/50001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/50001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:37: if (child->type() == ui::wm::WINDOW_TYPE_POPUP) On 2014/07/22 13:12:27, sky wrote: > Ok, I get it now. But you need a comment as to why this is necessary. But this isn't necessary any more (you've commented on an older patchset). Did you want a comment on the clipping_window_.SetType(ui::wm::WINDOW_TYPE_CONTROL); line? https://codereview.chromium.org/397293005/diff/70001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/397293005/diff/70001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:337: params.parent = clipping_window(); On 2014/07/22 13:12:27, sky wrote: > parent should be the native view here, right? That won't demonstrate the bug. If I change params.parent to child()->GetNativeView() it doesn't fail even without the patch. I don't think I can create an effective unit test in this file for this regression. RootWindowController is not compiled in views_unittests. The only reason this test in its current format does fail without the patch is this check in InitNativeWidget: if (parent && parent->type() != ui::wm::WINDOW_TYPE_UNKNOWN) which of course is not actually what I intended to test. Therefore I have deleted this test.
https://codereview.chromium.org/397293005/diff/50001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/50001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:37: if (child->type() == ui::wm::WINDOW_TYPE_POPUP) On 2014/07/22 21:45:30, Evan Stade wrote: > On 2014/07/22 13:12:27, sky wrote: > > Ok, I get it now. But you need a comment as to why this is necessary. > > But this isn't necessary any more (you've commented on an older patchset). Did > you want a comment on the clipping_window_.SetType(ui::wm::WINDOW_TYPE_CONTROL); > line? SOrry. Yes. I want a comment there. https://codereview.chromium.org/397293005/diff/70001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/397293005/diff/70001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:337: params.parent = clipping_window(); On 2014/07/22 21:45:30, Evan Stade wrote: > On 2014/07/22 13:12:27, sky wrote: > > parent should be the native view here, right? > > That won't demonstrate the bug. If I change params.parent to > child()->GetNativeView() it doesn't fail even without the patch. Isn't the real bug that widget->GetNativeView() ended up parented to the clipping_window? You didn't ask for that, but that's where it ended up, right? So it seems a test to verify that doesn't happen seems reasonable. > > I don't think I can create an effective unit test in this file for this > regression. RootWindowController is not compiled in views_unittests. The only > reason this test in its current format does fail without the patch is this check > in InitNativeWidget: > > if (parent && parent->type() != ui::wm::WINDOW_TYPE_UNKNOWN) > > which of course is not actually what I intended to test. Therefore I have > deleted this test.
https://codereview.chromium.org/397293005/diff/70001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura_unittest.cc (right): https://codereview.chromium.org/397293005/diff/70001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura_unittest.cc:337: params.parent = clipping_window(); On 2014/07/22 21:52:33, sky wrote: > On 2014/07/22 21:45:30, Evan Stade wrote: > > On 2014/07/22 13:12:27, sky wrote: > > > parent should be the native view here, right? > > > > That won't demonstrate the bug. If I change params.parent to > > child()->GetNativeView() it doesn't fail even without the patch. > > Isn't the real bug that widget->GetNativeView() ended up parented to the > clipping_window? You didn't ask for that, but that's where it ended up, right? > So it seems a test to verify that doesn't happen seems reasonable. > > > > > I don't think I can create an effective unit test in this file for this > > regression. RootWindowController is not compiled in views_unittests. The only > > reason this test in its current format does fail without the patch is this > check > > in InitNativeWidget: > > > > if (parent && parent->type() != ui::wm::WINDOW_TYPE_UNKNOWN) > > > > which of course is not actually what I intended to test. Therefore I have > > deleted this test. > That only happened because of code in RootWindowController, which is not compiled in views_unittests, hence impossible to test from views_unittests.
Ok, no test in the views side then:( https://codereview.chromium.org/397293005/diff/90001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/90001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:21: clipping_window_.SetType(ui::wm::WINDOW_TYPE_CONTROL); As this is so subtle I was hoping for a comment here. https://codereview.chromium.org/397293005/diff/90001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:151: Widget::ReparentNativeView(host_->native_view(), Did you intend to include this?
https://codereview.chromium.org/397293005/diff/90001/ui/views/controls/native... File ui/views/controls/native/native_view_host_aura.cc (right): https://codereview.chromium.org/397293005/diff/90001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:21: clipping_window_.SetType(ui::wm::WINDOW_TYPE_CONTROL); On 2014/07/22 22:36:59, sky wrote: > As this is so subtle I was hoping for a comment here. I will add one. https://codereview.chromium.org/397293005/diff/90001/ui/views/controls/native... ui/views/controls/native/native_view_host_aura.cc:151: Widget::ReparentNativeView(host_->native_view(), On 2014/07/22 22:36:59, sky wrote: > Did you intend to include this? no
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/397293005/110001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/31824) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/36410)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/31864)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/397293005/130001
Message was sent while issue was closed.
Change committed as 285168 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
