Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2270543003:
Display Window Icon In Picker UI (Closed)
DescriptionDisplay Window Icon In Picker UI
In this CL, we display the window icon with the window source
on the picker window for desktop capture.
BUG=631604
Committed: https://crrev.com/e7a9525b5e292c45f8d6db8091afed4ea5045aff
Cr-Commit-Position: refs/heads/master@{#415199}
Patch Set 1 #Patch Set 2 : Unittest crash #
Total comments: 16
Patch Set 3 : Fix Unittest crash #Patch Set 4 : Add file #
Total comments: 8
Patch Set 5 : Improve Comment #Patch Set 6 : Old Error Handler #
Total comments: 2
Patch Set 7 : App vs Browser #
Total comments: 11
Patch Set 8 : Add comments #Patch Set 9 : chrome/browser/ui/views cannot be included from outside folder #Patch Set 10 : Change Error Handler Back #
Total comments: 4
Patch Set 11 : Error Tracker #
Total comments: 2
Patch Set 12 : Use unique_ptr for Error Tracker #
Total comments: 14
Patch Set 13 : Improve Comments #
Total comments: 8
Messages
Total messages: 102 (60 generated)
Description was changed from ========== Display Window Icon In Picker UI BUG= ========== to ========== Display Window Icon In Picker UI BUG=631604 ==========
qiangchen@chromium.org changed reviewers: + msw@chromium.org
Description was changed from ========== Display Window Icon In Picker UI BUG=631604 ========== to ========== Display Window Icon In Picker UI In this CL, we display the window icon with the window source on the picker window for desktop capture. 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...
Can you take a look? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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 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...
You should seek a reviewer more familiar with GetWindowIcon. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:29: #if (defined(OS_LINUX) && !defined(OS_CHROMEOS)) || defined(OS_WIN) || \ q: would #if !defined(OS_CHROMEOS) suffice? Is this file used on android/ios or elsewhere that would need to be handled separately? Consider: #if defined(OS_CHROMEOS) // cros code #else // desktop code (if this file is not built on ios/android/etc.) #endif Or: #if defined(OS_CHROMEOS) // cros code #elif defined(OS_LINUX) || defined(OS_WIN) || defined(OS_MACOSX) // desktop code (if this file is build on ios/android/etc.) #else // ios/android/etc.'s return gfx::ImageSkia(); #endif https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:32: #elif defined(OS_CHROMEOS) Does the code here actually belong in the GetWindowIcon impl for cros? https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:38: if (icon_image_ptr) return *icon_image_ptr; No return on the same line as if; use 'git cl format'. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:40: // The default icon (for chrome window itself) does not enter window tree nit: "(for a Chrome window itself)". Also, I'm not sure what you mean by "does not enter window tree host", do you mean "is not a member of the window tree host"? if so, that seems odd... do you just mean that we never call RegisterAuraWindow for browser windows? if so, is that something we can fix instead? https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:43: icon_image_ptr = rb.GetNativeImageNamed(IDR_PRODUCT_LOGO_32).ToImageSkia(); Is it possible that something other than a chrome window couldn't be found by DesktopMediaID::GetAuraWindowById? If so, does it actually make sense to use a chrome logo for this unidentified window? That seems wrong. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:44: if (icon_image_ptr) return *icon_image_ptr; Ditto: No return on the same line as if; use 'git cl format'. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:56: icon_view_->set_interactive(false); nit: order before image_view_->set_interactive(false); to match other ordering. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:140: icon_view_->SetImageSize(gfx::Size(style_.icon_rect.width(), nit: icon_view_->SetImageSize(style_.icon_rect.size());
Oh, please also post before/after screenshots to the bug.
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 to run a CQ dry run
Patchset #4 (id:60001) has been deleted
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...
qiangchen@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@: One issue I found when hooking up icon display on the UI is that aura::Window does not have icon for chrome window itself. In previous CL, we intercept and store the icon into aura::Window in the function NativeWidgetAura::SetWindowIcons. But I found that function is never called for chrome browser window itself. (it is called if we launch an app, so no problem to get the icon of a chrome app) Do you have an idea of the code location where we load the chrome logo and display it out for ChromeOS? Is it feasible to inject the icon into chrome browser's aura::Window instance, or is it fine to use the logo whenever we detect an empty icon in aura::Window. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:29: #if (defined(OS_LINUX) && !defined(OS_CHROMEOS)) || defined(OS_WIN) || \ On 2016/08/23 21:37:12, msw wrote: > q: would #if !defined(OS_CHROMEOS) suffice? Is this file used on android/ios or > elsewhere that would need to be handled separately? Consider: > #if defined(OS_CHROMEOS) > // cros code > #else > // desktop code (if this file is not built on ios/android/etc.) > #endif > > Or: > #if defined(OS_CHROMEOS) > // cros code > #elif defined(OS_LINUX) || defined(OS_WIN) || defined(OS_MACOSX) > // desktop code (if this file is build on ios/android/etc.) > #else > // ios/android/etc.'s return gfx::ImageSkia(); > #endif > Moved to a single file as implementation of GetWindowIcon on chromeos, and thus UI part will look platform independent. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:32: #elif defined(OS_CHROMEOS) On 2016/08/23 21:37:12, msw wrote: > Does the code here actually belong in the GetWindowIcon impl for cros? N/A now. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:38: if (icon_image_ptr) return *icon_image_ptr; On 2016/08/23 21:37:12, msw wrote: > No return on the same line as if; use 'git cl format'. Done. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:40: // The default icon (for chrome window itself) does not enter window tree On 2016/08/23 21:37:11, msw wrote: > nit: "(for a Chrome window itself)". Also, I'm not sure what you mean by "does > not enter window tree host", do you mean "is not a member of the window tree > host"? if so, that seems odd... do you just mean that we never call > RegisterAuraWindow for browser windows? if so, is that something we can fix > instead? Changed to return empty icon if not aura window. For chrome window itself, its aura::Window instance does not contain a icon. I just found the corresponding NativeWidgetAura::SetWindowIcons does not get called at all, which is the place where we intercept the icon image. Let me ask aura guys to see whether it is easy to inject icon into aura::Window for chrome windows, or it is ok to do it here. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:43: icon_image_ptr = rb.GetNativeImageNamed(IDR_PRODUCT_LOGO_32).ToImageSkia(); On 2016/08/23 21:37:12, msw wrote: > Is it possible that something other than a chrome window couldn't be found by > DesktopMediaID::GetAuraWindowById? If so, does it actually make sense to use a > chrome logo for this unidentified window? That seems wrong. For ChromeOS, everything is aura window, so no such worry. For other OS, we do not use this code path, but use OS api directly. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:44: if (icon_image_ptr) return *icon_image_ptr; On 2016/08/23 21:37:12, msw wrote: > Ditto: No return on the same line as if; use 'git cl format'. Done. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:56: icon_view_->set_interactive(false); On 2016/08/23 21:37:12, msw wrote: > nit: order before image_view_->set_interactive(false); to match other ordering. Done. https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:140: icon_view_->SetImageSize(gfx::Size(style_.icon_rect.width(), On 2016/08/23 21:37:12, msw wrote: > nit: icon_view_->SetImageSize(style_.icon_rect.size()); Done.
See BrowserView::GetWindowIcon, it returns an empy icon. You'll have to be careful that changing the override doesn't impact windows. -Scott On Tue, Aug 23, 2016 at 4:24 PM, <qiangchen@chromium.org> wrote: > sky@: > One issue I found when hooking up icon display on the UI is that > aura::Window > does not have icon for chrome window itself. > > In previous CL, we intercept and store the icon into aura::Window in the > function NativeWidgetAura::SetWindowIcons. But I found that function is > never > called for chrome browser window itself. (it is called if we launch an app, > so > no problem to get the icon of a chrome app) > > Do you have an idea of the code location where we load the chrome logo and > display it out for ChromeOS? > Is it feasible to inject the icon into chrome browser's aura::Window > instance, > or is it fine to use the logo whenever we detect an empty icon in > aura::Window. > > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc > (right): > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:29: > #if (defined(OS_LINUX) && !defined(OS_CHROMEOS)) || defined(OS_WIN) || \ > On 2016/08/23 21:37:12, msw wrote: >> q: would #if !defined(OS_CHROMEOS) suffice? Is this file used on > android/ios or >> elsewhere that would need to be handled separately? Consider: >> #if defined(OS_CHROMEOS) >> // cros code >> #else >> // desktop code (if this file is not built on ios/android/etc.) >> #endif >> >> Or: >> #if defined(OS_CHROMEOS) >> // cros code >> #elif defined(OS_LINUX) || defined(OS_WIN) || defined(OS_MACOSX) >> // desktop code (if this file is build on ios/android/etc.) >> #else >> // ios/android/etc.'s return gfx::ImageSkia(); >> #endif >> > > Moved to a single file as implementation of GetWindowIcon on chromeos, > and thus UI part will look platform independent. > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:32: > #elif defined(OS_CHROMEOS) > On 2016/08/23 21:37:12, msw wrote: >> Does the code here actually belong in the GetWindowIcon impl for cros? > > N/A now. > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:38: > if (icon_image_ptr) return *icon_image_ptr; > On 2016/08/23 21:37:12, msw wrote: >> No return on the same line as if; use 'git cl format'. > > Done. > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:40: > // The default icon (for chrome window itself) does not enter window > tree > On 2016/08/23 21:37:11, msw wrote: >> nit: "(for a Chrome window itself)". Also, I'm not sure what you mean > by "does >> not enter window tree host", do you mean "is not a member of the > window tree >> host"? if so, that seems odd... do you just mean that we never call >> RegisterAuraWindow for browser windows? if so, is that something we > can fix >> instead? > > Changed to return empty icon if not aura window. > > For chrome window itself, its aura::Window instance does not contain a > icon. I just found the corresponding NativeWidgetAura::SetWindowIcons > does not get called at all, which is the place where we intercept the > icon image. > > Let me ask aura guys to see whether it is easy to inject icon into > aura::Window for chrome windows, or it is ok to do it here. > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:43: > icon_image_ptr = > rb.GetNativeImageNamed(IDR_PRODUCT_LOGO_32).ToImageSkia(); > On 2016/08/23 21:37:12, msw wrote: >> Is it possible that something other than a chrome window couldn't be > found by >> DesktopMediaID::GetAuraWindowById? If so, does it actually make sense > to use a >> chrome logo for this unidentified window? That seems wrong. > > For ChromeOS, everything is aura window, so no such worry. > For other OS, we do not use this code path, but use OS api directly. > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:44: > if (icon_image_ptr) return *icon_image_ptr; > On 2016/08/23 21:37:12, msw wrote: >> Ditto: No return on the same line as if; use 'git cl format'. > > Done. > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > File > chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc > (right): > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:56: > icon_view_->set_interactive(false); > On 2016/08/23 21:37:12, msw wrote: >> nit: order before image_view_->set_interactive(false); to match other > ordering. > > Done. > > https://codereview.chromium.org/2270543003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:140: > icon_view_->SetImageSize(gfx::Size(style_.icon_rect.width(), > On 2016/08/23 21:37:12, msw wrote: >> nit: icon_view_->SetImageSize(style_.icon_rect.size()); > > Done. > > https://codereview.chromium.org/2270543003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yeah, I found BrowserView::GetWindowIcon returns empty icon as default icon. So I think it relies on the caller to replace empty icon with product logo for actual display? If so, I think we are fine to let UI display product logo for empty icon. BTW, can you take a review of the files outside chrome/browser/ui folder? As msw@ does not own those files. Thanks.
https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_chromeos.cc (right): https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_chromeos.cc:23: // The default icon (for chrome window itself) does not enter window tree Can you clarify this comment? The current phrasing is strange. Consider something like "Chrome browser windows do not set an icon. Use a Chrome icon for any window without an icon." I still think we should somehow check that this is only done for actual Chrome browser windows. https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:12: int ErrorHandler(Display* display, XErrorEvent* event) { Why is this needed? Can you add an explanatory comment? https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:35: XSetErrorHandler(nullptr); Cache the old handler and restore it afterwards, like other callers: auto old_error_handler = XSetErrorHandler(&ErrorHandler); ... XSetErrorHandler(old_error_handler);
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...
Improved comments. https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_chromeos.cc (right): https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_chromeos.cc:23: // The default icon (for chrome window itself) does not enter window tree On 2016/08/24 17:01:28, msw wrote: > Can you clarify this comment? The current phrasing is strange. Consider > something like "Chrome browser windows do not set an icon. Use a Chrome icon for > any window without an icon." I still think we should somehow check that this is > only done for actual Chrome browser windows. Done. https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:12: int ErrorHandler(Display* display, XErrorEvent* event) { On 2016/08/24 17:01:28, msw wrote: > Why is this needed? Can you add an explanatory comment? Done. https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:35: XSetErrorHandler(nullptr); On 2016/08/24 17:01:28, msw wrote: > Cache the old handler and restore it afterwards, like other callers: > auto old_error_handler = XSetErrorHandler(&ErrorHandler); > ... > XSetErrorHandler(old_error_handler); Is this quite useful here? The error handler is just a function pointer. &ErrorHandler is a const. Doing caching is just like store a const in a variable, and reuse it later.
https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:35: XSetErrorHandler(nullptr); On 2016/08/24 18:03:30, qiangchenC wrote: > On 2016/08/24 17:01:28, msw wrote: > > Cache the old handler and restore it afterwards, like other callers: > > auto old_error_handler = XSetErrorHandler(&ErrorHandler); > > ... > > XSetErrorHandler(old_error_handler); > > Is this quite useful here? The error handler is just a function pointer. > &ErrorHandler is a const. Doing caching is just like store a const in a > variable, and reuse it later. My thought is that there may be some pre-existing error handler before you call XSetErrorHandler, but when you're all done here, you set it to null, instead of restoring the pre-existing error handler.
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...
Done. https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/80001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:35: XSetErrorHandler(nullptr); On 2016/08/24 18:13:56, msw wrote: > On 2016/08/24 18:03:30, qiangchenC wrote: > > On 2016/08/24 17:01:28, msw wrote: > > > Cache the old handler and restore it afterwards, like other callers: > > > auto old_error_handler = XSetErrorHandler(&ErrorHandler); > > > ... > > > XSetErrorHandler(old_error_handler); > > > > Is this quite useful here? The error handler is just a function pointer. > > &ErrorHandler is a const. Doing caching is just like store a const in a > > variable, and reuse it later. > > My thought is that there may be some pre-existing error handler before you call > XSetErrorHandler, but when you're all done here, you set it to null, instead of > restoring the pre-existing error handler. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2270543003/diff/120001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_chromeos.cc (right): https://codereview.chromium.org/2270543003/diff/120001/chrome/browser/media/w... chrome/browser/media/window_icon_util_chromeos.cc:29: icon_image_ptr = rb.GetNativeImageNamed(IDR_PRODUCT_LOGO_32).ToImageSkia(); REPEAT: I still think we should somehow check that this is only done for actual Chrome browser windows. (unless we actually want to show a chrome icon for any other window that doesn't set an icon)
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Did a detection between browser and app. Thanks.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
okay, lgtm i suppose
https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_chromeos.cc (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_chromeos.cc:16: DCHECK(id.type == content::DesktopMediaID::TYPE_WINDOW); DCHECK_EQ https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_chromeos.cc:45: icon_image_ptr = rb.GetImageSkiaNamed(idr); GetImageSkiaNamed always returns non-null. https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:12: // The error handler is for XGetWindowProperty. Otherwise, XGetWindowProperty Are the changes in this file related to this patch? https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:31: int columns; These might mean something to you, but as someone new to the code they mean nothing to me. Please add descriptions.
Fixed. https://codereview.chromium.org/2270543003/diff/120001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_chromeos.cc (right): https://codereview.chromium.org/2270543003/diff/120001/chrome/browser/media/w... chrome/browser/media/window_icon_util_chromeos.cc:29: icon_image_ptr = rb.GetNativeImageNamed(IDR_PRODUCT_LOGO_32).ToImageSkia(); On 2016/08/24 20:33:06, msw wrote: > REPEAT: I still think we should somehow check that this is only done for actual > Chrome browser windows. (unless we actually want to show a chrome icon for any > other window that doesn't set an icon) Done. https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_chromeos.cc (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_chromeos.cc:16: DCHECK(id.type == content::DesktopMediaID::TYPE_WINDOW); On 2016/08/25 15:44:51, sky wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_chromeos.cc:45: icon_image_ptr = rb.GetImageSkiaNamed(idr); On 2016/08/25 15:44:51, sky wrote: > GetImageSkiaNamed always returns non-null. Done. https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:12: // The error handler is for XGetWindowProperty. Otherwise, XGetWindowProperty On 2016/08/25 15:44:51, sky wrote: > Are the changes in this file related to this patch? Yes. As I tested in real case, sometimes crash. The reason is stated in the comment. https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:31: int columns; On 2016/08/25 15:44:51, sky wrote: > These might mean something to you, but as someone new to the code they mean > nothing to me. Please add descriptions. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 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...
sky@chromium.org changed reviewers: + derat@chromium.org, erg@chromium.org
+derat/+erg for changes to chrome/browser/media/window_icon_util_x11.cc . https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:12: // The error handler is for XGetWindowProperty. Otherwise, XGetWindowProperty On 2016/08/25 17:07:59, qiangchenC wrote: > On 2016/08/25 15:44:51, sky wrote: > > Are the changes in this file related to this patch? > > Yes. As I tested in real case, sometimes crash. The reason is stated in the > comment. Having to replace the error handler and ignore the value seems like a good indication you are doing something wrong. Especially when the failure case on 38 leaves your error handler installed. From what I can tell none of the other code in chrome that looks up properties has hacks like this. Your comment doesn't explain the error you were encountering, so I can't help diagnose it. I'm adding two reviewers more familiar with X.
replied. And I moved the taking default icon code to UI code, because presubmit trybot shows it is an error to include "chrome/browser/ui/views" from "chrome/browser/media" folder. https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:12: // The error handler is for XGetWindowProperty. Otherwise, XGetWindowProperty On 2016/08/25 19:44:17, sky wrote: > On 2016/08/25 17:07:59, qiangchenC wrote: > > On 2016/08/25 15:44:51, sky wrote: > > > Are the changes in this file related to this patch? > > > > Yes. As I tested in real case, sometimes crash. The reason is stated in the > > comment. > > Having to replace the error handler and ignore the value seems like a good > indication you are doing something wrong. Especially when the failure case on 38 > leaves your error handler installed. From what I can tell none of the other code > in chrome that looks up properties has hacks like this. > > Your comment doesn't explain the error you were encountering, so I can't help > diagnose it. I'm adding two reviewers more familiar with X. The |id.id| in the call of XGetWindowProperty is actually the window handler. If the handler is invalid, XGetWindowProperty would throw an exception. One case to trigger this is that window gets closed before XGetWindowProperty gets called. Just want to make sure failure to get icon would not block normal workflow. BTW, without doing this, it will definitely crash the unittest, as the unittest just makes fake id. Or do you think we just need to set error handler for unittest to suppress its crash?
Patchset #9 (id:180001) has been deleted
One more investigation on XSetErrorHandler in unittest. https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:12: // The error handler is for XGetWindowProperty. Otherwise, XGetWindowProperty On 2016/08/25 20:46:58, qiangchenC wrote: > On 2016/08/25 19:44:17, sky wrote: > > On 2016/08/25 17:07:59, qiangchenC wrote: > > > On 2016/08/25 15:44:51, sky wrote: > > > > Are the changes in this file related to this patch? > > > > > > Yes. As I tested in real case, sometimes crash. The reason is stated in the > > > comment. > > > > Having to replace the error handler and ignore the value seems like a good > > indication you are doing something wrong. Especially when the failure case on > 38 > > leaves your error handler installed. From what I can tell none of the other > code > > in chrome that looks up properties has hacks like this. > > > > Your comment doesn't explain the error you were encountering, so I can't help > > diagnose it. I'm adding two reviewers more familiar with X. > > The |id.id| in the call of XGetWindowProperty is actually the window handler. If > the handler is invalid, XGetWindowProperty would throw an exception. One case to > trigger this is that window gets closed before XGetWindowProperty gets called. > Just want to make sure failure to get icon would not block normal workflow. BTW, > without doing this, it will definitely crash the unittest, as the unittest just > makes fake id. Or do you think we just need to set error handler for unittest to > suppress its crash? A little more investigation when I try to SetErrorHandler in unittest. Xlib.h has many name conflicts with our testing frame work "gtest.h", and thus if I include both .h files in, it cannot compile.
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 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/2270543003/diff/220001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/220001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:32: XErrorHandler old_handler = XSetErrorHandler(&ErrorHandler); please use ui/gfx/x/x11_error_tracker.h instead of writing your own code for this. https://codereview.chromium.org/2270543003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/2270543003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:186: if (window) icon_image = LoadDefaultIcon(window); please see the style guide. the body should be on its own line
Fixed. Now using existing error tracker to accomplish the crash suppressor. https://codereview.chromium.org/2270543003/diff/220001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/220001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:32: XErrorHandler old_handler = XSetErrorHandler(&ErrorHandler); On 2016/08/25 23:39:58, Daniel Erat wrote: > please use ui/gfx/x/x11_error_tracker.h instead of writing your own code for > this. Done. https://codereview.chromium.org/2270543003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/2270543003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:186: if (window) icon_image = LoadDefaultIcon(window); On 2016/08/25 23:39:58, Daniel Erat wrote: > please see the style guide. the body should be on its own line 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...
The CQ bit was unchecked by qiangchen@chromium.org
https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:24: gfx::X11ErrorTracker error_tracker; this should probably be in an std::unique_ptr so you can destroy it after the Xlib call that it's protecting.
Dan, does it seem right to have to catch an error like this when getting a property? -Scott On Thu, Aug 25, 2016 at 6:50 PM, <derat@chromium.org> wrote: > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > File chrome/browser/media/window_icon_util_x11.cc (right): > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > chrome/browser/media/window_icon_util_x11.cc:24: gfx::X11ErrorTracker > error_tracker; > this should probably be in an std::unique_ptr so you can destroy it > after the Xlib call that it's protecting. > > https://codereview.chromium.org/2270543003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
unfortunately, yes. this is something that comes up in X11 window managers as well. an X client can destroy its windows at any time, with other clients (window manager, chrome, etc.) receiving notification of it in the form of an event. since there's no synchronization between different clients' communication with the X server, this window may already be gone (without chrome having had any way to learn about that) by the time that the server receives chrome's request containing the window's ID. this will result in chrome receiving an error. the obvious-seeming way to prevent this would be for chrome to grab the server, synchronize with it and drain its local event queue (to make sure it hears about the window being destroyed, if that happened), query the property if the window is still around, and then ungrab the server. but you should never do that since grabbing the server blocks all other clients (potentially indefinitely if there's a bug in your code). so everyone usually ends up just ignoring errors like this instead. i think it's actually more common to install a custom error handler that ignores all errors by default but gives you a way to check if an error has occurred between two points in time. i guess we don't do that in chrome, though. luckily, unavoidable X errors like these don't really happen unless you're sending requests to the X server that refer to other clients' resources (which is common in window managers but hopefully only happens in rare cases like this one in chrome). On 2016/08/26 03:27:25, sky wrote: > Dan, does it seem right to have to catch an error like this when > getting a property? > > -Scott > > On Thu, Aug 25, 2016 at 6:50 PM, <mailto:derat@chromium.org> wrote: > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > File chrome/browser/media/window_icon_util_x11.cc (right): > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > chrome/browser/media/window_icon_util_x11.cc:24: gfx::X11ErrorTracker > > error_tracker; > > this should probably be in an std::unique_ptr so you can destroy it > > after the Xlib call that it's protecting. > > > > https://codereview.chromium.org/2270543003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
but i'd expect this to be extremely rare in this particular case (although it can always potentially happen and we need to guard against it). qiang, have you actually seen this crash happen? does chrome update the displayed list of windows whenever a window is closed, or do we just show a static list, making it easy to repro this by bringing up the picker, closing some other app, and then clicking on its window in the picker? On 2016/08/26 04:41:39, Daniel Erat wrote: > unfortunately, yes. this is something that comes up in X11 window managers as > well. an X client can destroy its windows at any time, with other clients > (window manager, chrome, etc.) receiving notification of it in the form of an > event. since there's no synchronization between different clients' communication > with the X server, this window may already be gone (without chrome having had > any way to learn about that) by the time that the server receives chrome's > request containing the window's ID. this will result in chrome receiving an > error. > > the obvious-seeming way to prevent this would be for chrome to grab the server, > synchronize with it and drain its local event queue (to make sure it hears about > the window being destroyed, if that happened), query the property if the window > is still around, and then ungrab the server. but you should never do that since > grabbing the server blocks all other clients (potentially indefinitely if > there's a bug in your code). > > so everyone usually ends up just ignoring errors like this instead. i think it's > actually more common to install a custom error handler that ignores all errors > by default but gives you a way to check if an error has occurred between two > points in time. i guess we don't do that in chrome, though. luckily, unavoidable > X errors like these don't really happen unless you're sending requests to the X > server that refer to other clients' resources (which is common in window > managers but hopefully only happens in rare cases like this one in chrome). > > On 2016/08/26 03:27:25, sky wrote: > > Dan, does it seem right to have to catch an error like this when > > getting a property? > > > > -Scott > > > > On Thu, Aug 25, 2016 at 6:50 PM, <mailto:derat@chromium.org> wrote: > > > > > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > > File chrome/browser/media/window_icon_util_x11.cc (right): > > > > > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > > chrome/browser/media/window_icon_util_x11.cc:24: gfx::X11ErrorTracker > > > error_tracker; > > > this should probably be in an std::unique_ptr so you can destroy it > > > after the Xlib call that it's protecting. > > > > > > https://codereview.chromium.org/2270543003/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > >
On 2016/08/26 04:45:13, Daniel Erat wrote: > but i'd expect this to be extremely rare in this particular case (although it > can always potentially happen and we need to guard against it). > > qiang, have you actually seen this crash happen? does chrome update the > displayed list of windows whenever a window is closed, or do we just show a > static list, making it easy to repro this by bringing up the picker, closing > some other app, and then clicking on its window in the picker? > > On 2016/08/26 04:41:39, Daniel Erat wrote: > > unfortunately, yes. this is something that comes up in X11 window managers as > > well. an X client can destroy its windows at any time, with other clients > > (window manager, chrome, etc.) receiving notification of it in the form of an > > event. since there's no synchronization between different clients' > communication > > with the X server, this window may already be gone (without chrome having had > > any way to learn about that) by the time that the server receives chrome's > > request containing the window's ID. this will result in chrome receiving an > > error. > > > > the obvious-seeming way to prevent this would be for chrome to grab the > server, > > synchronize with it and drain its local event queue (to make sure it hears > about > > the window being destroyed, if that happened), query the property if the > window > > is still around, and then ungrab the server. but you should never do that > since > > grabbing the server blocks all other clients (potentially indefinitely if > > there's a bug in your code). > > > > so everyone usually ends up just ignoring errors like this instead. i think > it's > > actually more common to install a custom error handler that ignores all errors > > by default but gives you a way to check if an error has occurred between two > > points in time. i guess we don't do that in chrome, though. luckily, > unavoidable > > X errors like these don't really happen unless you're sending requests to the > X > > server that refer to other clients' resources (which is common in window > > managers but hopefully only happens in rare cases like this one in chrome). > > > > On 2016/08/26 03:27:25, sky wrote: > > > Dan, does it seem right to have to catch an error like this when > > > getting a property? > > > > > > -Scott > > > > > > On Thu, Aug 25, 2016 at 6:50 PM, <mailto:derat@chromium.org> wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > > > File chrome/browser/media/window_icon_util_x11.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > > > chrome/browser/media/window_icon_util_x11.cc:24: gfx::X11ErrorTracker > > > > error_tracker; > > > > this should probably be in an std::unique_ptr so you can destroy it > > > > after the Xlib call that it's protecting. > > > > > > > > https://codereview.chromium.org/2270543003/ > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > We do update the list dynamically, but we cannot detect a window close immediately when it is closed, actually we set update period to be 1s to not overload the system. It means for every second, we re-query the OS to update the window list, and then pull data (like title, icon, preview etc) for each window.
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...
Fixed. Thanks. https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:24: gfx::X11ErrorTracker error_tracker; On 2016/08/26 01:50:39, Daniel Erat wrote: > this should probably be in an std::unique_ptr so you can destroy it after the > Xlib call that it's protecting. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:24: std::unique_ptr<gfx::X11ErrorTracker> error_tracker( Please add a comment here, something similar to Dan's explanation here. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:29: #if defined(OS_CHROMEOS) I think you want USE_ASH here and below. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:30: // As aura framework uses empty icon to represent default icon, it relies on aura here is misleading because aura is used on linux and windows too. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:43: bool is_app = browser ? browser->is_app() && !browser->is_devtools() : true; I don't think you need the ternary here, it just makes it harder to read: is_app = browser && browser->is_app() && !browser->is_devtoops(); https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h (right): https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:31: // This parameter controls how many source view can be displayed in a row. source views? https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:34: // The size of a single item. How does columns and item_size relate? In the description of columns you say 'source view'. Is that the same as an item? https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:43: // When a source is selected, we paint the border to show it. This parameter This is a good description!
On 2016/08/26 15:43:41, qiangchenC wrote: > On 2016/08/26 04:45:13, Daniel Erat wrote: > > but i'd expect this to be extremely rare in this particular case (although it > > can always potentially happen and we need to guard against it). > > > > qiang, have you actually seen this crash happen? does chrome update the > > displayed list of windows whenever a window is closed, or do we just show a > > static list, making it easy to repro this by bringing up the picker, closing > > some other app, and then clicking on its window in the picker? > > > > On 2016/08/26 04:41:39, Daniel Erat wrote: > > > unfortunately, yes. this is something that comes up in X11 window managers > as > > > well. an X client can destroy its windows at any time, with other clients > > > (window manager, chrome, etc.) receiving notification of it in the form of > an > > > event. since there's no synchronization between different clients' > > communication > > > with the X server, this window may already be gone (without chrome having > had > > > any way to learn about that) by the time that the server receives chrome's > > > request containing the window's ID. this will result in chrome receiving an > > > error. > > > > > > the obvious-seeming way to prevent this would be for chrome to grab the > > server, > > > synchronize with it and drain its local event queue (to make sure it hears > > about > > > the window being destroyed, if that happened), query the property if the > > window > > > is still around, and then ungrab the server. but you should never do that > > since > > > grabbing the server blocks all other clients (potentially indefinitely if > > > there's a bug in your code). > > > > > > so everyone usually ends up just ignoring errors like this instead. i think > > it's > > > actually more common to install a custom error handler that ignores all > errors > > > by default but gives you a way to check if an error has occurred between two > > > points in time. i guess we don't do that in chrome, though. luckily, > > unavoidable > > > X errors like these don't really happen unless you're sending requests to > the > > X > > > server that refer to other clients' resources (which is common in window > > > managers but hopefully only happens in rare cases like this one in chrome). > > > > > > On 2016/08/26 03:27:25, sky wrote: > > > > Dan, does it seem right to have to catch an error like this when > > > > getting a property? > > > > > > > > -Scott > > > > > > > > On Thu, Aug 25, 2016 at 6:50 PM, <mailto:derat@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > > > > File chrome/browser/media/window_icon_util_x11.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > > > > chrome/browser/media/window_icon_util_x11.cc:24: gfx::X11ErrorTracker > > > > > error_tracker; > > > > > this should probably be in an std::unique_ptr so you can destroy it > > > > > after the Xlib call that it's protecting. > > > > > > > > > > https://codereview.chromium.org/2270543003/ > > > > > > > > -- > > > > You received this message because you are subscribed to the Google Groups > > > > "Chromium-reviews" group. > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > We do update the list dynamically, but we cannot detect a window close > immediately when it is closed, actually we set update period to be 1s to not > overload the system. > It means for every second, we re-query the OS to update the window list, and > then pull data (like title, icon, preview etc) for each window. i see. it'd probably be possible to detect windows being created or destroyed by selecting SubstructureNotifyMask on the root window to watch for CreateNotify and DestroyNotify events (probably you'd actually want to use MapNotify and UnmapNotify, though), but i see that third_party/webrtc/modules/desktop_capture/window_capturer_x11.cc just calls XQueryTree once per second right now. that's inefficient but probably not a big deal if this only happens while the window-picker is displayed.
erg@chromium.org changed reviewers: + thomasanderson@chromium.org
+thomasanderson fyi Thomas was working on an x11 window tree/property cache so we didn't have to roundtrip to the server. It's currently ignoring icons, but maybe that restriction could be lifted?
qiangchen@chromium.org changed reviewers: - thomasanderson@chromium.org
Fixed. Thanks. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:24: std::unique_ptr<gfx::X11ErrorTracker> error_tracker( On 2016/08/26 16:40:35, sky wrote: > Please add a comment here, something similar to Dan's explanation here. Done. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:29: #if defined(OS_CHROMEOS) On 2016/08/26 16:40:35, sky wrote: > I think you want USE_ASH here and below. Done. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:30: // As aura framework uses empty icon to represent default icon, it relies on On 2016/08/26 16:40:35, sky wrote: > aura here is misleading because aura is used on linux and windows too. Moved part of the comment below. Here we just need to explain what this function does, no need to explain why call this function. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:43: bool is_app = browser ? browser->is_app() && !browser->is_devtools() : true; On 2016/08/26 16:40:35, sky wrote: > I don't think you need the ternary here, it just makes it harder to read: > > is_app = browser && browser->is_app() && !browser->is_devtoops(); Done. But it should be !browser || (....) https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h (right): https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:31: // This parameter controls how many source view can be displayed in a row. On 2016/08/26 16:40:35, sky wrote: > source views? change all "items" and "sources" to be "source item", and remarked that source items are of DesktopMediaSourceView. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:34: // The size of a single item. On 2016/08/26 16:40:35, sky wrote: > How does columns and item_size relate? In the description of columns you say > 'source view'. Is that the same as an item? same. https://codereview.chromium.org/2270543003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:43: // When a source is selected, we paint the border to show it. This parameter On 2016/08/26 16:40:35, sky wrote: > This is a good description! Acknowledged.
Is "x11 window tree/property" a structure managing chrome windows? My GetWindowIcon function needs to work for all windows, including non-chrome windows. So I think it is impossible to kill the usage of XGetWindowProperty completely. On Fri, Aug 26, 2016 at 10:29 AM, <erg@chromium.org> wrote: > +thomasanderson fyi > > Thomas was working on an x11 window tree/property cache so we didn't have > to > roundtrip to the server. It's currently ignoring icons, but maybe that > restriction could be lifted? > > https://codereview.chromium.org/2270543003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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...
On 2016/08/26 17:45:18, chromium-reviews wrote: > Is "x11 window tree/property" a structure managing chrome windows? > > My GetWindowIcon function needs to work for all windows, including > non-chrome windows. So I think it is impossible to kill the usage of > XGetWindowProperty completely. Yes, it is tracking all windows including non-chrome windows. However, the cache doesn't exist yet in the chrome tree, and probably won't for a little while longer, and Tom might object to the memory overhead of caching all icons. To be clear, window_icon_util_x11.cc with the new description comment and use of the X11ErrorTracker lgtm. I'm just pointing out that in the future, there will be a better way to do this along the lines of what Dan was musing about.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, but wait for Dan's approval. https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:24: // The |error_tracker| essentially provides an empty X error handler for Excellent description. Thanks!
On 2016/08/26 18:12:02, Elliot Glaysher wrote: > On 2016/08/26 17:45:18, chromium-reviews wrote: > > Is "x11 window tree/property" a structure managing chrome windows? > > > > My GetWindowIcon function needs to work for all windows, including > > non-chrome windows. So I think it is impossible to kill the usage of > > XGetWindowProperty completely. > > Yes, it is tracking all windows including non-chrome windows. However, the cache > doesn't exist yet in the chrome tree, and probably won't for a little while > longer, and Tom might object to the memory overhead of caching all icons. > Now that we have a use case for _NET_WM_ICON, I guess we can include that in the cache, but only for ChromeOS. However, like erg@ pointed out, the cache is cache is not in the tree yet, and I wouldn't hold out it for this CL :) > To be clear, window_icon_util_x11.cc with the new description comment and use of > the X11ErrorTracker lgtm. I'm just pointing out that in the future, there will > be a better way to do this along the lines of what Dan was musing about.
On 2016/08/26 17:09:32, Daniel Erat wrote: > On 2016/08/26 15:43:41, qiangchenC wrote: > > On 2016/08/26 04:45:13, Daniel Erat wrote: > > > but i'd expect this to be extremely rare in this particular case (although > it > > > can always potentially happen and we need to guard against it). > > > > > > qiang, have you actually seen this crash happen? does chrome update the > > > displayed list of windows whenever a window is closed, or do we just show a > > > static list, making it easy to repro this by bringing up the picker, closing > > > some other app, and then clicking on its window in the picker? > > > > > > On 2016/08/26 04:41:39, Daniel Erat wrote: > > > > unfortunately, yes. this is something that comes up in X11 window managers > > as > > > > well. an X client can destroy its windows at any time, with other clients > > > > (window manager, chrome, etc.) receiving notification of it in the form of > > an > > > > event. since there's no synchronization between different clients' > > > communication > > > > with the X server, this window may already be gone (without chrome having > > had > > > > any way to learn about that) by the time that the server receives chrome's > > > > request containing the window's ID. this will result in chrome receiving > an > > > > error. > > > > > > > > the obvious-seeming way to prevent this would be for chrome to grab the > > > server, > > > > synchronize with it and drain its local event queue (to make sure it hears > > > about > > > > the window being destroyed, if that happened), query the property if the > > > window > > > > is still around, and then ungrab the server. but you should never do that > > > since > > > > grabbing the server blocks all other clients (potentially indefinitely if > > > > there's a bug in your code). > > > > > > > > so everyone usually ends up just ignoring errors like this instead. i > think > > > it's > > > > actually more common to install a custom error handler that ignores all > > errors > > > > by default but gives you a way to check if an error has occurred between > two > > > > points in time. i guess we don't do that in chrome, though. luckily, > > > unavoidable > > > > X errors like these don't really happen unless you're sending requests to > > the > > > X > > > > server that refer to other clients' resources (which is common in window > > > > managers but hopefully only happens in rare cases like this one in > chrome). > > > > > > > > On 2016/08/26 03:27:25, sky wrote: > > > > > Dan, does it seem right to have to catch an error like this when > > > > > getting a property? > > > > > > > > > > -Scott > > > > > > > > > > On Thu, Aug 25, 2016 at 6:50 PM, <mailto:derat@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > > > > > File chrome/browser/media/window_icon_util_x11.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2270543003/diff/240001/chrome/browser/media/w... > > > > > > chrome/browser/media/window_icon_util_x11.cc:24: gfx::X11ErrorTracker > > > > > > error_tracker; > > > > > > this should probably be in an std::unique_ptr so you can destroy it > > > > > > after the Xlib call that it's protecting. > > > > > > > > > > > > https://codereview.chromium.org/2270543003/ > > > > > > > > > > -- > > > > > You received this message because you are subscribed to the Google > Groups > > > > > "Chromium-reviews" group. > > > > > To unsubscribe from this group and stop receiving emails from it, send > an > > > > email > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > We do update the list dynamically, but we cannot detect a window close > > immediately when it is closed, actually we set update period to be 1s to not > > overload the system. > > It means for every second, we re-query the OS to update the window list, and > > then pull data (like title, icon, preview etc) for each window. > > i see. it'd probably be possible to detect windows being created or destroyed by > selecting SubstructureNotifyMask on the root window to watch for CreateNotify > and DestroyNotify events (probably you'd actually want to use MapNotify and > UnmapNotify, though), but i see that > third_party/webrtc/modules/desktop_capture/window_capturer_x11.cc just calls > XQueryTree once per second right now. that's inefficient but probably not a big > deal if this only happens while the window-picker is displayed. Thanks for your suggestion, but this seems a big structural change from window capturer in webrtc, and thus out of this CL's scope. I think one reason we did not do that way is that we want to make webrtc::WindowCapturer a common interface across the operating systems, and not all OS support this feature to let an application listen to another application's state change event. Any other thing you want to mention? Or can you sign off this CL?
https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:24: // The |error_tracker| essentially provides an empty X error handler for On 2016/08/26 19:38:36, sky wrote: > Excellent description. Thanks! Acknowledged.
lgtm for window_icon_util_x11.cc
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2270543003/#ps280001 (title: "Improve Comments")
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 ========== Display Window Icon In Picker UI In this CL, we display the window icon with the window source on the picker window for desktop capture. BUG=631604 ========== to ========== Display Window Icon In Picker UI In this CL, we display the window icon with the window source on the picker window for desktop capture. BUG=631604 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Display Window Icon In Picker UI In this CL, we display the window icon with the window source on the picker window for desktop capture. BUG=631604 ========== to ========== Display Window Icon In Picker UI In this CL, we display the window icon with the window source on the picker window for desktop capture. BUG=631604 Committed: https://crrev.com/e7a9525b5e292c45f8d6db8091afed4ea5045aff Cr-Commit-Position: refs/heads/master@{#415199} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/e7a9525b5e292c45f8d6db8091afed4ea5045aff Cr-Commit-Position: refs/heads/master@{#415199}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:30: std::unique_ptr<gfx::X11ErrorTracker> error_tracker( Can this just be the following? { gfx::X11ErrorTracker error_tracker; int status = XGetWindowProperty(...); } https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:17: #include "grit/theme_resources.h" BTW, we are trying to ever-so-slowly get rid of using the unqualified path. This should have been chrome/grit/theme_resources.h. I'll fix this in https://codereview.chromium.org/2290753004/, but please try not to add more. https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:47: #endif extra blank line after would be nice to mirror line 26.
Message was sent while issue was closed.
https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:30: std::unique_ptr<gfx::X11ErrorTracker> error_tracker( On 2016/08/30 09:39:34, Lei Zhang wrote: > Can this just be the following? > > { > gfx::X11ErrorTracker error_tracker; > int status = XGetWindowProperty(...); > } By other reviewer's comment, as we want to release it asap, and thus, we set it in a unique_ptr. Otherwise, we have to wait till the end of this function. https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:17: #include "grit/theme_resources.h" On 2016/08/30 09:39:34, Lei Zhang wrote: > BTW, we are trying to ever-so-slowly get rid of using the unqualified path. This > should have been chrome/grit/theme_resources.h. > > I'll fix this in https://codereview.chromium.org/2290753004/, but please try not > to add more. Acknowledged. https://codereview.chromium.org/2270543003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:47: #endif On 2016/08/30 09:39:34, Lei Zhang wrote: > extra blank line after would be nice to mirror line 26. Thanks. Will fix it next time I touch this file. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
