Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2222703002:
Aura Icon Capture (Closed)
|
Created:
4 years, 4 months ago by qiangchen Modified:
4 years, 4 months ago Reviewers:
sky CC:
chromium-reviews, sadrul, tfarina, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, kalyank, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAura Icon Capture
For better usability, we wants to display icon for window capture.
In this CL, we store the window icon into Aura::Window for aura windows.
In the future CL, we will use display the icon onto the UI.
BUG=631604
Committed: https://crrev.com/c18dd6cae23fdfc04d3b5d2e3e38b122c06583dd
Cr-Commit-Position: refs/heads/master@{#411095}
Patch Set 1 : Add Icons To Aura::Window #
Total comments: 8
Patch Set 2 : Using SetProperty #
Total comments: 6
Patch Set 3 : Remove duplicate code #
Total comments: 2
Patch Set 4 : Clear Property When Null #
Messages
Total messages: 33 (20 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Aura Icon Capture BUG= ========== to ========== Aura Icon Capture For better usability, we wants to display icon for window capture. In this CL, we store the window icon into Aura::Window for aura windows. In the future CL, we will use display the icon onto the UI. BUG=631604 ==========
The CQ bit was checked by qiangchen@chromium.org 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.
qiangchen@chromium.org changed reviewers: + sky@chromium.org
Can you take a look? Thanks.
Also, please update the bug to include a screenshot of what you are after. https://codereview.chromium.org/2222703002/diff/20001/ui/aura/window.h File ui/aura/window.h (right): https://codereview.chromium.org/2222703002/diff/20001/ui/aura/window.h#newcod... ui/aura/window.h:113: const gfx::ImageSkia icon() const { return icon_; } We generally do not add state like this to Window directly, rather to the properties (see SetProperty). For an example of what I'm suggesting, look at kShowStateKey. https://codereview.chromium.org/2222703002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:654: content_window_->SetIcon(!window_icon.isNull() ? window_icon : app_icon); Does the window capture work on non-chromeos platforms?
Updated the bug. But I encountered some difficulty when trying to use SetProperty, as the lifetime of the image would be an issue. https://codereview.chromium.org/2222703002/diff/20001/ui/aura/window.h File ui/aura/window.h (right): https://codereview.chromium.org/2222703002/diff/20001/ui/aura/window.h#newcod... ui/aura/window.h:113: const gfx::ImageSkia icon() const { return icon_; } On 2016/08/08 19:19:47, sky wrote: > We generally do not add state like this to Window directly, rather to the > properties (see SetProperty). For an example of what I'm suggesting, look at > kShowStateKey. I encounted some difficulty doing that way. Simply creating a const of type WindowProperty<gfx::ImageSkia> does not compile because there is no cast from gfx::ImageSkia to Int64. Using WindowProperty<gfx::ImageSkia*> does not work either, because gfx::ImageSkia's lifetime has expired at the time of getting. https://codereview.chromium.org/2222703002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:654: content_window_->SetIcon(!window_icon.isNull() ? window_icon : app_icon); On 2016/08/08 19:19:47, sky wrote: > Does the window capture work on non-chromeos platforms? Yes. But that part of development (OS API calls) must be in webrtc project, because the OS level capture code is there. See https://codereview.chromium.org/2202883003/.
https://codereview.chromium.org/2222703002/diff/20001/ui/aura/window.h File ui/aura/window.h (right): https://codereview.chromium.org/2222703002/diff/20001/ui/aura/window.h#newcod... ui/aura/window.h:113: const gfx::ImageSkia icon() const { return icon_; } On 2016/08/09 16:48:40, qiangchenC wrote: > On 2016/08/08 19:19:47, sky wrote: > > We generally do not add state like this to Window directly, rather to the > > properties (see SetProperty). For an example of what I'm suggesting, look at > > kShowStateKey. > > I encounted some difficulty doing that way. > Simply creating a const of type WindowProperty<gfx::ImageSkia> does not compile > because there is no cast from gfx::ImageSkia to Int64. > > Using WindowProperty<gfx::ImageSkia*> does not work either, because > gfx::ImageSkia's lifetime has expired at the time of getting. You probably want DEFINE_OWNED_WINDOW_PROPERTY_KEY. The setter than looks something like: window->SetProperty(kIconProperty, new ImageSkia(image)); https://codereview.chromium.org/2222703002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:654: content_window_->SetIcon(!window_icon.isNull() ? window_icon : app_icon); On 2016/08/09 16:48:40, qiangchenC wrote: > On 2016/08/08 19:19:47, sky wrote: > > Does the window capture work on non-chromeos platforms? > > Yes. But that part of development (OS API calls) must be in webrtc project, > because the OS level capture code is there. > > See https://codereview.chromium.org/2202883003/. I wanted to make sure you really wanted this on non-chromeos platforms. Make sense.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Fixed. Thanks. https://codereview.chromium.org/2222703002/diff/20001/ui/aura/window.h File ui/aura/window.h (right): https://codereview.chromium.org/2222703002/diff/20001/ui/aura/window.h#newcod... ui/aura/window.h:113: const gfx::ImageSkia icon() const { return icon_; } On 2016/08/09 19:12:11, sky wrote: > On 2016/08/09 16:48:40, qiangchenC wrote: > > On 2016/08/08 19:19:47, sky wrote: > > > We generally do not add state like this to Window directly, rather to the > > > properties (see SetProperty). For an example of what I'm suggesting, look at > > > kShowStateKey. > > > > I encounted some difficulty doing that way. > > Simply creating a const of type WindowProperty<gfx::ImageSkia> does not > compile > > because there is no cast from gfx::ImageSkia to Int64. > > > > Using WindowProperty<gfx::ImageSkia*> does not work either, because > > gfx::ImageSkia's lifetime has expired at the time of getting. > > You probably want DEFINE_OWNED_WINDOW_PROPERTY_KEY. The setter than looks > something like: > > window->SetProperty(kIconProperty, new ImageSkia(image)); Done. https://codereview.chromium.org/2222703002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:654: content_window_->SetIcon(!window_icon.isNull() ? window_icon : app_icon); On 2016/08/09 19:12:11, sky wrote: > On 2016/08/09 16:48:40, qiangchenC wrote: > > On 2016/08/08 19:19:47, sky wrote: > > > Does the window capture work on non-chromeos platforms? > > > > Yes. But that part of development (OS API calls) must be in webrtc project, > > because the OS level capture code is there. > > > > See https://codereview.chromium.org/2202883003/. > > I wanted to make sure you really wanted this on non-chromeos platforms. Make > sense. Yes. Currently we use aura capture for the chrome window itself, rather than using OS capturer. Thus we have to take icon here, because we do not store the OS window handle in the capture list for chrome window.
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/2222703002/diff/60001/ui/aura/client/aura_con... File ui/aura/client/aura_constants.cc (right): https://codereview.chromium.org/2222703002/diff/60001/ui/aura/client/aura_con... ui/aura/client/aura_constants.cc:42: DEFINE_OWNED_WINDOW_PROPERTY_KEY(gfx::ImageSkia, kWindowIconKey, NULL); NULL -> nullptr https://codereview.chromium.org/2222703002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:656: new gfx::ImageSkia(!window_icon.isNull() ? window_icon : app_icon)); Is it possible the app_icon is also null? I would clear the property in this case. https://codereview.chromium.org/2222703002/diff/60001/ui/views/widget/native_... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_aura.cc:359: window_->SetProperty( How about making this function public static in NativeWidgetAura so DesktopNativeWidgetAura can use it as well.
Patchset #3 (id:80001) has been deleted
fixed https://codereview.chromium.org/2222703002/diff/60001/ui/aura/client/aura_con... File ui/aura/client/aura_constants.cc (right): https://codereview.chromium.org/2222703002/diff/60001/ui/aura/client/aura_con... ui/aura/client/aura_constants.cc:42: DEFINE_OWNED_WINDOW_PROPERTY_KEY(gfx::ImageSkia, kWindowIconKey, NULL); On 2016/08/09 21:24:27, sky wrote: > NULL -> nullptr Done. https://codereview.chromium.org/2222703002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:656: new gfx::ImageSkia(!window_icon.isNull() ? window_icon : app_icon)); On 2016/08/09 21:24:27, sky wrote: > Is it possible the app_icon is also null? I would clear the property in this > case. Done. https://codereview.chromium.org/2222703002/diff/60001/ui/views/widget/native_... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_aura.cc:359: window_->SetProperty( On 2016/08/09 21:24:27, sky wrote: > How about making this function public static in NativeWidgetAura so > DesktopNativeWidgetAura can use it as well. Done.
The CQ bit was checked by qiangchen@chromium.org 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/2222703002/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:108: return; This should clear the property.
Done. https://codereview.chromium.org/2222703002/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/2222703002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:108: return; On 2016/08/09 22:32:25, sky wrote: > This should clear the property. Ah, you are right. I just thought the use case that icon is set only once.
LGTM
The CQ bit was checked by qiangchen@chromium.org 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 qiangchen@chromium.org
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 ========== Aura Icon Capture For better usability, we wants to display icon for window capture. In this CL, we store the window icon into Aura::Window for aura windows. In the future CL, we will use display the icon onto the UI. BUG=631604 ========== to ========== Aura Icon Capture For better usability, we wants to display icon for window capture. In this CL, we store the window icon into Aura::Window for aura windows. In the future CL, we will use display the icon onto the UI. BUG=631604 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Aura Icon Capture For better usability, we wants to display icon for window capture. In this CL, we store the window icon into Aura::Window for aura windows. In the future CL, we will use display the icon onto the UI. BUG=631604 ========== to ========== Aura Icon Capture For better usability, we wants to display icon for window capture. In this CL, we store the window icon into Aura::Window for aura windows. In the future CL, we will use display the icon onto the UI. BUG=631604 Committed: https://crrev.com/c18dd6cae23fdfc04d3b5d2e3e38b122c06583dd Cr-Commit-Position: refs/heads/master@{#411095} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c18dd6cae23fdfc04d3b5d2e3e38b122c06583dd Cr-Commit-Position: refs/heads/master@{#411095} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
