Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2280433004:
Fix missing shadows for tooltip and menu (Closed)
|
Created:
4 years, 3 months ago by Julien Isorce Samsung Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina, sadrul, yusukes+watch_chromium.org, derat+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix missing shadows for tooltip and menu
When using a transparent visual the window manager does not draw any
shadow by default (ex: 'Compiz' WM). Note that this default behavior
is WM dependent.
So the solution is to not use a transparent visual for any window
type which is not TYPE_DRAG and not TYPE_WINDOW.
Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW
because of crbug.com/593256 and crbug.com/369209.
BUG=640170
R=piman@chromium.org, sadrul@chromium.org
TEST=python testing/xvfb.py ./out/build/ ./out/build/views_unittests --gtest_filter=*WidgetTest.Transparency*
./out/build/ui_base_unittests --gtest_filter=*ChooseVisualForWindow*
Committed: https://crrev.com/fa6da651020b236ab9dc8b4d9f1198dc1d1049ca
Cr-Commit-Position: refs/heads/master@{#418221}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add tests #Patch Set 3 : Fix build if not X11 #Patch Set 4 : Fixed tests when running with --single-process-tests #Patch Set 5 : Rebase and add missing deps for gn build on chromeos #Patch Set 6 : Attempt to fix osx and win test failures #Patch Set 7 : Only do TYPE_WINDOW on OS_WIN as before, and fix Linux test failures #Patch Set 8 : Fixed test failures on mus (views_mus_unittests) #Patch Set 9 : Just rebase #
Total comments: 10
Patch Set 10 : Rebase and addressed remarks #Patch Set 11 : Forgot to re-add x11_util_unittest.cc because git cl patch failed so I had to use git apply (-gyp) #Patch Set 12 : Rebase #Patch Set 13 : Just rebase #Patch Set 14 : Rebase #Patch Set 15 : Rebase #
Messages
Total messages: 73 (61 generated)
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Antoine, please have a look. But note that I prefer the first version I uploaded, i.e. https://codereview.chromium.org/2268973004/. Thx!
non-OWNER lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2016/08/26 02:12:07, sadrul wrote: > lgtm Thx you both for the review. Before to land the patch I will add some new tests. There are already some tests (*WidgetTest.Transparency_*) for ui::ChooseVisualForWindow but I will add more.
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix missing shadows for tooltip and menu When using a transparent visual the window manager does not draw any shadow by default (ex: 'Compiz' WM). Note that this default behavior is WM dependent. So the solution is to not use a transparent visual for any window type which is not TYPE_DRAG and not TYPE_WINDOW. Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW because of crbug.com/593256 and crbug.com/369209. BUG=640170 R=piman@chromium.org, sadrul@chromium.org ========== to ========== Fix missing shadows for tooltip and menu When using a transparent visual the window manager does not draw any shadow by default (ex: 'Compiz' WM). Note that this default behavior is WM dependent. So the solution is to not use a transparent visual for any window type which is not TYPE_DRAG and not TYPE_WINDOW. Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW because of crbug.com/593256 and crbug.com/369209. BUG=640170 R=piman@chromium.org, sadrul@chromium.org TEST=python testing/xvfb.py ./out/build/ ./out/build/views_unittests --gtest_filter=*WidgetTest.Transparency* ./out/build/ui_base_unittests --gtest_filter=*ChooseVisualForWindow* ==========
On 2016/08/26 15:27:47, Julien Isorce wrote: > On 2016/08/26 02:12:07, sadrul wrote: > > lgtm > > Thx you both for the review. Before to land the patch I will add some new tests. > There are already some tests (*WidgetTest.Transparency_*) for > ui::ChooseVisualForWindow but I will add more. Hi, I improved existing tests and I added a new one in a new file ui/base/x/x11_util_unittest.cc . Please have a look, thx.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/30 16:48:04, Julien Isorce wrote: >> Dry run: This issue passed the CQ dry run. Hi Sadrul, please review the existing tests I improved to cover the changes that you already stamped. Thx.
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2280433004/diff/1/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/2280433004/diff/1/ui/base/x/x11_util.cc#newco... ui/base/x/x11_util.cc:1452: XWindowAttributes windowAttribs; window_attribs, or attribs. Sorry, missed this in earlier iteration. https://codereview.chromium.org/2280433004/diff/160001/ui/base/x/x11_util_uni... File ui/base/x/x11_util_unittest.cc (right): https://codereview.chromium.org/2280433004/diff/160001/ui/base/x/x11_util_uni... ui/base/x/x11_util_unittest.cc:20: XWindowAttributes windowAttribs; window_attribs, or just attribs https://codereview.chromium.org/2280433004/diff/160001/ui/base/x/x11_util_uni... ui/base/x/x11_util_unittest.cc:23: DCHECK(status != 0); DCHECK_NE https://codereview.chromium.org/2280433004/diff/160001/ui/base/x/x11_util_uni... ui/base/x/x11_util_unittest.cc:27: ui::ChooseVisualForWindow(has_compositing_manager, NULL, &depth); nullptr instead of NULL https://codereview.chromium.org/2280433004/diff/160001/ui/views/widget/widget... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/2280433004/diff/160001/ui/views/widget/widget... ui/views/widget/widget_unittest.cc:3743: init_params.parent = nullptr; Do you need to explicitly set child and parent here? https://codereview.chromium.org/2280433004/diff/160001/ui/views/widget/widget... ui/views/widget/widget_unittest.cc:3807: protected: private? Also, DISALLOW_COPY_AND_ASSIGN
Thx for the review. https://codereview.chromium.org/2280433004/diff/1/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/2280433004/diff/1/ui/base/x/x11_util.cc#newco... ui/base/x/x11_util.cc:1452: XWindowAttributes windowAttribs; On 2016/09/02 19:53:24, sadrul wrote: > window_attribs, or attribs. > > Sorry, missed this in earlier iteration. My fault, thx. https://codereview.chromium.org/2280433004/diff/160001/ui/base/x/x11_util_uni... File ui/base/x/x11_util_unittest.cc (right): https://codereview.chromium.org/2280433004/diff/160001/ui/base/x/x11_util_uni... ui/base/x/x11_util_unittest.cc:20: XWindowAttributes windowAttribs; On 2016/09/02 19:53:24, sadrul wrote: > window_attribs, or just attribs Done. https://codereview.chromium.org/2280433004/diff/160001/ui/base/x/x11_util_uni... ui/base/x/x11_util_unittest.cc:23: DCHECK(status != 0); On 2016/09/02 19:53:24, sadrul wrote: > DCHECK_NE Done. https://codereview.chromium.org/2280433004/diff/160001/ui/base/x/x11_util_uni... ui/base/x/x11_util_unittest.cc:27: ui::ChooseVisualForWindow(has_compositing_manager, NULL, &depth); On 2016/09/02 19:53:24, sadrul wrote: > nullptr instead of NULL Done. https://codereview.chromium.org/2280433004/diff/160001/ui/views/widget/widget... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/2280433004/diff/160001/ui/views/widget/widget... ui/views/widget/widget_unittest.cc:3743: init_params.parent = nullptr; On 2016/09/02 19:53:24, sadrul wrote: > Do you need to explicitly set child and parent here? No I will remove it, all are initialized to 0. https://codereview.chromium.org/2280433004/diff/160001/ui/views/widget/widget... ui/views/widget/widget_unittest.cc:3807: protected: On 2016/09/02 19:53:24, sadrul wrote: > private? > > Also, DISALLOW_COPY_AND_ASSIGN Done.
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am committing the CL because it is a priority 1 issue. Also the fix has been stamped already. Only the test has not been stamped but it was just some minor issues from the review. And I addressed them.
The CQ bit was checked by j.isorce@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2280433004/#ps280001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix missing shadows for tooltip and menu When using a transparent visual the window manager does not draw any shadow by default (ex: 'Compiz' WM). Note that this default behavior is WM dependent. So the solution is to not use a transparent visual for any window type which is not TYPE_DRAG and not TYPE_WINDOW. Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW because of crbug.com/593256 and crbug.com/369209. BUG=640170 R=piman@chromium.org, sadrul@chromium.org TEST=python testing/xvfb.py ./out/build/ ./out/build/views_unittests --gtest_filter=*WidgetTest.Transparency* ./out/build/ui_base_unittests --gtest_filter=*ChooseVisualForWindow* ========== to ========== Fix missing shadows for tooltip and menu When using a transparent visual the window manager does not draw any shadow by default (ex: 'Compiz' WM). Note that this default behavior is WM dependent. So the solution is to not use a transparent visual for any window type which is not TYPE_DRAG and not TYPE_WINDOW. Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW because of crbug.com/593256 and crbug.com/369209. BUG=640170 R=piman@chromium.org, sadrul@chromium.org TEST=python testing/xvfb.py ./out/build/ ./out/build/views_unittests --gtest_filter=*WidgetTest.Transparency* ./out/build/ui_base_unittests --gtest_filter=*ChooseVisualForWindow* ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Fix missing shadows for tooltip and menu When using a transparent visual the window manager does not draw any shadow by default (ex: 'Compiz' WM). Note that this default behavior is WM dependent. So the solution is to not use a transparent visual for any window type which is not TYPE_DRAG and not TYPE_WINDOW. Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW because of crbug.com/593256 and crbug.com/369209. BUG=640170 R=piman@chromium.org, sadrul@chromium.org TEST=python testing/xvfb.py ./out/build/ ./out/build/views_unittests --gtest_filter=*WidgetTest.Transparency* ./out/build/ui_base_unittests --gtest_filter=*ChooseVisualForWindow* ========== to ========== Fix missing shadows for tooltip and menu When using a transparent visual the window manager does not draw any shadow by default (ex: 'Compiz' WM). Note that this default behavior is WM dependent. So the solution is to not use a transparent visual for any window type which is not TYPE_DRAG and not TYPE_WINDOW. Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW because of crbug.com/593256 and crbug.com/369209. BUG=640170 R=piman@chromium.org, sadrul@chromium.org TEST=python testing/xvfb.py ./out/build/ ./out/build/views_unittests --gtest_filter=*WidgetTest.Transparency* ./out/build/ui_base_unittests --gtest_filter=*ChooseVisualForWindow* Committed: https://crrev.com/fa6da651020b236ab9dc8b4d9f1198dc1d1049ca Cr-Commit-Position: refs/heads/master@{#418221} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/fa6da651020b236ab9dc8b4d9f1198dc1d1049ca Cr-Commit-Position: refs/heads/master@{#418221} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
