CL for Objective-C readability

ios/web/crw_browsing_data_store.mm

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/web/public/crw_browsing_data_store.h"
 
 #import <Foundation/Foundation.h>
 
 #include "base/ios/ios_util.h"
 #import "base/ios/weak_nsobject.h"
@@ skipping 27 matching lines @@
 
 @interface CRWBrowsingDataStore ()
 // Returns a serial queue on which stash and restore operations can be scheduled
 // to be run. All stash/restore operations need to be run on the same queue
 // hence it is shared with all CRWBrowsingDataStores.
 + (NSOperationQueue*)operationQueueForStashAndRestoreOperations;
 // Returns a concurrent queue on which remove operations can be scheduled to be
 // run. All remove operations need to be run on the same queue hence it is
 // shared with all CRWBrowsingDataStores.
 + (NSOperationQueue*)operationQueueForRemoveOperations;
 - (instancetype)init NS_UNAVAILABLE;

[stuartmorgan, 2015/06/30 21:57:17]

Doesn't NS_UNAVAILABLE only make sense in the header?

[shreyasv1, 2015/07/07 21:42:05]

Done.

 
 // The array of all browsing data managers. Must be accessed from the main
 // thread.
 @property(nonatomic, readonly) NSArray* allBrowsingDataManagers;

[stuartmorgan, 2015/06/30 21:57:17]

Properties should be declared in a coherent section before methods.

[shreyasv1, 2015/07/07 21:42:05]

Done.
I can't find this requirement in the style guide. Can you please confirm that
this is a requirement.

[stuartmorgan, 2015/07/09 16:33:49]

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)

[shreyasv1, 2015/07/09 17:18:01]

It is not explicitly spelled out. Maybe it should be.
Ack.

 // Returns an array of browsing data managers for the given |browsingDataTypes|.
 - (NSArray*)browsingDataManagersForBrowsingDataTypes:
         (web::BrowsingDataTypes)browsingDataTypes;
 // Returns the selector that needs to be performed on the
 // CRWBrowsingDataManagers for the |operationType|. |operationType| cannot be
 // |NONE|.
 - (SEL)browsingDataManagerSelectorForOperationType:(OperationType)operationType;
 // Returns the selector that needs to be performed on the
 // CRWBrowsingDataManagers for a REMOVE operation.
 - (SEL)browsingDataManagerSelectorForRemoveOperationType;
 
 // Redefined to be read-write. Must be called from the main thread.
 @property(nonatomic, assign) web::BrowsingDataStoreMode mode;
 // Sets the mode iff there are no more stash or restore operations that are
 // still pending. |mode| can only be either |ACTIVE| or |INACTIVE|.
 // |handler| is called immediately (in the same runloop) with a BOOL indicating
 // whether the mode change was successful or not. |handler| can be nil.
 - (void)finalizeChangeToMode:(web::BrowsingDataStoreMode)mode
     andCallCompletionHandler:(void (^)(BOOL modeChangeWasSuccessful))handler;

[stuartmorgan, 2015/06/30 21:57:17]

s/andCall/with/?

[shreyasv1, 2015/07/07 21:42:05]

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.

[stuartmorgan, 2015/07/09 16:33:49]

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?

[shreyasv1, 2015/07/09 17:18:00]

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.

 
 // Changes the mode of the CRWBrowsingDataStore to |mode|. This is an
 // asynchronous operation and the mode is not changed immediately.
 // |completionHandler| can be nil.
 // |completionHandler| is called on the main thread. This block has no return
 // value and takes a single BOOL argument that indicates whether or not the
 // mode change was successfully changed to |mode|.
 - (void)changeMode:(web::BrowsingDataStoreMode)mode
     completionHandler:(void (^)(BOOL modeChangeWasSuccessful))completionHandler;
 
 // The number of stash or restore operations that are still pending.
 @property(nonatomic, assign) NSUInteger numberOfPendingStashOrRestoreOperations;
 
 // Performs operations of type |operationType| on each of the

[stuartmorgan, 2015/06/30 21:57:17]

"operations", or "an operation"? This doesn't match the signature.

[shreyasv1, 2015/07/07 21:42:05]

Good catch.

 // |browsingDataManagers|. |operationType| cannot be |NONE|.
 // Precondition: There must be no web views associated with the BrowserState.
 // |completionHandler| is called on the main thread and cannot be nil.
 - (void)performOperationWithType:(OperationType)operationType
             browsingDataManagers:(NSArray*)browsingDataManagers
                completionHandler:(ProceduralBlock)completionHandler;
 
 // Creates an NSOperation that calls |selector| on all the
 // |browsingDataManagers|. |selector| needs to be one of the methods in
 // CRWBrowsingDataManager. The created NSOperation will have a completionBlock
 // of |completionHandler|.
 - (NSOperation*)
     newOperationWithBrowsingDataManagers:(NSArray*)browsingDataManagers

[stuartmorgan, 2015/06/30 21:57:17]

Why return a retained object? Usually methods return autoreleased objects, which
makes it seem like there is a special reason this one doesn't.

[shreyasv1, 2015/07/07 21:42:05]

I don't this is enforced by the style guide. 
But consistency with the majority of the code base is a good point.
Done.

[stuartmorgan, 2015/07/09 16:33:49]

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...)

                                 selector:(SEL)selector
                        completionHandler:(ProceduralBlock)completionHandler;
 // Enqueues |operation| to be run on |queue|. All operations are serialized to
 // be run one after another.

[stuartmorgan, 2015/06/30 21:57:17]

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.

[shreyasv1, 2015/07/07 21:42:05]

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.

[stuartmorgan, 2015/07/09 16:33:49]

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.

[shreyasv1, 2015/07/09 17:18:00]

Acknowledged.

 - (void)addOperation:(NSOperation*)operation toQueue:(NSOperationQueue*)queue;
 @end
 
 @implementation CRWBrowsingDataStore {
   web::BrowserState* _browserState;  // Weak, owns this object.
   // The delegate.
   base::WeakNSProtocol<id<CRWBrowsingDataStoreDelegate>> _delegate;
   // The mode of the CRWBrowsingDataStore.
   web::BrowsingDataStoreMode _mode;
   // The dictionary that maps a browsing data type to its
   // CRWBrowsingDataManager.
   base::scoped_nsobject<NSDictionary> _browsingDataTypeMap;
   // The last operation that was enqueued to be run. Can be stash, restore or a
   // delete operation.
   base::scoped_nsobject<NSOperation> _lastDispatchedOperation;
   // The last dispatched stash or restore operation that was enqueued to be run.
   base::scoped_nsobject<NSOperation> _lastDispatchedStashOrRestoreOperation;
   // The number of stash or restore operations that are still pending. If this

[stuartmorgan, 2015/06/30 21:57:18]

s/this//

[shreyasv1, 2015/07/07 21:42:05]

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?

[stuartmorgan, 2015/07/09 16:33:50]

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.

[shreyasv1, 2015/07/09 17:18:00]

Done.

   // value > 0 the mode of the CRWBrowsingDataStore is |CHANGING|. The mode
   // can be made ACTIVE or INACTIVE only be set when this value is 0.
   NSUInteger _numberOfPendingStashOrRestoreOperations;
 }
 
 + (NSOperationQueue*)operationQueueForStashAndRestoreOperations {
   static dispatch_once_t onceToken = 0;
   static NSOperationQueue* operationQueueForStashAndRestoreOperations = nil;
   dispatch_once(&onceToken, ^{
     operationQueueForStashAndRestoreOperations =
         [[NSOperationQueue alloc] init];
     [operationQueueForStashAndRestoreOperations
         setMaxConcurrentOperationCount:1U];
     if (base::ios::IsRunningOnIOS8OrLater()) {
       [operationQueueForStashAndRestoreOperations
           setQualityOfService:NSQualityOfServiceUserInteractive];

[stuartmorgan, 2015/06/30 21:57:17]

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.

[shreyasv1, 2015/07/07 21:42:05]

|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.

[stuartmorgan, 2015/07/09 16:33:49]

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.

[shreyasv1, 2015/07/09 17:18:00]

Done. Also added this to the high level queue description.

     }
   });
   return operationQueueForStashAndRestoreOperations;
 }
 
 + (NSOperationQueue*)operationQueueForRemoveOperations {
   static dispatch_once_t onceToken = 0;
   static NSOperationQueue* operationQueueForRemoveOperations = nil;
   dispatch_once(&onceToken, ^{
     operationQueueForRemoveOperations = [[NSOperationQueue alloc] init];
@@ skipping 45 matching lines @@
   return result;
 }
 
 
 - (NSArray*)allBrowsingDataManagers {
   DCHECK([NSThread isMainThread]);
   return [_browsingDataTypeMap allValues];
 }
 
 - (NSArray*)browsingDataManagersForBrowsingDataTypes:
         (web::BrowsingDataTypes)browsingDataTypes {

[stuartmorgan, 2015/06/30 21:57:17]

Only indent 4

[shreyasv1, 2015/07/07 21:42:05]

git cl format artifact.

   __block NSMutableArray* result = [NSMutableArray array];
   [_browsingDataTypeMap
       enumerateKeysAndObjectsUsingBlock:^(NSNumber* dataType,
                                           id<CRWBrowsingDataManager> manager,
                                           BOOL*) {
         if ([dataType unsignedIntegerValue] & browsingDataTypes) {
           [result addObject:manager];
         }
       }];
   return result;
 }
 
 - (id<CRWBrowsingDataStoreDelegate>)delegate {
   return _delegate;
 }
 
 - (void)setDelegate:(id<CRWBrowsingDataStoreDelegate>)delegate {
   _delegate.reset(delegate);

[stuartmorgan, 2015/06/30 21:57:17]

Where is the ownership coming from? This looks like a crasher.

[shreyasv1, 2015/07/07 21:42:05]

How?
The ownership semantics (weak) are documented clearly in the @property.

[stuartmorgan, 2015/07/09 16:33:49]

Sigh. I hate that the Weak* family uses reset(), for exactly this reason :P

[shreyasv1, 2015/07/09 17:18:00]

Acknowledged.

 }
 
 - (SEL)browsingDataManagerSelectorForOperationType:
         (OperationType)operationType {

[stuartmorgan, 2015/06/30 21:57:17]

Only indent 4

[shreyasv1, 2015/07/07 21:42:05]

Done.

   switch (operationType) {
     case NONE:
       NOTREACHED();
       return nullptr;

[stuartmorgan, 2015/06/30 21:57:17]

Usually the invalid option goes at the end.

[shreyasv1, 2015/07/07 21:42:05]

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.

     case STASH:
       return @selector(stashData);
     case RESTORE:
       return @selector(restoreData);
     case REMOVE:
       return [self browsingDataManagerSelectorForRemoveOperationType];
   };
   NOTREACHED();
   return nullptr;

[stuartmorgan, 2015/06/30 21:57:17]

Does the compiler actually require this if you have all the enum cases handled?

[shreyasv1, 2015/07/07 21:42:05]

No, removed.

 }
 
 - (SEL)browsingDataManagerSelectorForRemoveOperationType {
   if (self.mode == web::ACTIVE) {
     return @selector(removeDataAtCanonicalPath);
   }
   if (self.mode == web::INACTIVE) {
     return @selector(removeDataAtStashPath);
   }
   DCHECK(_lastDispatchedStashOrRestoreOperation);

[stuartmorgan, 2015/06/30 21:57:17]

Seems like a short comment introducing what this whole block is doing could be
useful.

[shreyasv1, 2015/07/07 21:42:05]

Done.

   NSString* lastDispatchedStashOrRestoreOperationName =
       [_lastDispatchedStashOrRestoreOperation name];
   if ([lastDispatchedStashOrRestoreOperationName
           isEqual:kRestoreOperationName]) {
     return @selector(removeDataAtCanonicalPath);
   }
   if ([lastDispatchedStashOrRestoreOperationName isEqual:kStashOperationName]) {
     return @selector(removeDataAtStashPath);
   }
   NOTREACHED();
@@ skipping 98 matching lines @@
     web::BrowsingDataStoreMakeActivePolicy makeActivePolicy =
         [_delegate decideMakeActiveOperationPolicyForBrowsingDataStore:self];
     operationType = (makeActivePolicy == web::ADOPT) ? REMOVE : RESTORE;
   } else {
     // By default a |STASH| operation is performed when the mode is changed to
     // |INACTIVE|.
     operationType = STASH;
     web::BrowsingDataStoreMakeInactivePolicy makeInactivePolicy =
         [_delegate decideMakeInactiveOperationPolicyForBrowsingDataStore:self];
     operationType = (makeInactivePolicy == web::DELETE) ? REMOVE : STASH;
   }

[stuartmorgan, 2015/06/30 21:57:17]

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.

[shreyasv1, 2015/07/07 21:42:05]

Done.

   DCHECK_NE(NONE, operationType);
   [self performOperationWithType:operationType
             browsingDataManagers:[self allBrowsingDataManagers]
                completionHandler:completionHandlerAfterPerformingOperation];
 }
 
 - (void)removeDataOfTypes:(web::BrowsingDataTypes)browsingDataTypes
         completionHandler:(ProceduralBlock)completionHandler {
   DCHECK([NSThread isMainThread]);
 
@@ skipping 56 matching lines @@
 
   NSOperationQueue* queue = nil;
   switch (operationType) {
     case STASH:
     case RESTORE:
       queue = [CRWBrowsingDataStore operationQueueForStashAndRestoreOperations];
       break;
     case REMOVE:
       queue = [CRWBrowsingDataStore operationQueueForRemoveOperations];
       break;
     default:

[stuartmorgan, 2015/06/30 21:57:17]

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?

[shreyasv1, 2015/07/07 21:42:05]

Good catch.

       NOTREACHED();
       break;
   }
 
   [self addOperation:operation toQueue:queue];
 }
 
 - (NSOperation*)
     newOperationWithBrowsingDataManagers:(NSArray*)browsingDataManagers
                                 selector:(SEL)selector
@@ skipping 16 matching lines @@
   DCHECK(queue);
 
   if (_lastDispatchedOperation) {
     [operation addDependency:_lastDispatchedOperation];
   }
   _lastDispatchedOperation.reset([operation retain]);
   [queue addOperation:operation];
 }
 
 @end
+