|
|
Created:
4 years, 3 months ago by jianli Modified:
4 years, 2 months ago CC:
chromium-reviews, nasko+codewatch_chromium.org, creis+watch_chromium.org, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, jam, petewil+watch_chromium.org, chili+watch_chromium.org, darin-cc_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPersist offline page info in a navigation entry if needed
The offline page info from extra request headers needs to be persisted
in serialized navigation entry in order for it to be restored when
Chrome restarts. Otherwise, we may be opening an online page without
this info.
The design doc for this can be found here: https://docs.google.com/document/d/1vz0a9N-9egdm3yMDYMHcNVmQHOL8Qglz54lgG4ATwhc
BUG=644553
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/f62ca6f9d33e93733fccb3f3815d9554429dbb38
Cr-Commit-Position: refs/heads/master@{#422542}
Patch Set 1 : Patch #Patch Set 2 : Rebase #
Total comments: 10
Patch Set 3 : Address feedback #
Total comments: 10
Patch Set 4 : Fix #
Total comments: 4
Patch Set 5 : Write field by field to pickle #Patch Set 6 : Fix gn deps #Patch Set 7 : Add extended info support #
Total comments: 10
Patch Set 8 : Fix #Patch Set 9 : Add missing new file #
Total comments: 1
Patch Set 10 : Update #
Total comments: 25
Patch Set 11 : Address feedback #Patch Set 12 : Fix trybot #Patch Set 13 : Update comment regarding separator in extra headers #Patch Set 14 : Fix win compiling error due to using unique_ptr map in SESSIONS_EXPORT class #Messages
Total messages: 101 (67 generated)
Description was changed from ========== Persist offline page info in a navigation entry if needed The offline page info from extra request headers needs to be persisted in serialized navigation entry in order for it to be restored when Chrome restarts. Otherwise, we may be opening an online page without this info. BUG=644553 ========== to ========== Persist offline page info in a navigation entry if needed The offline page info from extra request headers needs to be persisted in serialized navigation entry in order for it to be restored when Chrome restarts. Otherwise, we may be opening an online page without this info. BUG=644553 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jianli@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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jianli@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...
jianli@chromium.org changed reviewers: + nasko@chromium.org, sky@chromium.org
sky for sessions nasko for contents
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:22: const char kOfflinePageHeader[] = "X-Chrome-offline"; Doesn't the full header value include ":" as well? I don't see it accounted for anywhere. https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:26: // TODO(jianli): Move this logic to components/offline_pages/request. Is this TODO supposed to be resolved in later patchset or entirely different CL? I'd suggest the former. https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:31: // The offline header will be the only extra header if it is present. There is no code that enforces that. Wouldn't adding another header in front of the X-Chrome-offline one totally break your feature silently? https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:37: std::string value = extra_headers.substr(arraysize(kOfflinePageHeader) - 1); Doesn't arraysize require including base/macros.h? https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:91: extra_headers = kOfflinePageHeader + navigation->offline_page_info_; Shouldn't header name and value be separated by ":"?
Forgot to add. Can you make the short design doc publicly available and linked from the CL description?
Description was changed from ========== Persist offline page info in a navigation entry if needed The offline page info from extra request headers needs to be persisted in serialized navigation entry in order for it to be restored when Chrome restarts. Otherwise, we may be opening an online page without this info. BUG=644553 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist offline page info in a navigation entry if needed The offline page info from extra request headers needs to be persisted in serialized navigation entry in order for it to be restored when Chrome restarts. Otherwise, we may be opening an online page without this info. The design doc for this can be found here: https://docs.google.com/document/d/1vz0a9N-9egdm3yMDYMHcNVmQHOL8Qglz54lgG4ATwhc BUG=644553 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The description has been updated with the link to the doc. https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:22: const char kOfflinePageHeader[] = "X-Chrome-offline"; On 2016/09/09 21:42:48, nasko (slow) wrote: > Doesn't the full header value include ":" as well? I don't see it accounted for > anywhere. Done. https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:26: // TODO(jianli): Move this logic to components/offline_pages/request. On 2016/09/09 21:42:48, nasko (slow) wrote: > Is this TODO supposed to be resolved in later patchset or entirely different CL? > I'd suggest the former. It will be in a different CL since doing it now in this patch will involve restructure components/offline_pages directory which might make it hard to merge to M54. https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:31: // The offline header will be the only extra header if it is present. On 2016/09/09 21:42:48, nasko (slow) wrote: > There is no code that enforces that. Wouldn't adding another header in front of > the X-Chrome-offline one totally break your feature silently? Most of time extra request header will not be sent. It will only be sent in a few limited scenarios, like offline pages or data saver mode. It is not likely that another header is also added together with offline header. Per discussion with creis, we feel that limiting the extra header to only single offline header will make the code more secure. https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:37: std::string value = extra_headers.substr(arraysize(kOfflinePageHeader) - 1); On 2016/09/09 21:42:48, nasko (slow) wrote: > Doesn't arraysize require including base/macros.h? Done. https://codereview.chromium.org/2310363002/diff/40001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:91: extra_headers = kOfflinePageHeader + navigation->offline_page_info_; On 2016/09/09 21:42:48, nasko (slow) wrote: > Shouldn't header name and value be separated by ":"? Done.
The CQ bit was checked by jianli@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...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jianli@chromium.org changed reviewers: + dimich@chromium.org
dimich, could you please review offline page changes?
lgtm
https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:23: const char kOfflinePageHeader[] = "X-Chrome-offline: "; Is X-Chrome-offline and persist=1 defined somewhere that this code should be using? https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:32: // The offline header will be the only extra header if it is present. How do you know that this is true? https://codereview.chromium.org/2310363002/diff/60001/components/sessions/cor... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2310363002/diff/60001/components/sessions/cor... components/sessions/core/serialized_navigation_entry.h:125: const std::string& offline_page_info() const { return offline_page_info_; } This needs better documentation. What is offline_page_info? What does it correspond to?
https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:23: const char kOfflinePageHeader[] = "X-Chrome-offline: "; On 2016/09/12 15:04:21, sky wrote: > Is X-Chrome-offline and persist=1 defined somewhere that this code should be > using? "X-Chrome-offline" is defined in offline_page_request_job.cc and "persist=1" is first introduced in this patch. I have another patch which adds one more field to "X-Chrome-offline" header and that patch contains all the definitions plus header parsing. The reason for using "X-Chrome-offline" and "persist=1" defined in this patch is to allow easier merge to M54 branch. Per TODO at line 27, I am going to move all the header definition and parsing into a common directory components/offline_pages/request after all these 2 patches get landed. The reason for not doing the move right now is that I need to reorganize those files under components/offline_pages. https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:32: // The offline header will be the only extra header if it is present. On 2016/09/12 15:04:21, sky wrote: > How do you know that this is true? Most of the time extra request header is set in some custom fetcher requests, that do not correspond to a navigation. For offline pages, it is set when we try to open a downloaded page in which case no other extra header will be set. https://codereview.chromium.org/2310363002/diff/60001/components/sessions/cor... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2310363002/diff/60001/components/sessions/cor... components/sessions/core/serialized_navigation_entry.h:125: const std::string& offline_page_info() const { return offline_page_info_; } On 2016/09/12 15:04:22, sky wrote: > This needs better documentation. What is offline_page_info? What does it > correspond to? Done.
The CQ bit was checked by jianli@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/2310363002/diff/60001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:32: // The offline header will be the only extra header if it is present. On 2016/09/12 21:17:46, jianli wrote: > On 2016/09/12 15:04:21, sky wrote: > > How do you know that this is true? > > Most of the time extra request header is set in some custom fetcher requests, > that do not correspond to a navigation. For offline pages, it is set when we try > to open a downloaded page in which case no other extra header will be set. What's to stop a page from using these headers? https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... components/sessions/core/serialized_navigation_entry.h:129: const std::string& offline_page_info() const { return offline_page_info_; } Why do we want to persist this as a string rather than something more meaningful? Enum for example?
https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:32: // The offline header will be the only extra header if it is present. On 2016/09/13 01:53:31, sky wrote: > On 2016/09/12 21:17:46, jianli wrote: > > On 2016/09/12 15:04:21, sky wrote: > > > How do you know that this is true? > > > > Most of the time extra request header is set in some custom fetcher requests, > > that do not correspond to a navigation. For offline pages, it is set when we > try > > to open a downloaded page in which case no other extra header will be set. > > What's to stop a page from using these headers? We want to keep these headers to allow loading the same offline page in order to provide consistent experience to the user if these headers are set. Note that these headers are only set in a few special scenarios. For example, when the user opens a downloaded page from the Download home. Normally these headers are not set when the user navigates to a page. https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... components/sessions/core/serialized_navigation_entry.h:129: const std::string& offline_page_info() const { return offline_page_info_; } On 2016/09/13 01:53:31, sky wrote: > Why do we want to persist this as a string rather than something more > meaningful? Enum for example? We have more than one properties needed to persist, like reason and id. In the future, we might want to add some more properties.
https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:32: // The offline header will be the only extra header if it is present. On 2016/09/13 20:29:18, jianli wrote: > On 2016/09/13 01:53:31, sky wrote: > > On 2016/09/12 21:17:46, jianli wrote: > > > On 2016/09/12 15:04:21, sky wrote: > > > > How do you know that this is true? > > > > > > Most of the time extra request header is set in some custom fetcher > requests, > > > that do not correspond to a navigation. For offline pages, it is set when we > > try > > > to open a downloaded page in which case no other extra header will be set. > > > > What's to stop a page from using these headers? > > We want to keep these headers to allow loading the same offline page in order to > provide consistent experience to the user if these headers are set. Note that > these headers are only set in a few special scenarios. For example, when the > user opens a downloaded page from the Download home. > > Normally these headers are not set when the user navigates to a page. Perhaps I wasn't clear. Let me rephrase my question. If I'm a website author and I serve up a page with X-Chrome-offline: persist=1 then won't we automatically persist the page? Shouldn't we only persist pages that Chrome chooses to persist? https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... components/sessions/core/serialized_navigation_entry.h:129: const std::string& offline_page_info() const { return offline_page_info_; } On 2016/09/13 20:29:18, jianli wrote: > On 2016/09/13 01:53:31, sky wrote: > > Why do we want to persist this as a string rather than something more > > meaningful? Enum for example? > > We have more than one properties needed to persist, like reason and id. In the > future, we might want to add some more properties. The down side of a string is that it's easy to miss an incompatible change and not realize the effects that may have. A real structure/enum is easier to see changes in and realize what the effects may be.
https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder.cc:32: // The offline header will be the only extra header if it is present. On 2016/09/13 23:30:42, sky wrote: > On 2016/09/13 20:29:18, jianli wrote: > > On 2016/09/13 01:53:31, sky wrote: > > > On 2016/09/12 21:17:46, jianli wrote: > > > > On 2016/09/12 15:04:21, sky wrote: > > > > > How do you know that this is true? > > > > > > > > Most of the time extra request header is set in some custom fetcher > > requests, > > > > that do not correspond to a navigation. For offline pages, it is set when > we > > > try > > > > to open a downloaded page in which case no other extra header will be set. > > > > > > What's to stop a page from using these headers? > > > > We want to keep these headers to allow loading the same offline page in order > to > > provide consistent experience to the user if these headers are set. Note that > > these headers are only set in a few special scenarios. For example, when the > > user opens a downloaded page from the Download home. > > > > Normally these headers are not set when the user navigates to a page. > > Perhaps I wasn't clear. Let me rephrase my question. If I'm a website author and > I serve up a page with X-Chrome-offline: persist=1 then won't we automatically > persist the page? Shouldn't we only persist pages that Chrome chooses to > persist? No, it is not possible for a web to do something like this since it only comes from extra request header which could only be set in the client code. https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... components/sessions/core/serialized_navigation_entry.h:129: const std::string& offline_page_info() const { return offline_page_info_; } On 2016/09/13 23:30:42, sky wrote: > On 2016/09/13 20:29:18, jianli wrote: > > On 2016/09/13 01:53:31, sky wrote: > > > Why do we want to persist this as a string rather than something more > > > meaningful? Enum for example? > > > > We have more than one properties needed to persist, like reason and id. In the > > future, we might want to add some more properties. > > The down side of a string is that it's easy to miss an incompatible change and > not realize the effects that may have. A real structure/enum is easier to see > changes in and realize what the effects may be. How do we want to persist the struct? Do we still want to serialize the struct into a string and then persist it? I can move OfflinePageHeader struct out of offline_page_request_job.cc and use it here. But I still need to build OfflinePageHeader struct from extra request header in ContentSerializedNavigationBuilder::FromNavigationEntry.
On 2016/09/13 23:44:16, jianli wrote: > https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... > File components/sessions/content/content_serialized_navigation_builder.cc > (right): > > https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... > components/sessions/content/content_serialized_navigation_builder.cc:32: // The > offline header will be the only extra header if it is present. > On 2016/09/13 23:30:42, sky wrote: > > On 2016/09/13 20:29:18, jianli wrote: > > > On 2016/09/13 01:53:31, sky wrote: > > > > On 2016/09/12 21:17:46, jianli wrote: > > > > > On 2016/09/12 15:04:21, sky wrote: > > > > > > How do you know that this is true? > > > > > > > > > > Most of the time extra request header is set in some custom fetcher > > > requests, > > > > > that do not correspond to a navigation. For offline pages, it is set > when > > we > > > > try > > > > > to open a downloaded page in which case no other extra header will be > set. > > > > > > > > What's to stop a page from using these headers? > > > > > > We want to keep these headers to allow loading the same offline page in > order > > to > > > provide consistent experience to the user if these headers are set. Note > that > > > these headers are only set in a few special scenarios. For example, when the > > > user opens a downloaded page from the Download home. > > > > > > Normally these headers are not set when the user navigates to a page. > > > > Perhaps I wasn't clear. Let me rephrase my question. If I'm a website author > and > > I serve up a page with X-Chrome-offline: persist=1 then won't we automatically > > persist the page? Shouldn't we only persist pages that Chrome chooses to > > persist? > > No, it is not possible for a web to do something like this since it only comes > from extra request header which could only be set in the client code. Ah, ok. That makes sense. > > https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... > File components/sessions/core/serialized_navigation_entry.h (right): > > https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... > components/sessions/core/serialized_navigation_entry.h:129: const std::string& > offline_page_info() const { return offline_page_info_; } > On 2016/09/13 23:30:42, sky wrote: > > On 2016/09/13 20:29:18, jianli wrote: > > > On 2016/09/13 01:53:31, sky wrote: > > > > Why do we want to persist this as a string rather than something more > > > > meaningful? Enum for example? > > > > > > We have more than one properties needed to persist, like reason and id. In > the > > > future, we might want to add some more properties. > > > > The down side of a string is that it's easy to miss an incompatible change and > > not realize the effects that may have. A real structure/enum is easier to see > > changes in and realize what the effects may be. > > How do we want to persist the struct? Do we still want to serialize the struct > into a string and then persist it? Field by field into the pickle. > I can move OfflinePageHeader struct out of offline_page_request_job.cc and use > it here. But I still need to build OfflinePageHeader struct from extra request > header in ContentSerializedNavigationBuilder::FromNavigationEntry.
The CQ bit was checked by jianli@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...
On 2016/09/13 23:53:57, sky wrote: > On 2016/09/13 23:44:16, jianli wrote: > > > https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... > > File components/sessions/content/content_serialized_navigation_builder.cc > > (right): > > > > > https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... > > components/sessions/content/content_serialized_navigation_builder.cc:32: // > The > > offline header will be the only extra header if it is present. > > On 2016/09/13 23:30:42, sky wrote: > > > On 2016/09/13 20:29:18, jianli wrote: > > > > On 2016/09/13 01:53:31, sky wrote: > > > > > On 2016/09/12 21:17:46, jianli wrote: > > > > > > On 2016/09/12 15:04:21, sky wrote: > > > > > > > How do you know that this is true? > > > > > > > > > > > > Most of the time extra request header is set in some custom fetcher > > > > requests, > > > > > > that do not correspond to a navigation. For offline pages, it is set > > when > > > we > > > > > try > > > > > > to open a downloaded page in which case no other extra header will be > > set. > > > > > > > > > > What's to stop a page from using these headers? > > > > > > > > We want to keep these headers to allow loading the same offline page in > > order > > > to > > > > provide consistent experience to the user if these headers are set. Note > > that > > > > these headers are only set in a few special scenarios. For example, when > the > > > > user opens a downloaded page from the Download home. > > > > > > > > Normally these headers are not set when the user navigates to a page. > > > > > > Perhaps I wasn't clear. Let me rephrase my question. If I'm a website author > > and > > > I serve up a page with X-Chrome-offline: persist=1 then won't we > automatically > > > persist the page? Shouldn't we only persist pages that Chrome chooses to > > > persist? > > > > No, it is not possible for a web to do something like this since it only comes > > from extra request header which could only be set in the client code. > > Ah, ok. That makes sense. > > > > > > https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... > > File components/sessions/core/serialized_navigation_entry.h (right): > > > > > https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... > > components/sessions/core/serialized_navigation_entry.h:129: const std::string& > > offline_page_info() const { return offline_page_info_; } > > On 2016/09/13 23:30:42, sky wrote: > > > On 2016/09/13 20:29:18, jianli wrote: > > > > On 2016/09/13 01:53:31, sky wrote: > > > > > Why do we want to persist this as a string rather than something more > > > > > meaningful? Enum for example? > > > > > > > > We have more than one properties needed to persist, like reason and id. In > > the > > > > future, we might want to add some more properties. > > > > > > The down side of a string is that it's easy to miss an incompatible change > and > > > not realize the effects that may have. A real structure/enum is easier to > see > > > changes in and realize what the effects may be. > > > > How do we want to persist the struct? Do we still want to serialize the struct > > into a string and then persist it? > > Field by field into the pickle. Done. Please review again. Thanks. > > > I can move OfflinePageHeader struct out of offline_page_request_job.cc and use > > it here. But I still need to build OfflinePageHeader struct from extra request > > header in ContentSerializedNavigationBuilder::FromNavigationEntry.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jianli@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by jianli@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...
I'll take a look at this first thing tomorrow. Quick question first though. Is this feature for all platforms? Thanks, -Scott On Tue, Sep 20, 2016 at 6:00 PM, <jianli@chromium.org> wrote: > On 2016/09/13 23:53:57, sky wrote: >> On 2016/09/13 23:44:16, jianli wrote: >> > >> > https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... >> > File >> > components/sessions/content/content_serialized_navigation_builder.cc >> > (right): >> > >> > >> > https://codereview.chromium.org/2310363002/diff/60001/components/sessions/con... >> > components/sessions/content/content_serialized_navigation_builder.cc:32: >> > // >> The >> > offline header will be the only extra header if it is present. >> > On 2016/09/13 23:30:42, sky wrote: >> > > On 2016/09/13 20:29:18, jianli wrote: >> > > > On 2016/09/13 01:53:31, sky wrote: >> > > > > On 2016/09/12 21:17:46, jianli wrote: >> > > > > > On 2016/09/12 15:04:21, sky wrote: >> > > > > > > How do you know that this is true? >> > > > > > >> > > > > > Most of the time extra request header is set in some custom >> > > > > > fetcher >> > > > requests, >> > > > > > that do not correspond to a navigation. For offline pages, it is >> > > > > > set >> > when >> > > we >> > > > > try >> > > > > > to open a downloaded page in which case no other extra header >> > > > > > will > be >> > set. >> > > > > >> > > > > What's to stop a page from using these headers? >> > > > >> > > > We want to keep these headers to allow loading the same offline page >> > > > in >> > order >> > > to >> > > > provide consistent experience to the user if these headers are set. >> > > > Note >> > that >> > > > these headers are only set in a few special scenarios. For example, >> > > > when >> the >> > > > user opens a downloaded page from the Download home. >> > > > >> > > > Normally these headers are not set when the user navigates to a >> > > > page. >> > > >> > > Perhaps I wasn't clear. Let me rephrase my question. If I'm a website > author >> > and >> > > I serve up a page with X-Chrome-offline: persist=1 then won't we >> automatically >> > > persist the page? Shouldn't we only persist pages that Chrome chooses >> > > to >> > > persist? >> > >> > No, it is not possible for a web to do something like this since it only > comes >> > from extra request header which could only be set in the client code. >> >> Ah, ok. That makes sense. >> >> > >> > >> > https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... >> > File components/sessions/core/serialized_navigation_entry.h (right): >> > >> > >> > https://codereview.chromium.org/2310363002/diff/80001/components/sessions/cor... >> > components/sessions/core/serialized_navigation_entry.h:129: const > std::string& >> > offline_page_info() const { return offline_page_info_; } >> > On 2016/09/13 23:30:42, sky wrote: >> > > On 2016/09/13 20:29:18, jianli wrote: >> > > > On 2016/09/13 01:53:31, sky wrote: >> > > > > Why do we want to persist this as a string rather than something >> > > > > more >> > > > > meaningful? Enum for example? >> > > > >> > > > We have more than one properties needed to persist, like reason and >> > > > id. > In >> > the >> > > > future, we might want to add some more properties. >> > > >> > > The down side of a string is that it's easy to miss an incompatible >> > > change >> and >> > > not realize the effects that may have. A real structure/enum is easier >> > > to >> see >> > > changes in and realize what the effects may be. >> > >> > How do we want to persist the struct? Do we still want to serialize the > struct >> > into a string and then persist it? >> >> Field by field into the pickle. > > Done. Please review again. Thanks. > >> >> > I can move OfflinePageHeader struct out of offline_page_request_job.cc >> > and > use >> > it here. But I still need to build OfflinePageHeader struct from extra > request >> > header in ContentSerializedNavigationBuilder::FromNavigationEntry. > > > > https://codereview.chromium.org/2310363002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On Tue, Sep 20, 2016 at 8:08 PM, Scott Violet <sky@chromium.org> wrote: > I'll take a look at this first thing tomorrow. Quick question first > though. Is this feature for all platforms? > No. It is only for Android at this point. > > Thanks, > > -Scott > > On Tue, Sep 20, 2016 at 6:00 PM, <jianli@chromium.org> wrote: > > On 2016/09/13 23:53:57, sky wrote: > >> On 2016/09/13 23:44:16, jianli wrote: > >> > > >> > > https://codereview.chromium.org/2310363002/diff/60001/ > components/sessions/content/content_serialized_navigation_builder.cc > >> > File > >> > components/sessions/content/content_serialized_navigation_builder.cc > >> > (right): > >> > > >> > > >> > > https://codereview.chromium.org/2310363002/diff/60001/ > components/sessions/content/content_serialized_navigation_ > builder.cc#newcode32 > >> > components/sessions/content/content_serialized_navigation_ > builder.cc:32: > >> > // > >> The > >> > offline header will be the only extra header if it is present. > >> > On 2016/09/13 23:30:42, sky wrote: > >> > > On 2016/09/13 20:29:18, jianli wrote: > >> > > > On 2016/09/13 01:53:31, sky wrote: > >> > > > > On 2016/09/12 21:17:46, jianli wrote: > >> > > > > > On 2016/09/12 15:04:21, sky wrote: > >> > > > > > > How do you know that this is true? > >> > > > > > > >> > > > > > Most of the time extra request header is set in some custom > >> > > > > > fetcher > >> > > > requests, > >> > > > > > that do not correspond to a navigation. For offline pages, it > is > >> > > > > > set > >> > when > >> > > we > >> > > > > try > >> > > > > > to open a downloaded page in which case no other extra header > >> > > > > > will > > be > >> > set. > >> > > > > > >> > > > > What's to stop a page from using these headers? > >> > > > > >> > > > We want to keep these headers to allow loading the same offline > page > >> > > > in > >> > order > >> > > to > >> > > > provide consistent experience to the user if these headers are > set. > >> > > > Note > >> > that > >> > > > these headers are only set in a few special scenarios. For > example, > >> > > > when > >> the > >> > > > user opens a downloaded page from the Download home. > >> > > > > >> > > > Normally these headers are not set when the user navigates to a > >> > > > page. > >> > > > >> > > Perhaps I wasn't clear. Let me rephrase my question. If I'm a > website > > author > >> > and > >> > > I serve up a page with X-Chrome-offline: persist=1 then won't we > >> automatically > >> > > persist the page? Shouldn't we only persist pages that Chrome > chooses > >> > > to > >> > > persist? > >> > > >> > No, it is not possible for a web to do something like this since it > only > > comes > >> > from extra request header which could only be set in the client code. > >> > >> Ah, ok. That makes sense. > >> > >> > > >> > > >> > > https://codereview.chromium.org/2310363002/diff/80001/ > components/sessions/core/serialized_navigation_entry.h > >> > File components/sessions/core/serialized_navigation_entry.h (right): > >> > > >> > > >> > > https://codereview.chromium.org/2310363002/diff/80001/ > components/sessions/core/serialized_navigation_entry.h#newcode129 > >> > components/sessions/core/serialized_navigation_entry.h:129: const > > std::string& > >> > offline_page_info() const { return offline_page_info_; } > >> > On 2016/09/13 23:30:42, sky wrote: > >> > > On 2016/09/13 20:29:18, jianli wrote: > >> > > > On 2016/09/13 01:53:31, sky wrote: > >> > > > > Why do we want to persist this as a string rather than something > >> > > > > more > >> > > > > meaningful? Enum for example? > >> > > > > >> > > > We have more than one properties needed to persist, like reason > and > >> > > > id. > > In > >> > the > >> > > > future, we might want to add some more properties. > >> > > > >> > > The down side of a string is that it's easy to miss an incompatible > >> > > change > >> and > >> > > not realize the effects that may have. A real structure/enum is > easier > >> > > to > >> see > >> > > changes in and realize what the effects may be. > >> > > >> > How do we want to persist the struct? Do we still want to serialize > the > > struct > >> > into a string and then persist it? > >> > >> Field by field into the pickle. > > > > Done. Please review again. Thanks. > > > >> > >> > I can move OfflinePageHeader struct out of offline_page_request_job.cc > >> > and > > use > >> > it here. But I still need to build OfflinePageHeader struct from extra > > request > >> > header in ContentSerializedNavigationBuilder::FromNavigationEntry. > > > > > > > > https://codereview.chromium.org/2310363002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/09/21 19:49:52, jianli wrote: > On Tue, Sep 20, 2016 at 8:08 PM, Scott Violet <mailto:sky@chromium.org> wrote: > > > I'll take a look at this first thing tomorrow. Quick question first > > though. Is this feature for all platforms? > > > > No. It is only for Android at this point. > > > > > Thanks, > > > > -Scott > > > > On Tue, Sep 20, 2016 at 6:00 PM, <mailto:jianli@chromium.org> wrote: > > > On 2016/09/13 23:53:57, sky wrote: > > >> On 2016/09/13 23:44:16, jianli wrote: > > >> > > > >> > > > https://codereview.chromium.org/2310363002/diff/60001/ > > components/sessions/content/content_serialized_navigation_builder.cc > > >> > File > > >> > components/sessions/content/content_serialized_navigation_builder.cc > > >> > (right): > > >> > > > >> > > > >> > > > https://codereview.chromium.org/2310363002/diff/60001/ > > components/sessions/content/content_serialized_navigation_ > > builder.cc#newcode32 > > >> > components/sessions/content/content_serialized_navigation_ > > builder.cc:32: > > >> > // > > >> The > > >> > offline header will be the only extra header if it is present. > > >> > On 2016/09/13 23:30:42, sky wrote: > > >> > > On 2016/09/13 20:29:18, jianli wrote: > > >> > > > On 2016/09/13 01:53:31, sky wrote: > > >> > > > > On 2016/09/12 21:17:46, jianli wrote: > > >> > > > > > On 2016/09/12 15:04:21, sky wrote: > > >> > > > > > > How do you know that this is true? > > >> > > > > > > > >> > > > > > Most of the time extra request header is set in some custom > > >> > > > > > fetcher > > >> > > > requests, > > >> > > > > > that do not correspond to a navigation. For offline pages, it > > is > > >> > > > > > set > > >> > when > > >> > > we > > >> > > > > try > > >> > > > > > to open a downloaded page in which case no other extra header > > >> > > > > > will > > > be > > >> > set. > > >> > > > > > > >> > > > > What's to stop a page from using these headers? > > >> > > > > > >> > > > We want to keep these headers to allow loading the same offline > > page > > >> > > > in > > >> > order > > >> > > to > > >> > > > provide consistent experience to the user if these headers are > > set. > > >> > > > Note > > >> > that > > >> > > > these headers are only set in a few special scenarios. For > > example, > > >> > > > when > > >> the > > >> > > > user opens a downloaded page from the Download home. > > >> > > > > > >> > > > Normally these headers are not set when the user navigates to a > > >> > > > page. > > >> > > > > >> > > Perhaps I wasn't clear. Let me rephrase my question. If I'm a > > website > > > author > > >> > and > > >> > > I serve up a page with X-Chrome-offline: persist=1 then won't we > > >> automatically > > >> > > persist the page? Shouldn't we only persist pages that Chrome > > chooses > > >> > > to > > >> > > persist? > > >> > > > >> > No, it is not possible for a web to do something like this since it > > only > > > comes > > >> > from extra request header which could only be set in the client code. > > >> > > >> Ah, ok. That makes sense. > > >> > > >> > > > >> > > > >> > > > https://codereview.chromium.org/2310363002/diff/80001/ > > components/sessions/core/serialized_navigation_entry.h > > >> > File components/sessions/core/serialized_navigation_entry.h (right): > > >> > > > >> > > > >> > > > https://codereview.chromium.org/2310363002/diff/80001/ > > components/sessions/core/serialized_navigation_entry.h#newcode129 > > >> > components/sessions/core/serialized_navigation_entry.h:129: const > > > std::string& > > >> > offline_page_info() const { return offline_page_info_; } > > >> > On 2016/09/13 23:30:42, sky wrote: > > >> > > On 2016/09/13 20:29:18, jianli wrote: > > >> > > > On 2016/09/13 01:53:31, sky wrote: > > >> > > > > Why do we want to persist this as a string rather than something > > >> > > > > more > > >> > > > > meaningful? Enum for example? > > >> > > > > > >> > > > We have more than one properties needed to persist, like reason > > and > > >> > > > id. > > > In > > >> > the > > >> > > > future, we might want to add some more properties. > > >> > > > > >> > > The down side of a string is that it's easy to miss an incompatible > > >> > > change > > >> and > > >> > > not realize the effects that may have. A real structure/enum is > > easier > > >> > > to > > >> see > > >> > > changes in and realize what the effects may be. > > >> > > > >> > How do we want to persist the struct? Do we still want to serialize > > the > > > struct > > >> > into a string and then persist it? > > >> > > >> Field by field into the pickle. > > > > > > Done. Please review again. Thanks. > > > > > >> > > >> > I can move OfflinePageHeader struct out of offline_page_request_job.cc > > >> > and > > > use > > >> > it here. But I still need to build OfflinePageHeader struct from extra > > > request > > >> > header in ContentSerializedNavigationBuilder::FromNavigationEntry. > > > > > > > > > > > > https://codereview.chromium.org/2310363002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. In that case then I'm going to suggest a more flexible way to embed random things in restore. How about making it so SerializedNavigationEntry has a map<string,string> of extra information that core session restore knows nothing about, but can read/write appropriately? I'm thinking of the following: ContentSerializedNavigationDriver has a way to register a handler per string, say something like: void RegisterExtendedInfoHandler(const std::string& key, std::unique_ptr<ExtendedInfoHandler> handler); ExtendedInfoHandler has functions to read/write the data given a NavigationEntry: virtual std::string GetExtraInfo(const content::NavigationEntry& entry); virtual void RestoreExtraInfo(const std::string& extran_info, content::NavigationEntry* entry); ContentSerializedNavigationBuilder handles querying the ExtendedInfoHandlers appropriately for their data. This leaves maintaining compatibility entirely up to the handler, but results in a more flexible way to experiment with state like you want. WDYT?
Generally I like this approach. But there is one problem that prevents the restoring into NavigationEntry work. The offline info is indeed stored in extra request header which is indeed defined in NavigationEntryImpl, instead of NavigationEntry. To work around this, this patch: 1) For restore, construct the extra request header and pass it to content::NavigationController::CreateNavigationEntry 2) For saving, we only expose GetExtraHeaders in NavigationEntry. Unless we also choose to expose SetExtraHeaders in NavigationEntry, ExtendedInfoHandler::RestoreExtraInfo cannot write the extra header in NavigationEntry. On Wed, Sep 21, 2016 at 1:37 PM, <sky@chromium.org> wrote: > On 2016/09/21 19:49:52, jianli wrote: > > On Tue, Sep 20, 2016 at 8:08 PM, Scott Violet <mailto:sky@chromium.org> > wrote: > > > > > I'll take a look at this first thing tomorrow. Quick question first > > > though. Is this feature for all platforms? > > > > > > > No. It is only for Android at this point. > > > > > > > > Thanks, > > > > > > -Scott > > > > > > On Tue, Sep 20, 2016 at 6:00 PM, <mailto:jianli@chromium.org> wrote: > > > > On 2016/09/13 23:53:57, sky wrote: > > > >> On 2016/09/13 23:44:16, jianli wrote: > > > >> > > > > >> > > > > https://codereview.chromium.org/2310363002/diff/60001/ > > > components/sessions/content/content_serialized_navigation_builder.cc > > > >> > File > > > >> > components/sessions/content/content_serialized_navigation_ > builder.cc > > > >> > (right): > > > >> > > > > >> > > > > >> > > > > https://codereview.chromium.org/2310363002/diff/60001/ > > > components/sessions/content/content_serialized_navigation_ > > > builder.cc#newcode32 > > > >> > components/sessions/content/content_serialized_navigation_ > > > builder.cc:32: > > > >> > // > > > >> The > > > >> > offline header will be the only extra header if it is present. > > > >> > On 2016/09/13 23:30:42, sky wrote: > > > >> > > On 2016/09/13 20:29:18, jianli wrote: > > > >> > > > On 2016/09/13 01:53:31, sky wrote: > > > >> > > > > On 2016/09/12 21:17:46, jianli wrote: > > > >> > > > > > On 2016/09/12 15:04:21, sky wrote: > > > >> > > > > > > How do you know that this is true? > > > >> > > > > > > > > >> > > > > > Most of the time extra request header is set in some > custom > > > >> > > > > > fetcher > > > >> > > > requests, > > > >> > > > > > that do not correspond to a navigation. For offline > pages, it > > > is > > > >> > > > > > set > > > >> > when > > > >> > > we > > > >> > > > > try > > > >> > > > > > to open a downloaded page in which case no other extra > header > > > >> > > > > > will > > > > be > > > >> > set. > > > >> > > > > > > > >> > > > > What's to stop a page from using these headers? > > > >> > > > > > > >> > > > We want to keep these headers to allow loading the same > offline > > > page > > > >> > > > in > > > >> > order > > > >> > > to > > > >> > > > provide consistent experience to the user if these headers are > > > set. > > > >> > > > Note > > > >> > that > > > >> > > > these headers are only set in a few special scenarios. For > > > example, > > > >> > > > when > > > >> the > > > >> > > > user opens a downloaded page from the Download home. > > > >> > > > > > > >> > > > Normally these headers are not set when the user navigates to > a > > > >> > > > page. > > > >> > > > > > >> > > Perhaps I wasn't clear. Let me rephrase my question. If I'm a > > > website > > > > author > > > >> > and > > > >> > > I serve up a page with X-Chrome-offline: persist=1 then won't we > > > >> automatically > > > >> > > persist the page? Shouldn't we only persist pages that Chrome > > > chooses > > > >> > > to > > > >> > > persist? > > > >> > > > > >> > No, it is not possible for a web to do something like this since > it > > > only > > > > comes > > > >> > from extra request header which could only be set in the client > code. > > > >> > > > >> Ah, ok. That makes sense. > > > >> > > > >> > > > > >> > > > > >> > > > > https://codereview.chromium.org/2310363002/diff/80001/ > > > components/sessions/core/serialized_navigation_entry.h > > > >> > File components/sessions/core/serialized_navigation_entry.h > (right): > > > >> > > > > >> > > > > >> > > > > https://codereview.chromium.org/2310363002/diff/80001/ > > > components/sessions/core/serialized_navigation_entry.h#newcode129 > > > >> > components/sessions/core/serialized_navigation_entry.h:129: const > > > > std::string& > > > >> > offline_page_info() const { return offline_page_info_; } > > > >> > On 2016/09/13 23:30:42, sky wrote: > > > >> > > On 2016/09/13 20:29:18, jianli wrote: > > > >> > > > On 2016/09/13 01:53:31, sky wrote: > > > >> > > > > Why do we want to persist this as a string rather than > something > > > >> > > > > more > > > >> > > > > meaningful? Enum for example? > > > >> > > > > > > >> > > > We have more than one properties needed to persist, like > reason > > > and > > > >> > > > id. > > > > In > > > >> > the > > > >> > > > future, we might want to add some more properties. > > > >> > > > > > >> > > The down side of a string is that it's easy to miss an > incompatible > > > >> > > change > > > >> and > > > >> > > not realize the effects that may have. A real structure/enum is > > > easier > > > >> > > to > > > >> see > > > >> > > changes in and realize what the effects may be. > > > >> > > > > >> > How do we want to persist the struct? Do we still want to > serialize > > > the > > > > struct > > > >> > into a string and then persist it? > > > >> > > > >> Field by field into the pickle. > > > > > > > > Done. Please review again. Thanks. > > > > > > > >> > > > >> > I can move OfflinePageHeader struct out of > offline_page_request_job.cc > > > >> > and > > > > use > > > >> > it here. But I still need to build OfflinePageHeader struct from > extra > > > > request > > > >> > header in ContentSerializedNavigationBui > lder::FromNavigationEntry. > > > > > > > > > > > > > > > > https://codereview.chromium.org/2310363002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > In that case then I'm going to suggest a more flexible way to embed random > things in restore. How about making it so SerializedNavigationEntry has a > map<string,string> of extra information that core session restore knows > nothing > about, but can read/write appropriately? > > I'm thinking of the following: ContentSerializedNavigationDriver has a > way to > register a handler per string, say something like: > > void RegisterExtendedInfoHandler(const std::string& key, > std::unique_ptr<ExtendedInfoHandler> handler); > > ExtendedInfoHandler has functions to read/write the data given a > NavigationEntry: > > virtual std::string GetExtraInfo(const content::NavigationEntry& entry); > virtual void RestoreExtraInfo(const std::string& extran_info, > content::NavigationEntry* entry); > > ContentSerializedNavigationBuilder handles querying the > ExtendedInfoHandlers > appropriately for their data. > > This leaves maintaining compatibility entirely up to the handler, but > results in > a more flexible way to experiment with state like you want. WDYT? > > https://codereview.chromium.org/2310363002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by jianli@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...
On 2016/09/21 20:37:17, sky wrote: > On 2016/09/21 19:49:52, jianli wrote: > > On Tue, Sep 20, 2016 at 8:08 PM, Scott Violet <mailto:sky@chromium.org> wrote: > > > > > I'll take a look at this first thing tomorrow. Quick question first > > > though. Is this feature for all platforms? > > > > > > > No. It is only for Android at this point. > > > > > > > > Thanks, > > > > > > -Scott > > > > > > On Tue, Sep 20, 2016 at 6:00 PM, <mailto:jianli@chromium.org> wrote: > > > > On 2016/09/13 23:53:57, sky wrote: > > > >> On 2016/09/13 23:44:16, jianli wrote: > > > >> > > > > >> > > > > https://codereview.chromium.org/2310363002/diff/60001/ > > > components/sessions/content/content_serialized_navigation_builder.cc > > > >> > File > > > >> > components/sessions/content/content_serialized_navigation_builder.cc > > > >> > (right): > > > >> > > > > >> > > > > >> > > > > https://codereview.chromium.org/2310363002/diff/60001/ > > > components/sessions/content/content_serialized_navigation_ > > > builder.cc#newcode32 > > > >> > components/sessions/content/content_serialized_navigation_ > > > builder.cc:32: > > > >> > // > > > >> The > > > >> > offline header will be the only extra header if it is present. > > > >> > On 2016/09/13 23:30:42, sky wrote: > > > >> > > On 2016/09/13 20:29:18, jianli wrote: > > > >> > > > On 2016/09/13 01:53:31, sky wrote: > > > >> > > > > On 2016/09/12 21:17:46, jianli wrote: > > > >> > > > > > On 2016/09/12 15:04:21, sky wrote: > > > >> > > > > > > How do you know that this is true? > > > >> > > > > > > > > >> > > > > > Most of the time extra request header is set in some custom > > > >> > > > > > fetcher > > > >> > > > requests, > > > >> > > > > > that do not correspond to a navigation. For offline pages, it > > > is > > > >> > > > > > set > > > >> > when > > > >> > > we > > > >> > > > > try > > > >> > > > > > to open a downloaded page in which case no other extra header > > > >> > > > > > will > > > > be > > > >> > set. > > > >> > > > > > > > >> > > > > What's to stop a page from using these headers? > > > >> > > > > > > >> > > > We want to keep these headers to allow loading the same offline > > > page > > > >> > > > in > > > >> > order > > > >> > > to > > > >> > > > provide consistent experience to the user if these headers are > > > set. > > > >> > > > Note > > > >> > that > > > >> > > > these headers are only set in a few special scenarios. For > > > example, > > > >> > > > when > > > >> the > > > >> > > > user opens a downloaded page from the Download home. > > > >> > > > > > > >> > > > Normally these headers are not set when the user navigates to a > > > >> > > > page. > > > >> > > > > > >> > > Perhaps I wasn't clear. Let me rephrase my question. If I'm a > > > website > > > > author > > > >> > and > > > >> > > I serve up a page with X-Chrome-offline: persist=1 then won't we > > > >> automatically > > > >> > > persist the page? Shouldn't we only persist pages that Chrome > > > chooses > > > >> > > to > > > >> > > persist? > > > >> > > > > >> > No, it is not possible for a web to do something like this since it > > > only > > > > comes > > > >> > from extra request header which could only be set in the client code. > > > >> > > > >> Ah, ok. That makes sense. > > > >> > > > >> > > > > >> > > > > >> > > > > https://codereview.chromium.org/2310363002/diff/80001/ > > > components/sessions/core/serialized_navigation_entry.h > > > >> > File components/sessions/core/serialized_navigation_entry.h (right): > > > >> > > > > >> > > > > >> > > > > https://codereview.chromium.org/2310363002/diff/80001/ > > > components/sessions/core/serialized_navigation_entry.h#newcode129 > > > >> > components/sessions/core/serialized_navigation_entry.h:129: const > > > > std::string& > > > >> > offline_page_info() const { return offline_page_info_; } > > > >> > On 2016/09/13 23:30:42, sky wrote: > > > >> > > On 2016/09/13 20:29:18, jianli wrote: > > > >> > > > On 2016/09/13 01:53:31, sky wrote: > > > >> > > > > Why do we want to persist this as a string rather than something > > > >> > > > > more > > > >> > > > > meaningful? Enum for example? > > > >> > > > > > > >> > > > We have more than one properties needed to persist, like reason > > > and > > > >> > > > id. > > > > In > > > >> > the > > > >> > > > future, we might want to add some more properties. > > > >> > > > > > >> > > The down side of a string is that it's easy to miss an incompatible > > > >> > > change > > > >> and > > > >> > > not realize the effects that may have. A real structure/enum is > > > easier > > > >> > > to > > > >> see > > > >> > > changes in and realize what the effects may be. > > > >> > > > > >> > How do we want to persist the struct? Do we still want to serialize > > > the > > > > struct > > > >> > into a string and then persist it? > > > >> > > > >> Field by field into the pickle. > > > > > > > > Done. Please review again. Thanks. > > > > > > > >> > > > >> > I can move OfflinePageHeader struct out of offline_page_request_job.cc > > > >> > and > > > > use > > > >> > it here. But I still need to build OfflinePageHeader struct from extra > > > > request > > > >> > header in ContentSerializedNavigationBuilder::FromNavigationEntry. > > > > > > > > > > > > > > > > https://codereview.chromium.org/2310363002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > In that case then I'm going to suggest a more flexible way to embed random > things in restore. How about making it so SerializedNavigationEntry has a > map<string,string> of extra information that core session restore knows nothing > about, but can read/write appropriately? > > I'm thinking of the following: ContentSerializedNavigationDriver has a way to > register a handler per string, say something like: > > void RegisterExtendedInfoHandler(const std::string& key, > std::unique_ptr<ExtendedInfoHandler> handler); > > ExtendedInfoHandler has functions to read/write the data given a > NavigationEntry: > > virtual std::string GetExtraInfo(const content::NavigationEntry& entry); > virtual void RestoreExtraInfo(const std::string& extran_info, > content::NavigationEntry* entry); > > ContentSerializedNavigationBuilder handles querying the ExtendedInfoHandlers > appropriately for their data. > > This leaves maintaining compatibility entirely up to the handler, but results in > a more flexible way to experiment with state like you want. WDYT? Changed to using ExtendedInfoHandler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... File components/sessions/core/serialized_navigation_driver.h (right): https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:24: class SESSIONS_EXPORT SerializedNavigationDriver { This code can't depend upon content. Your changes should be in the content side only. https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:26: // The interface that handles how the extended info is converted betweened the How about: This interface is used to store and retrieve arbitrary key/value pairs for a NavigationEntry that are not a core part of NavigationEntry. WARNING: implementations must deal with versioning. In particular RestoreExtendedInfo() may be called with data from a previous version of Chrome. https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:29: class SESSIONS_EXPORT ExtendedInfoHandler { Move this into it's own header. https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:77: // Registers a handler that could be used to read and write the extended 'could be' -> is https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:83: typedef std::map<std::string, std::unique_ptr<ExtendedInfoHandler>> using.
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... File components/sessions/core/serialized_navigation_driver.h (right): https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:24: class SESSIONS_EXPORT SerializedNavigationDriver { On 2016/09/27 22:30:27, sky wrote: > This code can't depend upon content. Your changes should be in the content side > only. Done. https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:26: // The interface that handles how the extended info is converted betweened the On 2016/09/27 22:30:27, sky wrote: > How about: > This interface is used to store and retrieve arbitrary key/value pairs for a > NavigationEntry that are not a core part of NavigationEntry. WARNING: > implementations must deal with versioning. In particular RestoreExtendedInfo() > may be called with data from a previous version of Chrome. Done. https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:29: class SESSIONS_EXPORT ExtendedInfoHandler { On 2016/09/27 22:30:27, sky wrote: > Move this into it's own header. Done. https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:77: // Registers a handler that could be used to read and write the extended On 2016/09/27 22:30:27, sky wrote: > 'could be' -> is Done. https://codereview.chromium.org/2310363002/diff/160001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:83: typedef std::map<std::string, std::unique_ptr<ExtendedInfoHandler>> On 2016/09/27 22:30:27, sky wrote: > using. Done.
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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jianli@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2310363002/diff/200001/components/sessions/co... File components/sessions/core/serialized_navigation_driver.h (right): https://codereview.chromium.org/2310363002/diff/200001/components/sessions/co... components/sessions/core/serialized_navigation_driver.h:63: virtual void RegisterExtendedInfoHandler( Can you clarify why you want to add these functions here? The functionality is content specific and should only be in the content side of things. Unless I'm missing something you should *not* change this code at all, all your changes should be in components/sessions/content. These functions should be non-virtual on ContentSerializedNavigationDriver.
On Tue, Sep 27, 2016 at 5:22 PM, <sky@chromium.org> wrote: > > https://codereview.chromium.org/2310363002/diff/200001/ > components/sessions/core/serialized_navigation_driver.h > File components/sessions/core/serialized_navigation_driver.h (right): > > https://codereview.chromium.org/2310363002/diff/200001/ > components/sessions/core/serialized_navigation_driver.h#newcode63 > components/sessions/core/serialized_navigation_driver.h:63: virtual void > RegisterExtendedInfoHandler( > Can you clarify why you want to add these functions here? The > functionality is content specific and should only be in the content side > of things. Unless I'm missing something you should *not* change this > code at all, all your changes should be in components/sessions/content. > These functions should be non-virtual on > ContentSerializedNavigationDriver. > It seems that there is not a way to get ContentSerializedNavigationDriver instance for all platforms. We're getting the single SerializedNavigationDriver instance via SerializedNavigationDriver.::Get(). ContentSerializedNavigationBuilder applies to both non-ios and ios. So I can't downcast SerializedNavigationDriver.::Get() to ContentSerializedNavigationDriver even if you choose to. > > https://codereview.chromium.org/2310363002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
See my earlier email. I suggested you put this on ContentSerializedNavigationDriver, which is just a bunch of static functions. Ios doesn't use content, but that isn't an issue as you only care about android. -Scott On Tue, Sep 27, 2016 at 5:46 PM, Jian Li <jianli@chromium.org> wrote: > > On Tue, Sep 27, 2016 at 5:22 PM, <sky@chromium.org> wrote: >> >> >> >> https://codereview.chromium.org/2310363002/diff/200001/components/sessions/co... >> File components/sessions/core/serialized_navigation_driver.h (right): >> >> >> https://codereview.chromium.org/2310363002/diff/200001/components/sessions/co... >> components/sessions/core/serialized_navigation_driver.h:63: virtual void >> RegisterExtendedInfoHandler( >> Can you clarify why you want to add these functions here? The >> functionality is content specific and should only be in the content side >> of things. Unless I'm missing something you should *not* change this >> code at all, all your changes should be in components/sessions/content. >> These functions should be non-virtual on >> ContentSerializedNavigationDriver. > > > It seems that there is not a way to get ContentSerializedNavigationDriver > instance for all platforms. We're getting the single > SerializedNavigationDriver instance via SerializedNavigationDriver.::Get(). > ContentSerializedNavigationBuilder applies to both non-ios and ios. So I > can't downcast SerializedNavigationDriver.::Get() to > ContentSerializedNavigationDriver even if you choose to. >> >> >> https://codereview.chromium.org/2310363002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
On 2016/09/28 17:01:10, sky wrote: > See my earlier email. I suggested you put this on > ContentSerializedNavigationDriver, which is just a bunch of static > functions. > > Ios doesn't use content, but that isn't an issue as you only care about > android. > > -Scott > > On Tue, Sep 27, 2016 at 5:46 PM, Jian Li <mailto:jianli@chromium.org> wrote: > > > > On Tue, Sep 27, 2016 at 5:22 PM, <mailto:sky@chromium.org> wrote: > >> > >> > >> > >> > https://codereview.chromium.org/2310363002/diff/200001/components/sessions/co... > >> File components/sessions/core/serialized_navigation_driver.h (right): > >> > >> > >> > https://codereview.chromium.org/2310363002/diff/200001/components/sessions/co... > >> components/sessions/core/serialized_navigation_driver.h:63: virtual void > >> RegisterExtendedInfoHandler( > >> Can you clarify why you want to add these functions here? The > >> functionality is content specific and should only be in the content side > >> of things. Unless I'm missing something you should *not* change this > >> code at all, all your changes should be in components/sessions/content. > >> These functions should be non-virtual on > >> ContentSerializedNavigationDriver. > > > > > > It seems that there is not a way to get ContentSerializedNavigationDriver > > instance for all platforms. We're getting the single > > SerializedNavigationDriver instance via SerializedNavigationDriver.::Get(). > > ContentSerializedNavigationBuilder applies to both non-ios and ios. So I > > can't downcast SerializedNavigationDriver.::Get() to > > ContentSerializedNavigationDriver even if you choose to. > >> > >> > >> https://codereview.chromium.org/2310363002/ > > > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Done. Thanks.
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder.cc:53: navigation.extended_info_map_[handler_entry.first] = I think you should only add the entry if GetExtendedInfo() returns a non-empty string. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder.cc:93: for (const auto& handler_entry : Rather than iterating over the handlers I think it more natural to iterate over the extended_info_map_ as that is the thing that needs to be mapped. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder.cc:101: const std::string& value = navigation->extended_info_map_.at(key); at -> [key] ? https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/content_serialized_navigation_builder_unittest.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder_unittest.cc:39: const std::string& info, content::NavigationEntry* entry) override { Write test coverage to ensure this is called too. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder_unittest.cc:44: }; DISALLOW. Also, run git cl format as your spacing looks off. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/content_serialized_navigation_driver.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_driver.cc:140: DCHECK(!key.empty()); Please also add a DCHECK that there isn't a handler registered for key already and that handler.get() is non-null. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/content_serialized_navigation_driver.h (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_driver.h:46: // info stored in SerializedNavigationEntry. Also add 'As part of serialization |key| is written to disk, as such once a handler is registered it should always be registered to the same key.' https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/extended_info_handler.h (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/extended_info_handler.h:27: // Returns the extended info from NavigationEntry for How about: "Returns the data to write to disk for the specified NavigationEntry." https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/extended_info_handler.h:32: // Puts the extended info, extracted from SerializedNavigationEntry, into And similar to the above, "Restores |info| which was obtained from a previous call to GetExtendedInfo() to a NavigationEntry. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/core/serialized_navigation_entry.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/core/serialized_navigation_entry.cc:351: extended_info_map_size >= 0) { > 0? https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/core/serialized_navigation_entry.cc:358: } else { I don't think you need the else case as everything should be reset by line 272. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/core/serialized_navigation_entry.h:127: #if !defined(OS_IOS) I'm nervous about making this non-ios specific. Seems easier to write it everywhere. On ios it'll be empty, but that's ok, right? https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/core/serialized_navigation_entry.h:168: // The map that tracks all the extended info that need to be persisted, but How about: provides storage for arbitrary key/value pairs used by features. This data is not synced.
https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder.cc:53: navigation.extended_info_map_[handler_entry.first] = On 2016/09/29 03:08:19, sky wrote: > I think you should only add the entry if GetExtendedInfo() returns a non-empty > string. Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder.cc:93: for (const auto& handler_entry : On 2016/09/29 03:08:19, sky wrote: > Rather than iterating over the handlers I think it more natural to iterate over > the extended_info_map_ as that is the thing that needs to be mapped. Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder.cc:101: const std::string& value = navigation->extended_info_map_.at(key); On 2016/09/29 03:08:19, sky wrote: > at -> [key] ? I can't use operator[] since navigation is const type. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/content_serialized_navigation_builder_unittest.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder_unittest.cc:39: const std::string& info, content::NavigationEntry* entry) override { On 2016/09/29 03:08:19, sky wrote: > Write test coverage to ensure this is called too. Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_builder_unittest.cc:44: }; On 2016/09/29 03:08:19, sky wrote: > DISALLOW. > Also, run git cl format as your spacing looks off. Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/content_serialized_navigation_driver.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_driver.cc:140: DCHECK(!key.empty()); On 2016/09/29 03:08:19, sky wrote: > Please also add a DCHECK that there isn't a handler registered for key already > and that handler.get() is non-null. Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/content_serialized_navigation_driver.h (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/content_serialized_navigation_driver.h:46: // info stored in SerializedNavigationEntry. On 2016/09/29 03:08:19, sky wrote: > Also add 'As part of serialization |key| is written to disk, as such once a > handler is registered it should always be registered to the same key.' Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/content/extended_info_handler.h (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/extended_info_handler.h:27: // Returns the extended info from NavigationEntry for On 2016/09/29 03:08:19, sky wrote: > How about: "Returns the data to write to disk for the specified > NavigationEntry." Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/content/extended_info_handler.h:32: // Puts the extended info, extracted from SerializedNavigationEntry, into On 2016/09/29 03:08:19, sky wrote: > And similar to the above, "Restores |info| which was obtained from a previous > call to GetExtendedInfo() to a NavigationEntry. Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/core/serialized_navigation_entry.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/core/serialized_navigation_entry.cc:351: extended_info_map_size >= 0) { On 2016/09/29 03:08:19, sky wrote: > > 0? Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/core/serialized_navigation_entry.cc:358: } else { On 2016/09/29 03:08:19, sky wrote: > I don't think you need the else case as everything should be reset by line 272. Done. https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... File components/sessions/core/serialized_navigation_entry.h (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/co... components/sessions/core/serialized_navigation_entry.h:127: #if !defined(OS_IOS) On 2016/09/29 03:08:20, sky wrote: > I'm nervous about making this non-ios specific. Seems easier to write it > everywhere. On ios it'll be empty, but that's ok, right? Yes. Removed #if.
The CQ bit was checked by jianli@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: 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...)
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
LGTM - thanks for the patience.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nasko, could you please review again for the new change to NavigationEntry? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
content/ LGTM
The CQ bit was checked by jianli@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.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, sky@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2310363002/#ps320001 (title: "Fix win compiling error due to using unique_ptr map in SESSIONS_EXPORT class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Persist offline page info in a navigation entry if needed The offline page info from extra request headers needs to be persisted in serialized navigation entry in order for it to be restored when Chrome restarts. Otherwise, we may be opening an online page without this info. The design doc for this can be found here: https://docs.google.com/document/d/1vz0a9N-9egdm3yMDYMHcNVmQHOL8Qglz54lgG4ATwhc BUG=644553 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist offline page info in a navigation entry if needed The offline page info from extra request headers needs to be persisted in serialized navigation entry in order for it to be restored when Chrome restarts. Otherwise, we may be opening an online page without this info. The design doc for this can be found here: https://docs.google.com/document/d/1vz0a9N-9egdm3yMDYMHcNVmQHOL8Qglz54lgG4ATwhc BUG=644553 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #14 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Persist offline page info in a navigation entry if needed The offline page info from extra request headers needs to be persisted in serialized navigation entry in order for it to be restored when Chrome restarts. Otherwise, we may be opening an online page without this info. The design doc for this can be found here: https://docs.google.com/document/d/1vz0a9N-9egdm3yMDYMHcNVmQHOL8Qglz54lgG4ATwhc BUG=644553 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist offline page info in a navigation entry if needed The offline page info from extra request headers needs to be persisted in serialized navigation entry in order for it to be restored when Chrome restarts. Otherwise, we may be opening an online page without this info. The design doc for this can be found here: https://docs.google.com/document/d/1vz0a9N-9egdm3yMDYMHcNVmQHOL8Qglz54lgG4ATwhc BUG=644553 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/f62ca6f9d33e93733fccb3f3815d9554429dbb38 Cr-Commit-Position: refs/heads/master@{#422542} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/f62ca6f9d33e93733fccb3f3815d9554429dbb38 Cr-Commit-Position: refs/heads/master@{#422542} |