Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10294003:
Use the asynchronous version of CopyFromBackingStore in CaptureVisibleTabFunction. (Closed)
|
Created:
8 years, 7 months ago by mazda Modified:
8 years, 7 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, jstritar Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse the asynchronous version of CopyFromBackingStore in CaptureVisibleTabFunction.
BUG=120003
TEST=Take Screenshot Extension with site permission modified (http://code.google.com/chrome/extensions/samples.html#e1697cacebad05218798bf3e8a0f724517f0e8c3) worked properly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135254
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 2
Patch Set 4 : Fix test #
Messages
Total messages: 17 (0 generated)
Can I ask you to review this change?
http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_tabs_module.cc (left): http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_tabs_module.cc:1679: wrapper->snapshot_tab_helper()->CaptureSnapshot(); This line looks redundant and I copied the code without this line. Is it correct?
http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_tabs_module.cc (left): http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_tabs_module.cc:1679: wrapper->snapshot_tab_helper()->CaptureSnapshot(); On 2012/05/02 01:23:25, mazda wrote: > This line looks redundant and I copied the code without this line. > Is it correct? It looks redundant to me, but the code has changed substantially since I last touched it. git blame shows that it was added in r128037. Adding the committer as a reviewer, in case there is a reason I am missing.
+jstritar I no longer work on chrome full time. I am happy to do reviews, but I think a full time team member should see all changes. Added jstritar, who has edited this code and is a full time extensions team member. http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_tabs_module.cc (right): http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_tabs_module.cc:1752: void CaptureVisibleTabFunction::CopyFromBackingStoreComplete( In this file, the functions are in the same order that they will be run in. This new function is out of that order, because it runs before Observe(). Please move it between RunImpl() and Observe(). http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_tabs_module.cc:1761: // TODO(mazda): Remove the following code once AsyncCopyFromBackingStore is It has been two years since I looked at this in detail, but at the time there were legitimate reasons why a backing store might not be available. One reason was a limit on the number of backing stores. If there were more tabs than the limit, a tab was not focused for a while might not have a backing store. If the tab was not focused, and some other tab was focused in the same window, a backing store would not be created until a user focused the tab. Unless that is no longer true, I don't think you can remove the code below.
Thank you for the review. I didn't notice that you are no longer working on Chrome fulltime. Sorry about that. http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_tabs_module.cc (right): http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_tabs_module.cc:1752: void CaptureVisibleTabFunction::CopyFromBackingStoreComplete( On 2012/05/02 03:47:14, Sam Kerner (Chrome) wrote: > In this file, the functions are in the same order that they will be run in. > This new function is out of that order, because it runs before Observe(). > Please move it between RunImpl() and Observe(). Done. http://codereview.chromium.org/10294003/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_tabs_module.cc:1761: // TODO(mazda): Remove the following code once AsyncCopyFromBackingStore is On 2012/05/02 03:47:14, Sam Kerner (Chrome) wrote: > It has been two years since I looked at this in detail, but at the time there > were legitimate reasons why a backing store might not be available. > > One reason was a limit on the number of backing stores. If there were more tabs > than the limit, a tab was not focused for a while might not have a backing > store. If the tab was not focused, and some other tab was focused in the same > window, a backing store would not be created until a user focused the tab. > > Unless that is no longer true, I don't think you can remove the code below. Thanks for the useful comment. I think your comment is still true on most platforms, so this TODO comment would not be appropriate. I deleted the comment. One exception at this time is Chrome OS. CopyFromBackingStore was modified so that it read page image data from GPU when the page is composited by GPU. If pages are always composited by GPU (that's true on Chrome OS), the code below should not be necessary.
I am happy with the change, but I want to let jstritar take a look.
Thanks. jstritar@ Could you review this CL?
jstritar: ping?
Jon is also no longer working on Chrome full-time, I can take a look. http://codereview.chromium.org/10294003/diff/16001/chrome/browser/extensions/... File chrome/browser/extensions/extension_tabs_module.cc (right): http://codereview.chromium.org/10294003/diff/16001/chrome/browser/extensions/... chrome/browser/extensions/extension_tabs_module.cc:1668: return false; This should also populate error_ with an error. http://codereview.chromium.org/10294003/diff/16001/chrome/browser/extensions/... chrome/browser/extensions/extension_tabs_module.cc:1693: return; This also needs a SendResponse(false) call. http://codereview.chromium.org/10294003/diff/16001/chrome/browser/extensions/... chrome/browser/extensions/extension_tabs_module.cc:1702: return; This can be removed.
Mihai, Thank you for the review! Please take another look at the change. http://codereview.chromium.org/10294003/diff/16001/chrome/browser/extensions/... File chrome/browser/extensions/extension_tabs_module.cc (right): http://codereview.chromium.org/10294003/diff/16001/chrome/browser/extensions/... chrome/browser/extensions/extension_tabs_module.cc:1668: return false; On 2012/05/03 16:27:30, Mihai Parparita wrote: > This should also populate error_ with an error. Done. Also added code to populate error_ to the return sentence in 1663. http://codereview.chromium.org/10294003/diff/16001/chrome/browser/extensions/... chrome/browser/extensions/extension_tabs_module.cc:1693: return; On 2012/05/03 16:27:30, Mihai Parparita wrote: > This also needs a SendResponse(false) call. Done. http://codereview.chromium.org/10294003/diff/16001/chrome/browser/extensions/... chrome/browser/extensions/extension_tabs_module.cc:1702: return; On 2012/05/03 16:27:30, Mihai Parparita wrote: > This can be removed. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mazda@chromium.org/10294003/23002
Try job failure for 10294003-23002 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
http://codereview.chromium.org/10294003/diff/23002/chrome/browser/extensions/... File chrome/browser/extensions/extension_tabs_module.cc (right): http://codereview.chromium.org/10294003/diff/23002/chrome/browser/extensions/... chrome/browser/extensions/extension_tabs_module.cc:1663: error_ = keys::kInternalVisibleTabCaptureError; It looks we should not set error here. Adding this broke several tests. I reverted the change of this part.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mazda@chromium.org/10294003/36001
Change committed as 135254
LGTM http://codereview.chromium.org/10294003/diff/23002/chrome/browser/extensions/... File chrome/browser/extensions/extension_tabs_module.cc (right): http://codereview.chromium.org/10294003/diff/23002/chrome/browser/extensions/... chrome/browser/extensions/extension_tabs_module.cc:1663: error_ = keys::kInternalVisibleTabCaptureError; On 2012/05/03 22:24:50, mazda wrote: > It looks we should not set error here. Adding this broke several tests. > I reverted the change of this part. Looks like Extension::CanCaptureVisiblePage(.., &error) sets error when it returns false. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
