Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368433002:
Add net::SdchSourceStream and net::SdchPolicyDelegate (Closed)
DescriptionAdd net::SdchSourceStream and net::SdchPolicyDelegate
This CL adds a SdchSourceStream which implements FilterSourceStream to do
sdch decoding. This CL additionally adds a SdchPolicyDelegate which will be
the glue for URLRequestHttpJob and SdchSourceStream.
This is a part of the effort to convert net::Filter into a pull-based
interface. See the linked bug for more details.
BUG=474859
Committed: https://crrev.com/ab5a1f3c40b48b2a0498e6469f9f8d676a728994
Cr-Commit-Position: refs/heads/master@{#426803}
Patch Set 1 : Base CL 1662763002 #Patch Set 2 : synced to 89ff2d42ad349d0b70d3e73787a069245e131d48 #Patch Set 3 : tidy up tests #
Total comments: 37
Patch Set 4 : Address Comments #Patch Set 5 : fix meta refresh #
Total comments: 48
Patch Set 6 : Address Comments #Patch Set 7 : Rebased to fix compile error #
Total comments: 3
Patch Set 8 : Address Randy comments to add delegate test and UMA #Patch Set 9 : Make SdchPolicyDelegate owned by SdchSourceStream #
Total comments: 5
Patch Set 10 : fix a unit test (synced to 1f4ebc806dbf7110dd999ff20e1dfd279685f336) #Patch Set 11 : Make URLRequestHttpJob call URLRequestJob::DestroySourceStream() #Patch Set 12 : fix compile error #
Total comments: 42
Patch Set 13 : Address Randy's comments #
Total comments: 4
Patch Set 14 : address comments #Patch Set 15 : Fix histograms #
Messages
Total messages: 103 (76 generated)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by xunjieli@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by xunjieli@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...
xunjieli@chromium.org changed reviewers: + rdsmith@chromium.org
Hi Randy, I am sending this out while the Gzip one is still in review. This is to speed the review cycle. If this is a bit too much in terms of your review load, please ignore this one until I get the Gzip one in a good shape. We are going to miss the M55 boat, so I am planning to land all the relevant pieces in M55, and the switch CL in M56. Thanks!
Patchset #2 (id:40001) has been deleted
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 xunjieli@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
First set of comments. Mostly nits and suggestions; I don't think I found any bugs, just things I'd like to make cleaner. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.cc:54: // needs to issue a meta-refresh to get it. nit: The second sentence of this comment appears to apply to the fall through case, but as I read it (primarily its positioning) implies it's handled in the next block of code? I'd almost rather have a nested if and an else with an IssueMetaRefreshIfPossible() to make the logic clear? (Up to you--just think for a second about what you think will be the clearest and easiest for future folks to read.) https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.cc:136: bool SdchPolicyDelegate::IssueMetaRefreshIfPossible(SdchSourceStream* source) { TODO with a bug for nuking the ugly meta-refresh hack? https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.h:22: // This class ascribes to the SdchSourceStream::Delegate interface to implement nit, suggestion "ascribes to" -> "implements" (this may be just a language difference between us--I'm not familiar with "ascribes to" as a phrase). https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.h:50: SdchPolicyDelegate(std::unique_ptr<Context> context); I'd really like to explore the possibility of getting rid of the Context object entirely, and just initializing the SdchPolicyDelegate with the information it needs. Would you be willing to see if that's a possible change? (This is partially because I hate FilterContext and want it to die rather than being further baked into the code, and partially because this seems like an excess amount of indirection, given that we already have the policy delegate.) I'd think that mime_type, url, IsCachedContent(), SdchManager (with lifetime guarantees), response code, and netlog could just be passed in. I'd *think* that the dictionaries advertised could be owned directly by this class, but I could imagine that being a tricky change. But could you see how hard it would be? This might be most easily explored in the context of a CL that included the instantiation of these classes; such a CL wouldn't land (that'd be part of the CL that Matt's reviewing) but would make it easier to evaluate this change. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.h:71: bool dictionary_requested_; Suggestion: As you know, I'm a bit allergic to these type of "state variables". It looks to me as if this is only used to distinguish between the case where SdchSourceStream decides it's got a bogus dictionary id and the case where it has a good dictionary id and tries and fails to get the dictionary. Would it be reasonable to simply split those two cases into two separate delegate calls and get rid of this boolean? It might result in simpler code for the delegate (-- code where fewer pieces of information need to be tracked in reading it). https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... File net/filter/sdch_source_stream.cc (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:37: passthrough_ = true; For reasons we've sorta explored in the gzip CL, I'm allergic to these kind of booleans. Can this be implemented instead by simply changing the state, possibly with a DCHECK about what the allowed states are from which one could transition to the (in this case) passthrough state? https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:61: InputState state = input_state_; Why the separate variable? It seems like it's only used on the next line. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:62: switch (state) { nit, suggestion: Order these states (here and in the enum) according to the order you expect to hit them in the stream, with eror staets (including pass through) at the end? https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:80: std::string server_hash = dictionary_id_.substr(0, kServerIdLength - 1); nit, suggestion: server_hash only appears to be used once, maybe move the definition inline? https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:111: input_state_ = STATE_DECODE; Sorry, I'm really confused. Under what circumstances do we want to transition from FLUSH_INTERNAL_BUFFER to DECODE? Isn't FLUSH_INTERNAL_BUFFER an error state? https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:141: } nit: It "feels" like this test is generic, not specific to STATE_DECODE, and hence should be outside of the switch statement. I haven't analyzed the code to see whether that's accurate or not, so up to you. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... File net/filter/sdch_source_stream.h (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:45: // a) returning true and calling ReplaceOutput or Passthrough, or nit: I think that RepalceOutput() or Passthrough() must be called *before* the function returns true; reasonable for the documentation to have this ordering? https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:51: virtual bool OnDictionaryError(SdchSourceStream* source) = 0; Suggestion: So I flip-flopped a bit on this in thinking about it, but my personal preference would be to have the StopDecoding/ReplaceOutput() possibilities for handling the error be embedded in this functions signature (e.g. to return an enum indicating the error state transition and to have a writeable string output parameter for replacing the output if that's needed). It makes these signatures more complicated and messy, but it eliminates the method signatures below and means that the state transition logic is inline in the case statement in the .cc file, as opposed to being magically invoked by the delegate from outside. If you really prefer it this way, I'm ok with it, but if I were doing it I'd probably just hack up these signatures and get rid of StopDecoding/ReplaceOutput. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:69: const std::string** text) = 0; Would you do me a favor and put O(5m) of thought into what the code would have to look like if this interface was async (you may totally have done this already), make sure this design doesn't horribly get in the way of whatever that is, and put in a todo sketching out how what changes would have to be made here for this to be async? I suspect you can figure out why I'm asking :-}. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:87: virtual void ReplaceOutput(const char* data, size_t size); nit, suggestion: If the above two are only called by the SdchSourceStreamDelegate, it should be possible to move them into the private: section. (This may require indirecting the calls through protected methods on the delegate class, since it's that class that has the private: privilege on SdchSourceStream.) Feel free to decide the plumbing's not worth it for the encapsulation gain. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:121: std::string dictionary_id_; nit, suggestion: I've had enough trouble with server vs. client id in the SDCH code that I'd suggest naming this dictionary_server_id_ to make clear which id is being discussed in the name. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:125: // return later. This could maybe become quite large. nit: "could maybe" seems redundant to me; eliminate "maybe"? https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:125: // return later. This could maybe become quite large. I find myself wanting to file a bug on upstream asking for control over the amount returned; given that it's a compressed format, there's no reason we couldn't buffer more efficiently on the input side. But my understanding is that we don't have a maintainer for vcdiff, and thus no effective bug tracker :-J. Maybe worthwhile filing a crbug.com and noting that it really refers to a dependent package? (Feel free to ignore this suggestion.)
Patchset #4 (id:100001) has been deleted
Thanks for the review! PTAL. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.cc:54: // needs to issue a meta-refresh to get it. On 2016/09/27 20:09:12, Randy Smith - Not in Mondays wrote: > nit: The second sentence of this comment appears to apply to the fall through > case, but as I read it (primarily its positioning) implies it's handled in the > next block of code? I'd almost rather have a nested if and an else with an > IssueMetaRefreshIfPossible() to make the logic clear? (Up to you--just think > for a second about what you think will be the clearest and easiest for future > folks to read.) Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.cc:136: bool SdchPolicyDelegate::IssueMetaRefreshIfPossible(SdchSourceStream* source) { On 2016/09/27 20:09:12, Randy Smith - Not in Mondays wrote: > TODO with a bug for nuking the ugly meta-refresh hack? Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.h:22: // This class ascribes to the SdchSourceStream::Delegate interface to implement On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > nit, suggestion "ascribes to" -> "implements" (this may be just a language > difference between us--I'm not familiar with "ascribes to" as a phrase). Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.h:50: SdchPolicyDelegate(std::unique_ptr<Context> context); On 2016/09/27 20:09:12, Randy Smith - Not in Mondays wrote: > I'd really like to explore the possibility of getting rid of the Context object > entirely, and just initializing the SdchPolicyDelegate with the information it > needs. Would you be willing to see if that's a possible change? (This is > partially because I hate FilterContext and want it to die rather than being > further baked into the code, and partially because this seems like an excess > amount of indirection, given that we already have the policy delegate.) > > I'd think that mime_type, url, IsCachedContent(), SdchManager (with lifetime > guarantees), response code, and netlog could just be passed in. I'd *think* > that the dictionaries advertised could be owned directly by this class, but I > could imagine that being a tricky change. But could you see how hard it would > be? > > This might be most easily explored in the context of a CL that included the > instantiation of these classes; such a CL wouldn't land (that'd be part of the > CL that Matt's reviewing) but would make it easier to evaluate this change. I don't like the extra indirection either. However, many of the fields that the Delegate requires are private members of URLRequestHttpJob. If we remove the Context, we need to change these private members (e.g. GetMimeType()) to public and add accessors for those that are not exposed (e.g. |is_cached_content_|) How about we revisit this in a follow-up? I can add a TODO. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.h:71: bool dictionary_requested_; On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > Suggestion: As you know, I'm a bit allergic to these type of "state variables". > It looks to me as if this is only used to distinguish between the case where > SdchSourceStream decides it's got a bogus dictionary id and the case where it > has a good dictionary id and tries and fails to get the dictionary. Would it be > reasonable to simply split those two cases into two separate delegate calls and > get rid of this boolean? It might result in simpler code for the delegate (-- > code where fewer pieces of information need to be tracked in reading it). Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... File net/filter/sdch_source_stream.cc (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:37: passthrough_ = true; On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > For reasons we've sorta explored in the gzip CL, I'm allergic to these kind of > booleans. Can this be implemented instead by simply changing the state, > possibly with a DCHECK about what the allowed states are from which one could > transition to the (in this case) passthrough state? Done. The pass-through case is a bit tricky. If there is a bogus dictionary id, we still want it to appear in the output stream, so we actually flush |buffered_output_| first before doing pass-through. I've changed the code accordingly. Let me know if it is confusing. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:61: InputState state = input_state_; On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > Why the separate variable? It seems like it's only used on the next line. Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:62: switch (state) { On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > nit, suggestion: Order these states (here and in the enum) according to the > order you expect to hit them in the stream, with eror staets (including pass > through) at the end? Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:80: std::string server_hash = dictionary_id_.substr(0, kServerIdLength - 1); On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > nit, suggestion: server_hash only appears to be used once, maybe move the > definition inline? Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:111: input_state_ = STATE_DECODE; On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > Sorry, I'm really confused. Under what circumstances do we want to transition > from FLUSH_INTERNAL_BUFFER to DECODE? Isn't FLUSH_INTERNAL_BUFFER an error > state? Done.FLUSH_INTERNAL_BUFFER is not an error state. When in STATE_DECODE, |buffered_output_| are filled with output. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.cc:141: } On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > nit: It "feels" like this test is generic, not specific to STATE_DECODE, and > hence should be outside of the switch statement. I haven't analyzed the code to > see whether that's accurate or not, so up to you. Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... File net/filter/sdch_source_stream.h (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:45: // a) returning true and calling ReplaceOutput or Passthrough, or On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > nit: I think that RepalceOutput() or Passthrough() must be called *before* the > function returns true; reasonable for the documentation to have this ordering? Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:51: virtual bool OnDictionaryError(SdchSourceStream* source) = 0; On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > Suggestion: So I flip-flopped a bit on this in thinking about it, but my > personal preference would be to have the StopDecoding/ReplaceOutput() > possibilities for handling the error be embedded in this functions signature > (e.g. to return an enum indicating the error state transition and to have a > writeable string output parameter for replacing the output if that's needed). > It makes these signatures more complicated and messy, but it eliminates the > method signatures below and means that the state transition logic is inline in > the case statement in the .cc file, as opposed to being magically invoked by the > delegate from outside. If you really prefer it this way, I'm ok with it, but if > I were doing it I'd probably just hack up these signatures and get rid of > StopDecoding/ReplaceOutput. Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:69: const std::string** text) = 0; On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > Would you do me a favor and put O(5m) of thought into what the code would have > to look like if this interface was async (you may totally have done this > already), make sure this design doesn't horribly get in the way of whatever that > is, and put in a todo sketching out how what changes would have to be made here > for this to be async? I suspect you can figure out why I'm asking :-}. Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:87: virtual void ReplaceOutput(const char* data, size_t size); On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > nit, suggestion: If the above two are only called by the > SdchSourceStreamDelegate, it should be possible to move them into the private: > section. (This may require indirecting the calls through protected methods on > the delegate class, since it's that class that has the private: privilege on > SdchSourceStream.) Feel free to decide the plumbing's not worth it for the > encapsulation gain. Acknowledged. These two methods are removed. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:121: std::string dictionary_id_; On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > nit, suggestion: I've had enough trouble with server vs. client id in the SDCH > code that I'd suggest naming this dictionary_server_id_ to make clear which id > is being discussed in the name. Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:125: // return later. This could maybe become quite large. On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > nit: "could maybe" seems redundant to me; eliminate "maybe"? Done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_source_... net/filter/sdch_source_stream.h:125: // return later. This could maybe become quite large. On 2016/09/27 20:09:13, Randy Smith - Not in Mondays wrote: > I find myself wanting to file a bug on upstream asking for control over the > amount returned; given that it's a compressed format, there's no reason we > couldn't buffer more efficiently on the input side. But my understanding is > that we don't have a maintainer for vcdiff, and thus no effective bug tracker > :-J. Maybe worthwhile filing a http://crbug.com and noting that it really refers to a > dependent package? (Feel free to ignore this suggestion.) Done.
The CQ bit was checked by xunjieli@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 xunjieli@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.
Sorry about taking so long on this. I think we're getting pretty close, though; 1-2 more rounds and we'll be done. https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/2368433002/diff/80001/net/filter/sdch_policy_... net/filter/sdch_policy_delegate.h:50: SdchPolicyDelegate(std::unique_ptr<Context> context); On 2016/09/30 15:32:17, xunjieli wrote: > On 2016/09/27 20:09:12, Randy Smith - Not in Mondays wrote: > > I'd really like to explore the possibility of getting rid of the Context > object > > entirely, and just initializing the SdchPolicyDelegate with the information it > > needs. Would you be willing to see if that's a possible change? (This is > > partially because I hate FilterContext and want it to die rather than being > > further baked into the code, and partially because this seems like an excess > > amount of indirection, given that we already have the policy delegate.) > > > > I'd think that mime_type, url, IsCachedContent(), SdchManager (with lifetime > > guarantees), response code, and netlog could just be passed in. I'd *think* > > that the dictionaries advertised could be owned directly by this class, but I > > could imagine that being a tricky change. But could you see how hard it would > > be? > > > > This might be most easily explored in the context of a CL that included the > > instantiation of these classes; such a CL wouldn't land (that'd be part of the > > CL that Matt's reviewing) but would make it easier to evaluate this change. > > I don't like the extra indirection either. However, many of the fields that the > Delegate requires are private members of URLRequestHttpJob. If we remove the > Context, we need to change these private members (e.g. GetMimeType()) to public > and add accessors for those that are not exposed (e.g. |is_cached_content_|) > > How about we revisit this in a follow-up? I can add a TODO. That's fine; thanks. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:22: SdchPolicyDelegate::SdchPolicyDelegate(std::unique_ptr<Context> context) In the original code, there is a data member possible_pass_through_ that is set based on the filter type (TENTATIVE_SDCH). I don't see anything like that in this code. Can you give me the reasoning for leaving that out? Does it really not change behavior to do so? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:27: // Dictionary errors are often the first indication that the SDCH stream has Is it reasonable to separate this comment out into separate comments for the two functions? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:28: // become corrupt. There are many possible causes: non-200 response codes, a How does a non-200 response code lead here? I would think that would be orthogonal to an SDCH dictionary error (i.e. the server might send Content-Encoding: sdch with a non-200 body, but it seems like there's no reason the non-200 body would lead to a corrupt dictionary id). (Fine to say that this logic is copied from old code and we'll postpone investigation until another CL.) https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:33: // mode if appropriate, or failing the request if the error is unrecoverable. Nice job turning the cascade in SdchFilter::ReadFilteredData() into these two functions; I find this much more readable and amenable to modification. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:71: return NONE; Why isn't this meta-refresh? I don't have confidence I'm completely tracking the logic (I'll return to it, but I've never had confidence that I was completely tracking the logic in the *original* code :-J.) but it seems like this is where we get on a non-cached response, where the dictionary id wasn't properly formatted, which seems like a classic possible case of a bad intermediate messing with us, which is what meta-refresh is for. ETA: It looks to me like the original cascade in sdch_filter.cc does meta-refresh here. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:102: // blacklist entry. If the content was cached, no blacklisting is needed. Hmmm. You can bounce this for this CL if you want, but now that this case is called out so clearly, wouldn't it be more appropriate to fail the request rather than issue a meta-refresh? Meta-refreshes are for interfering intermediates, and if we've gotten a valid dictionary id and retrieved the dictionary, that's an argument that there isn't anything too bogus going on in the intermediate case, and the server is just broken. For that, failing the request seems more appropriate. I'd be curious what you think of this logic even if your response is (as it probably should be) "This is a refactor CL; I don't want to change behavior in it." https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:148: SdchSourceStream* source, Why does this function (and its callers) have |source| as an argument? I don't think it's used. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... File net/filter/sdch_source_stream.cc (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:61: *consumed_bytes = input_buffer_size - input_data_size; Though (i.e. not even a suggestion :-}): I believe that in this conditional input_data_size will always be zero (because otherwise we would have copied more data into dictionary_server_id_). Would it be clearer to just set *consumed_bytes to input_buffer_size and have a DCHECK? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:67: // CouldBeDictionaryId() returns true. Is this comment still valid/positioned appropriately? I remember reading it and it making sense on the first round, but the code that I remember it referring to is well below this line. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:121: if (!buffered_output_.empty()) Suggestion: I'm uncomfortable with the layering semi-violation here. buffered_output_ is engaged with both in FlushBufferredOutput and here, requiring modifications to each location to be aware of the code at the other location. It's true, they're both part of the same class. But I'd be happier if the variable was only touched in one of inside FlushBufferredOutput() or out here. Given the use in STATE_PASS_THROUGH, how about passing bufferred_output_ as an argument, and pulling FLushBufferredOutput into a static or file-local function? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:123: if (!error_occurred_) I'm confused about how this works in the |error_occurred_ == true| case. Won't the while loop around the switch statement keep returning us to this state, which won't do anything? (Presuming there's still data available from upstream.) https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:124: input_state_ = STATE_DECODE; Ok, I think I'm clearer on this condition now; basically, in normal operation we ping-pong back and forth between STATE_DECODE and STATE_FLUSH_INTERNAL_BUFFER depending on whether we information buffered. However, I'm a bit uncomfortable about consistency among the states here. What we have is: * STATE_PASS_THROUGH: Takes care of flushing bufferred_output_ itself. * STATE_DECODE: Ping-pongs back and forth with FLUSH_INTERNAL_BUFFER as needed. * STATE_FLUSH_INTERNAL_BUFFER: Does double duty as its own state (meaning "we've had the output replaced, and we need to flush it and do nothing more") and as a helper state for DECODE. The two problems I see with that are a) the states don't actually map to different states of processing the input, and b) they aren't consistent in the relationship between the use of FlushBufferedOutput() and use of STATE_FLUSH_INTERNAL_BUFFER. My suggestion for fixing this would be: * Put a prefix at the beginning of the code for STATE_DECODE similar to the one at the beginning of STATE_PASS_THROUGH that flushes anything in the buffer. * Get rid of the error_occurred_ boolean (:-}) and rename STATE_FLUSH_INTERNAL_BUFFER to STATE_OUTPUT_REPLACED. WDYT? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... File net/filter/sdch_source_stream.h (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:39: enum ErrorRecover { nit: ErrorRecovery? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:53: // is returned, also write the output to be replaced with to nit: "it will also write ..." https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:62: // If REPLACE_OUTPUT is returned, also write the output to be replaced with ditto: "it will also write .." https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:70: // OnDictionaryError above. This method must not retain a reference to nit: OnDictionaryIdError. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:71: // |source| after returning. nit: For consistency I'd either have OnGetDictionaryError() refer to OnDictionaryIdError(), or have this duplicate the documentation as OnGetDictionaryError() does; your choice. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:80: // otherwise. Would you note somewhere in here that the delegate is required to make sure that the pointer written into |*text| remains valid for the lifetime of the delegate? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:85: // a refcounted buffer and add a callback. nit, suggestion: I'd refer to an IOBuffer rather than a refcounted buffer. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:118: // Helper method to handle error returned by Delegate. nit, suggestion: Add documentation indicating that it sets input_state_ and returns true if the error can be handles, and false if it should be returned as an error. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... File net/filter/sdch_source_stream_unittest.cc (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream_unittest.cc:516: Should we have a DecompressWithSmallOutputBuffer test? Or is that covered by one of the above and I missed it?
Patchset #6 (id:160001) has been deleted
Thanks a lot for the review! PTAL. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:22: SdchPolicyDelegate::SdchPolicyDelegate(std::unique_ptr<Context> context) On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > In the original code, there is a data member possible_pass_through_ that is set > based on the filter type (TENTATIVE_SDCH). I don't see anything like that in > this code. Can you give me the reasoning for leaving that out? Does it really > not change behavior to do so? Thanks for catching that. I have no idea why that is left out. Probably because the fixup logic is very clever that it's hard to fit it in this new design :\ I am not sure how we can incorporate that fixup logic. What do you recommend? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:27: // Dictionary errors are often the first indication that the SDCH stream has On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Is it reasonable to separate this comment out into separate comments for the two > functions? Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:28: // become corrupt. There are many possible causes: non-200 response codes, a On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > How does a non-200 response code lead here? I would think that would be > orthogonal to an SDCH dictionary error (i.e. the server might send > Content-Encoding: sdch with a non-200 body, but it seems like there's no reason > the non-200 body would lead to a corrupt dictionary id). > > (Fine to say that this logic is copied from old code and we'll postpone > investigation until another CL.) Acknowledged. Not sure what the reason is. I believe this is the same as the old code (excerpt below). } else if (filter_context_.GetResponseCode() != 200) { // We need to meta-refresh, with SDCH disabled. cause = RESPONSE_NOT_200; } https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:33: // mode if appropriate, or failing the request if the error is unrecoverable. On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Nice job turning the cascade in SdchFilter::ReadFilteredData() into these two > functions; I find this much more readable and amenable to modification. I'll give credit to Elly :) https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:71: return NONE; On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Why isn't this meta-refresh? I don't have confidence I'm completely tracking > the logic (I'll return to it, but I've never had confidence that I was > completely tracking the logic in the *original* code :-J.) but it seems like > this is where we get on a non-cached response, where the dictionary id wasn't > properly formatted, which seems like a classic possible case of a bad > intermediate messing with us, which is what meta-refresh is for. > > ETA: It looks to me like the original cascade in sdch_filter.cc does > meta-refresh here. Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:102: // blacklist entry. If the content was cached, no blacklisting is needed. On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Hmmm. You can bounce this for this CL if you want, but now that this case is > called out so clearly, wouldn't it be more appropriate to fail the request > rather than issue a meta-refresh? Meta-refreshes are for interfering > intermediates, and if we've gotten a valid dictionary id and retrieved the > dictionary, that's an argument that there isn't anything too bogus going on in > the intermediate case, and the server is just broken. For that, failing the > request seems more appropriate. I'd be curious what you think of this logic > even if your response is (as it probably should be) "This is a refactor CL; I > don't want to change behavior in it." That's a good point. I think we can include this in the "getting rid of meta-refresh" investigation. crbug.com/651821 https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:148: SdchSourceStream* source, On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Why does this function (and its callers) have |source| as an argument? I don't > think it's used. Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... File net/filter/sdch_source_stream.cc (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:61: *consumed_bytes = input_buffer_size - input_data_size; On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Though (i.e. not even a suggestion :-}): I believe that in this conditional > input_data_size will always be zero (because otherwise we would have copied more > data into dictionary_server_id_). Would it be clearer to just set > *consumed_bytes to input_buffer_size and have a DCHECK? Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:67: // CouldBeDictionaryId() returns true. On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Is this comment still valid/positioned appropriately? I remember reading it and > it making sense on the first round, but the code that I remember it referring to > is well below this line. Done. Good catch! https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:121: if (!buffered_output_.empty()) On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Suggestion: I'm uncomfortable with the layering semi-violation here. > buffered_output_ is engaged with both in FlushBufferredOutput and here, > requiring modifications to each location to be aware of the code at the other > location. It's true, they're both part of the same class. But I'd be happier > if the variable was only touched in one of inside FlushBufferredOutput() or out > here. > > Given the use in STATE_PASS_THROUGH, how about passing bufferred_output_ as an > argument, and pulling FLushBufferredOutput into a static or file-local function? Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:123: if (!error_occurred_) On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > I'm confused about how this works in the |error_occurred_ == true| case. Won't > the while loop around the switch statement keep returning us to this state, > which won't do anything? (Presuming there's still data available from > upstream.) Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:124: input_state_ = STATE_DECODE; On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Ok, I think I'm clearer on this condition now; basically, in normal operation we > ping-pong back and forth between STATE_DECODE and STATE_FLUSH_INTERNAL_BUFFER > depending on whether we information buffered. > > However, I'm a bit uncomfortable about consistency among the states here. What > we have is: > > * STATE_PASS_THROUGH: Takes care of flushing bufferred_output_ itself. > * STATE_DECODE: Ping-pongs back and forth with FLUSH_INTERNAL_BUFFER as needed. > * STATE_FLUSH_INTERNAL_BUFFER: Does double duty as its own state (meaning "we've > had the output replaced, and we need to flush it and do nothing more") and as a > helper state for DECODE. > The two problems I see with that are a) the states don't actually map to > different states of processing the input, and b) they aren't consistent in the > relationship between the use of FlushBufferedOutput() and use of > STATE_FLUSH_INTERNAL_BUFFER. > > My suggestion for fixing this would be: > * Put a prefix at the beginning of the code for STATE_DECODE similar to the one > at the beginning of STATE_PASS_THROUGH that flushes anything in the buffer. > * Get rid of the error_occurred_ boolean (:-}) and rename > STATE_FLUSH_INTERNAL_BUFFER to STATE_OUTPUT_REPLACED. > > WDYT? Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... File net/filter/sdch_source_stream.h (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:39: enum ErrorRecover { On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > nit: ErrorRecovery? Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:53: // is returned, also write the output to be replaced with to On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > nit: "it will also write ..." Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:62: // If REPLACE_OUTPUT is returned, also write the output to be replaced with On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > ditto: "it will also write .." Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:70: // OnDictionaryError above. This method must not retain a reference to On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > nit: OnDictionaryIdError. Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:71: // |source| after returning. On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > nit: For consistency I'd either have OnGetDictionaryError() refer to > OnDictionaryIdError(), or have this duplicate the documentation as > OnGetDictionaryError() does; your choice. Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:80: // otherwise. On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > Would you note somewhere in here that the delegate is required to make sure that > the pointer written into |*text| remains valid for the lifetime of the delegate? > Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:85: // a refcounted buffer and add a callback. On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > nit, suggestion: I'd refer to an IOBuffer rather than a refcounted buffer. Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream.h:118: // Helper method to handle error returned by Delegate. On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > nit, suggestion: Add documentation indicating that it sets input_state_ and > returns true if the error can be handles, and false if it should be returned as > an error. Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... File net/filter/sdch_source_stream_unittest.cc (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_source... net/filter/sdch_source_stream_unittest.cc:516: On 2016/10/04 20:06:30, Randy Smith - Not in Mondays wrote: > Should we have a DecompressWithSmallOutputBuffer test? Or is that covered by > one of the above and I missed it? On line 344? TEST_P(SdchSourceStreamTest, DecompressWithSmallOutputBuffer) {
The CQ bit was checked by xunjieli@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by xunjieli@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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Helen, one big issue I noticed (sorry for not noticing it earlier) is the lack of histograms. Is that intentional? Could you give me the argument for/against? Given the current lack of ownership of SDCH I'm even more scared about making evaluation of usage and problems even harder, which I think nuking the histograms would do. Also: Tests for SdchPolicyDelegate? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:22: SdchPolicyDelegate::SdchPolicyDelegate(std::unique_ptr<Context> context) On 2016/10/05 13:44:56, xunjieli wrote: > On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > > In the original code, there is a data member possible_pass_through_ that is > set > > based on the filter type (TENTATIVE_SDCH). I don't see anything like that in > > this code. Can you give me the reasoning for leaving that out? Does it > really > > not change behavior to do so? > > Thanks for catching that. I have no idea why that is left out. Probably because > the fixup logic is very clever that it's hard to fit it in this new design :\ I > am not sure how we can incorporate that fixup logic. What do you recommend? Ok. After a slog through the original code, I've convinced myself (95%; as I say, I'm never *completely* sure I know what that code is doing) that possible_pass_through_ has no effect on filter behavior (!). However, I'm still uncomfortable about not plumbing that information through, because long-term I think it should have an effect on filter behavior--specifically, I think it should result in a pass through in cases where we now initiate meta-refresh. I can't suggest how to do that without looking at the CL that initializes these classes--willing to give me a pointer to that CL? But I think the right thing to do is something like adding possible_pass_through as an argument to the SdchPolicyDelegate constructor based on the type of the filter; I presume that wouldn't be very hard to plumb? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:28: // become corrupt. There are many possible causes: non-200 response codes, a On 2016/10/05 13:44:56, xunjieli wrote: > On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > > How does a non-200 response code lead here? I would think that would be > > orthogonal to an SDCH dictionary error (i.e. the server might send > > Content-Encoding: sdch with a non-200 body, but it seems like there's no > reason > > the non-200 body would lead to a corrupt dictionary id). > > > > (Fine to say that this logic is copied from old code and we'll postpone > > investigation until another CL.) > > Acknowledged. Not sure what the reason is. I believe this is the same as the old > code (excerpt below). > > } else if (filter_context_.GetResponseCode() != 200) { > // We need to meta-refresh, with SDCH disabled. > cause = RESPONSE_NOT_200; > } TODO/bug? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:102: // blacklist entry. If the content was cached, no blacklisting is needed. On 2016/10/05 13:44:56, xunjieli wrote: > On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > > Hmmm. You can bounce this for this CL if you want, but now that this case is > > called out so clearly, wouldn't it be more appropriate to fail the request > > rather than issue a meta-refresh? Meta-refreshes are for interfering > > intermediates, and if we've gotten a valid dictionary id and retrieved the > > dictionary, that's an argument that there isn't anything too bogus going on in > > the intermediate case, and the server is just broken. For that, failing the > > request seems more appropriate. I'd be curious what you think of this logic > > even if your response is (as it probably should be) "This is a refactor CL; I > > don't want to change behavior in it." > > That's a good point. I think we can include this in the "getting rid of > meta-refresh" investigation. crbug.com/651821 Could you record the logic/specific sub-issue above in the bug? https://codereview.chromium.org/2368433002/diff/200001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/200001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:79: // through. This doesn't make sense to me, just because if we're here, the response looked like it has a valid dictionary hash, so I think the chances of it being unencoded are low. I agree this matches the old code, so I don't want to get you to change it in this CL, but put in a TODO that this is probably bogus? (I don't think the existing TODO covers the issue I'm discussing in this comment.) https://codereview.chromium.org/2368433002/diff/200001/net/filter/sdch_source... File net/filter/sdch_source_stream.cc (right): https://codereview.chromium.org/2368433002/diff/200001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:142: bytes_out += flushed; nit, suggestion (I don't have strong feelings here): This code is correct, but the majority of time spent in this state will have these five lines be a no-op because bufferred_output_ will be empty. I'm wondering about a performance optimization were we skip them if bufferred_output_ is empty. Up to you.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Patchset #8 (id:220001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the reviews! I didn't realize the missing UMAs. I agree it's very important to keep them. Uploaded a new patch with sdch_policy_delegate_unittest.cc. PTAL? https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:22: SdchPolicyDelegate::SdchPolicyDelegate(std::unique_ptr<Context> context) On 2016/10/07 18:46:25, Randy Smith - Not in Mondays wrote: > On 2016/10/05 13:44:56, xunjieli wrote: > > On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > > > In the original code, there is a data member possible_pass_through_ that is > > set > > > based on the filter type (TENTATIVE_SDCH). I don't see anything like that > in > > > this code. Can you give me the reasoning for leaving that out? Does it > > really > > > not change behavior to do so? > > > > Thanks for catching that. I have no idea why that is left out. Probably > because > > the fixup logic is very clever that it's hard to fit it in this new design :\ > I > > am not sure how we can incorporate that fixup logic. What do you recommend? > > Ok. After a slog through the original code, I've convinced myself (95%; as I > say, I'm never *completely* sure I know what that code is doing) that > possible_pass_through_ has no effect on filter behavior (!). However, I'm > still uncomfortable about not plumbing that information through, because > long-term I think it should have an effect on filter behavior--specifically, I > think it should result in a pass through in cases where we now initiate > meta-refresh. > > I can't suggest how to do that without looking at the CL that initializes these > classes--willing to give me a pointer to that CL? But I think the right thing > to do is something like adding possible_pass_through as an argument to the > SdchPolicyDelegate constructor based on the type of the filter; I presume that > wouldn't be very hard to plumb? Done. The CL is at https://codereview.chromium.org/2373003003/. However, it doesn't have the logic to add a "possible passthrough" sdch filter and a gzip filter (Filter::FixupEncodingTypes in original code) . I will port that part in that CL and add you as a reviewer (currently cc-ed). https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:28: // become corrupt. There are many possible causes: non-200 response codes, a On 2016/10/07 18:46:25, Randy Smith - Not in Mondays wrote: > On 2016/10/05 13:44:56, xunjieli wrote: > > On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > > > How does a non-200 response code lead here? I would think that would be > > > orthogonal to an SDCH dictionary error (i.e. the server might send > > > Content-Encoding: sdch with a non-200 body, but it seems like there's no > > reason > > > the non-200 body would lead to a corrupt dictionary id). > > > > > > (Fine to say that this logic is copied from old code and we'll postpone > > > investigation until another CL.) > > > > Acknowledged. Not sure what the reason is. I believe this is the same as the > old > > code (excerpt below). > > > > } else if (filter_context_.GetResponseCode() != 200) { > > // We need to meta-refresh, with SDCH disabled. > > cause = RESPONSE_NOT_200; > > } > > TODO/bug? Done. https://codereview.chromium.org/2368433002/diff/140001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:102: // blacklist entry. If the content was cached, no blacklisting is needed. On 2016/10/07 18:46:25, Randy Smith - Not in Mondays wrote: > On 2016/10/05 13:44:56, xunjieli wrote: > > On 2016/10/04 20:06:29, Randy Smith - Not in Mondays wrote: > > > Hmmm. You can bounce this for this CL if you want, but now that this case > is > > > called out so clearly, wouldn't it be more appropriate to fail the request > > > rather than issue a meta-refresh? Meta-refreshes are for interfering > > > intermediates, and if we've gotten a valid dictionary id and retrieved the > > > dictionary, that's an argument that there isn't anything too bogus going on > in > > > the intermediate case, and the server is just broken. For that, failing the > > > request seems more appropriate. I'd be curious what you think of this logic > > > even if your response is (as it probably should be) "This is a refactor CL; > I > > > don't want to change behavior in it." > > > > That's a good point. I think we can include this in the "getting rid of > > meta-refresh" investigation. crbug.com/651821 > > Could you record the logic/specific sub-issue above in the bug? Done. Added in the bug. https://codereview.chromium.org/2368433002/diff/200001/net/filter/sdch_source... File net/filter/sdch_source_stream.cc (right): https://codereview.chromium.org/2368433002/diff/200001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:142: bytes_out += flushed; On 2016/10/07 18:46:26, Randy Smith - Not in Mondays wrote: > nit, suggestion (I don't have strong feelings here): This code is correct, but > the majority of time spent in this state will have these five lines be a no-op > because bufferred_output_ will be empty. I'm wondering about a performance > optimization were we skip them if bufferred_output_ is empty. Up to you. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Hi Randy, thanks for the suggestion to change ownership of SdchPolicyDelegate. There's only one slight problem, see below. I have uploaded a new patch. Could you take a look? https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:295: // TODO(xunjieli): Do we need to call URLRequestHttpJob::RecordPacketStats Randy: One problem with this approach is that the Delegate can no longer call into URLRequestHttpJob to RecordPacketStats. Let me know if that's something you'd like me to fix.
The CQ bit was checked by xunjieli@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: Exceeded global retry quota
The CQ bit was checked by xunjieli@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: Exceeded global retry quota
https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:295: // TODO(xunjieli): Do we need to call URLRequestHttpJob::RecordPacketStats On 2016/10/12 13:27:12, xunjieli wrote: > Randy: One problem with this approach is that the Delegate can no longer call > into URLRequestHttpJob to RecordPacketStats. Let me know if that's something > you'd like me to fix. Oh, good point. However, after some code chasing, I note that this was a problem in the old code too (SdchFilter was owned URLRequestJob, not URLRequestHttpJob, but needed to call into URLRHJ when it was destroyed) and it was handled by a special call from ~URLRequestHttpJob asking the URLRJ to destroy the filters. Can we do the same thing here? (This would mean you wouldn't need to switch the ownership of the delegate if you didn't want to, or copy in the arguments to the constructor, though I continue to think that that's a useful refactor on its own.)
https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:295: // TODO(xunjieli): Do we need to call URLRequestHttpJob::RecordPacketStats On 2016/10/12 20:41:44, Randy Smith - Not in Mondays wrote: > On 2016/10/12 13:27:12, xunjieli wrote: > > Randy: One problem with this approach is that the Delegate can no longer call > > into URLRequestHttpJob to RecordPacketStats. Let me know if that's something > > you'd like me to fix. > > Oh, good point. > > However, after some code chasing, I note that this was a problem in the old code > too (SdchFilter was owned URLRequestJob, not URLRequestHttpJob, but needed to > call into URLRHJ when it was destroyed) and it was handled by a special call > from ~URLRequestHttpJob asking the URLRJ to destroy the filters. Can we do the > same thing here? (This would mean you wouldn't need to switch the ownership of > the delegate if you didn't want to, or copy in the arguments to the constructor, > though I continue to think that that's a useful refactor on its own.) > (For the record, I consider the above solution a hack. However, it's a previously existing hack, so I don't mind keeping it for the moment.)
On 2016/10/12 20:42:18, Randy Smith - Not in Mondays wrote: > https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... > File net/filter/sdch_policy_delegate.cc (right): > > https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... > net/filter/sdch_policy_delegate.cc:295: // TODO(xunjieli): Do we need to call > URLRequestHttpJob::RecordPacketStats > On 2016/10/12 20:41:44, Randy Smith - Not in Mondays wrote: > > On 2016/10/12 13:27:12, xunjieli wrote: > > > Randy: One problem with this approach is that the Delegate can no longer > call > > > into URLRequestHttpJob to RecordPacketStats. Let me know if that's something > > > you'd like me to fix. > > > > Oh, good point. > > > > However, after some code chasing, I note that this was a problem in the old > code > > too (SdchFilter was owned URLRequestJob, not URLRequestHttpJob, but needed to > > call into URLRHJ when it was destroyed) and it was handled by a special call > > from ~URLRequestHttpJob asking the URLRJ to destroy the filters. Can we do > the > > same thing here? (This would mean you wouldn't need to switch the ownership > of > > the delegate if you didn't want to, or copy in the arguments to the > constructor, > > though I continue to think that that's a useful refactor on its own.) > > > > (For the record, I consider the above solution a hack. However, it's a > previously existing hack, so I don't mind keeping it for the moment.) Can we just remove those histograms? Does anyone care about them?
https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:295: // TODO(xunjieli): Do we need to call URLRequestHttpJob::RecordPacketStats On 2016/10/12 20:42:18, Randy Smith - Not in Mondays wrote: > On 2016/10/12 20:41:44, Randy Smith - Not in Mondays wrote: > > On 2016/10/12 13:27:12, xunjieli wrote: > > > Randy: One problem with this approach is that the Delegate can no longer > call > > > into URLRequestHttpJob to RecordPacketStats. Let me know if that's something > > > you'd like me to fix. > > > > Oh, good point. > > > > However, after some code chasing, I note that this was a problem in the old > code > > too (SdchFilter was owned URLRequestJob, not URLRequestHttpJob, but needed to > > call into URLRHJ when it was destroyed) and it was handled by a special call > > from ~URLRequestHttpJob asking the URLRJ to destroy the filters. Can we do > the > > same thing here? (This would mean you wouldn't need to switch the ownership > of > > the delegate if you didn't want to, or copy in the arguments to the > constructor, > > though I continue to think that that's a useful refactor on its own.) > > > > (For the record, I consider the above solution a hack. However, it's a > previously existing hack, so I don't mind keeping it for the moment.) Thanks for looking into this! I have the same question as Matt. Since calling into URLJ to destroy filter in URLRHJ is a hack, can we get rid of it? What will be lost if we get rid of Sdch3.Network_Decode_Bytes_Processed_b, Sdch3.Experiment3_Decode and Sdch3.Experiment3_Holdback ? We can do the same hack, but I don't think there's anyone paying attention to those UMAs. I am not even sure what we gain from tracking "_Decode" and "_Holdback". We do have UMAs tracking failures. Do you feel strongly about porting these?
On 2016/10/12 21:04:33, xunjieli wrote: > https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... > File net/filter/sdch_policy_delegate.cc (right): > > https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... > net/filter/sdch_policy_delegate.cc:295: // TODO(xunjieli): Do we need to call > URLRequestHttpJob::RecordPacketStats > On 2016/10/12 20:42:18, Randy Smith - Not in Mondays wrote: > > On 2016/10/12 20:41:44, Randy Smith - Not in Mondays wrote: > > > On 2016/10/12 13:27:12, xunjieli wrote: > > > > Randy: One problem with this approach is that the Delegate can no longer > > call > > > > into URLRequestHttpJob to RecordPacketStats. Let me know if that's > something > > > > you'd like me to fix. > > > > > > Oh, good point. > > > > > > However, after some code chasing, I note that this was a problem in the old > > code > > > too (SdchFilter was owned URLRequestJob, not URLRequestHttpJob, but needed > to > > > call into URLRHJ when it was destroyed) and it was handled by a special call > > > from ~URLRequestHttpJob asking the URLRJ to destroy the filters. Can we do > > the > > > same thing here? (This would mean you wouldn't need to switch the ownership > > of > > > the delegate if you didn't want to, or copy in the arguments to the > > constructor, > > > though I continue to think that that's a useful refactor on its own.) > > > > > > > (For the record, I consider the above solution a hack. However, it's a > > previously existing hack, so I don't mind keeping it for the moment.) > > Thanks for looking into this! I have the same question as Matt. Since calling > into URLJ to destroy filter in URLRHJ is a hack, can we get rid of it? > > What will be lost if we get rid of > Sdch3.Network_Decode_Bytes_Processed_b, > Sdch3.Experiment3_Decode and > Sdch3.Experiment3_Holdback ? > > We can do the same hack, but I don't think there's anyone paying attention to > those UMAs. I am not even sure what we gain from tracking "_Decode" and > "_Holdback". We do have UMAs tracking failures. > Do you feel strongly about porting these? Yeah, I'm sorry, I do (specifically Decode and Holdback); a need for this just came up in the context of the background discussion about the value of SDCH and how to move forward with it. Is there a problem with porting the current fix for this problem into this CL? It holds to the "refactoring CLs shouldn't change functionality" (depending a bit on whether you consider histograms functionality or not).
SGTM. I will port the logic. will update this thread once that is ready. On Thu, Oct 13, 2016 at 11:25 AM, <rdsmith@chromium.org> wrote: > On 2016/10/12 21:04:33, xunjieli wrote: > > > https://codereview.chromium.org/2368433002/diff/260001/ > net/filter/sdch_policy_delegate.cc > > File net/filter/sdch_policy_delegate.cc (right): > > > > > https://codereview.chromium.org/2368433002/diff/260001/ > net/filter/sdch_policy_delegate.cc#newcode295 > > net/filter/sdch_policy_delegate.cc:295: // TODO(xunjieli): Do we need > to call > > URLRequestHttpJob::RecordPacketStats > > On 2016/10/12 20:42:18, Randy Smith - Not in Mondays wrote: > > > On 2016/10/12 20:41:44, Randy Smith - Not in Mondays wrote: > > > > On 2016/10/12 13:27:12, xunjieli wrote: > > > > > Randy: One problem with this approach is that the Delegate can no > longer > > > call > > > > > into URLRequestHttpJob to RecordPacketStats. Let me know if that's > > something > > > > > you'd like me to fix. > > > > > > > > Oh, good point. > > > > > > > > However, after some code chasing, I note that this was a problem in > the > old > > > code > > > > too (SdchFilter was owned URLRequestJob, not URLRequestHttpJob, but > needed > > to > > > > call into URLRHJ when it was destroyed) and it was handled by a > special > call > > > > from ~URLRequestHttpJob asking the URLRJ to destroy the filters. Can > we > do > > > the > > > > same thing here? (This would mean you wouldn't need to switch the > ownership > > > of > > > > the delegate if you didn't want to, or copy in the arguments to the > > > constructor, > > > > though I continue to think that that's a useful refactor on its own.) > > > > > > > > > > (For the record, I consider the above solution a hack. However, it's a > > > previously existing hack, so I don't mind keeping it for the moment.) > > > > Thanks for looking into this! I have the same question as Matt. Since > calling > > into URLJ to destroy filter in URLRHJ is a hack, can we get rid of it? > > > > What will be lost if we get rid of > > Sdch3.Network_Decode_Bytes_Processed_b, > > Sdch3.Experiment3_Decode and > > Sdch3.Experiment3_Holdback ? > > > > We can do the same hack, but I don't think there's anyone paying > attention to > > those UMAs. I am not even sure what we gain from tracking "_Decode" and > > "_Holdback". We do have UMAs tracking failures. > > Do you feel strongly about porting these? > > Yeah, I'm sorry, I do (specifically Decode and Holdback); a need for this > just > came up in the context of the background discussion about the value of > SDCH and > how to move forward with it. > > Is there a problem with porting the current fix for this problem into this > CL? > It holds to the "refactoring CLs shouldn't change functionality" > (depending a > bit on whether you consider histograms functionality or not). > > https://codereview.chromium.org/2368433002/ > -- 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.
Thanks for the review! PTAL at #PS11. https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/260001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:295: // TODO(xunjieli): Do we need to call URLRequestHttpJob::RecordPacketStats On 2016/10/12 21:04:33, xunjieli wrote: > On 2016/10/12 20:42:18, Randy Smith - Not in Mondays wrote: > > On 2016/10/12 20:41:44, Randy Smith - Not in Mondays wrote: > > > On 2016/10/12 13:27:12, xunjieli wrote: > > > > Randy: One problem with this approach is that the Delegate can no longer > > call > > > > into URLRequestHttpJob to RecordPacketStats. Let me know if that's > something > > > > you'd like me to fix. > > > > > > Oh, good point. > > > > > > However, after some code chasing, I note that this was a problem in the old > > code > > > too (SdchFilter was owned URLRequestJob, not URLRequestHttpJob, but needed > to > > > call into URLRHJ when it was destroyed) and it was handled by a special call > > > from ~URLRequestHttpJob asking the URLRJ to destroy the filters. Can we do > > the > > > same thing here? (This would mean you wouldn't need to switch the ownership > > of > > > the delegate if you didn't want to, or copy in the arguments to the > > constructor, > > > though I continue to think that that's a useful refactor on its own.) > > > > > > > (For the record, I consider the above solution a hack. However, it's a > > previously existing hack, so I don't mind keeping it for the moment.) > > Thanks for looking into this! I have the same question as Matt. Since calling > into URLJ to destroy filter in URLRHJ is a hack, can we get rid of it? > > What will be lost if we get rid of > Sdch3.Network_Decode_Bytes_Processed_b, > Sdch3.Experiment3_Decode and > Sdch3.Experiment3_Holdback ? > > We can do the same hack, but I don't think there's anyone paying attention to > those UMAs. I am not even sure what we gain from tracking "_Decode" and > "_Holdback". We do have UMAs tracking failures. > Do you feel strongly about porting these? Done.
The CQ bit was checked by xunjieli@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by xunjieli@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Not surprisingly, a major textual change leads to more comments. Sorry :-J. These are mostly (but not completely) nits and suggestions; I still don't think we're very far away. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:55: if (!types->empty() && types->front() == SourceStream::TYPE_SDCH) { This looks to me like a functionality change? In the original FixupEncodingTypes(), this conditional was after the early return for dictionaries not being advertised; here, it's before. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:80: // filters if headers are not found. This is how they should work, but IIUC TENTATIVE filters currently fail the request/do a meta refresh if the response isn't encoded with SDCH. Presuming you agree with my understanding, the comment shouldn't contradict reality. ETA: And now that I do a detailed comparison with filter.cc, I see that the comment is copied. Maybe add a sentence saying that this is a goal not a description of the code? https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:86: // content encoding of "sdch,gzip" being corrupted into "sdch" with on change nit: "on" -> "no" https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:87: // of the actual content. Another common corruption is to either disscard nit: "discard" https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:97: // never surfaced in the wild (and is unlikely to). Sadly, this last sentence is no longer true. I have no idea what to do about that. I guess just remove it? (I believe Google search sends JS content SDCH encoded, at least some times.) https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:130: // tentative decodings to be done afterwards. Vodaphone UK reportedyl will nit: "reportedly" https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:153: return PASS_THROUGH; Sadly, I think this is a change from the original code (sadly, because I think this is the correct thing to do). I believe the original code goes through the meta-refresh logic in this case. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:189: return IssueMetaRefreshIfPossible(replace_output); Hmmm. In the old code, no matter how we left the error handling, we logged a response cause. This looks like a catch-all if we don't find anything else. IIUC, this is the RESPONSE_CORRUPT_SDCH case; do you agree? https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:210: return IssueMetaRefreshIfPossible(replace_output); Much like the above, the lack of a LogCorruptionDetection() here looks wrong to me, since the old code had one for every possible exist from the error path (by design). I believe this is the RESPONSE_NO_DICTIONARY case, and that this is a better place for logging it than below. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:267: LogCorruptionDetection(RESPONSE_NO_DICTIONARY); This seems like the wrong place to log a corruption reason. All the other ones are logged in one of the error handling routines, and this error was not logged in this code in the original (though we did call LogSdchProblem). Move the corruption call to the location I call out above? https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:272: void SdchPolicyDelegate::OnStreamDestroyed( Why a separate function rather than in ~SdchPolicyDelegate? https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:289: // FIXME(xunjieli): Why do we do an early return here? For the logging, maybe because we've already recorded the problems whenever we filled the cache? (Guess). For the RecordPacketStats, I think that's specifically targeted at network requests, not cache filled requests. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:349: void SdchPolicyDelegate::LogSdchProblem(NetLogWithSource netlog, nit: // static https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:356: void SdchPolicyDelegate::LogCorruptionDetection( Suggestion: It feels weird/inconsistent to have two routines in a row that both do netlogging, but have one take a netlog argument and one not (even though one's static and the other isn't). Maybe make them both static and pass in all the args this routine needs? Though at that point this could just be a file local routine, which would put it elsewhere in the file, so the consistency wouldn't matter as much. Up to you whether or how much of this suggestion you want to take. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:55: static void LogSdchProblem(NetLogWithSource netlog, SdchProblemCode problem); Suggestion: This feels like a very generic (in an SDCH context) utility function--I don't think it has much relationship to the rest of what the delegate does. Maybe more appropriate for SdchManager? Or alternatively just eliminate it and call the two lines everywhere it's called? But I don't feel strongly. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:110: // domain either temporarily or permanently. Suggestion: Indicate at least in the abstract what results in a temporary blacklist and what in a permanent blacklist. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:114: // LogSdchProblem(). TODO(xunjieli): Can we get rid of this? Just noting that I found it very useful in trying to actually do useful statistics on SDCH behavior in the wild. When we cleanup the delegate/all the extra stuff we do to handle corner cases we can probably cleanup this, but I'd recommend against doing so before that. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:120: static std::unique_ptr<base::Value> NetLogResponseCorruptionDetectionCallback( Why a static class member function instead of a file local function? https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate_unittest.cc (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate_unittest.cc:143: TEST_F(SdchPolicyDelegateTest, FixUpSdchContentEncodings) { nit, suggestion: Given that the next test is also testing various aspects of FixUpSdchContentEncodings, maybe change this name to more specifically name what it's testing? https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_source... File net/filter/sdch_source_stream.cc (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:41: possible_pass_through_(type == SourceStream::TYPE_SDCH_POSSIBLE), It looks to me as if this variable is only used by passing it to the delegate (and the DCHECK below, which I'd be comfortable skipping to simplify the class interface). How would you feel about pulling it out of the constructor for SdchSourceStream() and passing it into the delegate constructor? I think it's more appropriately stored in the delegate, which is where it's going to be used.
The CQ bit was checked by xunjieli@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #13 (id:330001) has been deleted
The CQ bit was checked by xunjieli@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...
Thanks for the review! I have addressed all except the one about eliminating SdchPolicyDelegate::OnStreamDestroyed(). PTAL. Thanks! https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:55: if (!types->empty() && types->front() == SourceStream::TYPE_SDCH) { On 2016/10/13 22:05:25, Randy Smith - Not in Mondays wrote: > This looks to me like a functionality change? In the original > FixupEncodingTypes(), this conditional was after the early return for > dictionaries not being advertised; here, it's before. Done. Sorry about that! Now it should match to FixupEncodingTypes() perfectly. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:80: // filters if headers are not found. On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > This is how they should work, but IIUC TENTATIVE filters currently fail the > request/do a meta refresh if the response isn't encoded with SDCH. Presuming > you agree with my understanding, the comment shouldn't contradict reality. > > ETA: And now that I do a detailed comparison with filter.cc, I see that the > comment is copied. Maybe add a sentence saying that this is a goal not a > description of the code? Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:86: // content encoding of "sdch,gzip" being corrupted into "sdch" with on change On 2016/10/13 22:05:25, Randy Smith - Not in Mondays wrote: > nit: "on" -> "no" Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:87: // of the actual content. Another common corruption is to either disscard On 2016/10/13 22:05:25, Randy Smith - Not in Mondays wrote: > nit: "discard" Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:97: // never surfaced in the wild (and is unlikely to). On 2016/10/13 22:05:25, Randy Smith - Not in Mondays wrote: > Sadly, this last sentence is no longer true. I have no idea what to do about > that. I guess just remove it? (I believe Google search sends JS content SDCH > encoded, at least some times.) Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:130: // tentative decodings to be done afterwards. Vodaphone UK reportedyl will On 2016/10/13 22:05:25, Randy Smith - Not in Mondays wrote: > nit: "reportedly" Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:153: return PASS_THROUGH; On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > Sadly, I think this is a change from the original code (sadly, because I think > this is the correct thing to do). I believe the original code goes through the > meta-refresh logic in this case. Done. Ah, you are right! I didn't realize that. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:189: return IssueMetaRefreshIfPossible(replace_output); On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > Hmmm. In the old code, no matter how we left the error handling, we logged a > response cause. This looks like a catch-all if we don't find anything else. > IIUC, this is the RESPONSE_CORRUPT_SDCH case; do you agree? Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:210: return IssueMetaRefreshIfPossible(replace_output); On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > Much like the above, the lack of a LogCorruptionDetection() here looks wrong to > me, since the old code had one for every possible exist from the error path (by > design). I believe this is the RESPONSE_NO_DICTIONARY case, and that this is a > better place for logging it than below. Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:267: LogCorruptionDetection(RESPONSE_NO_DICTIONARY); On 2016/10/13 22:05:25, Randy Smith - Not in Mondays wrote: > This seems like the wrong place to log a corruption reason. All the other ones > are logged in one of the error handling routines, and this error was not logged > in this code in the original (though we did call LogSdchProblem). Move the > corruption call to the location I call out above? Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:272: void SdchPolicyDelegate::OnStreamDestroyed( On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > Why a separate function rather than in ~SdchPolicyDelegate? I used a separate function to pass in |input_state|, |buffered_output| and |decoder|. Any suggestions on how to work around that if we were to use ~SdchPolicyDelegate? Thanks! https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:289: // FIXME(xunjieli): Why do we do an early return here? On 2016/10/13 22:05:25, Randy Smith - Not in Mondays wrote: > For the logging, maybe because we've already recorded the problems whenever we > filled the cache? (Guess). For the RecordPacketStats, I think that's > specifically targeted at network requests, not cache filled requests. Done. Ah, that makes sense. I added a comment. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:349: void SdchPolicyDelegate::LogSdchProblem(NetLogWithSource netlog, On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > nit: // static Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:356: void SdchPolicyDelegate::LogCorruptionDetection( On 2016/10/13 22:05:25, Randy Smith - Not in Mondays wrote: > Suggestion: It feels weird/inconsistent to have two routines in a row that both > do netlogging, but have one take a netlog argument and one not (even though > one's static and the other isn't). Maybe make them both static and pass in all > the args this routine needs? Though at that point this could just be a file > local routine, which would put it elsewhere in the file, so the consistency > wouldn't matter as much. Up to you whether or how much of this suggestion you > want to take. Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:55: static void LogSdchProblem(NetLogWithSource netlog, SdchProblemCode problem); On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > Suggestion: This feels like a very generic (in an SDCH context) utility > function--I don't think it has much relationship to the rest of what the > delegate does. Maybe more appropriate for SdchManager? Or alternatively just > eliminate it and call the two lines everywhere it's called? But I don't feel > strongly. Done. Moved to SdchManager. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:110: // domain either temporarily or permanently. On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > Suggestion: Indicate at least in the abstract what results in a temporary > blacklist and what in a permanent blacklist. Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:114: // LogSdchProblem(). TODO(xunjieli): Can we get rid of this? On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > Just noting that I found it very useful in trying to actually do useful > statistics on SDCH behavior in the wild. When we cleanup the delegate/all the > extra stuff we do to handle corner cases we can probably cleanup this, but I'd > recommend against doing so before that. Acknowledged. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:120: static std::unique_ptr<base::Value> NetLogResponseCorruptionDetectionCallback( On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > Why a static class member function instead of a file local function? Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate_unittest.cc (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate_unittest.cc:143: TEST_F(SdchPolicyDelegateTest, FixUpSdchContentEncodings) { On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > nit, suggestion: Given that the next test is also testing various aspects of > FixUpSdchContentEncodings, maybe change this name to more specifically name what > it's testing? Done. https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_source... File net/filter/sdch_source_stream.cc (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_source... net/filter/sdch_source_stream.cc:41: possible_pass_through_(type == SourceStream::TYPE_SDCH_POSSIBLE), On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > It looks to me as if this variable is only used by passing it to the delegate > (and the DCHECK below, which I'd be comfortable skipping to simplify the class > interface). How would you feel about pulling it out of the constructor for > SdchSourceStream() and passing it into the delegate constructor? I think it's > more appropriately stored in the delegate, which is where it's going to be used. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM. Thanks for your patience! https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:272: void SdchPolicyDelegate::OnStreamDestroyed( On 2016/10/14 18:23:34, xunjieli wrote: > On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > > Why a separate function rather than in ~SdchPolicyDelegate? > > I used a separate function to pass in |input_state|, |buffered_output| and > |decoder|. Any suggestions on how to work around that if we were to use > ~SdchPolicyDelegate? Thanks! Ah, good point; I should have thought of that. The only options I see are to do what you've done or to give the delegate a pointer to the stream to probe it on destruction (which is actually dangerous, since the stream will be partially destroyed at that point). Ok, this is the right thing. One thought/suggestion (up to you): Looking at this interface, I wince a bit at passing the decoder up--that feels like extra information/layer dependencies that might be avoidable. I'd be tempted to look at whether or not the information about the decode (presence+FinishDecoding()) could be contained in the input_state variable, and then just leaving out passing in the decoder (and including the related include files). I'd also suggest changing |const std::string& bufferred_output| to |bool buffered_output_present| (passing minimal information). But I'm not committed to either of those; if it looks like a hassle or you don't like the suggestions you're free not to bother. https://codereview.chromium.org/2368433002/diff/350001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/350001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:148: // some proxies strip encoding completely. nit, suggestion: I think this comment would be clearer if it was after this if condition (since the if conditional is the opposite of what's in the comment, and the additional decoding is later in the function). https://codereview.chromium.org/2368433002/diff/350001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/2368433002/diff/350001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:81: // Set when SdchPolicyDelegate is used as a possible pass-through. nit: I'd say that it's actually the stream that's a possible pass through, that's just managed from SdchPolicyDelegate.
Thanks for the review! I should thank YOU for your patience and for bearing with me on this! https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/320001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:272: void SdchPolicyDelegate::OnStreamDestroyed( On 2016/10/19 19:49:41, Randy Smith - Not in Mondays wrote: > On 2016/10/14 18:23:34, xunjieli wrote: > > On 2016/10/13 22:05:26, Randy Smith - Not in Mondays wrote: > > > Why a separate function rather than in ~SdchPolicyDelegate? > > > > I used a separate function to pass in |input_state|, |buffered_output| and > > |decoder|. Any suggestions on how to work around that if we were to use > > ~SdchPolicyDelegate? Thanks! > > Ah, good point; I should have thought of that. > > The only options I see are to do what you've done or to give the delegate a > pointer to the stream to probe it on destruction (which is actually dangerous, > since the stream will be partially destroyed at that point). Ok, this is the > right thing. > > One thought/suggestion (up to you): Looking at this interface, I wince a bit at > passing the decoder up--that feels like extra information/layer dependencies > that might be avoidable. I'd be tempted to look at whether or not the > information about the decode (presence+FinishDecoding()) could be contained in > the input_state variable, and then just leaving out passing in the decoder (and > including the related include files). I'd also suggest changing |const > std::string& bufferred_output| to |bool buffered_output_present| (passing > minimal information). But I'm not committed to either of those; if it looks > like a hassle or you don't like the suggestions you're free not to bother. Done. https://codereview.chromium.org/2368433002/diff/350001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.cc (right): https://codereview.chromium.org/2368433002/diff/350001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.cc:148: // some proxies strip encoding completely. On 2016/10/19 19:49:41, Randy Smith - Not in Mondays wrote: > nit, suggestion: I think this comment would be clearer if it was after this if > condition (since the if conditional is the opposite of what's in the comment, > and the additional decoding is later in the function). Done. https://codereview.chromium.org/2368433002/diff/350001/net/filter/sdch_policy... File net/filter/sdch_policy_delegate.h (right): https://codereview.chromium.org/2368433002/diff/350001/net/filter/sdch_policy... net/filter/sdch_policy_delegate.h:81: // Set when SdchPolicyDelegate is used as a possible pass-through. On 2016/10/19 19:49:41, Randy Smith - Not in Mondays wrote: > nit: I'd say that it's actually the stream that's a possible pass through, > that's just managed from SdchPolicyDelegate. Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2368433002/#ps370001 (title: "address comments")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #15 (id:390001) has been deleted
Patchset #15 (id:410001) has been deleted
xunjieli@chromium.org changed reviewers: + jwd@chromium.org
jwd@: Please review changes in tools/metrics/histograms/histograms.xml Thanks!
The CQ bit was checked by xunjieli@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.
jwd@: A friendly ping? Could you take a look at tools/metrics/histograms/histograms.xml? Thanks!
lgtm
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2368433002/#ps430001 (title: "Fix histograms")
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.
Committed patchset #15 (id:430001)
Message was sent while issue was closed.
Description was changed from ========== Add net::SdchSourceStream and net::SdchPolicyDelegate This CL adds a SdchSourceStream which implements FilterSourceStream to do sdch decoding. This CL additionally adds a SdchPolicyDelegate which will be the glue for URLRequestHttpJob and SdchSourceStream. This is a part of the effort to convert net::Filter into a pull-based interface. See the linked bug for more details. BUG=474859 ========== to ========== Add net::SdchSourceStream and net::SdchPolicyDelegate This CL adds a SdchSourceStream which implements FilterSourceStream to do sdch decoding. This CL additionally adds a SdchPolicyDelegate which will be the glue for URLRequestHttpJob and SdchSourceStream. This is a part of the effort to convert net::Filter into a pull-based interface. See the linked bug for more details. BUG=474859 Committed: https://crrev.com/ab5a1f3c40b48b2a0498e6469f9f8d676a728994 Cr-Commit-Position: refs/heads/master@{#426803} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/ab5a1f3c40b48b2a0498e6469f9f8d676a728994 Cr-Commit-Position: refs/heads/master@{#426803} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
