Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2282463002:
Create a Delayed Response Provider for GCDWebServer. (Closed)
DescriptionCreate a Delayed Response Provider for GCDWebServer.
This provider will provide the response from another provider after a
delay.
BUG=
Committed: https://crrev.com/6eeceba83633b457f6b5469457fbd69053deec54
Cr-Commit-Position: refs/heads/master@{#415207}
Patch Set 1 #
Total comments: 4
Patch Set 2 : typo #Patch Set 3 : namespace #
Total comments: 12
Patch Set 4 : comments #
Total comments: 8
Patch Set 5 : comments #
Total comments: 6
Patch Set 6 : ivars -> interface #
Total comments: 4
Patch Set 7 : whoops #
Messages
Total messages: 31 (11 generated)
olivierrobin@chromium.org changed reviewers: + baxley@chromium.org, gambard@chromium.org, marq@chromium.org
LGTM, thanks! https://codereview.chromium.org/2282463002/diff/1/ios/web/public/test/respons... File ios/web/public/test/response_providers/delayed_response_provider.h (right): https://codereview.chromium.org/2282463002/diff/1/ios/web/public/test/respons... ios/web/public/test/response_providers/delayed_response_provider.h:12: // A Response provider that delay the response provided by another reponse nit:s/delay/delays
https://codereview.chromium.org/2282463002/diff/1/ios/web/public/test/respons... File ios/web/public/test/response_providers/delayed_response_provider.h (right): https://codereview.chromium.org/2282463002/diff/1/ios/web/public/test/respons... ios/web/public/test/response_providers/delayed_response_provider.h:11: Add it to the web namespace?
https://codereview.chromium.org/2282463002/diff/1/ios/web/public/test/respons... File ios/web/public/test/response_providers/delayed_response_provider.h (right): https://codereview.chromium.org/2282463002/diff/1/ios/web/public/test/respons... ios/web/public/test/response_providers/delayed_response_provider.h:11: On 2016/08/25 15:26:50, gambard wrote: > Add it to the web namespace? Done. https://codereview.chromium.org/2282463002/diff/1/ios/web/public/test/respons... ios/web/public/test/response_providers/delayed_response_provider.h:12: // A Response provider that delay the response provided by another reponse On 2016/08/25 15:26:12, baxley wrote: > nit:s/delay/delays Done.
olivierrobin@chromium.org changed reviewers: + stkhapugin@chromium.org
+stk as NSProxy expert
lgtm with comments https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.h (right): https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:16: class DelayedResponseProvider : public web::ResponseProvider { nit: You are in namespace web, so I guess you can remove web:: https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:18: // Designated creator. Will create a DelayedResponseProvider that delays the s/creator/constructor. s/Will create/Creates. https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:25: // Returns true if the request is handled by the |delayed_provider_|. Optional: rephrase it: "Forwards to |delayed_provider_|." here and below. It's clearer, as this is exactly what you do. https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:32: You may want disallow copy and assign https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:12: @interface DelayedGCDWebServerResponse : NSProxy Class comment here. https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:104: return static_cast<id>([DelayedGCDWebServerResponse ObjCCastStrict or ObjCCast instead of static_cast; cast to GCDWebServerResponse instead of id.
https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.h (right): https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:16: class DelayedResponseProvider : public web::ResponseProvider { On 2016/08/26 13:06:48, stkhapugin wrote: > nit: You are in namespace web, so I guess you can remove web:: Done. https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:18: // Designated creator. Will create a DelayedResponseProvider that delays the On 2016/08/26 13:06:47, stkhapugin wrote: > s/creator/constructor. s/Will create/Creates. Done. https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:25: // Returns true if the request is handled by the |delayed_provider_|. On 2016/08/26 13:06:48, stkhapugin wrote: > Optional: rephrase it: "Forwards to |delayed_provider_|." here and below. It's > clearer, as this is exactly what you do. Done. https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:32: On 2016/08/26 13:06:47, stkhapugin wrote: > You may want disallow copy and assign Done. https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:12: @interface DelayedGCDWebServerResponse : NSProxy On 2016/08/26 13:06:48, stkhapugin wrote: > Class comment here. Done. https://codereview.chromium.org/2282463002/diff/40001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:104: return static_cast<id>([DelayedGCDWebServerResponse On 2016/08/26 13:06:48, stkhapugin wrote: > ObjCCastStrict or ObjCCast instead of static_cast; cast to GCDWebServerResponse > instead of id. Done.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org, stkhapugin@chromium.org Link to the patchset: https://codereview.chromium.org/2282463002/#ps60001 (title: "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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.h (right): https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:18: // Designated Constructor. Creates a DelayedResponseProvider that delays the Does C++ have a notion of a designated constructor? https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:24: base::scoped_nsobject<GCDWebServerResponse> _response; is ios/third_party on ARC yet? https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:55: [_response asyncReadDataWithCompletion:block]; This (and the block below) guarantees that the receiver will be retained until after the |_delay|, even if (for example) the test it was called in completes. Is this intentional? If so, please document it. Otherwise, use a weakified _response.
https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.h (right): https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.h:18: // Designated Constructor. Creates a DelayedResponseProvider that delays the On 2016/08/29 08:16:36, marq wrote: > Does C++ have a notion of a designated constructor? Removed https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:24: base::scoped_nsobject<GCDWebServerResponse> _response; On 2016/08/29 08:16:36, marq wrote: > is ios/third_party on ARC yet? GCDWebServer is ARC, yes. Should I do something specific in that case? https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:55: [_response asyncReadDataWithCompletion:block]; On 2016/08/29 08:16:36, marq wrote: > This (and the block below) guarantees that the receiver will be retained until > after the |_delay|, even if (for example) the test it was called in completes. > Is this intentional? If so, please document it. Otherwise, use a weakified > _response. Done.
https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:24: base::scoped_nsobject<GCDWebServerResponse> _response; On 2016/08/29 08:48:10, Olivier Robin wrote: > On 2016/08/29 08:16:36, marq wrote: > > is ios/third_party on ARC yet? > > GCDWebServer is ARC, yes. Should I do something specific in that case? Two things. First, there's no need to use scoped_nsobject, as object properties are strong by default and ARC will manage releasing them on dealloc. Second, you should add the ARC guard macro to this file, just after the last include: #if !defined(__has_feature) || !__has_feature(objc_arc) #error "This file requires ARC support." #endif (At least, I think that's what we're doing with ARC .m files for now -- I don't know if only some files get it or not). https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:25: base::scoped_nsobject<GCDWebServerResponse> _response; Missed this earlier: chromium style is to not put ivar declarations in the @implementation; please move this to the @interface. https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:38: _response.reset([response retain]); If this file was compiled under ARC, then the -retain call would be an error. So maybe never mind with the ARC stuff and it will get converted with other code?
https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/60001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:24: base::scoped_nsobject<GCDWebServerResponse> _response; On 2016/08/29 08:59:12, marq wrote: > On 2016/08/29 08:48:10, Olivier Robin wrote: > > On 2016/08/29 08:16:36, marq wrote: > > > is ios/third_party on ARC yet? > > > > GCDWebServer is ARC, yes. Should I do something specific in that case? > > Two things. First, there's no need to use scoped_nsobject, as object properties > are strong by default and ARC will manage releasing them on dealloc. > > Second, you should add the ARC guard macro to this file, just after the last > include: > > #if !defined(__has_feature) || !__has_feature(objc_arc) > #error "This file requires ARC support." > #endif > > (At least, I think that's what we're doing with ARC .m files for now -- I don't > know if only some files get it or not). But this file is not compiled with ARC. GCDWebServerResponse is. So adding this will result as an error. Should I create a new target in BUILD.gn to add ARC flags? https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:25: base::scoped_nsobject<GCDWebServerResponse> _response; On 2016/08/29 08:59:12, marq wrote: > Missed this earlier: chromium style is to not put ivar declarations in the > @implementation; please move this to the @interface. Done. https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:38: _response.reset([response retain]); On 2016/08/29 08:59:12, marq wrote: > If this file was compiled under ARC, then the -retain call would be an error. So > maybe never mind with the ARC stuff and it will get converted with other code? This file is not compiled with ARC. As said above, I may be missing something, but should I used this object as arc because its implementation file is arc?
LGTM with weakification fixes. https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:38: _response.reset([response retain]); On 2016/08/29 10:42:14, Olivier Robin wrote: > On 2016/08/29 08:59:12, marq wrote: > > If this file was compiled under ARC, then the -retain call would be an error. > So > > maybe never mind with the ARC stuff and it will get converted with other code? > > This file is not compiled with ARC. As said above, I may be missing something, > but should I used this object as arc because its implementation file is arc? Sorry, I misunderstood. If this file isn't compiled with ARC, scoped_nsobject is correct. https://codereview.chromium.org/2282463002/diff/100001/ios/web/public/test/re... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/100001/ios/web/public/test/re... ios/web/public/test/response_providers/delayed_response_provider.mm:59: base::scoped_nsobject<GCDWebServerResponse> response(_response); You don't need to strongify here; you can just use weakResponse on the next line (since weak nsobjects that have been deallocated are just nil). https://codereview.chromium.org/2282463002/diff/100001/ios/web/public/test/re... ios/web/public/test/response_providers/delayed_response_provider.mm:67: base::scoped_nsobject<GCDWebServerResponse> response(_response); s/_response/weakResponse/
https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/80001/ios/web/public/test/res... ios/web/public/test/response_providers/delayed_response_provider.mm:38: _response.reset([response retain]); On 2016/08/29 16:03:19, marq wrote: > On 2016/08/29 10:42:14, Olivier Robin wrote: > > On 2016/08/29 08:59:12, marq wrote: > > > If this file was compiled under ARC, then the -retain call would be an > error. > > So > > > maybe never mind with the ARC stuff and it will get converted with other > code? > > > > This file is not compiled with ARC. As said above, I may be missing something, > > but should I used this object as arc because its implementation file is arc? > > Sorry, I misunderstood. If this file isn't compiled with ARC, scoped_nsobject is > correct. Acknowledged. https://codereview.chromium.org/2282463002/diff/100001/ios/web/public/test/re... File ios/web/public/test/response_providers/delayed_response_provider.mm (right): https://codereview.chromium.org/2282463002/diff/100001/ios/web/public/test/re... ios/web/public/test/response_providers/delayed_response_provider.mm:59: base::scoped_nsobject<GCDWebServerResponse> response(_response); On 2016/08/29 16:03:19, marq wrote: > You don't need to strongify here; you can just use weakResponse on the next line > (since weak nsobjects that have been deallocated are just nil). As discussed offline, keeping the strong. https://codereview.chromium.org/2282463002/diff/100001/ios/web/public/test/re... ios/web/public/test/response_providers/delayed_response_provider.mm:67: base::scoped_nsobject<GCDWebServerResponse> response(_response); On 2016/08/29 16:03:19, marq wrote: > s/_response/weakResponse/ Whoops! Done
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org, stkhapugin@chromium.org, marq@chromium.org Link to the patchset: https://codereview.chromium.org/2282463002/#ps120001 (title: "whoops")
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 dsansome@chromium.org
dsansome@chromium.org changed reviewers: + dsansome@chromium.org
(I unset the commit bit on this change to try fix a stuck CQ - https://bugs.chromium.org/p/chromium/issues/detail?id=642077. The issue is fixed now so I'm sending this to CQ again for you)
The CQ bit was checked by dsansome@chromium.org
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 #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Create a Delayed Response Provider for GCDWebServer. This provider will provide the response from another provider after a delay. BUG= ========== to ========== Create a Delayed Response Provider for GCDWebServer. This provider will provide the response from another provider after a delay. BUG= Committed: https://crrev.com/6eeceba83633b457f6b5469457fbd69053deec54 Cr-Commit-Position: refs/heads/master@{#415207} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6eeceba83633b457f6b5469457fbd69053deec54 Cr-Commit-Position: refs/heads/master@{#415207} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
