|
|
Created:
3 years, 9 months ago by Tom (Use chromium acct) Modified:
3 years, 7 months ago Reviewers:
sadrul CC:
chromium-reviews, yusukes+watch_chromium.org, derat+watch_chromium.org, tfarina, shuchen+watch_chromium.org, dcheng, nona+watch_chromium.org, James Su Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable transparency when dragging from the omnibox
Previously, dragging text out of the omnibox would render with a solid
background on Linux because transparency wasn't available. However,
after https://codereview.chromium.org/2347383002/, transparency is
enabled. This CL enables transparency on text dragged out of the
omnibox on Linux.
In addition, when dragging ALL text from the omnibox, a special URL
drag window with some text and an icon is used instead. Transparency
was previously not enabled on this window because
DWTHX11::IsTranslucentWindowOpacitySupported() was returning the
wrong value.
R=sadrul@chromium.org
Review-Url: https://codereview.chromium.org/2781733002
Cr-Commit-Position: refs/heads/master@{#470092}
Committed: https://chromium.googlesource.com/chromium/src/+/5785866a9b2a0db4854552eb87af7229d4b11a43
Patch Set 1 #Patch Set 2 : Add nogncheck #Patch Set 3 : Fix CrOS build #
Total comments: 5
Patch Set 4 : const #Patch Set 5 : Revert button_drag_utils #
Total comments: 5
Patch Set 6 : Add comment #
Messages
Total messages: 50 (35 generated)
Description was changed from ========== Enable transparency when dragging from the omnibox Previously, dragging text out of the omnibox would render with a solid background on Linux because transparency wasn't available. However, after https://codereview.chromium.org/2347383002/, transparency is enabled. This CL enables transparency on text dragged out of the omnibox on Linux. In addition, when dragging ALL text from the omnibox, a special URL drag window with some text and an icon is used instead. Transparency is also enabled on this window on all platforms. This behavior was already present on Windows and Mac (haven't tested CrOS), so they must take different codepaths. BUG= R=sadrul@chromium.org ========== to ========== Enable transparency when dragging from the omnibox Previously, dragging text out of the omnibox would render with a solid background on Linux because transparency wasn't available. However, after https://codereview.chromium.org/2347383002/, transparency is enabled. This CL enables transparency on text dragged out of the omnibox on Linux. In addition, when dragging ALL text from the omnibox, a special URL drag window with some text and an icon is used instead. Transparency is also enabled on this window on all platforms. This behavior was already present on Windows and Mac (haven't tested CrOS), so they must take different codepaths. R=sadrul@chromium.org ==========
sadrul ptal
The CQ bit was checked by thomasanderson@google.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 thomasanderson@google.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_chromeos_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 thomasanderson@google.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/2781733002/diff/40001/ui/base/x/x11_util_inte... File ui/base/x/x11_util_internal.h (right): https://codereview.chromium.org/2781733002/diff/40001/ui/base/x/x11_util_inte... ui/base/x/x11_util_internal.h:82: bool ArgbVisualAvailable(); const method https://codereview.chromium.org/2781733002/diff/40001/ui/views/button_drag_ut... File ui/views/button_drag_utils.cc (left): https://codereview.chromium.org/2781733002/diff/40001/ui/views/button_drag_ut... ui/views/button_drag_utils.cc:59: ui::NativeTheme::kColorId_TextfieldDefaultBackground); We still need to do this on non-x11, right?
Description was changed from ========== Enable transparency when dragging from the omnibox Previously, dragging text out of the omnibox would render with a solid background on Linux because transparency wasn't available. However, after https://codereview.chromium.org/2347383002/, transparency is enabled. This CL enables transparency on text dragged out of the omnibox on Linux. In addition, when dragging ALL text from the omnibox, a special URL drag window with some text and an icon is used instead. Transparency is also enabled on this window on all platforms. This behavior was already present on Windows and Mac (haven't tested CrOS), so they must take different codepaths. R=sadrul@chromium.org ========== to ========== Enable transparency when dragging from the omnibox Previously, dragging text out of the omnibox would render with a solid background on Linux because transparency wasn't available. However, after https://codereview.chromium.org/2347383002/, transparency is enabled. This CL enables transparency on text dragged out of the omnibox on Linux. In addition, when dragging ALL text from the omnibox, a special URL drag window with some text and an icon is used instead. Transparency is also enabled on this window on all platforms. This behavior was already present on Windows and Mac, and CrOs, so they must take different codepaths. R=sadrul@chromium.org ==========
https://codereview.chromium.org/2781733002/diff/40001/ui/base/x/x11_util_inte... File ui/base/x/x11_util_internal.h (right): https://codereview.chromium.org/2781733002/diff/40001/ui/base/x/x11_util_inte... ui/base/x/x11_util_internal.h:82: bool ArgbVisualAvailable(); On 2017/03/28 15:52:07, sadrul wrote: > const method Done. https://codereview.chromium.org/2781733002/diff/40001/ui/views/button_drag_ut... File ui/views/button_drag_utils.cc (left): https://codereview.chromium.org/2781733002/diff/40001/ui/views/button_drag_ut... ui/views/button_drag_utils.cc:59: ui::NativeTheme::kColorId_TextfieldDefaultBackground); On 2017/03/28 15:52:07, sadrul wrote: > We still need to do this on non-x11, right? Windows, Mac, and CrOs already have transparency with this type of drag, so they must be taking a different codepath. AFAIK, this change is Linux-only.
The CQ bit was checked by thomasanderson@google.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...
https://codereview.chromium.org/2781733002/diff/40001/ui/views/button_drag_ut... File ui/views/button_drag_utils.cc (left): https://codereview.chromium.org/2781733002/diff/40001/ui/views/button_drag_ut... ui/views/button_drag_utils.cc:59: ui::NativeTheme::kColorId_TextfieldDefaultBackground); On 2017/03/28 18:28:10, Tom Anderson wrote: > On 2017/03/28 15:52:07, sadrul wrote: > > We still need to do this on non-x11, right? > > Windows, Mac, and CrOs already have transparency with this type of drag, so they > must be taking a different codepath. Or maybe the default theme on those platforms for this is transparent? Can you verify? > AFAIK, this change is Linux-only.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Enable transparency when dragging from the omnibox Previously, dragging text out of the omnibox would render with a solid background on Linux because transparency wasn't available. However, after https://codereview.chromium.org/2347383002/, transparency is enabled. This CL enables transparency on text dragged out of the omnibox on Linux. In addition, when dragging ALL text from the omnibox, a special URL drag window with some text and an icon is used instead. Transparency is also enabled on this window on all platforms. This behavior was already present on Windows and Mac, and CrOs, so they must take different codepaths. R=sadrul@chromium.org ========== to ========== Enable transparency when dragging from the omnibox Previously, dragging text out of the omnibox would render with a solid background on Linux because transparency wasn't available. However, after https://codereview.chromium.org/2347383002/, transparency is enabled. This CL enables transparency on text dragged out of the omnibox on Linux. In addition, when dragging ALL text from the omnibox, a special URL drag window with some text and an icon is used instead. Transparency was previously not enabled on this window because DWTHX11::IsTranslucentWindowOpacitySupported() was returning the wrong value. R=sadrul@chromium.org ==========
On 2017/03/28 18:52:40, sadrul wrote: > https://codereview.chromium.org/2781733002/diff/40001/ui/views/button_drag_ut... > File ui/views/button_drag_utils.cc (left): > > https://codereview.chromium.org/2781733002/diff/40001/ui/views/button_drag_ut... > ui/views/button_drag_utils.cc:59: > ui::NativeTheme::kColorId_TextfieldDefaultBackground); > On 2017/03/28 18:28:10, Tom Anderson wrote: > > On 2017/03/28 15:52:07, sadrul wrote: > > > We still need to do this on non-x11, right? > > > > Windows, Mac, and CrOs already have transparency with this type of drag, so > they > > must be taking a different codepath. > > Or maybe the default theme on those platforms for this is transparent? Can you > verify? > After some debugging, I believe you were right about removing the change in button_drag_utils. The bug was actually in DWTHX11::IsTranslucentWindowOpacitySupported() returning a bad value. ptal Also, ShouldWindowContentsBeTransparent is only used on Windows to determine if Chrome should render using the glass frame, so I changed it to just return false.
The CQ bit was checked by thomasanderson@google.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: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by thomasanderson@google.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: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by thomasanderson@google.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_...)
https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1177: return ui::XVisualManager::GetInstance()->ArgbVisualAvailable(); Here's my understanding of the code: |use_argb_visual_| is essentially ArgbVisualAvailable() && <we want to enable transparent visuals for this window>. The latter part is decided in DesktopWindowTreeHostX11::InitX11Window(), where as far as I can see, |enable_transparent_visuals| is set correctly (i.e. it is set to true for drag windows). So, continuing to use |use_argb_visual_| here should do the right thing?
https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1177: return ui::XVisualManager::GetInstance()->ArgbVisualAvailable(); On 2017/03/29 19:51:03, sadrul wrote: > Here's my understanding of the code: > > |use_argb_visual_| is essentially ArgbVisualAvailable() && <we want to enable > transparent visuals for this window>. > > The latter part is decided in DesktopWindowTreeHostX11::InitX11Window(), where > as far as I can see, |enable_transparent_visuals| is set correctly (i.e. it is > set to true for drag windows). So, continuing to use |use_argb_visual_| here > should do the right thing? https://cs.chromium.org/chromium/src/ui/views/button_drag_utils.cc?rcl=64eeaf... ^ That seems to call IsTranslucentWindowOpacitySupported() before InitX11Window()
https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1177: return ui::XVisualManager::GetInstance()->ArgbVisualAvailable(); On 2017/03/29 21:09:22, Tom Anderson wrote: > On 2017/03/29 19:51:03, sadrul wrote: > > Here's my understanding of the code: > > > > |use_argb_visual_| is essentially ArgbVisualAvailable() && <we want to enable > > transparent visuals for this window>. > > > > The latter part is decided in DesktopWindowTreeHostX11::InitX11Window(), where > > as far as I can see, |enable_transparent_visuals| is set correctly (i.e. it is > > set to true for drag windows). So, continuing to use |use_argb_visual_| here > > should do the right thing? > > https://cs.chromium.org/chromium/src/ui/views/button_drag_utils.cc?rcl=64eeaf... > > ^ That seems to call IsTranslucentWindowOpacitySupported() before > InitX11Window() Well, either that or it's calling IsTranslucentWindowOpacitySupported() on the browser window instead of the drag window. Haven't actually debugged this
ping
On 2017/03/29 21:11:23, Tom Anderson wrote: > https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): > > https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1177: return > ui::XVisualManager::GetInstance()->ArgbVisualAvailable(); > On 2017/03/29 21:09:22, Tom Anderson wrote: > > On 2017/03/29 19:51:03, sadrul wrote: > > > Here's my understanding of the code: > > > > > > |use_argb_visual_| is essentially ArgbVisualAvailable() && <we want to > enable > > > transparent visuals for this window>. > > > > > > The latter part is decided in DesktopWindowTreeHostX11::InitX11Window(), > where > > > as far as I can see, |enable_transparent_visuals| is set correctly (i.e. it > is > > > set to true for drag windows). So, continuing to use |use_argb_visual_| here > > > should do the right thing? > > > > > https://cs.chromium.org/chromium/src/ui/views/button_drag_utils.cc?rcl=64eeaf... > > > > ^ That seems to call IsTranslucentWindowOpacitySupported() before > > InitX11Window() > > Well, either that or it's calling IsTranslucentWindowOpacitySupported() on the > browser window instead of the drag window. Haven't actually debugged this Did you get a chance to look at this?
On 2017/04/18 17:29:28, sadrul wrote: > On 2017/03/29 21:11:23, Tom Anderson wrote: > > > https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... > > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): > > > > > https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... > > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1177: return > > ui::XVisualManager::GetInstance()->ArgbVisualAvailable(); > > On 2017/03/29 21:09:22, Tom Anderson wrote: > > > On 2017/03/29 19:51:03, sadrul wrote: > > > > Here's my understanding of the code: > > > > > > > > |use_argb_visual_| is essentially ArgbVisualAvailable() && <we want to > > enable > > > > transparent visuals for this window>. > > > > > > > > The latter part is decided in DesktopWindowTreeHostX11::InitX11Window(), > > where > > > > as far as I can see, |enable_transparent_visuals| is set correctly (i.e. > it > > is > > > > set to true for drag windows). So, continuing to use |use_argb_visual_| > here > > > > should do the right thing? > > > > > > > > > https://cs.chromium.org/chromium/src/ui/views/button_drag_utils.cc?rcl=64eeaf... > > > > > > ^ That seems to call IsTranslucentWindowOpacitySupported() before > > > InitX11Window() > > > > Well, either that or it's calling IsTranslucentWindowOpacitySupported() on the > > browser window instead of the drag window. Haven't actually debugged this > > Did you get a chance to look at this? It appears to be the former: button_drag_utils.cc is calling IsTranslucentWindowOpacitySupported() before InitX11Window() is called.
lgtm https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1177: return ui::XVisualManager::GetInstance()->ArgbVisualAvailable(); On 2017/03/29 21:11:23, Tom Anderson wrote: > On 2017/03/29 21:09:22, Tom Anderson wrote: > > On 2017/03/29 19:51:03, sadrul wrote: > > > Here's my understanding of the code: > > > > > > |use_argb_visual_| is essentially ArgbVisualAvailable() && <we want to > enable > > > transparent visuals for this window>. > > > > > > The latter part is decided in DesktopWindowTreeHostX11::InitX11Window(), > where > > > as far as I can see, |enable_transparent_visuals| is set correctly (i.e. it > is > > > set to true for drag windows). So, continuing to use |use_argb_visual_| here > > > should do the right thing? > > > > > https://cs.chromium.org/chromium/src/ui/views/button_drag_utils.cc?rcl=64eeaf... > > > > ^ That seems to call IsTranslucentWindowOpacitySupported() before > > InitX11Window() > > Well, either that or it's calling IsTranslucentWindowOpacitySupported() on the > browser window instead of the drag window. Haven't actually debugged this Thanks for looking into it. Mind adding a comment here that |use_argb_visual_| is initialized in InitX11Window(), but this function can be called before the initialization happens?
https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2781733002/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1177: return ui::XVisualManager::GetInstance()->ArgbVisualAvailable(); On 2017/05/08 15:27:04, sadrul wrote: > On 2017/03/29 21:11:23, Tom Anderson wrote: > > On 2017/03/29 21:09:22, Tom Anderson wrote: > > > On 2017/03/29 19:51:03, sadrul wrote: > > > > Here's my understanding of the code: > > > > > > > > |use_argb_visual_| is essentially ArgbVisualAvailable() && <we want to > > enable > > > > transparent visuals for this window>. > > > > > > > > The latter part is decided in DesktopWindowTreeHostX11::InitX11Window(), > > where > > > > as far as I can see, |enable_transparent_visuals| is set correctly (i.e. > it > > is > > > > set to true for drag windows). So, continuing to use |use_argb_visual_| > here > > > > should do the right thing? > > > > > > > > > https://cs.chromium.org/chromium/src/ui/views/button_drag_utils.cc?rcl=64eeaf... > > > > > > ^ That seems to call IsTranslucentWindowOpacitySupported() before > > > InitX11Window() > > > > Well, either that or it's calling IsTranslucentWindowOpacitySupported() on the > > browser window instead of the drag window. Haven't actually debugged this > > Thanks for looking into it. Mind adding a comment here that |use_argb_visual_| > is initialized in InitX11Window(), but this function can be called before the > initialization happens? Done.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2781733002/#ps100001 (title: "Add comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494266693537940, "parent_rev": "9c0c398442ed5dc4d3c52f735b57cb84a22c977d", "commit_rev": "5785866a9b2a0db4854552eb87af7229d4b11a43"}
Message was sent while issue was closed.
Description was changed from ========== Enable transparency when dragging from the omnibox Previously, dragging text out of the omnibox would render with a solid background on Linux because transparency wasn't available. However, after https://codereview.chromium.org/2347383002/, transparency is enabled. This CL enables transparency on text dragged out of the omnibox on Linux. In addition, when dragging ALL text from the omnibox, a special URL drag window with some text and an icon is used instead. Transparency was previously not enabled on this window because DWTHX11::IsTranslucentWindowOpacitySupported() was returning the wrong value. R=sadrul@chromium.org ========== to ========== Enable transparency when dragging from the omnibox Previously, dragging text out of the omnibox would render with a solid background on Linux because transparency wasn't available. However, after https://codereview.chromium.org/2347383002/, transparency is enabled. This CL enables transparency on text dragged out of the omnibox on Linux. In addition, when dragging ALL text from the omnibox, a special URL drag window with some text and an icon is used instead. Transparency was previously not enabled on this window because DWTHX11::IsTranslucentWindowOpacitySupported() was returning the wrong value. R=sadrul@chromium.org Review-Url: https://codereview.chromium.org/2781733002 Cr-Commit-Position: refs/heads/master@{#470092} Committed: https://chromium.googlesource.com/chromium/src/+/5785866a9b2a0db4854552eb87af... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5785866a9b2a0db4854552eb87af... |