Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2945213002:
[ios] Snapshot for WebState (Closed)
|
Created:
3 years, 6 months ago by edchin Modified:
3 years, 6 months ago Reviewers:
rohitrao (Google), kkhorimoto (Do not use), Eugene But (OOO till 7-30), marq (ping after 24h), kkhorimoto, rohitrao (ping after 24h) CC:
chromium-reviews, ios-reviews_chromium.org, ios-reviews+web_chromium.org, baxley+watch_chromium.org, Eugene But (OOO till 7-30), huangml+watch_chromium.org, liaoyuke+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Snapshot for WebState
This CL adds a method to WebState that asynchronously takes a full-res
snapshot of the rendered web page. This CL does not implement the new
iOS 11 WKWebView snapshotting method, which will be implemented in
a separate CL.
BUG=none
Review-Url: https://codereview.chromium.org/2945213002
Cr-Commit-Position: refs/heads/master@{#482140}
Committed: https://chromium.googlesource.com/chromium/src/+/2c89a25cf7851a3830f884b8d368d3eceacfc018
Patch Set 1 #
Total comments: 37
Patch Set 2 : Address comments. #
Total comments: 4
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Address comments #Patch Set 5 : Address comments #
Messages
Total messages: 52 (37 generated)
The CQ bit was checked by edchin@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...
edchin@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@google.com, marq@chromium.org, rohitrao@google.com
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... File ios/web/public/web_state/web_state.h (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:255: // Takes a full-resolution snapshot of this WebState. Document when |callback| will be called? https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:675: const base::Callback<void(const gfx::Image& snapshot)>& callback) const { The CL description describes this method as asynchronous, but this implementation is entirely synchronous -- a caller will have |callback| execute before |TakeSnapshot()| returns. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:681: UIGraphicsBeginImageContextWithOptions(view.bounds.size, YES, 0); Is this preferable to UIView's -snapshotViewAfterScreenUpdates: method? https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:685: callback.Run(gfx::Image(snapshot)); What's the cost of converting to/from a gfx::Image? What benefit do we get from this? https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_unittest.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:27: unsigned char buffer[4] = {0, 0, 0, 0}; Add comments to explain how this all works. Also make sure it doesn't explode on a 32-bit build. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:158: ASSERT_EQ(viewSize.width, snapshotImage.size.width); s/ASSERT/EXPECT for these.
rohitrao@chromium.org changed reviewers: + rohitrao@chromium.org
https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:675: const base::Callback<void(const gfx::Image& snapshot)>& callback) const { On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > The CL description describes this method as asynchronous, but this > implementation is entirely synchronous -- a caller will have |callback| execute > before |TakeSnapshot()| returns. Should we post callback to the current TaskRunner, to help avoid writing code that accidentally requires a synchronous callback? https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:681: UIGraphicsBeginImageContextWithOptions(view.bounds.size, YES, 0); On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > Is this preferable to UIView's -snapshotViewAfterScreenUpdates: method? I don't think that call actually works consistently enough =) https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:685: callback.Run(gfx::Image(snapshot)); On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > What's the cost of converting to/from a gfx::Image? What benefit do we get from > this? gfx::Images are pretty cheap, as long as you don't do anything that requires converting the underlying image to a new format. I'm not sure what the arguments for using it here are, other than that we don't have many raw UIKit types in WebState methods.
https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:675: const base::Callback<void(const gfx::Image& snapshot)>& callback) const { On 2017/06/21 11:38:15, rohitrao (ping after 24h) wrote: > On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > > The CL description describes this method as asynchronous, but this > > implementation is entirely synchronous -- a caller will have |callback| > execute > > before |TakeSnapshot()| returns. > > Should we post callback to the current TaskRunner, to help avoid writing code > that accidentally requires a synchronous callback? I'll implement Rohit's suggestion for now. The ios 11 snapshotting method is actually asynchronous, and this synchronous code will be dropped in the future with ios 10. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:681: UIGraphicsBeginImageContextWithOptions(view.bounds.size, YES, 0); On 2017/06/21 11:38:15, rohitrao (ping after 24h) wrote: > On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > > Is this preferable to UIView's -snapshotViewAfterScreenUpdates: method? > > I don't think that call actually works consistently enough =) Ditto. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:685: callback.Run(gfx::Image(snapshot)); On 2017/06/21 11:38:15, rohitrao (ping after 24h) wrote: > On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > > What's the cost of converting to/from a gfx::Image? What benefit do we get > from > > this? > > gfx::Images are pretty cheap, as long as you don't do anything that requires > converting the underlying image to a new format. > > I'm not sure what the arguments for using it here are, other than that we don't > have many raw UIKit types in WebState methods. There was some discussion on this on the design doc. One reason is that web layer uses Chrome/C++ primitives where possible (GURL instead of NSURL, std::string instead of NSString, Callback instead of blocks).
kkhorimoto@chromium.org changed reviewers: + kkhorimoto@chromium.org
https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... File ios/web/public/web_state/web_state.h (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:255: // Takes a full-resolution snapshot of this WebState. You should also document that this API is asynchronous on iOS 11, but synchronous for previous versions of iOS. https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:257: const base::Callback<void(const gfx::Image& snapshot)>& callback) Optional: There's some precedent for typedefing Callbacks in web (e.g. java_script_dialog_callback.h). You might want to do the same here to make formatting a little easier.
Generally looks good. Thanks for adding API to web layer. https://codereview.chromium.org/2945213002/diff/1/ios/web/public/test/fakes/t... File ios/web/public/test/fakes/test_web_state.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/public/test/fakes/t... ios/web/public/test/fakes/test_web_state.mm:236: callback.Run(gfx::Image()); Should this be asynchronous call? https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... File ios/web/public/web_state/web_state.h (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:256: virtual void TakeSnapshot( Do you want to pass width argument? Not every client will need view's size. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:675: const base::Callback<void(const gfx::Image& snapshot)>& callback) const { On 2017/06/21 16:57:24, edchin wrote: > On 2017/06/21 11:38:15, rohitrao (ping after 24h) wrote: > > On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > > > The CL description describes this method as asynchronous, but this > > > implementation is entirely synchronous -- a caller will have |callback| > > execute > > > before |TakeSnapshot()| returns. > > > > Should we post callback to the current TaskRunner, to help avoid writing code > > that accidentally requires a synchronous callback? > > I'll implement Rohit's suggestion for now. The ios 11 snapshotting method is > actually asynchronous, > and this synchronous code will be dropped in the future with ios 10. I believe the API should be either always synchronous or asynchronous. Having different API contact depending on iOS version, is quite inconvenient for the clients. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_unittest.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:23: UIColor* colorAtPoint(UIImage* image, CGPoint point) { Please follow C++ Style Guide for names (start function name with capital letter and use_underscores for variables). https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:25: CGImageRef imageRef = CGImageCreateWithImageInRect(image.CGImage, sourceRect); Is there any gfx:Image API to get color at point? https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:147: __block bool execution_complete = false; nit: s/execution_complete/snapshot_taken ? Sounds like current name is coming from script execution test. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:150: dispatch_after( This is just for the wait, right? If so, how about using SpinRunLoopWithMaxDelay instead of GCD? https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:156: CGSize viewSize = web_state()->GetView().bounds.size; s/viewSize/view_size Same comment for other variables
The CQ bit was checked by edchin@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by edchin@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.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by edchin@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...
PTAL. Comments addressed. https://codereview.chromium.org/2945213002/diff/1/ios/web/public/test/fakes/t... File ios/web/public/test/fakes/test_web_state.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/public/test/fakes/t... ios/web/public/test/fakes/test_web_state.mm:236: callback.Run(gfx::Image()); On 2017/06/21 22:11:28, Eugene But wrote: > Should this be asynchronous call? Done. Using SequencedTaskRunner. https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... File ios/web/public/web_state/web_state.h (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:255: // Takes a full-resolution snapshot of this WebState. On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > Document when |callback| will be called? Done. https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:255: // Takes a full-resolution snapshot of this WebState. On 2017/06/21 20:06:52, kkhorimoto_ wrote: > You should also document that this API is asynchronous on iOS 11, but > synchronous for previous versions of iOS. Made this asynchronous in pre-ios-11 versions as well. Used SequencedTaskRunner. https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:256: virtual void TakeSnapshot( On 2017/06/21 22:11:29, Eugene But wrote: > Do you want to pass width argument? Not every client will need view's size. Resizing will take place elsewhere. That place will have the infrastructure to make operations cancellable. So let's keep web simple and move such complexity elsewhere. https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:257: const base::Callback<void(const gfx::Image& snapshot)>& callback) On 2017/06/21 20:06:52, kkhorimoto_ wrote: > Optional: There's some precedent for typedefing Callbacks in web (e.g. > java_script_dialog_callback.h). You might want to do the same here to make > formatting a little easier. Done. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:675: const base::Callback<void(const gfx::Image& snapshot)>& callback) const { On 2017/06/21 22:11:29, Eugene But wrote: > On 2017/06/21 16:57:24, edchin wrote: > > On 2017/06/21 11:38:15, rohitrao (ping after 24h) wrote: > > > On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > > > > The CL description describes this method as asynchronous, but this > > > > implementation is entirely synchronous -- a caller will have |callback| > > > execute > > > > before |TakeSnapshot()| returns. > > > > > > Should we post callback to the current TaskRunner, to help avoid writing > code > > > that accidentally requires a synchronous callback? > > > > I'll implement Rohit's suggestion for now. The ios 11 snapshotting method is > > actually asynchronous, > > and this synchronous code will be dropped in the future with ios 10. > I believe the API should be either always synchronous or asynchronous. Having > different API contact depending on iOS version, is quite inconvenient for the > clients. Made pre-ios-11 also asynchronous using SequencedTaskRunner. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_unittest.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:25: CGImageRef imageRef = CGImageCreateWithImageInRect(image.CGImage, sourceRect); On 2017/06/21 22:11:29, Eugene But wrote: > Is there any gfx:Image API to get color at point? Yes, there are some gfx::test support functions. I have updated and removed this code. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:27: unsigned char buffer[4] = {0, 0, 0, 0}; On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > Add comments to explain how this all works. Also make sure it doesn't explode on > a 32-bit build. Replaced with gfx::test support functions. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:147: __block bool execution_complete = false; On 2017/06/21 22:11:29, Eugene But wrote: > nit: s/execution_complete/snapshot_taken ? Sounds like current name is coming > from script execution test. Done. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:150: dispatch_after( On 2017/06/21 22:11:30, Eugene But wrote: > This is just for the wait, right? If so, how about using SpinRunLoopWithMaxDelay > instead of GCD? Done. Used MinDelay over MaxDelay. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:156: CGSize viewSize = web_state()->GetView().bounds.size; On 2017/06/21 22:11:29, Eugene But wrote: > s/viewSize/view_size > > Same comment for other variables Done. https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_unittest.mm:158: ASSERT_EQ(viewSize.width, snapshotImage.size.width); On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > s/ASSERT/EXPECT for these. Done.
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...)
LGTM https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/web_state/web_state... ios/web/web_state/web_state_impl.mm:685: callback.Run(gfx::Image(snapshot)); On 2017/06/21 16:57:24, edchin wrote: > On 2017/06/21 11:38:15, rohitrao (ping after 24h) wrote: > > On 2017/06/21 11:13:23, marq (ping after 24h) wrote: > > > What's the cost of converting to/from a gfx::Image? What benefit do we get > > from > > > this? > > > > gfx::Images are pretty cheap, as long as you don't do anything that requires > > converting the underlying image to a new format. > > > > I'm not sure what the arguments for using it here are, other than that we > don't > > have many raw UIKit types in WebState methods. > > There was some discussion on this on the design doc. One reason is that web > layer uses Chrome/C++ primitives where possible (GURL instead of NSURL, > std::string instead of NSString, Callback instead of blocks). I agree that this approach is consistent with that; whether that's how web should work is a separate conversation, then.
The CQ bit was checked by edchin@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.
https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... File ios/web/public/web_state/web_state.h (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:256: virtual void TakeSnapshot( On 2017/06/22 07:01:07, edchin wrote: > On 2017/06/21 22:11:29, Eugene But wrote: > > Do you want to pass width argument? Not every client will need view's size. > > Resizing will take place elsewhere. That place will have the infrastructure to > make operations cancellable. So let's keep web simple and move such > complexity elsewhere. What is the advantage of resizing elsewhere? Changing graphics context size will not make code complex and WKWebView already supports width argument. https://codereview.chromium.org/2945213002/diff/80001/ios/web/web_state/web_s... File ios/web/web_state/web_state_unittest.mm (right): https://codereview.chromium.org/2945213002/diff/80001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:128: base::test::ios::SpinRunLoopWithMinDelay(base::TimeDelta::FromSecondsD(0.1)); nit: Could you please explain in the comments why delay is needed.
https://codereview.chromium.org/2945213002/diff/80001/ios/web/web_state/web_s... File ios/web/web_state/web_state_unittest.mm (right): https://codereview.chromium.org/2945213002/diff/80001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:138: gfx::test::ToPlatformType(snapshot), 200, 200), Sorry, for not asking earlier. I assume this works well on different screen sizes.
PTAL. Will extract one line change in //ui/gfx/image into a separate CL, so this won't land until that lands. https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... File ios/web/public/web_state/web_state.h (right): https://codereview.chromium.org/2945213002/diff/1/ios/web/public/web_state/we... ios/web/public/web_state/web_state.h:256: virtual void TakeSnapshot( On 2017/06/22 19:34:14, Eugene But wrote: > On 2017/06/22 07:01:07, edchin wrote: > > On 2017/06/21 22:11:29, Eugene But wrote: > > > Do you want to pass width argument? Not every client will need view's size. > > > > Resizing will take place elsewhere. That place will have the infrastructure to > > make operations cancellable. So let's keep web simple and move such > > complexity elsewhere. > What is the advantage of resizing elsewhere? Changing graphics context size will > not make code complex and WKWebView already supports width argument. Per our offline discussion, I've added a CGSize argument. This improves memory and runtime efficiency over taking a full-res, full-size snapshot and then resizing. https://codereview.chromium.org/2945213002/diff/80001/ios/web/web_state/web_s... File ios/web/web_state/web_state_unittest.mm (right): https://codereview.chromium.org/2945213002/diff/80001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:128: base::test::ios::SpinRunLoopWithMinDelay(base::TimeDelta::FromSecondsD(0.1)); On 2017/06/22 19:34:14, Eugene But wrote: > nit: Could you please explain in the comments why delay is needed. Done. https://codereview.chromium.org/2945213002/diff/80001/ios/web/web_state/web_s... ios/web/web_state/web_state_unittest.mm:138: gfx::test::ToPlatformType(snapshot), 200, 200), On 2017/06/22 20:09:37, Eugene But wrote: > Sorry, for not asking earlier. I assume this works well on different screen > sizes. This worked fine. But now there's an even better standardized test with the target_size. https://codereview.chromium.org/2945213002/diff/100001/ui/gfx/image/image_uni... File ui/gfx/image/image_unittest_util_ios.mm (right): https://codereview.chromium.org/2945213002/diff/100001/ui/gfx/image/image_uni... ui/gfx/image/image_unittest_util_ios.mm:19: image.CGImage, CGRectMake(x * image.scale, y * image.scale, 1, 1))); I will pull this out into a separate CL and comment it so it can be advertised appropriately.
The CQ bit was checked by edchin@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.
lgtm! Thanks https://codereview.chromium.org/2945213002/diff/100001/ios/web/public/test/fa... File ios/web/public/test/fakes/test_web_state.h (right): https://codereview.chromium.org/2945213002/diff/100001/ios/web/public/test/fa... ios/web/public/test/fakes/test_web_state.h:80: const CGSize target_size) const override; Optional nit: I think it is super uncommon to have const near value argument in the header. AFAIK this is because compiler does not actually check if the argument is const for method predeclaration.
lgtm
The CQ bit was checked by edchin@chromium.org to run a CQ dry run
Description was changed from ========== [ios] Snapshot for WebState This CL adds a method to WebState that asynchronously takes a full-res snapshot of the rendered web page. This CL does not implement the new iOS 11 WKWebView snapshotting method, which will be implemented in a separate CL. BUG= ========== to ========== [ios] Snapshot for WebState This CL adds a method to WebState that asynchronously takes a full-res snapshot of the rendered web page. This CL does not implement the new iOS 11 WKWebView snapshotting method, which will be implemented in a separate CL. BUG=none ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edchin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org, eugenebut@chromium.org, kkhorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2945213002/#ps140001 (title: "Address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498286366752420,
"parent_rev": "18ea620c7031007948cc97031ce516d5512c5271", "commit_rev":
"2c89a25cf7851a3830f884b8d368d3eceacfc018"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Snapshot for WebState This CL adds a method to WebState that asynchronously takes a full-res snapshot of the rendered web page. This CL does not implement the new iOS 11 WKWebView snapshotting method, which will be implemented in a separate CL. BUG=none ========== to ========== [ios] Snapshot for WebState This CL adds a method to WebState that asynchronously takes a full-res snapshot of the rendered web page. This CL does not implement the new iOS 11 WKWebView snapshotting method, which will be implemented in a separate CL. BUG=none Review-Url: https://codereview.chromium.org/2945213002 Cr-Commit-Position: refs/heads/master@{#482140} Committed: https://chromium.googlesource.com/chromium/src/+/2c89a25cf7851a3830f884b8d368... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2c89a25cf7851a3830f884b8d368... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
