Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1854643002:
Implement PrerenderManager::AddPrerenderForOffline() (Closed)
|
Created:
4 years, 8 months ago by gabadie Modified:
4 years, 7 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org, davidben+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement PrerenderManager::AddPrerenderForOffline()
Offline's operation of fetching and snapshoting a web page
is going to be experienced with Prerender. This CL add the
offline Prerender origin in that purpose with the
PrerenderManager::AddPrerenderForOffline() method.
BUG=599500
Committed: https://crrev.com/fde8d42d16c0fac930e7fe39e86cb3ef6d24e9c8
Cr-Commit-Position: refs/heads/master@{#390370}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : Addresses pasko' comments #
Total comments: 16
Patch Set 4 : Addresses pasko's comments #
Total comments: 7
Patch Set 5 : Addresses pasko's comments #
Total comments: 5
Patch Set 6 : Rebase #Patch Set 7 : Takes care of FINAL_STATUS_BLOCK_THIRD_PARTY_COOKIES #Patch Set 8 : Reverts changes of chrome/chrome_browser.gypi #
Total comments: 10
Patch Set 9 : Rebase #Patch Set 10 : Addresses mmenke's comments #
Total comments: 6
Patch Set 11 : Makes offline originated prerenders unfindable #
Total comments: 4
Patch Set 12 : Rebase #Patch Set 13 : Fixes histograms.xml #Patch Set 14 : Rebase #Patch Set 15 : s/scoped_ptr/std::unique_ptr/g to fix compilation failure caused by recent landed changes #Patch Set 16 : Fixes PrerenderAllowedForOfflineAndForcedCellular #
Messages
Total messages: 59 (20 generated)
gabadie@chromium.org changed reviewers: + pasko@chromium.org
Hey egor, Here is the first revision of the CL bringing support of prerendering for offline folks as we have discussed offline with them. PTAL.
Description was changed from ========== Implements PrerenderManager::AddPrerenderForOffline() BUG=599500 ========== to ========== Implement PrerenderManager::AddPrerenderForOffline() BUG=599500 ==========
Looks mostly as discussed, thanks. Some top-level remarks. 1. For things that are not trivial bugfixes commit messages generally should be more verbose. In this case it should say something about introducing the new origin, the way how the new functionality will be used. 2. GN support is also needed. https://codereview.chromium.org/1854643002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.h:155: // cellular networks as long as the user setting for prerendering is ON. I thought we should ignore the privacy setting here, asked in the bug. https://codereview.chromium.org/1854643002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_origin.h (right): https://codereview.chromium.org/1854643002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_origin.h:27: ORIGIN_OFFLINE = 14, adding new origin requires changes in histograms.xml There is a recent example of doing it here: https://codereview.chromium.org/1767243002
Description was changed from ========== Implement PrerenderManager::AddPrerenderForOffline() BUG=599500 ========== to ========== Implement PrerenderManager::AddPrerenderForOffline() Offline's operation of fetching and snapshoting a web page is going to be experienced with Prerender. This CL add the offline Prerender origin in that purpose with the PrerenderManager::AddPrerenderForOffline() method. BUG=599500 ==========
Thanks Egor! I have uploaded a new revision, PTAL. On 2016/04/04 12:55:56, pasko wrote: > Looks mostly as discussed, thanks. Some top-level remarks. > > 1. For things that are not trivial bugfixes commit messages generally should be > more verbose. In this case it should say something about introducing the new > origin, the way how the new functionality will be used. Done > > 2. GN support is also needed. GN is already supported by this CL: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/BUI... https://codereview.chromium.org/1854643002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.h:155: // cellular networks as long as the user setting for prerendering is ON. On 2016/04/04 12:55:56, pasko wrote: > I thought we should ignore the privacy setting here, asked in the bug. Done. https://codereview.chromium.org/1854643002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_origin.h (right): https://codereview.chromium.org/1854643002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_origin.h:27: ORIGIN_OFFLINE = 14, On 2016/04/04 12:55:56, pasko wrote: > adding new origin requires changes in histograms.xml > > There is a recent example of doing it here: > https://codereview.chromium.org/1767243002 Done.
On 2016/04/04 14:05:07, gabadie wrote: > Thanks Egor! > > I have uploaded a new revision, PTAL. > > On 2016/04/04 12:55:56, pasko wrote: > > Looks mostly as discussed, thanks. Some top-level remarks. > > > > 1. For things that are not trivial bugfixes commit messages generally should > be > > more verbose. In this case it should say something about introducing the new > > origin, the way how the new functionality will be used. > Done > > > > 2. GN support is also needed. > GN is already supported by this CL: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/BUI... > Wow. Crazy. Somehow I have not noticed this hack until now, and it is used virtually everywhere. Ack. Thanks for pointing this out.
https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_constants.h (right): https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_constants.h:10: /* Most of the prerender are used in less than 5 minutes. */ C++ style comments necessary here. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_constants.h:13: /* Offline originated prerenders have an higher life expectancy to have more And here too - C++ style comments. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:155: // cellular networks as long as the user setting for prerendering is ON. The last part is no longer true? https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_origin.h (right): https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_origin.h:34: // Returns whether the origin can be prerendered regardless of the privacy These two functions are implementation detail of PrerenderManager, consumers of prerender_origin.h must not have visibility into it. The new use in unittests is not consistent with testing PrerenderManager, and those EXPECT_TRUE(IsSomething(origin)) for an origin that was defined a few lines above is something that does not have to be tested. Please make this logic back private to the PrerenderManager. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_origin.h:35: // settings or cellular network. s/cellular network/network type/ https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_origin.h:39: bool IsCellularForcedOrigin(Origin origin); Forced prerender on cell networks is not a property of the origin, so the semantics of this function is confusing. How about not having these helpful functions? Their names hide important details, the farther from context they are, the less readable the code is. https://codereview.chromium.org/1854643002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1854643002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90382: + <suffix name="offline" label="Offline triggered prerender."/> These seem to be sorted alphabetically, please maintain the sort order. https://codereview.chromium.org/1854643002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90382: + <suffix name="offline" label="Offline triggered prerender."/> "Prerender triggered for saving a page for offline use."
Thanks Egor! I have uploaded a new revision addressing all your comments. PTAL. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_constants.h (right): https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_constants.h:10: /* Most of the prerender are used in less than 5 minutes. */ On 2016/04/04 17:04:22, pasko wrote: > C++ style comments necessary here. Done. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_constants.h:13: /* Offline originated prerenders have an higher life expectancy to have more On 2016/04/04 17:04:22, pasko wrote: > And here too - C++ style comments. Done. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:155: // cellular networks as long as the user setting for prerendering is ON. On 2016/04/04 17:04:22, pasko wrote: > The last part is no longer true? Good point. Done. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_origin.h (right): https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_origin.h:34: // Returns whether the origin can be prerendered regardless of the privacy On 2016/04/04 17:04:22, pasko wrote: > These two functions are implementation detail of PrerenderManager, consumers of > prerender_origin.h must not have visibility into it. > > The new use in unittests is not consistent with testing PrerenderManager, and > those EXPECT_TRUE(IsSomething(origin)) for an origin that was defined a few > lines above is something that does not have to be tested. > > Please make this logic back private to the PrerenderManager. Done. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_origin.h:35: // settings or cellular network. On 2016/04/04 17:04:22, pasko wrote: > s/cellular network/network type/ Done. https://codereview.chromium.org/1854643002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_origin.h:39: bool IsCellularForcedOrigin(Origin origin); On 2016/04/04 17:04:22, pasko wrote: > Forced prerender on cell networks is not a property of the origin, so the > semantics of this function is confusing. > > How about not having these helpful functions? Their names hide important > details, the farther from context they are, the less readable the code is. Done. https://codereview.chromium.org/1854643002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1854643002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90382: + <suffix name="offline" label="Offline triggered prerender."/> On 2016/04/04 17:04:22, pasko wrote: > "Prerender triggered for saving a page for offline use." Done. https://codereview.chromium.org/1854643002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90382: + <suffix name="offline" label="Offline triggered prerender."/> On 2016/04/04 17:04:22, pasko wrote: > These seem to be sorted alphabetically, please maintain the sort order. Done. https://codereview.chromium.org/1854643002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1854643002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90385: + label="Prerender triggered for saving a page for offline use."/> tools/metrics/histograms/pretty_print.py precommit hook didn't left me the choice on the addtional indentation here. :(
lgtm given you accept the documentation-related changes below. https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:1343: // settings because want to be able to snapshot webpages independently of slight rephrasing: "because they are controlled by the offliner logic via PrerenderHandle." https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:154: // Adds a prerender from an external request that will always prerender. This is not an "external request", calling it such is confusing (or maybe I misunderstand the terminology). Please add the comment from the doc as we discussed, some obvious stuff can be stripped. My suggestion: // Adds a prerender for the background loader. Returns a caller-owned // PrerenderHandle* if the URL was added, NULL if it was not. // // The caller may set an observer on the handle to receive load events. When the // caller is done using the WebContents, it should call OnCancel() on the handle // to free the resources associated with the prerender. // // The caller must provide two guarantees: // 1. It must never ask to as for a swap-in; // 2. The SessionStorageNamespace must not be shared with any tab / page load // Â Â Â to avoid swapping in from there. https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1114: TEST_F(PrerenderTest, PrerenderForcedOriginOnCellular) { a more descriptive name woudl be preferred: PrerenderAllowedForOfflineAndForcedCellular
pasko@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: even though prerendering is not your focus now, I would appreciate if you could take a look at this. I plan to become an owner of chrome/browser/prerender/, but I do not feel confident enough yet, and would like to learn from you a little more.
Thanks Egor! I have addressed all your requested changes in the new CL. mmenke@chromium.org: PTAL. https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:1343: // settings because want to be able to snapshot webpages independently of On 2016/04/05 11:49:56, pasko wrote: > slight rephrasing: "because they are controlled by the offliner logic via > PrerenderHandle." Done. https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:154: // Adds a prerender from an external request that will always prerender. On 2016/04/05 11:49:56, pasko wrote: > This is not an "external request", calling it such is confusing (or maybe I > misunderstand the terminology). > > Please add the comment from the doc as we discussed, some obvious stuff can be > stripped. My suggestion: > > // Adds a prerender for the background loader. Returns a caller-owned > // PrerenderHandle* if the URL was added, NULL if it was not. > // > // The caller may set an observer on the handle to receive load events. When the > // caller is done using the WebContents, it should call OnCancel() on the handle > // to free the resources associated with the prerender. > // > // The caller must provide two guarantees: > // 1. It must never ask to as for a swap-in; > // 2. The SessionStorageNamespace must not be shared with any tab / page load > // to avoid swapping in from there. > My bad. Done. https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/1854643002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1114: TEST_F(PrerenderTest, PrerenderForcedOriginOnCellular) { On 2016/04/05 11:49:56, pasko wrote: > a more descriptive name woudl be preferred: > PrerenderAllowedForOfflineAndForcedCellular Done.
https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:545: DCHECK(prerender_contents->origin() != ORIGIN_OFFLINE); You don't seem to be enforcing this anywhere? Site instances shouldn't match, I guess, but I'm really no authority on that. https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:168: const gfx::Size& size); I expressed my concerns about the doc, but they're basically: * This is not a prerender. * You want to expose a rather different API here, where the consumer is the one that micromanages the prerender, rather than the PrerenderManager. It seems to me like it makes more sense for this new behavior and prerender to sit on top of the same API, rather than for this to sit on top of prerender.
https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_constants.h (right): https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_constants.h:16: const int kOfflinePrerenderLifeExpectancyMinutes = 20; dougarnett@ said: "5 minutes is plenty.", and the point about timeout was removed from the doc, so this should be changed back then.
prerender unit tests are pretty unreliable, so if we're going to go this route, I think we're going to need some browser tests: Use the offline origin for a URL, wait for the prerender to start, then quit. You're going to want extra magic for offline pages, so think this is something we'll need to avoid regressions. Use the new offline origin for a URL. Then navigate to that URL. The offline page should not have been used, and should still be prerendering. Then shut down the offline page, and wait for it to be deleted, to make sure normal cleanup works. Navigate to a URL, then use the new offline origin for the URL. The offline page should *not* be cancelled by RECENTLY_VISITIED. Prerender a URL, then use the offline origin for the same URL. Both prerenders should start. Navigate to the URL. The real prerender should be used, the offline origin should not be affected. Use the offline origin for a URL twice, make sure both go through (I'm not sure why you're explicitly allowing this, but if you are, it should be tested). I may be missing a few interesting cases.
On 2016/04/08 16:29:38, mmenke wrote:
> prerender unit tests are pretty unreliable, so if we're going to go this
route,
> I think we're going to need some browser tests:
>
> Use the offline origin for a URL, wait for the prerender to start, then quit.
> You're going to want extra magic for offline pages, so think this is something
> we'll need to avoid regressions.
Not sure to understand where the extra magic comes from. Would I have missed
something?
>
> Use the new offline origin for a URL. Then navigate to that URL. The offline
> page should not have been used, and should still be prerendering. Then shut
> down the offline page, and wait for it to be deleted, to make sure normal
> cleanup works.
PrerenderBrowserTest.{PrerenderSessionStorage,PrerenderPageNewTab} already do it
because the offliners would have its own session storage not shared with any
tabs.
>
> Navigate to a URL, then use the new offline origin for the URL. The offline
> page should *not* be cancelled by RECENTLY_VISITIED.
Good point. Might be doable with an preprender unittest thaw. It has been agreed
that offline is best-efforts and we may fight some corner cases to improve
offlining successrate. This to me, this point seams to be one easy of them.
Therefore my opinion is that it should be taken care of in a separate CL to let
the offline folks iterate with this existing API to make an offliner proof of
concept working (or show a concrete result why it would not work at all because
of something we have not though yet) with prerendering rather than going into
the utopia in making a perfect CL first.
>
> Prerender a URL, then use the offline origin for the same URL. Both
prerenders
> should start. Navigate to the URL. The real prerender should be used, the
> offline origin should not be affected.
I don't see the point of this test, both prerendering would have separate
session storage and will work. But we should add missing browser test for that.
>
> Use the offline origin for a URL twice, make sure both go through (I'm not
sure
> why you're explicitly allowing this, but if you are, it should be tested).
Not sure to understand. This CL would throw a duplicate final status. No need
for browser test for this that can be tested with unittests.
>
> I may be missing a few interesting cases.
I can't think of any cases for now that would be specially interesting with
offlinin. The offline prerendering would rely on the fact that the offliner
would use it own session storage. That it would be the exact same behavior as
cross tab prerendering conflicts.
https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:545: DCHECK(prerender_contents->origin() != ORIGIN_OFFLINE); On 2016/04/05 18:48:26, mmenke wrote: > You don't seem to be enforcing this anywhere? Site instances shouldn't match, I > guess, but I'm really no authority on that. We rely on the assemption that the offline should have its own namespace storage not shared with any tabs. Therefore this should never ever be happening. https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:168: const gfx::Size& size); On 2016/04/05 18:48:26, mmenke wrote: > I expressed my concerns about the doc, but they're basically: > > * This is not a prerender. > * You want to expose a rather different API here, where the consumer is the one > that micromanages the prerender, rather than the PrerenderManager. It seems to > me like it makes more sense for this new behavior and prerender to sit on top of > the same API, rather than for this to sit on top of prerender. Ass agreed offline with pasko, this point sounds like a lot of work for a first offline proof of concept, but legible in the long term in the case where the offliner using prerendering is a success. Moreover, with NoState-Prefetch implemented, my suspicion is we are going to need to refactor prerendering/prefetch/preconnect behind a same unique API. That could be the opportunity to do this change you are proposing.
On 2016/04/14 18:24:51, gabadie wrote:
> On 2016/04/08 16:29:38, mmenke wrote:
> > prerender unit tests are pretty unreliable, so if we're going to go this
> route,
> > I think we're going to need some browser tests:
> >
> > Use the offline origin for a URL, wait for the prerender to start, then
quit.
> > You're going to want extra magic for offline pages, so think this is
something
> > we'll need to avoid regressions.
> Not sure to understand where the extra magic comes from. Would I have missed
> something?
I'm concerned about changing cleanup / teardown behavior here, since my
understanding is you wanted to change some behaviors around how we keep them
around. In particular, I want to make sure they continue to be destroyed in
PrerenderManager::Shutdown. I'm not concerned about this CL breaking that, I'm
concerned about that breaking in the future.
> > Use the new offline origin for a URL. Then navigate to that URL. The
offline
> > page should not have been used, and should still be prerendering. Then shut
> > down the offline page, and wait for it to be deleted, to make sure normal
> > cleanup works.
> PrerenderBrowserTest.{PrerenderSessionStorage,PrerenderPageNewTab} already do
it
> because the offliners would have its own session storage not shared with any
> tabs.
> >
> > Navigate to a URL, then use the new offline origin for the URL. The offline
> > page should *not* be cancelled by RECENTLY_VISITIED.
> Good point. Might be doable with an preprender unittest thaw. It has been
agreed
> that offline is best-efforts and we may fight some corner cases to improve
> offlining successrate. This to me, this point seams to be one easy of them.
> Therefore my opinion is that it should be taken care of in a separate CL to
let
> the offline folks iterate with this existing API to make an offliner proof of
> concept working (or show a concrete result why it would not work at all
because
> of something we have not though yet) with prerendering rather than going into
> the utopia in making a perfect CL first.
A lot of this logic just doesn't make much sense in the offline case. Not sure
that basic correctness is all that utopian. Another case that seems problematic
is that the "BlockThirdPartyCookies" check completely disables prerenders when
enabled.
> > Prerender a URL, then use the offline origin for the same URL. Both
> prerenders
> > should start. Navigate to the URL. The real prerender should be used, the
> > offline origin should not be affected.
> I don't see the point of this test, both prerendering would have separate
> session storage and will work. But we should add missing browser test for
that.
Hrm, I'd forgotten that we don't just key on URL any more. There is still the
DoesRateLimitAllowPrerender check you added the workaround for, though.
> > Use the offline origin for a URL twice, make sure both go through (I'm not
> sure
> > why you're explicitly allowing this, but if you are, it should be tested).
> Not sure to understand. This CL would throw a duplicate final status. No need
> for browser test for this that can be tested with unittests.
> >
> > I may be missing a few interesting cases.
> I can't think of any cases for now that would be specially interesting with
> offlinin. The offline prerendering would rely on the fact that the offliner
> would use it own session storage. That it would be the exact same behavior as
> cross tab prerendering conflicts.
One other thing I'm concerned about regressing is timing out offline pages (Again, since you're planning to change timeout logic, not because of what you change in this CL). I could picture a future in which we accidentally regress that, and offline pages that, for whatever reason, never finish loading, remain in the background eternally, regressing performance with no one ever noticing due to the lack of UI surface area. Be great if we could manage a test for that, just to protect against such a regression. Not sure how well we test the "periodic cleanup" logic, currently, though.
On 2016/04/14 19:50:03, mmenke wrote:
> On 2016/04/14 18:24:51, gabadie wrote:
> > On 2016/04/08 16:29:38, mmenke wrote:
> > > prerender unit tests are pretty unreliable, so if we're going to go this
> > route,
> > > I think we're going to need some browser tests:
> > >
> > > Use the offline origin for a URL, wait for the prerender to start, then
> quit.
> > > You're going to want extra magic for offline pages, so think this is
> something
> > > we'll need to avoid regressions.
> > Not sure to understand where the extra magic comes from. Would I have missed
> > something?
>
> I'm concerned about changing cleanup / teardown behavior here, since my
> understanding is you wanted to change some behaviors around how we keep them
> around. In particular, I want to make sure they continue to be destroyed in
> PrerenderManager::Shutdown. I'm not concerned about this CL breaking that,
I'm
> concerned about that breaking in the future.
There is no change on how we keep them around, and I don’t see why we would need
one.
>
> > > Use the new offline origin for a URL. Then navigate to that URL. The
> offline
> > > page should not have been used, and should still be prerendering. Then
shut
> > > down the offline page, and wait for it to be deleted, to make sure normal
> > > cleanup works.
> > PrerenderBrowserTest.{PrerenderSessionStorage,PrerenderPageNewTab} already
do
> it
> > because the offliners would have its own session storage not shared with any
> > tabs.
> > >
> > > Navigate to a URL, then use the new offline origin for the URL. The
offline
> > > page should *not* be cancelled by RECENTLY_VISITIED.
> > Good point. Might be doable with an preprender unittest thaw. It has been
> agreed
> > that offline is best-efforts and we may fight some corner cases to improve
> > offlining successrate. This to me, this point seams to be one easy of them.
> > Therefore my opinion is that it should be taken care of in a separate CL to
> let
> > the offline folks iterate with this existing API to make an offliner proof
of
> > concept working (or show a concrete result why it would not work at all
> because
> > of something we have not though yet) with prerendering rather than going
into
> > the utopia in making a perfect CL first.
>
> A lot of this logic just doesn't make much sense in the offline case. Not
sure
> that basic correctness is all that utopian. Another case that seems
problematic
> is that the "BlockThirdPartyCookies" check completely disables prerenders when
> enabled.
Oh BlockThirdPartyCookies is brand new. I haven’t noticed its landing. Thanks!
>
> > > Prerender a URL, then use the offline origin for the same URL. Both
> > prerenders
> > > should start. Navigate to the URL. The real prerender should be used,
the
> > > offline origin should not be affected.
> > I don't see the point of this test, both prerendering would have separate
> > session storage and will work. But we should add missing browser test for
> that.
>
> Hrm, I'd forgotten that we don't just key on URL any more. There is still the
> DoesRateLimitAllowPrerender check you added the workaround for, though.
Good point.
>
> > > Use the offline origin for a URL twice, make sure both go through (I'm not
> > sure
> > > why you're explicitly allowing this, but if you are, it should be tested).
> > Not sure to understand. This CL would throw a duplicate final status. No
need
> > for browser test for this that can be tested with unittests.
> > >
> > > I may be missing a few interesting cases.
> > I can't think of any cases for now that would be specially interesting with
> > offlinin. The offline prerendering would rely on the fact that the offliner
> > would use it own session storage. That it would be the exact same behavior
as
> > cross tab prerendering conflicts.
On 2016/04/15 15:31:10, mmenke wrote:
> One other thing I'm concerned about regressing is timing out offline pages
> (Again, since you're planning to change timeout logic, not because of what you
> change in this CL). I could picture a future in which we accidentally regress
> that, and offline pages that, for whatever reason, never finish loading,
remain
> in the background eternally, regressing performance with no one ever noticing
> due to the lack of UI surface area. Be great if we could manage a test for
> that, just to protect against such a regression. Not sure how well we test
the
> "periodic cleanup" logic, currently, though.
Right, the test was missing, my bad. But after discussing with offline people,
turn out we don’t need an extended duration for timeout. FYI,
PrerenderTest.ExpireTest already test the existing timeout.
Here is the list of missing test in this CL:
FINAL_STATUS_RATE_LIMIT_EXCEEDED: But the test no longer exists in the first
place. :’(
FINAL_STATUS_BLOCK_THIRD_PARTY_COOKIES: Done in the new revision.
Here is a list of existing Prerender browser|unit test issues:
FINAL_STATUS_TOO_MANY_PROCESSES: missing tests from the begining
https://chromium.googlesource.com/chromium/src/+/c52ea7cfb37d45a6ad14d8ab61ab...
FINAL_STATUS_RATE_LIMIT_EXCEEDED: unittest have been removed by
https://chromium.googlesource.com/chromium/src/+/a2439eeab37f7cb7a118493fb55e...
FINAL_STATUS_CROSS_SITE_NAVIGATION_PENDING: missing tests from the begining
https://chromium.googlesource.com/chromium/src/+/9e1ad4b286a342d27726908a0df4...
FINAL_STATUS_DUPLICATE: not testing final status properly
FINAL_STATUS_NAVIGATION_INTERCEPTED: missing tests from the begining
https://chromium.googlesource.com/chromium/src/+/887f3f3d5ba1f105148b312ab9db...
FINAL_STATUS_CELLULAR_NETWORK: not testing final status properly
FINAL_STATUS_BLOCK_THIRD_PARTY_COOKIES: not testing final status properly
Uploaded a new revision removing the different timeout duration and taking care
of FINAL_STATUS_BLOCK_THIRD_PARTY_COOKIES. PTAL! =D
On 2016/04/19 13:08:55, gabadie wrote:
> On 2016/04/14 19:50:03, mmenke wrote:
> > On 2016/04/14 18:24:51, gabadie wrote:
> > > On 2016/04/08 16:29:38, mmenke wrote:
> > > > prerender unit tests are pretty unreliable, so if we're going to go this
> > > route,
> > > > I think we're going to need some browser tests:
> > > >
> > > > Use the offline origin for a URL, wait for the prerender to start, then
> > quit.
> > > > You're going to want extra magic for offline pages, so think this is
> > something
> > > > we'll need to avoid regressions.
> > > Not sure to understand where the extra magic comes from. Would I have
missed
> > > something?
> >
> > I'm concerned about changing cleanup / teardown behavior here, since my
> > understanding is you wanted to change some behaviors around how we keep them
> > around. In particular, I want to make sure they continue to be destroyed in
> > PrerenderManager::Shutdown. I'm not concerned about this CL breaking that,
> I'm
> > concerned about that breaking in the future.
> There is no change on how we keep them around, and I don’t see why we would
need
> one.
The "Offlining requests will need some different policy treatment" section
implies otherwise. There were some more explicit comments about affecting how
PeriodicCleanup works in an older draft as well.
> > > > Use the new offline origin for a URL. Then navigate to that URL. The
> > offline
> > > > page should not have been used, and should still be prerendering. Then
> shut
> > > > down the offline page, and wait for it to be deleted, to make sure
normal
> > > > cleanup works.
> > > PrerenderBrowserTest.{PrerenderSessionStorage,PrerenderPageNewTab} already
> do
> > it
> > > because the offliners would have its own session storage not shared with
any
> > > tabs.
> > > >
> > > > Navigate to a URL, then use the new offline origin for the URL. The
> offline
> > > > page should *not* be cancelled by RECENTLY_VISITIED.
> > > Good point. Might be doable with an preprender unittest thaw. It has been
> > agreed
> > > that offline is best-efforts and we may fight some corner cases to improve
> > > offlining successrate. This to me, this point seams to be one easy of
them.
> > > Therefore my opinion is that it should be taken care of in a separate CL
to
> > let
> > > the offline folks iterate with this existing API to make an offliner proof
> of
> > > concept working (or show a concrete result why it would not work at all
> > because
> > > of something we have not though yet) with prerendering rather than going
> into
> > > the utopia in making a perfect CL first.
> >
> > A lot of this logic just doesn't make much sense in the offline case. Not
> sure
> > that basic correctness is all that utopian. Another case that seems
> problematic
> > is that the "BlockThirdPartyCookies" check completely disables prerenders
when
> > enabled.
> Oh BlockThirdPartyCookies is brand new. I haven’t noticed its landing. Thanks!
> >
> > > > Prerender a URL, then use the offline origin for the same URL. Both
> > > prerenders
> > > > should start. Navigate to the URL. The real prerender should be used,
> the
> > > > offline origin should not be affected.
> > > I don't see the point of this test, both prerendering would have separate
> > > session storage and will work. But we should add missing browser test for
> > that.
> >
> > Hrm, I'd forgotten that we don't just key on URL any more. There is still
the
> > DoesRateLimitAllowPrerender check you added the workaround for, though.
> Good point.
> >
> > > > Use the offline origin for a URL twice, make sure both go through (I'm
not
> > > sure
> > > > why you're explicitly allowing this, but if you are, it should be
tested).
> > > Not sure to understand. This CL would throw a duplicate final status. No
> need
> > > for browser test for this that can be tested with unittests.
> > > >
> > > > I may be missing a few interesting cases.
> > > I can't think of any cases for now that would be specially interesting
with
> > > offlinin. The offline prerendering would rely on the fact that the
offliner
> > > would use it own session storage. That it would be the exact same behavior
> as
> > > cross tab prerendering conflicts.
>
> On 2016/04/15 15:31:10, mmenke wrote:
> > One other thing I'm concerned about regressing is timing out offline pages
> > (Again, since you're planning to change timeout logic, not because of what
you
> > change in this CL). I could picture a future in which we accidentally
regress
> > that, and offline pages that, for whatever reason, never finish loading,
> remain
> > in the background eternally, regressing performance with no one ever
noticing
> > due to the lack of UI surface area. Be great if we could manage a test for
> > that, just to protect against such a regression. Not sure how well we test
> the
> > "periodic cleanup" logic, currently, though.
> Right, the test was missing, my bad. But after discussing with offline people,
> turn out we don’t need an extended duration for timeout. FYI,
> PrerenderTest.ExpireTest already test the existing timeout.
>
> Here is the list of missing test in this CL:
> FINAL_STATUS_RATE_LIMIT_EXCEEDED: But the test no longer exists in the first
> place. :’(
> FINAL_STATUS_BLOCK_THIRD_PARTY_COOKIES: Done in the new revision.
>
> Here is a list of existing Prerender browser|unit test issues:
> FINAL_STATUS_TOO_MANY_PROCESSES: missing tests from the begining
>
https://chromium.googlesource.com/chromium/src/+/c52ea7cfb37d45a6ad14d8ab61ab...
> FINAL_STATUS_RATE_LIMIT_EXCEEDED: unittest have been removed by
>
https://chromium.googlesource.com/chromium/src/+/a2439eeab37f7cb7a118493fb55e...
> FINAL_STATUS_CROSS_SITE_NAVIGATION_PENDING: missing tests from the begining
>
https://chromium.googlesource.com/chromium/src/+/9e1ad4b286a342d27726908a0df4...
> FINAL_STATUS_DUPLICATE: not testing final status properly
> FINAL_STATUS_NAVIGATION_INTERCEPTED: missing tests from the begining
>
https://chromium.googlesource.com/chromium/src/+/887f3f3d5ba1f105148b312ab9db...
> FINAL_STATUS_CELLULAR_NETWORK: not testing final status properly
> FINAL_STATUS_BLOCK_THIRD_PARTY_COOKIES: not testing final status properly
>
> Uploaded a new revision removing the different timeout duration and taking
care
> of FINAL_STATUS_BLOCK_THIRD_PARTY_COOKIES. PTAL! =D
Sorry for slowness - took two days off, and came back to 14 codereviews. I'll
try to get to looking at this CL today, but not sure I'll have time.
LGTM https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:544: DCHECK(prerender_contents->origin() != ORIGIN_OFFLINE); DCHECK_NE https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:544: DCHECK(prerender_contents->origin() != ORIGIN_OFFLINE); Can we make this a CHECK? I'm generally not a fan of CHECKs, but this is a sufficiently nebulous condition that I want a firmer guarantee we aren't breaking anything. Or could keep this as a DCHECK, and make FindPrerenderData not return prerenders with the new origin. https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:1189: // TODO(gabadie,pasko): Re-implements missing tests for nit: implements -> implement https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:162: // 1. It must never ask to as for a swap-in; -"to as" https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:430: TEST_F(PrerenderTest, OfflinePrerenderDontRespectsThirdPartyCookiesPref) { Maybe call this OfflinePrerenderIgnoresThirdPartyCookiesPref?
Thanks Matt! I have addressed all your comments in the new revision. pasko@chromium.org: Is this last revision still look good to you? https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:544: DCHECK(prerender_contents->origin() != ORIGIN_OFFLINE); On 2016/04/22 16:41:33, mmenke wrote: > Can we make this a CHECK? I'm generally not a fan of CHECKs, but this is a > sufficiently nebulous condition that I want a firmer guarantee we aren't > breaking anything. > > Or could keep this as a DCHECK, and make FindPrerenderData not return prerenders > with the new origin. Done. https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:544: DCHECK(prerender_contents->origin() != ORIGIN_OFFLINE); On 2016/04/22 16:41:33, mmenke wrote: > DCHECK_NE Done. https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:1189: // TODO(gabadie,pasko): Re-implements missing tests for On 2016/04/22 16:41:33, mmenke wrote: > nit: implements -> implement Done. https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:162: // 1. It must never ask to as for a swap-in; On 2016/04/22 16:41:33, mmenke wrote: > -"to as" Done. https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/1854643002/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:430: TEST_F(PrerenderTest, OfflinePrerenderDontRespectsThirdPartyCookiesPref) { On 2016/04/22 16:41:33, mmenke wrote: > Maybe call this OfflinePrerenderIgnoresThirdPartyCookiesPref? Done.
The CQ bit was checked by pasko@chromium.org
lgtm lgtm with 2 changes in tests nice! https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:544: CHECK_NE(ORIGIN_OFFLINE, prerender_contents->origin()); This comment does not propose to change anything, just thoughts and sharing common hopefully-wisdom: Just as mmenke@ I am not a fan of CHECKs, so FindPrerenderData() not returning stuff with offline origin would be safer, though it'd look If we see a CHECK in crash reports, we do not have much context with it. I think what we would do is patch FindPrerenderData() when/if this happens. Let's see. https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:430: TEST_F(PrerenderTest, OfflinePrerenderDontRespectsThirdPartyCookiesPref) { s/DontRespects/DoesntRespect/ https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:1132: TEST_F(PrerenderTest, OfflinePrerenderIgnoresThirdPartyCookiesPref) { This test has nothing to do with cookies pref, right? SomePrerenderOriginsAllowedOnCellular
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1854643002/#ps180001 (title: "Addresses mmenke's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1854643002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1854643002/180001
The CQ bit was unchecked by pasko@chromium.org
clicked on CQ by accident, discarding CQ now
Thanks for the review. Here is the CL that replace the CHECK with FindPrerenderData not returning Offline prerenders. https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:544: CHECK_NE(ORIGIN_OFFLINE, prerender_contents->origin()); On 2016/04/25 11:29:11, pasko wrote: > This comment does not propose to change anything, just thoughts and sharing > common hopefully-wisdom: > > Just as mmenke@ I am not a fan of CHECKs, so FindPrerenderData() not returning > stuff with offline origin would be safer, though it'd look > > If we see a CHECK in crash reports, we do not have much context with it. I think > what we would do is patch FindPrerenderData() when/if this happens. Let's see. I have uploaded a new CL that removed that CHECK, but added a test that FindPrerenderData() no longer find offline originated prerenders. https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:430: TEST_F(PrerenderTest, OfflinePrerenderDontRespectsThirdPartyCookiesPref) { On 2016/04/25 11:29:11, pasko wrote: > s/DontRespects/DoesntRespect/ Done. https://codereview.chromium.org/1854643002/diff/180001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:1132: TEST_F(PrerenderTest, OfflinePrerenderIgnoresThirdPartyCookiesPref) { On 2016/04/25 11:29:11, pasko wrote: > This test has nothing to do with cookies pref, right? > > SomePrerenderOriginsAllowedOnCellular Oh oups. I have mixed up mmenke's renaming proposal with the test above. Done. https://codereview.chromium.org/1854643002/diff/200001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/200001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:989: url, origin, FINAL_STATUS_DUPLICATE); Duplicated offline prerender would no longer be detected because relying on FindPrerenderData(). But since the offliner should take care of having only one prerender at a time... Is it really an issue? I don't think so.
still lgtm, thank you https://codereview.chromium.org/1854643002/diff/200001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/1854643002/diff/200001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:989: url, origin, FINAL_STATUS_DUPLICATE); On 2016/04/25 12:35:12, gabadie wrote: > Duplicated offline prerender would no longer be detected because relying on > FindPrerenderData(). But since the offliner should take care of having only one > prerender at a time... Is it really an issue? I don't think so. I think it is good enough. In worst case we run them concurrently .. with other restrictions offline has (in background, charging, etc). I don't see how we could misbehave in this case.
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1854643002/#ps200001 (title: "Makes offline originated prerenders unfindable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1854643002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1854643002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
gabadie@chromium.org changed reviewers: + mpearson@chromium.org
Hey Mark, Would you mind rubber stamp histograms.xml? =)
On 2016/04/25 13:22:22, gabadie wrote: > Hey Mark, > > Would you mind rubber stamp histograms.xml? =) I meant, reviewing. =D
Though only a two-line histogram.xml change, it turned out to definitely need a review, not a rubber stamp. :-P --mark https://codereview.chromium.org/1854643002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1854643002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:92207: + <suffix name="offline" Uh, you put this additional suffix in a place so that it will immediately be marked obsolete. Oops. Please fix.
Oups. Ok I totaly failed on histograms.xml ^^' I have fixed it accordingly. mpearson@chromium.org: PTAL https://codereview.chromium.org/1854643002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1854643002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:92207: + <suffix name="offline" On 2016/04/25 20:40:41, Mark P wrote: > Uh, you put this additional suffix in a place so that it will immediately be > marked obsolete. Oops. Please fix. Done.
histograms.xml lgtm
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1854643002/#ps240001 (title: "Fixes histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1854643002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1854643002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gabadie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1854643002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1854643002/300001
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 gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, mpearson@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1854643002/#ps300001 (title: "Fixes PrerenderAllowedForOfflineAndForcedCellular")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1854643002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1854643002/300001
Message was sent while issue was closed.
Description was changed from ========== Implement PrerenderManager::AddPrerenderForOffline() Offline's operation of fetching and snapshoting a web page is going to be experienced with Prerender. This CL add the offline Prerender origin in that purpose with the PrerenderManager::AddPrerenderForOffline() method. BUG=599500 ========== to ========== Implement PrerenderManager::AddPrerenderForOffline() Offline's operation of fetching and snapshoting a web page is going to be experienced with Prerender. This CL add the offline Prerender origin in that purpose with the PrerenderManager::AddPrerenderForOffline() method. BUG=599500 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/fde8d42d16c0fac930e7fe39e86cb3ef6d24e9c8 Cr-Commit-Position: refs/heads/master@{#390370}
Message was sent while issue was closed.
Description was changed from ========== Implement PrerenderManager::AddPrerenderForOffline() Offline's operation of fetching and snapshoting a web page is going to be experienced with Prerender. This CL add the offline Prerender origin in that purpose with the PrerenderManager::AddPrerenderForOffline() method. BUG=599500 ========== to ========== Implement PrerenderManager::AddPrerenderForOffline() Offline's operation of fetching and snapshoting a web page is going to be experienced with Prerender. This CL add the offline Prerender origin in that purpose with the PrerenderManager::AddPrerenderForOffline() method. BUG=599500 Committed: https://crrev.com/fde8d42d16c0fac930e7fe39e86cb3ef6d24e9c8 Cr-Commit-Position: refs/heads/master@{#390370} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
