Add module for counting date-last-roll-call and persisting those counts

components/update_client/component_metadata.cc

+// Copyright 2016 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.
+
+#include "components/update_client/component_metadata.h"
+
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/important_file_writer.h"
+#include "base/json/json_file_value_serializer.h"
+#include "base/json/json_writer.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/sequenced_task_runner.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/values.h"
+
+namespace update_client {
+
+ComponentMetadata::ComponentMetadata(
+    const base::FilePath& path,
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+    : path_(path),
+      file_writer_(path, task_runner),
+      ready_(false),
+      task_runner_(task_runner) {
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&ComponentMetadata::Initialize,
+                 scoped_refptr<ComponentMetadata>(this),

[Sorin Jianu, 2016/04/07 04:03:19]

is the cast needed, here and below?

[waffles, 2016/04/07 17:04:43]

Not sure about this one, let's discuss offline. There are several places in the
this directory where we follow this pattern, so if they can be cleaned up, let's
take it in a second CL, but keep consistency here.

[Sorin Jianu, 2016/04/07 18:27:35]

If the code compiles without the function-style cast, then I suggest removing
it. C++ is a statically typed language, and in most cases, if the code compiles,
it will have the correct meaning. 

+                 base::ThreadTaskRunnerHandle::Get()));
+}
+
+ComponentMetadata::~ComponentMetadata() {
+}
+
+void ComponentMetadata::Initialize(
+    const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner) {
+  DCHECK(!ready_);
+  LoadFromFile();
+  main_task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&ComponentMetadata::Ready,
+                 scoped_refptr<ComponentMetadata>(this)));
+}
+
+void ComponentMetadata::LoadFromFile() {
+  if (path_.empty() || !base::PathExists(path_)) {
+    metadata_ = scoped_ptr<base::DictionaryValue>(new base::DictionaryValue());
+    return;
+  }
+  JSONFileValueDeserializer deserializer(path_);
+  std::string error;
+  scoped_ptr<base::Value> root = deserializer.Deserialize(NULL, &error);
+  if (!root.get() || !root->IsType(base::Value::TYPE_DICTIONARY)) {

[Sorin Jianu, 2016/04/07 04:03:19]

Can 61-66 be written using ?

matadata_ = scoped_ptr<base::DictionaryValue>(root && root->IsType() ? ...

Alternatively, can we use the idiomatic if (a && a->) which is easier to read
instead of doing De Morgan in my head?

[waffles, 2016/04/07 17:04:43]

Done.

+    metadata_ = scoped_ptr<base::DictionaryValue>(new base::DictionaryValue());
+    return;
+  }
+  metadata_ = scoped_ptr<base::DictionaryValue>(
+      static_cast<base::DictionaryValue*>(root.release()));
+}
+
+void ComponentMetadata::Ready() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ready_ = true;
+}
+
+int ComponentMetadata::DateLastRollCall(const std::string& id) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!ready_)
+    return -2;
+  DCHECK(metadata_);
+  int dlrc;

[Sorin Jianu, 2016/04/07 04:03:19]

not initialized

[waffles, 2016/04/07 17:04:43]

Done.

+  // We assume ids do not contain '.' characters.
+  if (!metadata_->GetInteger("apps." + id + ".dlrc", &dlrc))
+    return -2;
+  return dlrc;
+}
+
+void ComponentMetadata::SetDateLastRollCall(
+    const std::vector<std::string>& ids, int datenum) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!ready_)  // Trying to write before we've ever read it: drop the update.

[Sorin Jianu, 2016/04/07 04:03:19]

Will this cause any issue? Is it ok to silently fail the call?
Maybe have this function return bool, so that the caller knows the call failed.

[waffles, 2016/04/07 17:04:43]

At this time, it's not clear to me what the caller can do differently based on
that information (we don't want to abandon the updatecheck just because of a
failure here), so I'm inclined to punt this. I've expanded the comment to call
out this behavior.

[Sorin Jianu, 2016/04/07 18:27:35]

The caller could assert, if it wants to enforce some correctness issue. Don't
call the function before initialization is complete. Also, since the
initialization is asynchronous, we might consider a callback be called, so that
a caller can synchronize its execution and not call this function too soon, if
the error in this call is unrecoverable, as far as the caller is concerned. TBD
in a future CL.

+    return;
+  DCHECK(metadata_);
+  for (auto id : ids) {
+    // We assume ids do not contain '.' characters.
+    metadata_->SetInteger("apps." + id + ".dlrc", datenum);
+  }
+  if (path_.empty())
+    return;
+  scoped_ptr<std::string> serialized(new std::string());
+  base::JSONWriter::Write(*metadata_, serialized.get());
+  file_writer_.WriteNow(std::move(serialized));
+}
+
+}  // namespace update_client