Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(30)

Issue 495523003: Change SDCHDictionaryFetcher to use URLRequest instead of URLFetcher. (Closed)

Created:
6 years, 1 month ago by Randy Smith (Not in Mondays)
Modified:
6 years ago
Reviewers:
Ryan Sleevi, mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org
Project:
chromium
Visibility:
Public.

Description

Change SDCHDictionaryFetcher to use URLRequest instead of URLFetcher. Also shifts mechanism for minimizing interference with top-level traffic from delaying dispatch to using the IDLE request priority. BUG=387890 BUG=383404 R=rsleevi@chromium.org Committed: https://crrev.com/1df5b71300297a83efb39b8666ba0ff44369947c Cr-Commit-Position: refs/heads/master@{#294002}

Patch Set 1 #

Patch Set 2 : Results of self-review. #

Total comments: 14

Patch Set 3 : Incorporated comments. #

Patch Set 4 : Copied over load flags assignment from old code. #

Total comments: 20

Patch Set 5 : Note problems with SdchDictionaryFetcher layering and remove use of TrivialURLRequestContextGetter. #

Patch Set 6 : Partial response to Ryan's comments. #

Patch Set 7 : DoLoop implementation. #

Total comments: 8

Patch Set 8 : Non-DoLoop (recursion solved with sledgehammer) version. #

Patch Set 9 : Checkpoitn (with temporary logging). #

Patch Set 10 : Cleaned up state machine & handled recursion properly. #

Total comments: 20

Patch Set 11 : Incorporated comments. #

Patch Set 12 : Sync'd to psuedo-rev p293320. #

Patch Set 13 : Some more comment tweaking. #

Total comments: 14

Patch Set 14 : Incorporated comments, encapsulated SdchFetcher::Delegate. #

Patch Set 15 : Added virtual dtor to SdchManager. #

Total comments: 2

Patch Set 16 : Tweaked comment. #

Patch Set 17 : Added NET_EXPORT to delegate class. #

Patch Set 18 : Fixed try job failures. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+688 lines, -326 lines) Patch
M chrome/browser/profiles/off_the_record_profile_io_data.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -9 lines 0 comments Download
M chrome/browser/profiles/profile_impl_io_data.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -9 lines 0 comments Download
M net/base/sdch_dictionary_fetcher.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +54 lines, -42 lines 0 comments Download
M net/base/sdch_dictionary_fetcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +169 lines, -39 lines 0 comments Download
A net/base/sdch_dictionary_fetcher_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +193 lines, -0 lines 0 comments Download
M net/base/sdch_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 12 chunks +38 lines, -22 lines 0 comments Download
M net/base/sdch_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 8 chunks +89 lines, -89 lines 0 comments Download
M net/base/sdch_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 17 chunks +52 lines, -42 lines 0 comments Download
M net/filter/sdch_filter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 19 chunks +47 lines, -36 lines 0 comments Download
M net/net.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M net/url_request/url_fetcher_core.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +15 lines, -15 lines 0 comments Download
M net/url_request/url_fetcher_core.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 8 chunks +26 lines, -23 lines 0 comments Download

Messages

Total messages: 47 (5 generated)
Randy Smith (Not in Mondays)
Ryan: PTAL? A couple of specific question: * Under what circumstances will URLRequest make an ...
6 years, 1 month ago (2014-08-20 18:05:25 UTC) #1
Ryan Sleevi
Ryan: PTAL? A couple of specific question: * Under what circumstances will URLRequest make an ...
6 years, 1 month ago (2014-08-20 18:27:56 UTC) #2
mmenke
Drive-by thoughts on two of your questions. On 2014/08/20 18:05:25, rdsmith wrote: > Ryan: PTAL? ...
6 years, 1 month ago (2014-08-20 18:28:56 UTC) #3
Randy Smith (Not in Mondays)
Thanks, Ryan and Matt! PTAL? High level responses and comments: * OnCertificateRequested: Well, I went ...
6 years, 1 month ago (2014-08-20 19:21:50 UTC) #4
Ryan Sleevi
On 2014/08/20 19:21:50, rdsmith wrote: > Thanks, Ryan and Matt! PTAL? > > High level ...
6 years, 1 month ago (2014-08-20 19:38:20 UTC) #5
Randy Smith (Not in Mondays)
On 2014/08/20 19:38:20, Ryan Sleevi wrote: > On 2014/08/20 19:21:50, rdsmith wrote: > > Thanks, ...
6 years, 1 month ago (2014-08-20 19:45:12 UTC) #6
Ryan Sleevi
On 2014/08/20 19:45:12, rdsmith wrote: > My image, which I think is pretty close to ...
6 years, 1 month ago (2014-08-20 19:50:30 UTC) #7
Randy Smith (Not in Mondays)
Ryan and I had an offline conversation, which I'm going to attempt to summarize here; ...
6 years ago (2014-08-25 18:45:41 UTC) #8
Randy Smith (Not in Mondays)
Ryan: PTAL? I've made a note in sdch_dictionary_fetcher.h about the layering and yanked a no-longer-necessary ...
6 years ago (2014-08-25 19:11:17 UTC) #9
Ryan Sleevi
Unit tests that make sure backoff works as expected? https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary_fetcher.cc File net/base/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary_fetcher.cc#newcode69 net/base/sdch_dictionary_fetcher.cc:69: ...
6 years ago (2014-08-25 20:00:01 UTC) #10
Randy Smith (Not in Mondays)
On 2014/08/25 20:00:01, Ryan Sleevi wrote: > Unit tests that make sure backoff works as ...
6 years ago (2014-08-25 20:12:20 UTC) #11
Ryan Sleevi
Sorry, missed this. noideadog here too. +mmenke On Mon, Aug 25, 2014 at 1:12 PM, ...
6 years ago (2014-08-26 00:32:41 UTC) #12
Randy Smith (Not in Mondays)
rdsmith@chromium.org changed reviewers: + mmenke@chromium.org
6 years ago (2014-08-26 15:34:17 UTC) #13
Randy Smith (Not in Mondays)
I don't see Matt on the cc list, so my own +mmenke (for c#11,12). https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary_fetcher.cc ...
6 years ago (2014-08-26 15:34:17 UTC) #14
Randy Smith (Not in Mondays)
On 2014/08/26 15:34:17, rdsmith wrote: > I don't see Matt on the cc list, so ...
6 years ago (2014-08-26 15:40:40 UTC) #15
Randy Smith (Not in Mondays)
On 2014/08/26 15:40:40, rdsmith wrote: > On 2014/08/26 15:34:17, rdsmith wrote: > > I don't ...
6 years ago (2014-08-26 15:43:31 UTC) #16
mmenke
On 2014/08/26 15:43:31, rdsmith wrote: > Ok, Ryan, feel free to respond to what I ...
6 years ago (2014-08-26 17:15:47 UTC) #17
Randy Smith (Not in Mondays)
On 2014/08/26 17:15:47, mmenke wrote: > On 2014/08/26 15:43:31, rdsmith wrote: > > Ok, Ryan, ...
6 years ago (2014-08-26 17:50:53 UTC) #18
mmenke
On 2014/08/26 17:50:53, rdsmith wrote: > On 2014/08/26 17:15:47, mmenke wrote: > > On 2014/08/26 ...
6 years ago (2014-08-26 17:56:03 UTC) #19
Randy Smith (Not in Mondays)
Ryan: I'm working on some simple unit tests for SdchDictionaryFetcher, but while I do that ...
6 years ago (2014-09-02 19:35:03 UTC) #20
Ryan Sleevi
So, your sledgehammer totally works, but at an understandable cost. As you noted, your DoLoop ...
6 years ago (2014-09-02 20:04:20 UTC) #21
Randy Smith (Not in Mondays)
Ok, I think I'm ready for another round. As noted offline, you've convinced me around ...
6 years ago (2014-09-02 23:34:33 UTC) #22
Ryan Sleevi
I'll need to think more about the SdchManager virtualization. I have a little bit of ...
6 years ago (2014-09-04 01:14:21 UTC) #23
Randy Smith (Not in Mondays)
Thanks for the comments! I'll get to work on incorporating them. One quick question in ...
6 years ago (2014-09-04 17:13:55 UTC) #24
Randy Smith (Not in Mondays)
Comments incorporated or responded to. https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionary_fetcher.cc File net/base/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionary_fetcher.cc#newcode116 net/base/sdch_dictionary_fetcher.cc:116: // request on initial ...
6 years ago (2014-09-04 18:52:57 UTC) #25
Randy Smith (Not in Mondays)
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionary_fetcher.h File net/base/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionary_fetcher.h#newcode67 net/base/sdch_dictionary_fetcher.h:67: virtual void OnReadCompleted(URLRequest* request, int bytes_read) OVERRIDE; On 2014/09/04 ...
6 years ago (2014-09-04 18:54:14 UTC) #26
Ryan Sleevi
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionary_fetcher.cc File net/base/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionary_fetcher.cc#newcode200 net/base/sdch_dictionary_fetcher.cc:200: if (in_loop_) On 2014/09/04 18:52:57, rdsmith wrote: > On ...
6 years ago (2014-09-04 19:05:30 UTC) #27
mmenke
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionary_fetcher.cc File net/base/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionary_fetcher.cc#newcode216 net/base/sdch_dictionary_fetcher.cc:216: return; On 2014/09/04 19:05:30, Ryan Sleevi wrote: > On ...
6 years ago (2014-09-04 19:34:24 UTC) #28
Randy Smith (Not in Mondays)
Ryan: I'm not sure if your last comments meant you were satisfied with the changes ...
6 years ago (2014-09-04 19:48:13 UTC) #29
Ryan Sleevi
On 2014/09/04 19:48:13, rdsmith wrote: > Ryan: I'm not sure if your last comments meant ...
6 years ago (2014-09-04 19:54:24 UTC) #30
Ryan Sleevi
Ok, never mind, I see you were talking about the circular dependency between Fetcher and ...
6 years ago (2014-09-04 20:06:21 UTC) #31
Ryan Sleevi
Er, I forgot that was the phrase that pays. To be explicit: I do think ...
6 years ago (2014-09-04 20:07:27 UTC) #32
Randy Smith (Not in Mondays)
Ryan: Having done all this work, I'm now no longer sure it was what you ...
6 years ago (2014-09-07 14:09:39 UTC) #33
Ryan Sleevi
Thanks Randy. You actually picked up on the circular dependency I was talking about. Namely ...
6 years ago (2014-09-08 19:29:54 UTC) #34
Randy Smith (Not in Mondays)
Fair enough. Thanks for the thorough (:-}) review! https://codereview.chromium.org/495523003/diff/270001/net/base/sdch_manager.h File net/base/sdch_manager.h (right): https://codereview.chromium.org/495523003/diff/270001/net/base/sdch_manager.h#newcode49 net/base/sdch_manager.h:49: // ...
6 years ago (2014-09-08 20:03:29 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/495523003/290001
6 years ago (2014-09-08 20:05:18 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/9437) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg/builds/13399)
6 years ago (2014-09-08 20:17:32 UTC) #39
mmenke
On 2014/09/08 20:17:32, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years ago (2014-09-08 20:34:22 UTC) #40
Randy Smith (Not in Mondays)
Ryan, FYI: PS18 tweaked test infrastructure added earlier to make debugging a try job failure ...
6 years ago (2014-09-09 19:01:10 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/495523003/330001
6 years ago (2014-09-09 19:58:14 UTC) #45
commit-bot: I haz the power
Committed patchset #18 (id:330001) as f0a2e41195e5e6efad55f96e5b2dae0a194c2dbc
6 years ago (2014-09-09 20:40:19 UTC) #46
commit-bot: I haz the power
6 years ago (2014-09-10 03:55:22 UTC) #47
Message was sent while issue was closed.
Patchset 18 (id:??) landed as
https://crrev.com/1df5b71300297a83efb39b8666ba0ff44369947c
Cr-Commit-Position: refs/heads/master@{#294002}

Powered by Google App Engine
This is Rietveld 408576698