Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2007783002:
Adds PrerenderingOffliner implementation for handling Loader callbacks and then performing SavePage… (Closed)
|
Created:
4 years, 7 months ago by dougarnett Modified:
4 years, 6 months ago CC:
chromium-reviews, davidben+watch_chromium.org, cbentzel+watch_chromium.org, romax+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, gavinp+prer_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ploader Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds PrerenderingOffliner implementation for handling Loader callbacks and then performing SavePage request for successful loads.
BUG=601173
Committed: https://crrev.com/f4a897e49edf2b11ba38119c706ce8e94309c39b
Cr-Commit-Position: refs/heads/master@{#396386}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Added StopLoading call upon Save callback and addressed some petewil comments #
Total comments: 8
Patch Set 3 : sync to master #Patch Set 4 : Backed out mocked Offliner save methods and SavePage tests and addressed some other feedback #
Total comments: 2
Patch Set 5 : Update for petewil comment #
Depends on Patchset: Messages
Total messages: 18 (5 generated)
dougarnett@chromium.org changed reviewers: + fgorski@chromium.org, petewil@google.com
petewil@chromium.org changed reviewers: + petewil@chromium.org
https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:34: if (request.request_id() != pending_request_id_) So if this happens, it looks like the request is ignored. Isn't that a problem? https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:40: // Loader (if this loaded web_contents is being reclaimed). This sentence is a bit strangely worded, please clarify it some. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:46: weak_ptr_factory_.GetWeakPtr(), request, I normally just have the class inherit publicly from base::SupportsWeakPtr<RequestCoordinator>, then I use AsWeakPtr() where you used weak_ptr_factory_.GetWeakPtr(). It makes the code easier to read at the point of the bind, and you can get rid of the weak_ptr_factory_. You will need to #include "base/memory/weak_ptr.h". https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:49: // Clear pending request and then run its completion callback. So does this mean we only get one callback for success? If we are using a RightMomentDetector kind of strategy, I could see us getting several, maybe one "ok 5 seconds have gone by with no onPageLoaded event, and one "ok the page is loaded", and one, "Ok, the page loaded and 2 seconds have passed." https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:82: // there is no current pending request. But it only makes sure of that while debugging. Do we need a stronger guarantee at runtime? https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:89: weak_ptr_factory_.GetWeakPtr(), request, callback)); You can use AsWeakPtr() here too. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:92: class TestPrerenderingOffliner : public PrerenderingOffliner { This is not the testing strategy I normally see. I normally see the real class being created, and passing in factories to create dependencies (or passing in the dependencies themselves). I dislike that this approach is modifying the class under test while testing, and I am concerned that it might make tests pass that shouldn't.
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:46: weak_ptr_factory_.GetWeakPtr(), request, On 2016/05/24 00:40:07, Pete Williamson wrote: > I normally just have the class inherit publicly from > base::SupportsWeakPtr<RequestCoordinator>, then I use AsWeakPtr() where you used > weak_ptr_factory_.GetWeakPtr(). It makes the code easier to read at the point > of the bind, and you can get rid of the weak_ptr_factory_. You will need to > #include "base/memory/weak_ptr.h". It's generally considered bad form to use SupportsWeakPtr, since the destruction order is important and prone to bugs. SupportsWeakPtr is generally fine in tests where you don't care so much about test lifetimes, but in production code a WeakPtrFactory is more appropriate.
https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:34: if (request.request_id() != pending_request_id_) On 2016/05/24 00:40:07, Pete Williamson wrote: > So if this happens, it looks like the request is ignored. Isn't that a problem? The callback is ignored because the request has already had some save resolution. The scenario (race condition) is that the prerenderer stops rendering (its WebContents get destroyed) and triggers this callback through the Loader but in the meantime we have gotten a save callback (I see now that I am missing the StopLoading() call and adding it). https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:40: // Loader (if this loaded web_contents is being reclaimed). On 2016/05/24 00:40:07, Pete Williamson wrote: > This sentence is a bit strangely worded, please clarify it some. I agree - please check new wording https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:46: weak_ptr_factory_.GetWeakPtr(), request, On 2016/05/24 00:40:07, Pete Williamson wrote: > I normally just have the class inherit publicly from > base::SupportsWeakPtr<RequestCoordinator>, then I use AsWeakPtr() where you used > weak_ptr_factory_.GetWeakPtr(). It makes the code easier to read at the point > of the bind, and you can get rid of the weak_ptr_factory_. You will need to > #include "base/memory/weak_ptr.h". Are we transitioning to that pattern? The GetWeakPtr() pattern seems to be the most common in the existing offline_pages code base - eg, besides unittests, only RequestCoordinator using AsWeakPtr at this point. I'm happy to follow whichever pattern is preferred. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:49: // Clear pending request and then run its completion callback. On 2016/05/24 00:40:07, Pete Williamson wrote: > So does this mean we only get one callback for success? If we are using a > RightMomentDetector kind of strategy, I could see us getting several, maybe one > "ok 5 seconds have gone by with no onPageLoaded event, and one "ok the page is > loaded", and one, "Ok, the page loaded and 2 seconds have passed." Yes, only supporting one save attempt initially. I favor gathering empirical evidence/motivation before supporting multiple ones. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:82: // there is no current pending request. On 2016/05/24 00:40:07, Pete Williamson wrote: > But it only makes sure of that while debugging. Do we need a stronger guarantee > at runtime? Backing off the comment for now. Debug time might be sufficient since we own the caller (Coordinator). Maybe we can discuss this afternoon. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:92: class TestPrerenderingOffliner : public PrerenderingOffliner { On 2016/05/24 00:40:07, Pete Williamson wrote: > This is not the testing strategy I normally see. I normally see the real class > being created, and passing in factories to create dependencies (or passing in > the dependencies themselves). I dislike that this approach is modifying the > class under test while testing, and I am concerned that it might make tests pass > that shouldn't. I'm not really happy with it either (I have TODO's with their declarations). I was surprised the OfflinePageModel is not an interface and wonder if we should make it one (but not sure about tackling that in this CL). The other approach I considered was defining some other interface (Snapshotter, PageSaver) that is mockable and the actual implementation uses OPM.
Quick comments. Didn't look at the tests yet. https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:92: if (!accepted) I understand that callback will be called if LoadPage returned true. Is it guaranteed to not call the callback if it returned false? https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:133: DCHECK_EQ(pending_request_id_, 0); we are using the magic number 0 all over this code. Did you consider extracting it, e.g. kInvalidRequestId. Also, I am wondering if only allowing positive request IDs is reasonable, just valid (non-zero) could be a more sound approach. Please consider that. https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.h (right): https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.h:47: // TODO(dougarnett): Consider making OfflinePageModel mockable instead. we are going to get there. https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.h:81: int64_t pending_request_id_; Please document this one. I am wondering why you are passing around completion callback and save page request as argument other callbacks, but store the pending_request_id_ here. Documentation should explain that. Or you could consider storing a bit more.
https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:34: if (request.request_id() != pending_request_id_) On 2016/05/24 19:07:26, dougarnett wrote: > On 2016/05/24 00:40:07, Pete Williamson wrote: > > So if this happens, it looks like the request is ignored. Isn't that a > problem? > > The callback is ignored because the request has already had some save > resolution. > The scenario (race condition) is that the prerenderer stops rendering (its > WebContents get destroyed) and triggers this callback through the Loader but in > the meantime we have gotten a save callback (I see now that I am missing the > StopLoading() call and adding it). OK, per our discussion, maybe add more DCHECKs to document. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:40: // Loader (if this loaded web_contents is being reclaimed). On 2016/05/24 19:07:26, dougarnett wrote: > On 2016/05/24 00:40:07, Pete Williamson wrote: > > This sentence is a bit strangely worded, please clarify it some. > > I agree - please check new wording New wording is OK. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:46: weak_ptr_factory_.GetWeakPtr(), request, On 2016/05/24 19:07:26, dougarnett wrote: > On 2016/05/24 00:40:07, Pete Williamson wrote: > > I normally just have the class inherit publicly from > > base::SupportsWeakPtr<RequestCoordinator>, then I use AsWeakPtr() where you > used > > weak_ptr_factory_.GetWeakPtr(). It makes the code easier to read at the point > > of the bind, and you can get rid of the weak_ptr_factory_. You will need to > > #include "base/memory/weak_ptr.h". > > Are we transitioning to that pattern? The GetWeakPtr() pattern seems to be the > most common in the existing offline_pages code base - eg, besides unittests, > only RequestCoordinator using AsWeakPtr at this point. > > I'm happy to follow whichever pattern is preferred. OK, then we should do as Justin suggests. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:49: // Clear pending request and then run its completion callback. On 2016/05/24 19:07:26, dougarnett wrote: > On 2016/05/24 00:40:07, Pete Williamson wrote: > > So does this mean we only get one callback for success? If we are using a > > RightMomentDetector kind of strategy, I could see us getting several, maybe > one > > "ok 5 seconds have gone by with no onPageLoaded event, and one "ok the page is > > loaded", and one, "Ok, the page loaded and 2 seconds have passed." > > Yes, only supporting one save attempt initially. I favor gathering empirical > evidence/motivation before supporting multiple ones. Ack. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:82: // there is no current pending request. On 2016/05/24 19:07:26, dougarnett wrote: > On 2016/05/24 00:40:07, Pete Williamson wrote: > > But it only makes sure of that while debugging. Do we need a stronger > guarantee > > at runtime? > > Backing off the comment for now. Debug time might be sufficient since we own the > caller (Coordinator). Maybe we can discuss this afternoon. ok https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:89: weak_ptr_factory_.GetWeakPtr(), request, callback)); On 2016/05/24 00:40:07, Pete Williamson wrote: > You can use AsWeakPtr() here too. Per Justin's comment above, you can ignore this. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:92: class TestPrerenderingOffliner : public PrerenderingOffliner { On 2016/05/24 19:07:27, dougarnett wrote: > On 2016/05/24 00:40:07, Pete Williamson wrote: > > This is not the testing strategy I normally see. I normally see the real > class > > being created, and passing in factories to create dependencies (or passing in > > the dependencies themselves). I dislike that this approach is modifying the > > class under test while testing, and I am concerned that it might make tests > pass > > that shouldn't. > > I'm not really happy with it either (I have TODO's with their declarations). I > was surprised the OfflinePageModel is not an interface and wonder if we should > make it one (but not sure about tackling that in this CL). The other approach I > considered was defining some other interface (Snapshotter, PageSaver) that is > mockable and the actual implementation uses OPM. > We discussed breaking out the unit tests to a new changelist, and seeing what our other options are for solving this, including making OfflinePageModel mockable, or making an interface for it, or inheriting a test OfflinePageModel from it.
Backed out the tests and approach for mocking OfflinePageModel save related methods. Plan is to instead create a follow-up CL with testing approach for those save methods and put back the test cases. Also, an underlying PrerenderingLoader CL was reverted to that will need to be re-landed before this one can land. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:34: if (request.request_id() != pending_request_id_) On 2016/05/24 21:18:41, Pete Williamson wrote: > On 2016/05/24 19:07:26, dougarnett wrote: > > On 2016/05/24 00:40:07, Pete Williamson wrote: > > > So if this happens, it looks like the request is ignored. Isn't that a > > problem? > > > > The callback is ignored because the request has already had some save > > resolution. > > The scenario (race condition) is that the prerenderer stops rendering (its > > WebContents get destroyed) and triggers this callback through the Loader but > in > > the meantime we have gotten a save callback (I see now that I am missing the > > StopLoading() call and adding it). > > OK, per our discussion, maybe add more DCHECKs to document. Reworked to keep ptr to pending request - hopefully easier to follow. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:40: // Loader (if this loaded web_contents is being reclaimed). On 2016/05/24 21:18:42, Pete Williamson wrote: > On 2016/05/24 19:07:26, dougarnett wrote: > > On 2016/05/24 00:40:07, Pete Williamson wrote: > > > This sentence is a bit strangely worded, please clarify it some. > > > > I agree - please check new wording > > New wording is OK. Acknowledged. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:46: weak_ptr_factory_.GetWeakPtr(), request, On 2016/05/24 21:18:41, Pete Williamson wrote: > On 2016/05/24 19:07:26, dougarnett wrote: > > On 2016/05/24 00:40:07, Pete Williamson wrote: > > > I normally just have the class inherit publicly from > > > base::SupportsWeakPtr<RequestCoordinator>, then I use AsWeakPtr() where you > > used > > > weak_ptr_factory_.GetWeakPtr(). It makes the code easier to read at the > point > > > of the bind, and you can get rid of the weak_ptr_factory_. You will need to > > > #include "base/memory/weak_ptr.h". > > > > Are we transitioning to that pattern? The GetWeakPtr() pattern seems to be the > > most common in the existing offline_pages code base - eg, besides unittests, > > only RequestCoordinator using AsWeakPtr at this point. > > > > I'm happy to follow whichever pattern is preferred. > > OK, then we should do as Justin suggests. Acknowledged. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner.cc:82: // there is no current pending request. On 2016/05/24 21:18:41, Pete Williamson wrote: > On 2016/05/24 19:07:26, dougarnett wrote: > > On 2016/05/24 00:40:07, Pete Williamson wrote: > > > But it only makes sure of that while debugging. Do we need a stronger > > guarantee > > > at runtime? > > > > Backing off the comment for now. Debug time might be sufficient since we own > the > > caller (Coordinator). Maybe we can discuss this afternoon. > > ok Revised to return false if already have pending request. https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2007783002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:92: class TestPrerenderingOffliner : public PrerenderingOffliner { On 2016/05/24 21:18:42, Pete Williamson wrote: > On 2016/05/24 19:07:27, dougarnett wrote: > > On 2016/05/24 00:40:07, Pete Williamson wrote: > > > This is not the testing strategy I normally see. I normally see the real > > class > > > being created, and passing in factories to create dependencies (or passing > in > > > the dependencies themselves). I dislike that this approach is modifying the > > > class under test while testing, and I am concerned that it might make tests > > pass > > > that shouldn't. > > > > I'm not really happy with it either (I have TODO's with their declarations). I > > was surprised the OfflinePageModel is not an interface and wonder if we should > > make it one (but not sure about tackling that in this CL). The other approach > I > > considered was defining some other interface (Snapshotter, PageSaver) that is > > mockable and the actual implementation uses OPM. > > > > We discussed breaking out the unit tests to a new changelist, and seeing what > our other options are for solving this, including making OfflinePageModel > mockable, or making an interface for it, or inheriting a test OfflinePageModel > from it. Acknowledged. https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:92: if (!accepted) On 2016/05/24 19:58:28, fgorski wrote: > I understand that callback will be called if LoadPage returned true. > Is it guaranteed to not call the callback if it returned false? Yes, updated the interface method comment. https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:133: DCHECK_EQ(pending_request_id_, 0); On 2016/05/24 19:58:28, fgorski wrote: > we are using the magic number 0 all over this code. > > Did you consider extracting it, e.g. kInvalidRequestId. > > Also, I am wondering if only allowing positive request IDs is reasonable, just > valid (non-zero) could be a more sound approach. Please consider that. Revised to keep pointer to pending request instead (leverage nullptr instead special id value) https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.h (right): https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.h:47: // TODO(dougarnett): Consider making OfflinePageModel mockable instead. On 2016/05/24 19:58:28, fgorski wrote: > we are going to get there. Acknowledged. May break out subinterface in follow-up CL. https://codereview.chromium.org/2007783002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.h:81: int64_t pending_request_id_; On 2016/05/24 19:58:28, fgorski wrote: > Please document this one. > > I am wondering why you are passing around completion callback and save page > request as argument other callbacks, but store the pending_request_id_ here. > Documentation should explain that. > Or you could consider storing a bit more. Tracking ptr to pending request now and included comment wrt save page callback could be received for old, canceled request.
Mostly looks good. One comment to address, and of course we need to get the patch that this depends on re-landed. https://codereview.chromium.org/2007783002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:37: CHECK_EQ(request.request_id(), pending_request_->request_id()); I'm not sure we need to crash in release if we get a race condition where we see a different ID. (I'm definitely OK with crashing in debug, though). Maybe we should just log the race condition instead of crashing.
Have re-landed the dependent CL. https://codereview.chromium.org/2007783002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2007783002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:37: CHECK_EQ(request.request_id(), pending_request_->request_id()); On 2016/05/25 18:34:38, Pete Williamson wrote: > I'm not sure we need to crash in release if we get a race condition where we see > a different ID. (I'm definitely OK with crashing in debug, though). Maybe we > should just log the race condition instead of crashing. Trying combo of DCHECK and conditional to ignore. Background here is that since we are able to cancel/stop the load operation, we should not see a callback for a different request while we are pending. We do have a race that we handle with above !pending_request_. However, we don't have an api currently to cancel the save operation, so for its callback we do handle/ignore a callback for an older request.
lgtm
The CQ bit was checked by dougarnett@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007783002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007783002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Adds PrerenderingOffliner implementation for handling Loader callbacks and then performing SavePage request for successful loads. BUG=601173 ========== to ========== Adds PrerenderingOffliner implementation for handling Loader callbacks and then performing SavePage request for successful loads. BUG=601173 Committed: https://crrev.com/f4a897e49edf2b11ba38119c706ce8e94309c39b Cr-Commit-Position: refs/heads/master@{#396386} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f4a897e49edf2b11ba38119c706ce8e94309c39b Cr-Commit-Position: refs/heads/master@{#396386} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
