Refactor TransportSecurityState.

chrome/browser/transport_security_persister.h

Index: chrome/browser/transport_security_persister.h
===================================================================
--- chrome/browser/transport_security_persister.h	(revision 133741)
+++ chrome/browser/transport_security_persister.h	(working copy)
@@ -14,7 +14,7 @@
 // correct transport security information.
 //
 // To load the state, we schedule a Task on the file thread which loads,
-// deserialises and configures the TransportSecurityState.
+// deserializes and configures the TransportSecurityState.
 //
 // The TransportSecurityState object supports running a callback function
 // when it changes. This object registers the callback, pointing at itself.
@@ -26,8 +26,8 @@
 //
 // ...
 //
-// TransportSecurityPersister::SerialiseState
-//   copies the current state of the TransportSecurityState, serialises
+// TransportSecurityPersister::SerializeState
+//   copies the current state of the TransportSecurityState, serializes
 //   and writes to disk.
 
 #ifndef CHROME_BROWSER_TRANSPORT_SECURITY_PERSISTER_H_
@@ -55,12 +55,64 @@
   // Called by the TransportSecurityState when it changes its state.
   virtual void StateIsDirty(net::TransportSecurityState*) OVERRIDE;
 
-  // ImportantFileWriter::DataSerializer:
+  // Implements the interface of ImportantFileWriter::DataSerializer.

[Ryan Sleevi, 2012/04/26 19:21:12]

The original form was consistent with
http://dev.chromium.org/developers/coding-style#TOC-Code-formatting

"When you implement an interface" ...

[palmer, 2012/04/27 23:52:34]

Done.

+  //
+  // Serializes |transport_security_state_| into |*output|. Returns true if
+  // all DomainStates were serialized correctly.
+  //
+  // The serialization format is JSON; the JSON represents a dictionary of
+  // host:DomainState pairs (host is a string). The DomainState is
+  // represented as a dictionary containing the following keys and value
+  // types (not all keys will always be present):
+  //

[Ryan Sleevi, 2012/04/26 19:21:12]

Should you include a version with the serialization form, especially as things
are still a bit draft-y?

[palmer, 2012/04/27 23:52:34]

The appearance of draftiness is largely due to backwards compat; I actually
don't anticipate changing this much in the future. If we change it it will be to
add things, which JSON allows easily and for which we can fall back gracefully.
One of the things we'd need to fall back gracefully for would be the version
field...

+  //     "include_subdomains": true|false
+  //     "created": double
+  //     "expiry": double
+  //     "dynamic_spki_hashes_expiry": double
+  //     "mode": "default"|"force-https"
+  //             legacy value synonyms "strict" = "force-https"
+  //                                   "pinning-only" = "default"
+  //             legacy value "spdy-only" is unused and ignored
+  //     "static_spki_hashes": list of strings
+  //         legacy key synonym "preloaded_spki_hashes"
+  //     "bad_static_spki_hashes": list of strings
+  //         legacy key synonym "bad_preloaded_spki_hashes"
+  //     "dynamic_spki_hashes": list of strings
+  //
+  // The keys are
+  // SHA256(net::TransportSecurityState::CanonicalizeHost(domain)). The

[Ryan Sleevi, 2012/04/26 19:21:12]

There are two dictionaries (the first is host -> data, the second is key ->
value). So it's not clear here what "keys" you're referring to, especially since
you s/host/domain/ (from line 64).

Might need some word smithing.

[palmer, 2012/04/27 23:52:34]

Done.

+  // reason for hashing them is so that the stored state does not trivially
+  // reveal a user's browsing history to an attacker reading the serialized
+  // state on disk.
   virtual bool SerializeData(std::string* data) OVERRIDE;
 
+  // Parses an array of JSON-encoded TransportSecurityState::DomainState
+  // entries. For use in loading entries defined on the command line
+  // (switches::kHstsHosts).
+  bool DeserializeFromCommandLine(const std::string& serialized);
+
+  // Clears any existing non-static entries, and then re-populates
+  // |transport_security_state_|.
+  //
+  // Sets |*dirty| to true if the new state differs from the persisted
+  // state; false otherwise.
+  bool LoadEntries(const std::string& serialized, bool* dirty);
+
  private:
   class Loader;
 
+  // Populates |state| from the JSON string |serialized|. Returns true if
+  // all entries were parsed and deserialized correctly. If |forced| is
+  // true, updates |state|'s map of "forced" DomainState entries; normally,
+  // leave this false.
+  //
+  // Sets |*dirty| to true if the new state differs from the persisted
+  // state; false otherwise.
+  static bool Deserialize(const std::string& serialized,
+                          bool* dirty,
+                          bool forced,

[Ryan Sleevi, 2012/04/26 19:21:12]

nit: Function parameter ordering

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

|forced| should be before |dirty|

[palmer, 2012/04/27 23:52:34]

Done.

+                          net::TransportSecurityState* state);
+
   void CompleteLoad(const std::string& state);
 
   net::TransportSecurityState* transport_security_state_;