Chromium Code Reviews
Help | Chromium Project | Sign in
(587)

Issue 12886022: Implement offline mode behind a flag (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by rdsmith
Modified:
12 months ago
CC:
chromium-reviews_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam, gavinp+disk_chromium.org
Visibility:
Public.

Description

Implement offline mode behind a flag.

BUG=2204
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195374

Patch Set 1 #

Patch Set 2 : Got rid of debugging, useless flag, and crashing DCHECK. #

Patch Set 3 : Shifted to separate offline manager class. #

Patch Set 4 : Deal with handling validated cache entries as indicative of an operating network. #

Patch Set 5 : Shifted to track (and diff from) https://codereview.chromium.org/12310075/. #

Patch Set 6 : Fix some merge glitches. #

Patch Set 7 : Rebased to r190685 (past landing of https://codereview.chromium.org/12310075). #

Total comments: 10

Patch Set 8 : Added flag to control existence of offline manager. #

Patch Set 9 : Merged to r191150. #

Patch Set 10 : Incorporated Darin's comments. #

Patch Set 11 : Added back in removed whitespace. #

Total comments: 10

Patch Set 12 : Incorporated Ricardo's comments. #

Total comments: 9

Patch Set 13 : Made OfflinePolicy per-tab rather than global. #

Patch Set 14 : Added tests. #

Total comments: 14

Patch Set 15 : Incorporated Ricardo's comments. #

Total comments: 2

Patch Set 16 : Tightened test check. #

Patch Set 17 : Sync'd to r193064. #

Total comments: 20

Patch Set 18 : Incorporated comments. #

Total comments: 8

Patch Set 19 : Incorporated last round of comments. #

Patch Set 20 : Merged to r194985. #

Patch Set 21 : Setup OfflinePolicy for all started requests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+492 lines, -36 lines) Lint Patch
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments ? errors Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +7 lines, -0 lines 0 comments 1 errors Download
A content/browser/loader/global_routing_id.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +42 lines, -0 lines 0 comments 1 errors Download
A content/browser/loader/offline_policy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +56 lines, -0 lines 0 comments 2 errors Download
A content/browser/loader/offline_policy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +73 lines, -0 lines 0 comments 3 errors Download
A content/browser/loader/offline_policy_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +96 lines, -0 lines 0 comments 2 errors Download
M content/browser/loader/resource_dispatcher_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +8 lines, -3 lines 0 comments ? errors Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 15 chunks +60 lines, -25 lines 0 comments ? errors Download
M content/browser/loader/resource_request_info_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -0 lines 0 comments ? errors Download
M content/browser/loader/resource_request_info_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +5 lines, -0 lines 0 comments ? errors Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +3 lines, -0 lines 0 comments ? errors Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments ? errors Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments ? errors Download
M content/public/common/content_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +3 lines, -0 lines 0 comments ? errors Download
M net/http/http_cache_transaction.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments ? errors Download
M net/http/http_cache_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 5 chunks +44 lines, -2 lines 0 comments ? errors Download
M net/http/http_network_layer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +75 lines, -6 lines 0 comments ? errors Download
M net/http/http_network_transaction.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments ? errors Download
M net/http/http_response_info.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -0 lines 0 comments ? errors Download
M net/http/http_response_info.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +3 lines, -0 lines 0 comments ? errors Download
M net/http/http_transaction_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 28
rdsmith
Darin: Could you give me your opinion of the approach taken in this CL? It's ...
1 year ago #1
darin
just some initial thoughts... https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/offline_manager.cc File content/browser/loader/offline_manager.cc (right): https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/offline_manager.cc#newcode19 content/browser/loader/offline_manager.cc:19: int child_id, int route_id, const ...
1 year ago #2
rdsmith
Darin: Incorporated your comments. Gonna work on tests next; you're welcome to respond now, or ...
1 year ago #3
rvargas (OOO until 4_21)
sounds reasonable. https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/offline_policy.h File content/browser/loader/offline_policy.h (right): https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/offline_policy.h#newcode5 content/browser/loader/offline_policy.h:5: // This class controls under what conditions ...
1 year ago #4
rdsmith
Ricardo: One question for you below. Status: Have OfflinePolicy unit tests, haven't written unit tests ...
1 year ago #5
rvargas (OOO until 4_21)
https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/offline_policy.h File content/browser/loader/offline_policy.h (right): https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/offline_policy.h#newcode33 content/browser/loader/offline_policy.h:33: // Return any additional load flags to be |'d ...
1 year ago #6
droger_google
We're looking into the offline mode on iOS, and as Randy suggested, it may be ...
1 year ago #7
rdsmith
(No source changes; just discussion with David.) Darin: Question for you on proper use of ...
1 year ago #8
rdsmith
Made offline policy per-tab rather than global. David, let me know what you think.
1 year ago #9
rdsmith
Ok, I think I'm ready for review. Darin: chrome/*, content/*. Ricardo: net/* Let me know ...
1 year ago #10
droger_google
On 2013/04/05 19:40:31, rdsmith wrote: > Made offline policy per-tab rather than global. David, let ...
1 year ago #11
droger_google
I am still not happy with webkit/glue/resource_type.h, and I don't know what can be done ...
1 year ago #12
blundell (OOO until April 28)
https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/offline_policy.h File content/browser/loader/offline_policy.h (right): https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/offline_policy.h#newcode28 content/browser/loader/offline_policy.h:28: class CONTENT_EXPORT OfflinePolicy { We're currently determining (with darin ...
1 year ago #13
blundell (OOO until April 28)
On 2013/04/08 16:49:29, droger_google wrote: > I am still not happy with webkit/glue/resource_type.h, and I ...
1 year ago #14
rvargas (OOO until 4_21)
net LGTM https://codereview.chromium.org/12886022/diff/73001/net/http/http_cache_unittest.cc File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/12886022/diff/73001/net/http/http_cache_unittest.cc#newcode506 net/http/http_cache_unittest.cc:506: RunTransactionTestWithResponseInfo(cache.http_cache(), kSimpleGET_Transaction, SimpleGET_NetworkAccessed_Cache is a superset of ...
1 year ago #15
rdsmith
Incorporated Ricardo's comments. Ricardo, you might or might not want to glance at the changes--they ...
1 year ago #16
rvargas (OOO until 4_21)
still looks good (PS15) https://codereview.chromium.org/12886022/diff/72003/net/http/http_network_layer_unittest.cc File net/http/http_network_layer_unittest.cc (right): https://codereview.chromium.org/12886022/diff/72003/net/http/http_network_layer_unittest.cc#newcode323 net/http/http_network_layer_unittest.cc:323: ASSERT_EQ(OK, rv); nit: merge this ...
1 year ago #17
rdsmith
Thanks! https://codereview.chromium.org/12886022/diff/72003/net/http/http_network_layer_unittest.cc File net/http/http_network_layer_unittest.cc (right): https://codereview.chromium.org/12886022/diff/72003/net/http/http_network_layer_unittest.cc#newcode323 net/http/http_network_layer_unittest.cc:323: ASSERT_EQ(OK, rv); On 2013/04/08 21:54:46, rvargas wrote: > ...
1 year ago #18
darin
I just focused on content/ code. https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/offline_policy.cc File content/browser/loader/offline_policy.cc (right): https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/offline_policy.cc#newcode24 content/browser/loader/offline_policy.cc:24: if (!CommandLine::ForCurrentProcess()->HasSwitch( it ...
1 year ago #19
darin
It is OK to use webkit/glue/resource_type.h. It is true that it should probably move elsewhere, ...
1 year ago #20
rdsmith
Darin: PTAL. Note on change to use of ResourceType::Type: The original comment came from discussion ...
1 year ago #21
darin
I think you made a yak happy. LGTM for content/ bits. https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/global_routing_id.h File content/browser/loader/global_routing_id.h (right): ...
1 year ago #22
rdsmith
Thanks! I'll merge this to LKGR and throw it at the CQ. https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/global_routing_id.h File content/browser/loader/global_routing_id.h ...
12 months ago #23
darin
On Thu, Apr 18, 2013 at 1:04 PM, <rdsmith@chromium.org> wrote: > Thanks! I'll merge this ...
12 months ago #24
rdsmith
Ricardo: Could you review the PS20->PS21 change to resource_dispatcher_host_impl.cc? (Darin: I'm happy to take a ...
12 months ago #25
rvargas (OOO until 4_21)
lgtm
12 months ago #26
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12886022/130001
12 months ago #27
I haz the power (commit-bot)
12 months ago #28
Message was sent while issue was closed.
Change committed as 195374
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6