Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1215433003:
CL for Objective-C readability (Closed)
DescriptionCL for Objective-C readability
- Added more clarity to some comments.
- Added mode to the description string.
- Moved @properties before methods and added #pragma
in the implementations
- Refactored code to obtain an OperationType for a mode change in
to its own method (operationTypeToChangeMode).
- Changed finalizeChangeToMode to return a BOOL
- Changed operationQueueForStashAndRestoreOperations's QoS
to user initiated, since that's more appropriate.
Committed: https://crrev.com/1767559652e5f2990819b8f5d1e63c546f349238
Cr-Commit-Position: refs/heads/master@{#338318}
Patch Set 1 #Patch Set 2 : y #
Total comments: 67
Patch Set 3 : y #
Total comments: 7
Patch Set 4 : y #
Total comments: 6
Patch Set 5 : #
Total comments: 1
Messages
Total messages: 19 (5 generated)
shreyasv@chromium.org changed reviewers: + stuartmorgan@chromium.org
Overall this looks great. Specific notes/nits below. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (right): https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:48: - (instancetype)init NS_UNAVAILABLE; Doesn't NS_UNAVAILABLE only make sense in the header? https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:52: @property(nonatomic, readonly) NSArray* allBrowsingDataManagers; Properties should be declared in a coherent section before methods. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:71: andCallCompletionHandler:(void (^)(BOOL modeChangeWasSuccessful))handler; s/andCall/with/? https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:85: // Performs operations of type |operationType| on each of the "operations", or "an operation"? This doesn't match the signature. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:98: newOperationWithBrowsingDataManagers:(NSArray*)browsingDataManagers Why return a retained object? Usually methods return autoreleased objects, which makes it seem like there is a special reason this one doesn't. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:102: // be run one after another. It's not at all obvious from the comments why there are two different queues, or why they are serialized manually rather than via max operations. A comment at the top of the file explaining the queue setup at a high level would be very helpful for someone new to this file. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:120: // The number of stash or restore operations that are still pending. If this s/this// https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:136: setQualityOfService:NSQualityOfServiceUserInteractive]; Why interactive? From the descriptions, NSQualityOfServiceUserInitiated sounds like a much better match. If there's really a compelling reason for this to be something other than what one would expect, that needs to be commented. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:202: (web::BrowsingDataTypes)browsingDataTypes { Only indent 4 https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:220: _delegate.reset(delegate); Where is the ownership coming from? This looks like a crasher. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:224: (OperationType)operationType { Only indent 4 https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:228: return nullptr; Usually the invalid option goes at the end. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:237: return nullptr; Does the compiler actually require this if you have all the enum cases handled? https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:247: DCHECK(_lastDispatchedStashOrRestoreOperation); Seems like a short comment introducing what this whole block is doing could be useful. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:366: } Consider making the whole block from operationType down to here a helper method (it's a clear, stand-alone chunk of logic), so that the higher-level flow of this method is clearer. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:443: default: Why include a default here (vs NONE)? If someone adds another operation type, don't we want failing to update this to be a compile error? https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store_unittest.mm (right): https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store_unittest.mm:24: (CRWBrowsingDataStore*)browsingDataStore NS_DESIGNATED_INITIALIZER; Only indent 4 https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store_unittest.mm:39: (CRWBrowsingDataStore*)browsingDataStore { Same https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... File ios/web/public/crw_browsing_data_store.h (right): https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:57: // TODO(shreyasv): Verify the preconditions for the following 3 methods when Isn't this an implementation note, not an interface issue? If so, it belongs in the .mm https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:61: // if there is no delegate present, the default behavior of this method is to If https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:74: // if there is no delegate present, the default behavior of this method is to If https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:75: // value and takes a single BOOL argument that indicates whether or not the Looks like a line or two of comments is missing. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:80: // the mode was successfully changed to |INACTIVE|. ... or maybe just partially duplicated https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:81: // The mode change to |ACTIVE| can fail if another |makeActive| or INACTIVE https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:94: // Returns YES if there is still a pending operation that has not finished Properties should not use the method descriptive verb structure. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:96: // CRWBrowsingDataStore when there are pending operations is undefined s/is/results in/ https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:98: @property(nonatomic, assign, readonly) BOOL hasPendingOperations; This should be declared up with the other properties, before the methods.
Added initial reviewers to CC, since I just noticed that they are not there.
https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (right): https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:48: - (instancetype)init NS_UNAVAILABLE; On 2015/06/30 21:57:17, stuartmorgan wrote: > Doesn't NS_UNAVAILABLE only make sense in the header? Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:52: @property(nonatomic, readonly) NSArray* allBrowsingDataManagers; On 2015/06/30 21:57:17, stuartmorgan wrote: > Properties should be declared in a coherent section before methods. Done. I can't find this requirement in the style guide. Can you please confirm that this is a requirement. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:71: andCallCompletionHandler:(void (^)(BOOL modeChangeWasSuccessful))handler; On 2015/06/30 21:57:17, stuartmorgan wrote: > s/andCall/with/? This came up in an earlier code review. In UIKit methods similar to this are named doFoo:withCompletionHandler. This semantically means that |foo| will be done and completionHandler will be invoked at a later time. This gives the reader an idea that the API Is async. However, that's not the case here. The 'and' prefix is to make it very clear that the API will call |handler| immediately (in the same run loop). So, I would like to leave the 'and' prefix as is. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:85: // Performs operations of type |operationType| on each of the On 2015/06/30 21:57:17, stuartmorgan wrote: > "operations", or "an operation"? This doesn't match the signature. Good catch. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:98: newOperationWithBrowsingDataManagers:(NSArray*)browsingDataManagers On 2015/06/30 21:57:17, stuartmorgan wrote: > Why return a retained object? Usually methods return autoreleased objects, which > makes it seem like there is a special reason this one doesn't. I don't this is enforced by the style guide. But consistency with the majority of the code base is a good point. Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:102: // be run one after another. On 2015/06/30 21:57:17, stuartmorgan wrote: > It's not at all obvious from the comments why there are two different queues, or > why they are serialized manually rather than via max operations. A comment at > the top of the file explaining the queue setup at a high level would be very > helpful for someone new to this file. There are 2 different points here: 1) I have added a high level comment at the top of the file explaining the queue set up. This should hopefully add clarity on why there are 2 queues. 2) I am a bit confused as to how setMaxConcurrentOperationCount can be used to serialize here. setMaxConcurrentOperationCount can only be set on a queue. This API can take a concurrent queue as an argument but still make sure that operations are serialized. So the use of setMaxConcurrentOperationCount will not work in practice. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:120: // The number of stash or restore operations that are still pending. If this On 2015/06/30 21:57:18, stuartmorgan wrote: > s/this// doesn't that make value ambiguous? The pronoun makes it clearer? How about I just replace the pronoun with the actual noun it's trying to represent? https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:136: setQualityOfService:NSQualityOfServiceUserInteractive]; On 2015/06/30 21:57:17, stuartmorgan wrote: > Why interactive? From the descriptions, NSQualityOfServiceUserInitiated sounds > like a much better match. > > If there's really a compelling reason for this to be something other than what > one would expect, that needs to be commented. |STASH|, |RESTORE| operations are the core bits that need to be performed in order to change the mode of a CRWBrowsingDataStore and hence they are prioritized with a QoS of NSQualityOfServiceUserInteractive. Added this at the top section which describes the queue set up. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:202: (web::BrowsingDataTypes)browsingDataTypes { On 2015/06/30 21:57:17, stuartmorgan wrote: > Only indent 4 git cl format artifact. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:220: _delegate.reset(delegate); On 2015/06/30 21:57:17, stuartmorgan wrote: > Where is the ownership coming from? This looks like a crasher. How? The ownership semantics (weak) are documented clearly in the @property. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:224: (OperationType)operationType { On 2015/06/30 21:57:17, stuartmorgan wrote: > Only indent 4 Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:228: return nullptr; On 2015/06/30 21:57:17, stuartmorgan wrote: > Usually the invalid option goes at the end. This one is tricky. I feel that it is natural to read a switch statement based on the enum values in the definition. Left as is. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:237: return nullptr; On 2015/06/30 21:57:17, stuartmorgan wrote: > Does the compiler actually require this if you have all the enum cases handled? No, removed. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:247: DCHECK(_lastDispatchedStashOrRestoreOperation); On 2015/06/30 21:57:17, stuartmorgan wrote: > Seems like a short comment introducing what this whole block is doing could be > useful. Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:366: } On 2015/06/30 21:57:17, stuartmorgan wrote: > Consider making the whole block from operationType down to here a helper method > (it's a clear, stand-alone chunk of logic), so that the higher-level flow of > this method is clearer. Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:443: default: On 2015/06/30 21:57:17, stuartmorgan wrote: > Why include a default here (vs NONE)? If someone adds another operation type, > don't we want failing to update this to be a compile error? Good catch. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store_unittest.mm (right): https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store_unittest.mm:24: (CRWBrowsingDataStore*)browsingDataStore NS_DESIGNATED_INITIALIZER; On 2015/06/30 21:57:18, stuartmorgan wrote: > Only indent 4 Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store_unittest.mm:39: (CRWBrowsingDataStore*)browsingDataStore { On 2015/06/30 21:57:18, stuartmorgan wrote: > Same Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... File ios/web/public/crw_browsing_data_store.h (right): https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:57: // TODO(shreyasv): Verify the preconditions for the following 3 methods when On 2015/06/30 21:57:18, stuartmorgan wrote: > Isn't this an implementation note, not an interface issue? If so, it belongs in > the .mm Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:61: // if there is no delegate present, the default behavior of this method is to On 2015/06/30 21:57:18, stuartmorgan wrote: > If Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:74: // if there is no delegate present, the default behavior of this method is to On 2015/06/30 21:57:18, stuartmorgan wrote: > If Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:75: // value and takes a single BOOL argument that indicates whether or not the On 2015/06/30 21:57:18, stuartmorgan wrote: > Looks like a line or two of comments is missing. Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:80: // the mode was successfully changed to |INACTIVE|. On 2015/06/30 21:57:18, stuartmorgan wrote: > ... or maybe just partially duplicated Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:81: // The mode change to |ACTIVE| can fail if another |makeActive| or On 2015/06/30 21:57:18, stuartmorgan wrote: > INACTIVE Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:94: // Returns YES if there is still a pending operation that has not finished On 2015/06/30 21:57:18, stuartmorgan wrote: > Properties should not use the method descriptive verb structure. Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:96: // CRWBrowsingDataStore when there are pending operations is undefined On 2015/06/30 21:57:18, stuartmorgan wrote: > s/is/results in/ Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:98: @property(nonatomic, assign, readonly) BOOL hasPendingOperations; On 2015/06/30 21:57:18, stuartmorgan wrote: > This should be declared up with the other properties, before the methods. Done.
https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (right): https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:52: @property(nonatomic, readonly) NSArray* allBrowsingDataManagers; On 2015/07/07 21:42:05, shreyasv1 wrote: > On 2015/06/30 21:57:17, stuartmorgan wrote: > > Properties should be declared in a coherent section before methods. > > Done. > I can't find this requirement in the style guide. Can you please confirm that > this is a requirement. http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Properties It looks like the simplified version removed that though (the link above hasn't been updated since the streamlining) https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:71: andCallCompletionHandler:(void (^)(BOOL modeChangeWasSuccessful))handler; On 2015/07/07 21:42:05, shreyasv1 wrote: > On 2015/06/30 21:57:17, stuartmorgan wrote: > > s/andCall/with/? > > This came up in an earlier code review. > In UIKit methods similar to this are named doFoo:withCompletionHandler. > This semantically means that |foo| will be done and completionHandler will be > invoked at a later time. > This gives the reader an idea that the API Is async. > However, that's not the case here. > The 'and' prefix is to make it very clear that the API will call |handler| > immediately (in the same run loop). > > So, I would like to leave the 'and' prefix as is. When you have to invent a new naming variation, that's a good signal to stop and ask why. Why are there no methods anywhere in UIKit that have this behavior, and thus need a naming convention like this? In this case, I would suggest that it's because this seems like a convoluted way of just returning a BOOL. What's advantage from an API standpoint for this (which as you've noted, is non-standard, and can potentially be confused with other API that has a different behavior) vs clients just executing code after calling this, based on a return value? https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:98: newOperationWithBrowsingDataManagers:(NSArray*)browsingDataManagers On 2015/07/07 21:42:05, shreyasv1 wrote: > On 2015/06/30 21:57:17, stuartmorgan wrote: > > Why return a retained object? Usually methods return autoreleased objects, > which > > makes it seem like there is a special reason this one doesn't. > > I don't this is enforced by the style guide. > But consistency with the majority of the code base is a good point. Being consistent with the overwhelming majority of code is essentially the central tenant of the style guide. Just because something isn't written down in the style guide doesn't automatically mean you can do whatever you want from a style perspective (and a readability review is not just a quiz of the things written in the guide). There are specific cases where the style guide has explicitly chosen to be neutral (e.g., braces for one-line if statements), but it's not the case that *all* things not explicitly specified are equally fine. (For instance, there's no written rule that says that you should have one space between each word in a comment, but that doesn't mean it's fine to write a comment where the inter-word whitespace follows the Fibonacci sequence...) https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:102: // be run one after another. On 2015/07/07 21:42:05, shreyasv1 wrote: > 2) I am a bit confused as to how setMaxConcurrentOperationCount can be used to > serialize here. setMaxConcurrentOperationCount can only be set on a queue. This > API can take a concurrent queue as an argument but still make sure that > operations are serialized. So the use of setMaxConcurrentOperationCount will not > work in practice. But this API is a private helper method for this class; it only needs to have the functionality you describe given the current queue setup. If this class instead used one serial queue, then you wouldn't need that behavior. The comment at the top makes the two-queue setup clear though, thanks. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:120: // The number of stash or restore operations that are still pending. If this On 2015/07/07 21:42:05, shreyasv1 wrote: > On 2015/06/30 21:57:18, stuartmorgan wrote: > > s/this// > > doesn't that make value ambiguous? > The pronoun makes it clearer? Yes, I just mis-parsed the sentence. I think what would probably help more is to change "> 0" to "greater than 0", because using "> 0" makes me think the LHS is a variable name. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:136: setQualityOfService:NSQualityOfServiceUserInteractive]; On 2015/07/07 21:42:05, shreyasv1 wrote: > On 2015/06/30 21:57:17, stuartmorgan wrote: > > Why interactive? From the descriptions, NSQualityOfServiceUserInitiated sounds > > like a much better match. > > > > If there's really a compelling reason for this to be something other than what > > one would expect, that needs to be commented. > > |STASH|, |RESTORE| operations are the core bits that need to be performed in > order to change the mode of a CRWBrowsingDataStore and hence they are > prioritized with a QoS of NSQualityOfServiceUserInteractive. I don't understand how the second part of that follows from the first. The descriptions I see for these are: NSQualityOfServiceUserInteractive: this QoS is used for work directly involved in providing an interactive UI such as processing events or drawing to the screen. NSQualityOfServiceUserInitiated: this QoS is used for performing work that has been explicitly requested by the user and for which results must be immediately presented in order to allow for further user interaction. This code is not doing event processing, or drawing to the screen. It is doing work that has been explicitly requested and which blocks further user interaction. It seems that UserInitiated clearly describes this case, so I don't understand why the code is using something higher than that. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:220: _delegate.reset(delegate); On 2015/07/07 21:42:05, shreyasv1 wrote: > On 2015/06/30 21:57:17, stuartmorgan wrote: > > Where is the ownership coming from? This looks like a crasher. > > How? > The ownership semantics (weak) are documented clearly in the @property. Sigh. I hate that the Weak* family uses reset(), for exactly this reason :P https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (right): https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:32: // The name of the NSOperation that performs a |RESTORE| operation. FWIW, while the | are okay here, they are only required when it would be ambiguous without them; given that these tokens are ALL CAPS, there's not really any chance of confusion, so they would all be fine (and arguably a bit more readable) without the bars. https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:196: modeString]; This shouldn't be indented any more than the line above it. https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:209: return [super automaticallyNotifiesObserversForKey:(NSString*)key]; Remove (NSString*) https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store_unittest.mm (right): https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store_unittest.mm:26: // Designated init. |browsingDataStore| cannot be null. No need to comment that it's the designated initializer now that we are using the annotation.
https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (right): https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:52: @property(nonatomic, readonly) NSArray* allBrowsingDataManagers; On 2015/07/09 16:33:49, stuartmorgan wrote: > On 2015/07/07 21:42:05, shreyasv1 wrote: > > On 2015/06/30 21:57:17, stuartmorgan wrote: > > > Properties should be declared in a coherent section before methods. > > > > Done. > > I can't find this requirement in the style guide. Can you please confirm that > > this is a requirement. > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Properties > > It looks like the simplified version removed that though (the link above hasn't > been updated since the streamlining) It is not explicitly spelled out. Maybe it should be. Ack. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:71: andCallCompletionHandler:(void (^)(BOOL modeChangeWasSuccessful))handler; I actually had a BOOL return methods originally but the initial reviewer asked me to change it. https://codereview.chromium.org/1154923003/diff/20001/ios/web/crw_browsing_da... Changed back to a BOOL return method. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:102: // be run one after another. On 2015/07/09 16:33:49, stuartmorgan wrote: > On 2015/07/07 21:42:05, shreyasv1 wrote: > > 2) I am a bit confused as to how setMaxConcurrentOperationCount can be used to > > serialize here. setMaxConcurrentOperationCount can only be set on a queue. > This > > API can take a concurrent queue as an argument but still make sure that > > operations are serialized. So the use of setMaxConcurrentOperationCount will > not > > work in practice. > > But this API is a private helper method for this class; it only needs to have > the functionality you describe given the current queue setup. If this class > instead used one serial queue, then you wouldn't need that behavior. > > The comment at the top makes the two-queue setup clear though, thanks. Acknowledged. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:120: // The number of stash or restore operations that are still pending. If this On 2015/07/09 16:33:50, stuartmorgan wrote: > On 2015/07/07 21:42:05, shreyasv1 wrote: > > On 2015/06/30 21:57:18, stuartmorgan wrote: > > > s/this// > > > > doesn't that make value ambiguous? > > The pronoun makes it clearer? > > Yes, I just mis-parsed the sentence. I think what would probably help more is to > change "> 0" to "greater than 0", because using "> 0" makes me think the LHS is > a variable name. Done. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:136: setQualityOfService:NSQualityOfServiceUserInteractive]; On 2015/07/09 16:33:49, stuartmorgan wrote: > On 2015/07/07 21:42:05, shreyasv1 wrote: > > On 2015/06/30 21:57:17, stuartmorgan wrote: > > > Why interactive? From the descriptions, NSQualityOfServiceUserInitiated > sounds > > > like a much better match. > > > > > > If there's really a compelling reason for this to be something other than > what > > > one would expect, that needs to be commented. > > > > |STASH|, |RESTORE| operations are the core bits that need to be performed in > > order to change the mode of a CRWBrowsingDataStore and hence they are > > prioritized with a QoS of NSQualityOfServiceUserInteractive. > > I don't understand how the second part of that follows from the first. The > descriptions I see for these are: > > NSQualityOfServiceUserInteractive: this QoS is used for work directly involved > in providing an interactive UI such as processing events or drawing to the > screen. > > NSQualityOfServiceUserInitiated: this QoS is used for performing work that has > been explicitly requested by the user and for which results must be immediately > presented in order to allow for further user interaction. > > This code is not doing event processing, or drawing to the screen. It is doing > work that has been explicitly requested and which blocks further user > interaction. It seems that UserInitiated clearly describes this case, so I don't > understand why the code is using something higher than that. Done. Also added this to the high level queue description. https://codereview.chromium.org/1215433003/diff/20001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:220: _delegate.reset(delegate); On 2015/07/09 16:33:49, stuartmorgan wrote: > On 2015/07/07 21:42:05, shreyasv1 wrote: > > On 2015/06/30 21:57:17, stuartmorgan wrote: > > > Where is the ownership coming from? This looks like a crasher. > > > > How? > > The ownership semantics (weak) are documented clearly in the @property. > > Sigh. I hate that the Weak* family uses reset(), for exactly this reason :P Acknowledged. https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (right): https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:32: // The name of the NSOperation that performs a |RESTORE| operation. On 2015/07/09 16:33:50, stuartmorgan wrote: > FWIW, while the | are okay here, they are only required when it would be > ambiguous without them; given that these tokens are ALL CAPS, there's not really > any chance of confusion, so they would all be fine (and arguably a bit more > readable) without the bars. My concern is with consistency. I noticed that some comments have |STASH| while some have stash. I would like to be consistent throughout the file since |STASH|, |RESTORE|, |REMOVE represent OperationTypes and have a special meaning. Left as is -- for the sake of consistency. Let me know if you don't agree with this and I will be happy to change it. https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:209: return [super automaticallyNotifiesObserversForKey:(NSString*)key]; On 2015/07/09 16:33:50, stuartmorgan wrote: > Remove (NSString*) Done. https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store_unittest.mm (right): https://codereview.chromium.org/1215433003/diff/40001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store_unittest.mm:26: // Designated init. |browsingDataStore| cannot be null. On 2015/07/09 16:33:50, stuartmorgan wrote: > No need to comment that it's the designated initializer now that we are using > the annotation. Done.
I think this is in good shape. Eugene or Rohit, please review the CL, then I'll approve it.
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
lgtm https://codereview.chromium.org/1215433003/diff/60001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (right): https://codereview.chromium.org/1215433003/diff/60001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:172: - (instancetype)init { You don't need this since you marked it as NS_UNAVAILABLE in the header (unless I'm missing something). https://codereview.chromium.org/1215433003/diff/60001/ios/web/public/crw_brow... File ios/web/public/crw_browsing_data_store.h (right): https://codereview.chromium.org/1215433003/diff/60001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:55: // |browserState| cannot be nullptr. The initial mode of the s/nullptr/null Because |browserState| can not be NULL or 0 as well. https://codereview.chromium.org/1215433003/diff/60001/ios/web/public/crw_brow... File ios/web/public/crw_browsing_data_store_delegate.h (right): https://codereview.chromium.org/1215433003/diff/60001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store_delegate.h:54: I guess you don't need this change
lgtm
https://codereview.chromium.org/1215433003/diff/60001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (right): https://codereview.chromium.org/1215433003/diff/60001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:172: - (instancetype)init { On 2015/07/09 20:08:08, eugenebut wrote: > You don't need this since you marked it as NS_UNAVAILABLE in the header (unless > I'm missing something). Removed here and in the unittest. https://codereview.chromium.org/1215433003/diff/60001/ios/web/public/crw_brow... File ios/web/public/crw_browsing_data_store.h (right): https://codereview.chromium.org/1215433003/diff/60001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store.h:55: // |browserState| cannot be nullptr. The initial mode of the On 2015/07/09 20:08:08, eugenebut wrote: > s/nullptr/null > > Because |browserState| can not be NULL or 0 as well. Done. https://codereview.chromium.org/1215433003/diff/60001/ios/web/public/crw_brow... File ios/web/public/crw_browsing_data_store_delegate.h (right): https://codereview.chromium.org/1215433003/diff/60001/ios/web/public/crw_brow... ios/web/public/crw_browsing_data_store_delegate.h:54: On 2015/07/09 20:08:08, eugenebut wrote: > I guess you don't need this change Yep. I was going to remove it, just before landing.
The CQ bit was checked by shreyasv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stuartmorgan@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/1215433003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1215433003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1767559652e5f2990819b8f5d1e63c546f349238 Cr-Commit-Position: refs/heads/master@{#338318}
Message was sent while issue was closed.
justincohen@chromium.org changed reviewers: + justincohen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1215433003/diff/80001/ios/web/crw_browsing_da... File ios/web/crw_browsing_data_store.mm (left): https://codereview.chromium.org/1215433003/diff/80001/ios/web/crw_browsing_da... ios/web/crw_browsing_data_store.mm:178: - (instancetype)init { Removing this breaks iOS9 compile. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
