Make Directory's code style a little more consistent.

sync/syncable/directory.h

 // Copyright 2013 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.
 
 #ifndef SYNC_SYNCABLE_DIRECTORY_H_
 #define SYNC_SYNCABLE_DIRECTORY_H_
 
 #include <deque>
 #include <set>
 #include <string>
@@ skipping 402 matching lines @@
   // persist modified entry on disk. e.g. SyncBackupManager uses this to
   // preserve sync preferences in DB on disk.
   void UnmarkDirtyEntry(WriteTransaction* trans, Entry* entry);
 
   // Clears |id_set| and fills it with the ids of attachments that need to be
   // uploaded to the sync server.
   void GetAttachmentIdsToUpload(BaseTransaction* trans,
                                 ModelType type,
                                 AttachmentIdSet* id_set);
 
- protected:  // for friends, mainly used by Entry constructors
-  virtual EntryKernel* GetEntryByHandle(int64 handle);
-  virtual EntryKernel* GetEntryByHandle(int64 metahandle,
-      ScopedKernelLock* lock);
-  virtual EntryKernel* GetEntryById(const Id& id);
-  EntryKernel* GetEntryByServerTag(const std::string& tag);
-  virtual EntryKernel* GetEntryByClientTag(const std::string& tag);
-  bool ReindexId(BaseWriteTransaction* trans, EntryKernel* const entry,
-                 const Id& new_id);
-  bool ReindexParentId(BaseWriteTransaction* trans, EntryKernel* const entry,
-                       const Id& new_parent_id);
-  // Update the attachment index for |metahandle| removing it from the index
-  // under |old_metadata| entries and add it under |new_metadata| entries.
-  void UpdateAttachmentIndex(const int64 metahandle,
-                             const sync_pb::AttachmentMetadata& old_metadata,
-                             const sync_pb::AttachmentMetadata& new_metadata);
-  void ClearDirtyMetahandles();
-
-  DirOpenResult OpenImpl(
-      const std::string& name,
-      DirectoryChangeDelegate* delegate,
-      const WeakHandle<TransactionObserver>& transaction_observer);
-
  private:
   struct Kernel {
     // |delegate| must not be NULL.  |transaction_observer| must be
     // initialized.
     Kernel(const std::string& name, const KernelLoadInfo& info,
            DirectoryChangeDelegate* delegate,
            const WeakHandle<TransactionObserver>& transaction_observer);
 
     ~Kernel();
 
@@ skipping 80 matching lines @@
     // The next metahandle is protected by kernel mutex.
     int64 next_metahandle;
 
     // The delegate for directory change events.  Must not be NULL.
     DirectoryChangeDelegate* const delegate;
 
     // The transaction observer.
     const WeakHandle<TransactionObserver> transaction_observer;
   };
 
-  // These private versions expect the kernel lock to already be held
-  // before calling.
-  EntryKernel* GetEntryById(const Id& id, ScopedKernelLock* const lock);
+  // You'll notice that some of these methods have two forms.  One that takes a
+  // ScopedKernelLock and one that doesn't.  The general pattern is that those
+  // without a ScopedKernelLock parameter construct one internally before
+  // calling the form that takes one.
+
+  virtual EntryKernel* GetEntryByHandle(int64 handle);
+  virtual EntryKernel* GetEntryByHandle(const ScopedKernelLock& lock,
+                                        int64 metahandle);
+
+  virtual EntryKernel* GetEntryById(const Id& id);
+  virtual EntryKernel* GetEntryById(const ScopedKernelLock& lock, const Id& id);
+
+  EntryKernel* GetEntryByServerTag(const std::string& tag);
+  virtual EntryKernel* GetEntryByClientTag(const std::string& tag);
+
+  // For new entry creation only
+  bool InsertEntry(const ScopedKernelLock& lock,

[Nicolas Zea, 2014/09/22 19:46:26]

nit: be consistent about ordering of locking version vs non-locking one?

[maniscalco, 2014/09/22 19:54:59]

Done.

+                   BaseWriteTransaction* trans,
+                   EntryKernel* entry);
+  bool InsertEntry(BaseWriteTransaction* trans, EntryKernel* entry);
+
+  bool ReindexId(BaseWriteTransaction* trans, EntryKernel* const entry,
+                 const Id& new_id);
+  bool ReindexParentId(BaseWriteTransaction* trans, EntryKernel* const entry,

[Nicolas Zea, 2014/09/22 19:46:25]

nit: newline above, since this isn't a pair of locking/not locking methods

[maniscalco, 2014/09/22 19:54:59]

Done.

+                       const Id& new_parent_id);
+
+  // Update the attachment index for |metahandle| removing it from the index
+  // under |old_metadata| entries and add it under |new_metadata| entries.
+  void UpdateAttachmentIndex(const int64 metahandle,
+                             const sync_pb::AttachmentMetadata& old_metadata,
+                             const sync_pb::AttachmentMetadata& new_metadata);
+
+  // Remove each of |metahandle|'s attachment ids from index_by_attachment_id.
+  void RemoveFromAttachmentIndex(
+      const ScopedKernelLock& lock,
+      const int64 metahandle,
+      const sync_pb::AttachmentMetadata& attachment_metadata);
+  // Add each of |metahandle|'s attachment ids to the index_by_attachment_id.

[Nicolas Zea, 2014/09/22 19:46:26]

nit: newline above?

[maniscalco, 2014/09/22 19:54:59]

Done.

+  void AddToAttachmentIndex(
+      const ScopedKernelLock& lock,
+      const int64 metahandle,
+      const sync_pb::AttachmentMetadata& attachment_metadata);
+
+  void ClearDirtyMetahandles(const ScopedKernelLock& lock);
+
+  DirOpenResult OpenImpl(
+      const std::string& name,
+      DirectoryChangeDelegate* delegate,
+      const WeakHandle<TransactionObserver>& transaction_observer);
 
   // A helper that implements the logic of checking tree invariants.
   bool CheckTreeInvariants(syncable::BaseTransaction* trans,
                            const MetahandleSet& handles);
 
   // Helper to prime metahandles_map, ids_map, parent_child_index,
   // unsynced_metahandles, unapplied_update_metahandles, server_tags_map and
   // client_tags_map from metahandles_index.  The input |handles_map| will be
   // cleared during the initialization process.
   void InitializeIndices(MetahandlesMap* handles_map);
 
   // Constructs a consistent snapshot of the current Directory state and
   // indices (by deep copy) under a ReadTransaction for use in |snapshot|.
   // See SaveChanges() for more information.
   void TakeSnapshotForSaveChanges(SaveChangesSnapshot* snapshot);
 
   // Purges from memory any unused, safe to remove entries that were
   // successfully deleted on disk as a result of the SaveChanges that processed
   // |snapshot|.  See SaveChanges() for more information.
   bool VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot);
 
   // Rolls back dirty bits in the event that the SaveChanges that
   // processed |snapshot| failed, for example, due to no disk space.
   void HandleSaveChangesFailure(const SaveChangesSnapshot& snapshot);
 
-  // For new entry creation only
-  bool InsertEntry(BaseWriteTransaction* trans,
-                   EntryKernel* entry, ScopedKernelLock* lock);
-  bool InsertEntry(BaseWriteTransaction* trans, EntryKernel* entry);
-
   // Used by CheckTreeInvariants
   void GetAllMetaHandles(BaseTransaction* trans, MetahandleSet* result);
   bool SafeToPurgeFromMemory(WriteTransaction* trans,
                              const EntryKernel* const entry) const;
 
   // A helper used by GetTotalNodeCount.
   void GetChildSetForKernel(
       BaseTransaction*,
       EntryKernel* kernel_,
       std::deque<const OrderedChildSet*>* child_sets) const;
 
   // Append the handles of the children of |parent_id| to |result|.
-  void AppendChildHandles(
-      const ScopedKernelLock& lock,
-      const Id& parent_id, Directory::Metahandles* result);
+  void AppendChildHandles(const ScopedKernelLock& lock,
+                          const Id& parent_id,
+                          Directory::Metahandles* result);
 
   // Helper methods used by PurgeDisabledTypes.
   void UnapplyEntry(EntryKernel* entry);
-  void DeleteEntry(bool save_to_journal,
+  void DeleteEntry(const ScopedKernelLock& lock,
+                   bool save_to_journal,
                    EntryKernel* entry,
-                   EntryKernelSet* entries_to_journal,
-                   const ScopedKernelLock& lock);
-
-  // Remove each of |metahandle|'s attachment ids from index_by_attachment_id.
-  void RemoveFromAttachmentIndex(
-      const int64 metahandle,
-      const sync_pb::AttachmentMetadata& attachment_metadata,
-      const ScopedKernelLock& lock);
-  // Add each of |metahandle|'s attachment ids to the index_by_attachment_id.
-  void AddToAttachmentIndex(
-      const int64 metahandle,
-      const sync_pb::AttachmentMetadata& attachment_metadata,
-      const ScopedKernelLock& lock);
+                   EntryKernelSet* entries_to_journal);
 
   // A private version of the public GetMetaHandlesOfType for when you already
   // have a ScopedKernelLock.
   void GetMetaHandlesOfType(const ScopedKernelLock& lock,
                             BaseTransaction* trans,
                             ModelType type,
                             std::vector<int64>* result);
 
   Kernel* kernel_;
 
@@ skipping 13 matching lines @@
   // are deleted in native models as well.
   scoped_ptr<DeleteJournal> delete_journal_;
 
   DISALLOW_COPY_AND_ASSIGN(Directory);
 };
 
 }  // namespace syncable
 }  // namespace syncer
 
 #endif // SYNC_SYNCABLE_DIRECTORY_H_