Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(25)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 593503003: Support error handling for Surface readbacks (Closed)

Created:
6 years, 3 months ago by sivag
Modified:
6 years, 1 month ago
CC:
chromium-reviews, nasko+codewatch_chromium.org, James Su, vsevik, creis+watch_chromium.org, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, devtools-reviews_chromium.org, jam, penghuang+watch_chromium.org, yurys, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, paulirish+reviews_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, aandrey+blink_chromium.org, pfeldman, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Support error handling for Surface read-backs. CopyFromCompositingSurface, CopyFromBackingStore return callbacks with a result value of boolean (pass or fail) and the resultant SkBitmap.But in few cases for example, we want to request output in a differnt format say RGB_565. It depends on the hardware whether it supports that texture format for readback, so on few systems CopyFromCompositingSurface may fail and return false status, with empty SkBitmap. The user of the api should be able to know the exact reason of failure,(there can be a different reasons) to take appropriate action,say in the above case FORMAT_NOT_SUPPORTED. 1.This patch defines enum to support possible failure reasons. 2.Changes the signature of readback callback by removing boolean and passing ReadbackResponse enum. BUG=430871 Committed: https://crrev.com/59e171f76d69b1a87ba1599190b1c307daf48a34 Cr-Commit-Position: refs/heads/master@{#304244}

Patch Set 1 #

Patch Set 2 : error handling #

Patch Set 3 : #

Patch Set 4 : Removed unnecessary headers. #

Total comments: 12

Patch Set 5 : Adding new error response enum and callback. #

Patch Set 6 : Added android support. #

Patch Set 7 : #

Patch Set 8 : Fix for build issues. #

Patch Set 9 : Fix athena/flashui build issue. #

Patch Set 10 : Use content namespace to resolve readback params. #

Total comments: 8

Patch Set 11 : Code changed as per review comments. #

Total comments: 2

Patch Set 12 : Removed extra braces as per the review comments. #

Total comments: 2

Patch Set 13 : Changed POD response to non const. #

Patch Set 14 : Fix for browser_test after rebase. #

Patch Set 15 : Fix for browser test rebase issue. #

Patch Set 16 : Formatted code and fixed build issue in test. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+221 lines, -192 lines) Patch
M athena/content/content_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -1 line 0 comments Download
M athena/content/content_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/thumbnails/thumbnail_tab_helper.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/thumbnails/thumbnail_tab_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/fullscreen/flash_fullscreen_interactive_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/pdf/pdf_browsertest_base.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/ui/pdf/pdf_browsertest_base.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -3 lines 0 comments Download
M content/browser/android/content_readback_handler.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -2 lines 0 comments Download
M content/browser/android/content_readback_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +8 lines, -10 lines 0 comments Download
M content/browser/android/content_view_core_impl.h View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/android/content_view_core_impl.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_guest.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/compositor/delegated_frame_host.h View 1 2 3 4 2 chunks +4 lines, -4 lines 0 comments Download
M content/browser/compositor/delegated_frame_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 9 chunks +13 lines, -13 lines 1 comment Download
M content/browser/devtools/protocol/color_picker.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M content/browser/devtools/protocol/color_picker.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/devtools/protocol/page_handler.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +6 lines, -6 lines 0 comments Download
M content/browser/devtools/protocol/page_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -3 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M content/browser/frame_host/navigation_entry_screenshot_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -2 lines 0 comments Download
M content/browser/frame_host/navigation_entry_screenshot_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -4 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.h View 1 2 3 4 5 2 chunks +5 lines, -5 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/media/capture/web_contents_video_capture_device.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -5 lines 0 comments Download
M content/browser/media/capture/web_contents_video_capture_device_unittest.cc View 1 2 3 4 5 2 chunks +5 lines, -6 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -5 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +18 lines, -22 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 10 chunks +19 lines, -19 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -4 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 2 3 4 5 3 chunks +5 lines, -11 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 7 chunks +22 lines, -25 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -5 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac.mm View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/android/content_view_core.h View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
A content/public/browser/readback_types.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +28 lines, -0 lines 0 comments Download
M content/public/browser/render_widget_host.h View 1 2 3 4 2 chunks +5 lines, -5 lines 0 comments Download
M content/test/test_render_view_host.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/test/test_render_view_host.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M extensions/browser/api/capture_web_contents_function.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -1 line 0 comments Download
M extensions/browser/api/capture_web_contents_function.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 36 (14 generated)
sivag
@piman, though its a work in progress, i would like to know your comments on ...
6 years, 2 months ago (2014-09-24 17:38:44 UTC) #2
piman
It makes sense at a high level. I have some comments about naming and where ...
6 years, 2 months ago (2014-10-02 18:08:26 UTC) #3
sivag
ptal.. https://codereview.chromium.org/593503003/diff/60001/content/common/readback_types.h File content/common/readback_types.h (right): https://codereview.chromium.org/593503003/diff/60001/content/common/readback_types.h#newcode5 content/common/readback_types.h:5: #ifndef CONTENT_COMMON_READBACK_TYPES_H_ On 2014/10/02 18:08:26, piman (Very slow ...
6 years, 1 month ago (2014-11-06 13:34:16 UTC) #7
piman
mostly nits except one thing. https://codereview.chromium.org/593503003/diff/180001/chrome/browser/ui/pdf/pdf_browsertest_base.cc File chrome/browser/ui/pdf/pdf_browsertest_base.cc (right): https://codereview.chromium.org/593503003/diff/180001/chrome/browser/ui/pdf/pdf_browsertest_base.cc#newcode118 chrome/browser/ui/pdf/pdf_browsertest_base.cc:118: ASSERT_EQ((response == content::READBACK_SUCCESS), true); ...
6 years, 1 month ago (2014-11-07 01:55:42 UTC) #8
sivag
ptal.. https://codereview.chromium.org/593503003/diff/180001/chrome/browser/ui/pdf/pdf_browsertest_base.cc File chrome/browser/ui/pdf/pdf_browsertest_base.cc (right): https://codereview.chromium.org/593503003/diff/180001/chrome/browser/ui/pdf/pdf_browsertest_base.cc#newcode118 chrome/browser/ui/pdf/pdf_browsertest_base.cc:118: ASSERT_EQ((response == content::READBACK_SUCCESS), true); On 2014/11/07 01:55:42, piman ...
6 years, 1 month ago (2014-11-07 12:27:42 UTC) #9
piman
lgtm
6 years, 1 month ago (2014-11-07 19:00:23 UTC) #12
sivag
@cpu, brettw ptal chrome/ Extensions/ Athena/ folder changes thanks.
6 years, 1 month ago (2014-11-07 21:06:54 UTC) #13
cpu_(ooo_6.6-7.5)
removing myself adding oshima. Please use the owners file closest to the changes at hand.
6 years, 1 month ago (2014-11-10 18:35:30 UTC) #16
oshima
athena/ (assuming that's what you need) lgtm with a nit https://codereview.chromium.org/593503003/diff/200001/athena/content/content_proxy.cc File athena/content/content_proxy.cc (right): https://codereview.chromium.org/593503003/diff/200001/athena/content/content_proxy.cc#newcode167 ...
6 years, 1 month ago (2014-11-10 19:36:01 UTC) #17
sivag
https://codereview.chromium.org/593503003/diff/200001/athena/content/content_proxy.cc File athena/content/content_proxy.cc (right): https://codereview.chromium.org/593503003/diff/200001/athena/content/content_proxy.cc#newcode167 athena/content/content_proxy.cc:167: if ((response != content::READBACK_SUCCESS) || bitmap.empty() || On 2014/11/10 ...
6 years, 1 month ago (2014-11-11 04:32:57 UTC) #18
sivag
+sky, kalman. @sky for src/chrome/ changes @kalman for src/extensions/ ptal..
6 years, 1 month ago (2014-11-11 04:39:09 UTC) #20
sky
https://codereview.chromium.org/593503003/diff/220001/chrome/browser/thumbnails/thumbnail_tab_helper.cc File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right): https://codereview.chromium.org/593503003/diff/220001/chrome/browser/thumbnails/thumbnail_tab_helper.cc#newcode68 chrome/browser/thumbnails/thumbnail_tab_helper.cc:68: const content::ReadbackResponse& response) { We generally don't POD types ...
6 years, 1 month ago (2014-11-11 16:43:18 UTC) #21
not at google - send to devlin
lgtm proxy to sky
6 years, 1 month ago (2014-11-11 16:52:37 UTC) #22
sivag
ptal.. https://codereview.chromium.org/593503003/diff/220001/chrome/browser/thumbnails/thumbnail_tab_helper.cc File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right): https://codereview.chromium.org/593503003/diff/220001/chrome/browser/thumbnails/thumbnail_tab_helper.cc#newcode68 chrome/browser/thumbnails/thumbnail_tab_helper.cc:68: const content::ReadbackResponse& response) { On 2014/11/11 16:43:18, sky ...
6 years, 1 month ago (2014-11-12 18:53:49 UTC) #23
sivag
@Sky ptal at the chrome/ changes.
6 years, 1 month ago (2014-11-14 12:25:27 UTC) #25
sky
LGTM
6 years, 1 month ago (2014-11-14 17:50:42 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/593503003/300001
6 years, 1 month ago (2014-11-14 17:53:49 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_triggered_tests/builds/69895)
6 years, 1 month ago (2014-11-14 18:38:26 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/593503003/300001
6 years, 1 month ago (2014-11-14 18:42:45 UTC) #32
commit-bot: I haz the power
Committed patchset #16 (id:300001)
6 years, 1 month ago (2014-11-14 19:06:08 UTC) #33
commit-bot: I haz the power
Patchset 16 (id:??) landed as https://crrev.com/59e171f76d69b1a87ba1599190b1c307daf48a34 Cr-Commit-Position: refs/heads/master@{#304244}
6 years, 1 month ago (2014-11-14 19:06:50 UTC) #34
Jeffrey Yasskin
6 years, 1 month ago (2014-11-15 01:26:43 UTC) #36
Message was sent while issue was closed. 
https://codereview.chromium.org/593503003/diff/300001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/593503003/diff/300001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:591: callback.Run(*bitmap,
content::READBACK_SUCCESS);
This line is incorrect if 'result' is false. I believe this caused
https://crbug.com/433553.

Powered by Google App Engine
This is Rietveld 408576698