Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(26)

Issue 2310363002: Persist offline page info in a navigation entry if needed (Closed)

Created:
4 years, 3 months ago by jianli
Modified:
4 years, 2 months ago
Reviewers:
sky, nasko, Dmitry Titov
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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+346 lines, -3 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/browser/android/offline_pages/offline_page_info_handler.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +32 lines, -0 lines 0 comments Download
A chrome/browser/android/offline_pages/offline_page_info_handler.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +64 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M components/sessions/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M components/sessions/content/content_serialized_navigation_builder.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +27 lines, -0 lines 0 comments Download
M components/sessions/content/content_serialized_navigation_builder_unittest.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +64 lines, -0 lines 0 comments Download
M components/sessions/content/content_serialized_navigation_driver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +24 lines, -0 lines 0 comments Download
M components/sessions/content/content_serialized_navigation_driver.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +14 lines, -0 lines 0 comments Download
A components/sessions/content/extended_info_handler.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +40 lines, -0 lines 0 comments Download
M components/sessions/core/serialized_navigation_entry.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +9 lines, -0 lines 0 comments Download
M components/sessions/core/serialized_navigation_entry.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +18 lines, -0 lines 0 comments Download
M components/sessions/core/serialized_navigation_entry_test_helper.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M components/sessions/core/serialized_navigation_entry_test_helper.cc View 1 2 3 4 5 6 10 2 chunks +9 lines, -0 lines 0 comments Download
M components/sessions/core/serialized_navigation_entry_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +12 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_entry_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_entry_impl.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +12 lines, -0 lines 0 comments Download
M content/public/browser/navigation_entry.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 101 (67 generated)
jianli
sky for sessions nasko for contents
4 years, 3 months ago (2016-09-07 00:58:32 UTC) #10
nasko
https://codereview.chromium.org/2310363002/diff/40001/components/sessions/content/content_serialized_navigation_builder.cc File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/40001/components/sessions/content/content_serialized_navigation_builder.cc#newcode22 components/sessions/content/content_serialized_navigation_builder.cc:22: const char kOfflinePageHeader[] = "X-Chrome-offline"; Doesn't the full header ...
4 years, 3 months ago (2016-09-09 21:42:49 UTC) #13
nasko
Forgot to add. Can you make the short design doc publicly available and linked from ...
4 years, 3 months ago (2016-09-09 21:43:26 UTC) #14
jianli
The description has been updated with the link to the doc. https://codereview.chromium.org/2310363002/diff/40001/components/sessions/content/content_serialized_navigation_builder.cc File components/sessions/content/content_serialized_navigation_builder.cc (right): ...
4 years, 3 months ago (2016-09-09 22:32:48 UTC) #16
nasko
LGTM
4 years, 3 months ago (2016-09-09 23:32:56 UTC) #19
jianli
dimich, could you please review offline page changes?
4 years, 3 months ago (2016-09-09 23:42:18 UTC) #23
Dmitry Titov
lgtm
4 years, 3 months ago (2016-09-10 01:25:34 UTC) #24
sky
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#newcode23 components/sessions/content/content_serialized_navigation_builder.cc:23: const char kOfflinePageHeader[] = "X-Chrome-offline: "; Is X-Chrome-offline and ...
4 years, 3 months ago (2016-09-12 15:04:22 UTC) #25
jianli
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#newcode23 components/sessions/content/content_serialized_navigation_builder.cc:23: const char kOfflinePageHeader[] = "X-Chrome-offline: "; On 2016/09/12 15:04:21, ...
4 years, 3 months ago (2016-09-12 21:17:46 UTC) #26
sky
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 ...
4 years, 3 months ago (2016-09-13 01:53:32 UTC) #31
jianli
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 ...
4 years, 3 months ago (2016-09-13 20:29:18 UTC) #32
sky
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 ...
4 years, 3 months ago (2016-09-13 23:30:43 UTC) #33
jianli
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 ...
4 years, 3 months ago (2016-09-13 23:44:16 UTC) #34
sky
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 ...
4 years, 3 months ago (2016-09-13 23:53:57 UTC) #35
jianli
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 ...
4 years, 3 months ago (2016-09-21 01:00:35 UTC) #38
sky
I'll take a look at this first thing tomorrow. Quick question first though. Is this ...
4 years, 3 months ago (2016-09-21 03:09:00 UTC) #48
jianli
On Tue, Sep 20, 2016 at 8:08 PM, Scott Violet <sky@chromium.org> wrote: > I'll take ...
4 years, 3 months ago (2016-09-21 19:49:52 UTC) #51
sky
On 2016/09/21 19:49:52, jianli wrote: > On Tue, Sep 20, 2016 at 8:08 PM, Scott ...
4 years, 3 months ago (2016-09-21 20:37:17 UTC) #52
jianli
Generally I like this approach. But there is one problem that prevents the restoring into ...
4 years, 3 months ago (2016-09-21 21:08:34 UTC) #53
jianli
On 2016/09/21 20:37:17, sky wrote: > On 2016/09/21 19:49:52, jianli wrote: > > On Tue, ...
4 years, 2 months ago (2016-09-27 21:23:53 UTC) #56
sky
https://codereview.chromium.org/2310363002/diff/160001/components/sessions/core/serialized_navigation_driver.h File components/sessions/core/serialized_navigation_driver.h (right): https://codereview.chromium.org/2310363002/diff/160001/components/sessions/core/serialized_navigation_driver.h#newcode24 components/sessions/core/serialized_navigation_driver.h:24: class SESSIONS_EXPORT SerializedNavigationDriver { This code can't depend upon ...
4 years, 2 months ago (2016-09-27 22:30:28 UTC) #59
jianli
https://codereview.chromium.org/2310363002/diff/160001/components/sessions/core/serialized_navigation_driver.h File components/sessions/core/serialized_navigation_driver.h (right): https://codereview.chromium.org/2310363002/diff/160001/components/sessions/core/serialized_navigation_driver.h#newcode24 components/sessions/core/serialized_navigation_driver.h:24: class SESSIONS_EXPORT SerializedNavigationDriver { On 2016/09/27 22:30:27, sky wrote: ...
4 years, 2 months ago (2016-09-27 23:30:33 UTC) #61
sky
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 ...
4 years, 2 months ago (2016-09-28 00:22:01 UTC) #69
jianli
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 ...
4 years, 2 months ago (2016-09-28 00:46:27 UTC) #70
sky
See my earlier email. I suggested you put this on ContentSerializedNavigationDriver, which is just a ...
4 years, 2 months ago (2016-09-28 17:01:10 UTC) #71
jianli
On 2016/09/28 17:01:10, sky wrote: > See my earlier email. I suggested you put this ...
4 years, 2 months ago (2016-09-28 22:00:54 UTC) #74
sky
https://codereview.chromium.org/2310363002/diff/240001/components/sessions/content/content_serialized_navigation_builder.cc File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/content/content_serialized_navigation_builder.cc#newcode53 components/sessions/content/content_serialized_navigation_builder.cc:53: navigation.extended_info_map_[handler_entry.first] = I think you should only add the ...
4 years, 2 months ago (2016-09-29 03:08:20 UTC) #78
jianli
https://codereview.chromium.org/2310363002/diff/240001/components/sessions/content/content_serialized_navigation_builder.cc File components/sessions/content/content_serialized_navigation_builder.cc (right): https://codereview.chromium.org/2310363002/diff/240001/components/sessions/content/content_serialized_navigation_builder.cc#newcode53 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 ...
4 years, 2 months ago (2016-09-29 22:02:29 UTC) #79
sky
LGTM - thanks for the patience.
4 years, 2 months ago (2016-09-29 23:35:58 UTC) #85
jianli
nasko, could you please review again for the new change to NavigationEntry? Thanks.
4 years, 2 months ago (2016-09-29 23:46:57 UTC) #87
nasko
content/ LGTM
4 years, 2 months ago (2016-09-30 23:01:28 UTC) #90
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2310363002/320001
4 years, 2 months ago (2016-10-03 20:29:42 UTC) #97
commit-bot: I haz the power
Committed patchset #14 (id:320001)
4 years, 2 months ago (2016-10-03 21:38:48 UTC) #99
commit-bot: I haz the power
4 years, 2 months ago (2016-10-03 21:42:02 UTC) #101
Message was sent while issue was closed.
Patchset 14 (id:??) landed as
https://crrev.com/f62ca6f9d33e93733fccb3f3815d9554429dbb38
Cr-Commit-Position: refs/heads/master@{#422542}

Powered by Google App Engine
This is Rietveld 408576698