Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246923002:
Icon Capture Utility For Linux, Windows and Mac (Closed)
|
Created:
4 years, 4 months ago by qiangchen Modified:
4 years, 4 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIcon Capture Utility For Linux, Windows and Mac
For better usability, we wants to display icon for window capture.
In this CL, we wrote utility classes which calls OS API to extract window icon images, and provide them to Chromium.
In the future CL, we will use display the icon onto the UI.
BUG=631604
Committed: https://crrev.com/da7b21cac1ded0c70cb330e4f76a1f7915e38ccf
Cr-Commit-Position: refs/heads/master@{#413483}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Static Function #
Total comments: 20
Patch Set 3 : Comment Fix #
Total comments: 2
Patch Set 4 : Comment Fix #
Messages
Total messages: 37 (27 generated)
Description was changed from ========== Icon Capture Utility For Linux, Windows and Mac Icon Capture Linux In# BUG= ========== to ========== Icon Capture Utility For Linux, Windows and Mac For better usability, we wants to display icon for window capture. In this CL, we wrote utility classes which calls OS API to extract window icon images, and provide them to Chromium. In the future CL, we will use display the icon onto the UI. BUG=631604 ==========
qiangchen@chromium.org changed reviewers: + sergeyu@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Move the code to chromium.
https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util.h (right): https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util.h:17: const webrtc::DesktopCaptureOptions& options); I don't think there is any reason to depend on webrtc::DesktopCaptureOptions here. You are currently use it to pass X11 connection to the X11 version of the capturer. It would be cleaner to just reuse the X11 connection that chrome already has, i.e. gfx::GetXDisplay(). Then you wouldn't even need this as an interface, and instead you would have a simple function that returns a window icon.. https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_win.cc (right): https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_win.cc:20: if (icon_handle == NULL) use nullptr instead of NULL. https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:42: long width, height; These are not set before being used below https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:44: unsigned long i = 0; Please don't use types like "unsigned long" and "long". See https://google.github.io/styleguide/cppguide.html#Integer_Types I think simple int would be appropriate for all these variables. https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:51: i = i + 2 + data[i] * data[i + 1]; It's very confusing what happens here. Is it that the returned value may contain multiple icons and we want the last one? At very least this logic deserves a comment above, but I think this code can be simplified as well. There are some error cases that are not handled properly. For example with size = 0 uninitialized width/height are used below. When size=1 this code will try to use data[1], which is outside of the buffer. Why is it always the last icon in the list? Is it guaranteed to be the biggest? https://codereview.chromium.org/2246923002/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2246923002/diff/60001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:380: 'browser/media/window_icon_util.h', gyp is no longer supported. Please update GN files. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NZkPr-CXvQ0/dn1rb...
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Patchset #2 (id:120001) has been deleted
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.
Fixed and replied, thanks. https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util.h (right): https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util.h:17: const webrtc::DesktopCaptureOptions& options); On 2016/08/15 22:28:39, Sergey Ulanov wrote: > I don't think there is any reason to depend on webrtc::DesktopCaptureOptions > here. You are currently use it to pass X11 connection to the X11 version of the > capturer. It would be cleaner to just reuse the X11 connection that chrome > already has, i.e. gfx::GetXDisplay(). Then you wouldn't even need this as an > interface, and instead you would have a simple function that returns a window > icon.. Done. https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_win.cc (right): https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_win.cc:20: if (icon_handle == NULL) On 2016/08/15 22:28:39, Sergey Ulanov wrote: > use nullptr instead of NULL. Done. https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:42: long width, height; On 2016/08/15 22:28:39, Sergey Ulanov wrote: > These are not set before being used below Done. https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:44: unsigned long i = 0; On 2016/08/15 22:28:39, Sergey Ulanov wrote: > Please don't use types like "unsigned long" and "long". See > https://google.github.io/styleguide/cppguide.html#Integer_Types > > I think simple int would be appropriate for all these variables. It is due to weird setting of linux for XGetProperty. For 32-bit machine, it uses an int32_t to represent a pixel, while for 64-bit machine, it uses an int64_t to represent a pixel, though only the lower 32 bits really contain information. That's why we need to set |data| to "long *". We can change the type for width and height, and then a cast will be needed to take out data. https://codereview.chromium.org/2246923002/diff/60001/chrome/browser/media/wi... chrome/browser/media/window_icon_util_x11.cc:51: i = i + 2 + data[i] * data[i + 1]; On 2016/08/15 22:28:39, Sergey Ulanov wrote: > It's very confusing what happens here. Is it that the returned value may contain > multiple icons and we want the last one? At very least this logic deserves a > comment above, but I think this code can be simplified as well. There are some > error cases that are not handled properly. For example with size = 0 > uninitialized width/height are used below. When size=1 this code will try to use > data[1], which is outside of the buffer. Why is it always the last icon in the > list? Is it guaranteed to be the biggest? The |size| is not how many icons are there, but just tells us how big the data section is. The data structure is [width1, height1, image_data for icon1, width2, height2, image data for icon2, .....] The code here took the biggest icon, not the last one. https://codereview.chromium.org/2246923002/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2246923002/diff/60001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:380: 'browser/media/window_icon_util.h', On 2016/08/15 22:28:39, Sergey Ulanov wrote: > gyp is no longer supported. Please update GN files. > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NZkPr-CXvQ0/dn1rb... The gypi file is injected into chrome/BUILD.GN here: https://cs.chromium.org/chromium/src/chrome/browser/BUILD.gn?q=chrome_browser... And I found in chrome/BUILD.GN, it seldomly directly refers to a source file name. Possibly we should just update this gypi file so far, and let GN clean up project to take care of the migration. One concern is that I am not sure of the build file structure after migration. In case that we are going to list file names in some .gni file and import them into BUILD.GN, it is better to put our newly added files together with their neighbors.
https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util.h (right): https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util.h:12: class WindowIconUtil { Don't need a class here. Just define a simple function not a static method. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_win.cc (right): https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:10: HWND hwnd = reinterpret_cast<HWND>(id.id); DCHECK that the id.type is TYPE_WINDOW https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:14: if (icon_handle == nullptr) nit: "!icon_handle" instead of "icon_handle == nullptr" https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:15: icon_handle = (HICON)GetClassLongPtr(hwnd, GCLP_HICON); Don't use c-style casts. In this case I think you want static_cast<>. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:16: if (icon_handle == nullptr) add {} for multi-line if statement https://google.github.io/styleguide/cppguide.html#Conditionals https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:23: icon_handle = (HICON)GetClassLongPtr(hwnd, GCLP_HICONSM); static_cast<> https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:33: while (i < static_cast<int>(size)) { Please add a comment above to explain what happens here, particularly format of the data returned. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:34: if (i == 0 || static_cast<int>(data[i] * data[i + 1]) > width * height) { data[i+1] may be outside of the buffer size. Need to verify that i + 1 < size before using data[i+1]. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:37: start = i + 2; verify here that start + 2 + width*height < size https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:50: pixels_data[result.rowBytesAsPixels() * y + x] = SkPreMultiplyColor( nit: you can allocate SkBitmap of type kUnpremul_SkAlphaType, then you wouldn't need to premultiply each pixel. and this can be simple memcpy().
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.
Fixed, thanks. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util.h (right): https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util.h:12: class WindowIconUtil { On 2016/08/18 05:38:18, Sergey Ulanov wrote: > Don't need a class here. Just define a simple function not a static method. Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_win.cc (right): https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:10: HWND hwnd = reinterpret_cast<HWND>(id.id); On 2016/08/18 05:38:18, Sergey Ulanov wrote: > DCHECK that the id.type is TYPE_WINDOW Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:14: if (icon_handle == nullptr) On 2016/08/18 05:38:18, Sergey Ulanov wrote: > nit: "!icon_handle" instead of "icon_handle == nullptr" Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:15: icon_handle = (HICON)GetClassLongPtr(hwnd, GCLP_HICON); On 2016/08/18 05:38:18, Sergey Ulanov wrote: > Don't use c-style casts. In this case I think you want static_cast<>. Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:16: if (icon_handle == nullptr) On 2016/08/18 05:38:18, Sergey Ulanov wrote: > add {} for multi-line if statement > https://google.github.io/styleguide/cppguide.html#Conditionals Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_win.cc:23: icon_handle = (HICON)GetClassLongPtr(hwnd, GCLP_HICONSM); On 2016/08/18 05:38:18, Sergey Ulanov wrote: > static_cast<> Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:33: while (i < static_cast<int>(size)) { On 2016/08/18 05:38:19, Sergey Ulanov wrote: > Please add a comment above to explain what happens here, particularly format of > the data returned. Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:34: if (i == 0 || static_cast<int>(data[i] * data[i + 1]) > width * height) { On 2016/08/18 05:38:18, Sergey Ulanov wrote: > data[i+1] may be outside of the buffer size. Need to verify that i + 1 < size > before using data[i+1]. Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:37: start = i + 2; On 2016/08/18 05:38:19, Sergey Ulanov wrote: > verify here that start + 2 + width*height < size Done. https://codereview.chromium.org/2246923002/diff/140001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:50: pixels_data[result.rowBytesAsPixels() * y + x] = SkPreMultiplyColor( On 2016/08/18 05:38:19, Sergey Ulanov wrote: > nit: you can allocate SkBitmap of type kUnpremul_SkAlphaType, then you wouldn't > need to premultiply each pixel. and this can be simple memcpy(). Create SkBitmap with Unpremul_SkAlphaType. But memcpy cannot work, because we are converting long to uint32
lgtm https://codereview.chromium.org/2246923002/diff/160001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2246923002/diff/160001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:33: // number of |data| is |size|. Also mention that we want to select the biggest icon.
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/2246923002/diff/160001/chrome/browser/media/w... File chrome/browser/media/window_icon_util_x11.cc (right): https://codereview.chromium.org/2246923002/diff/160001/chrome/browser/media/w... chrome/browser/media/window_icon_util_x11.cc:33: // number of |data| is |size|. On 2016/08/20 04:24:10, Sergey Ulanov wrote: > Also mention that we want to select the biggest icon. Done.
The CQ bit was unchecked by qiangchen@chromium.org
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2246923002/#ps180001 (title: "Comment Fix")
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 ========== Icon Capture Utility For Linux, Windows and Mac For better usability, we wants to display icon for window capture. In this CL, we wrote utility classes which calls OS API to extract window icon images, and provide them to Chromium. In the future CL, we will use display the icon onto the UI. BUG=631604 ========== to ========== Icon Capture Utility For Linux, Windows and Mac For better usability, we wants to display icon for window capture. In this CL, we wrote utility classes which calls OS API to extract window icon images, and provide them to Chromium. 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:180001)
Message was sent while issue was closed.
Description was changed from ========== Icon Capture Utility For Linux, Windows and Mac For better usability, we wants to display icon for window capture. In this CL, we wrote utility classes which calls OS API to extract window icon images, and provide them to Chromium. In the future CL, we will use display the icon onto the UI. BUG=631604 ========== to ========== Icon Capture Utility For Linux, Windows and Mac For better usability, we wants to display icon for window capture. In this CL, we wrote utility classes which calls OS API to extract window icon images, and provide them to Chromium. In the future CL, we will use display the icon onto the UI. BUG=631604 Committed: https://crrev.com/da7b21cac1ded0c70cb330e4f76a1f7915e38ccf Cr-Commit-Position: refs/heads/master@{#413483} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/da7b21cac1ded0c70cb330e4f76a1f7915e38ccf Cr-Commit-Position: refs/heads/master@{#413483} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
