Fixes by code review.

2021-08-12 14:26:40 +07:00
parent 11ab9ed2d6
commit daa7d1e340
12 changed files with 84 additions and 73 deletions
@@ -427,19 +427,11 @@ void RewardsInternalsDOMHandler::GetAdDiagnostics(const base::ListValue* args) {

 void RewardsInternalsDOMHandler::OnGetAdDiagnostics(const bool success,
                                                    const std::string& json) {
-  constexpr char kName[] = "name";
-  constexpr char kValue[] = "value";
-
  if (!web_ui()->CanCallJavascript()) {
    return;
  }

  base::Value diagnostics(base::Value::Type::LIST);
-  base::Value status(base::Value::Type::DICTIONARY);
-  status.SetStringKey(kName, "Status");
-  status.SetStringKey(kValue, "Ads are not initialized yet. Press Refresh button please.");
-  diagnostics.Append(std::move(status));
-
  if (success && !json.empty()) {
    absl::optional<base::Value> serialized_json = base::JSONReader::Read(json);
    if (serialized_json && serialized_json->is_list() &&
@@ -452,8 +444,9 @@ void RewardsInternalsDOMHandler::OnGetAdDiagnostics(const bool success,
  DCHECK(diagnostics.is_list()) << "Diagnostics should be a list";
  for (const auto& entry : diagnostics.GetList()) {
    DCHECK(entry.is_dict()) << "Diagnostics entry should be a dictionary";
-    DCHECK(entry.FindKey(kName)) << "Diagnostics entry should has 'name' key";
-    DCHECK(entry.FindKey(kValue)) << "Diagnostics entry should has 'value' key";
+    DCHECK(entry.FindKey("name")) << "Diagnostics entry should has 'name' key";
+    DCHECK(entry.FindKey("value"))
+        << "Diagnostics entry should has 'value' key";
  }
 #endif  // DCHECK_IS_ON()

@@ -1026,6 +1026,7 @@ void CustomizeWebUIHTMLSource(const std::string &name,
      }
    }, {
      std::string("rewards-internals"), {
+        { "adsNotYetInitialized", IDS_BRAVE_REWARDS_INTERNALS_ADS_NOT_YET_INITIALIZED },         // NOLINT
        { "amount", IDS_BRAVE_REWARDS_INTERNALS_AMOUNT },
        { "autoRefresh", IDS_BRAVE_REWARDS_INTERNALS_AUTO_REFRESH },
        { "balanceInfo", IDS_BRAVE_REWARDS_INTERNALS_BALANCE_INFO },
@@ -433,7 +433,7 @@ void AdsServiceImpl::GetAccountStatement(GetAccountStatementCallback callback) {

 void AdsServiceImpl::GetAdDiagnostics(GetAdDiagnosticsCallback callback) {
  if (!connected()) {
-    std::move(callback).Run(/* success */ false, std::string{});
+    std::move(callback).Run(/* success */ false, "");
    return;
  }

@@ -17,14 +17,18 @@ interface Props {

 const getEntries = (entries: RewardsInternals.AdDiagnosticsEntry[]) => {
  if (!entries || entries.length === 0) {
-    return null
+    return (
+      <DiagnosticsEntry>
+        {getLocale('adsNotYetInitialized')}
+      </DiagnosticsEntry>
+    )
  }

  return entries.map(entry => (
-        <DiagnosticsEntry key={entry.name}>
-          {entry.name}: {entry.value}
-        </DiagnosticsEntry>
-      ))
+    <DiagnosticsEntry key={entry.name}>
+      {entry.name}: {entry.value}
+    </DiagnosticsEntry>
+  ))
 }

 export const AdDiagnostics = (props: Props) => {
@@ -14,8 +14,8 @@ declare namespace RewardsInternals {
    promotions: Promotion[]
    log: string
    fullLog: string
-    externalWallet: ExternalWallet,
-    eventLogs: EventLog[],
+    externalWallet: ExternalWallet
+    eventLogs: EventLog[]
    adDiagnostics: AdDiagnosticsEntry[]
  }

@@ -503,6 +503,7 @@
      <message name="IDS_BRAVE_REWARDS_MINIMUM_BALANCE_WARNING" desc=""><ph name="PROVIDER">$1</ph> requires a minimum balance of <ph name="AMOUNT">$2</ph> BAT to create an account. If you connected an account previously,</message>

      <!-- WebUI rewards internals resources -->
+      <message name="IDS_BRAVE_REWARDS_INTERNALS_ADS_NOT_YET_INITIALIZED" desc="Ads not yet initialized">Ads have not yet been initialized. Please click ‘Refresh’.</message>
      <message name="IDS_BRAVE_REWARDS_INTERNALS_AMOUNT" desc="Amount">Amount:</message>
      <message name="IDS_BRAVE_REWARDS_INTERNALS_AUTO_REFRESH" desc="">Automatically refresh every 5 seconds</message>
      <message name="IDS_BRAVE_REWARDS_INTERNALS_BALANCE_INFO" desc="">Balance info</message>
@@ -20,7 +20,13 @@ namespace ads {

 namespace {

-std::string TimeToString(const base::Time& time) {
+AdDiagnostics* g_ad_diagnostics = nullptr;
+
+std::string ToString(const bool value) {
+  return value ? "true" : "false";
+}
+
+std::string ToString(const base::Time& time) {
  if (time.is_null())
    return {};
  return base::UTF16ToUTF8(base::TimeFormatShortDateAndTime(time));
@@ -28,9 +34,25 @@ std::string TimeToString(const base::Time& time) {

 }  // namespace

-AdDiagnostics::AdDiagnostics() = default;
+AdDiagnostics::AdDiagnostics() {
+  DCHECK(!g_ad_diagnostics);
+  g_ad_diagnostics = this;
+}

-AdDiagnostics::~AdDiagnostics() = default;
+AdDiagnostics::~AdDiagnostics() {
+  DCHECK(g_ad_diagnostics);
+  g_ad_diagnostics = nullptr;
+}
+
+// static
+AdDiagnostics* AdDiagnostics::Get() {
+  DCHECK(g_ad_diagnostics);
+  return g_ad_diagnostics;
+}
+
+void AdDiagnostics::SetLastUnIdleTimestamp(const base::Time& value) {
+  last_unidle_timestamp_ = value;
+}

 void AdDiagnostics::GetAdDiagnostics(GetAdDiagnosticsCallback callback) const {
  base::Value diagnostics = CollectDiagnostics();
@@ -44,12 +66,10 @@ void AdDiagnostics::GetAdDiagnostics(GetAdDiagnosticsCallback callback) const {
 base::Value AdDiagnostics::CollectDiagnostics() const {
  base::Value diagnostics(base::Value::Type::LIST);

-  AddDiagnosticsEntry(kDiagnosticsAdsEnabled,
-                      AdsClientHelper::Get()->GetBooleanPref(prefs::kEnabled),
-                      &diagnostics);
-
-  AddDiagnosticsEntry(kDiagnosticsAdsInitialized, ads_initialized_,
-                      &diagnostics);
+  AddDiagnosticsEntry(
+      kDiagnosticsAdsEnabled,
+      ToString(AdsClientHelper::Get()->GetBooleanPref(prefs::kEnabled)),
+      &diagnostics);

  AddDiagnosticsEntry(kDiagnosticsLocale,
                      brave_l10n::LocaleHelper::GetInstance()->GetLocale(),
@@ -58,7 +78,7 @@ base::Value AdDiagnostics::CollectDiagnostics() const {
  CollectCatalogDiagnostics(&diagnostics);

  AddDiagnosticsEntry(kDiagnosticsLastUnIdleTimestamp,
-                      TimeToString(last_unidle_timestamp_), &diagnostics);
+                      ToString(last_unidle_timestamp_), &diagnostics);

  return diagnostics;
 }
@@ -73,7 +93,7 @@ void AdDiagnostics::CollectCatalogDiagnostics(base::Value* diagnostics) const {
  const int64_t catalog_last_updated =
      AdsClientHelper::Get()->GetInt64Pref(prefs::kCatalogLastUpdated);
  const base::Time time = base::Time::FromDoubleT(catalog_last_updated);
-  AddDiagnosticsEntry(kDiagnosticsCatalogLastUpdated, TimeToString(time),
+  AddDiagnosticsEntry(kDiagnosticsCatalogLastUpdated, ToString(time),
                      diagnostics);
 }

@@ -6,6 +6,7 @@
 #ifndef BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_AD_DIAGNOSTICS_AD_DIAGNOSTICS_H_
 #define BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_AD_DIAGNOSTICS_AD_DIAGNOSTICS_H_

+#include "base/time/time.h"
 #include "bat/ads/ads.h"

 namespace base {
@@ -21,18 +22,16 @@ class AdDiagnostics final {
  AdDiagnostics& operator=(const AdDiagnostics&) = delete;
  ~AdDiagnostics();

-  void GetAdDiagnostics(GetAdDiagnosticsCallback callback) const;
+  static AdDiagnostics* Get();

-  void set_ads_initialized(const bool value) { ads_initialized_ = value; }
-  void set_last_unidle_timestamp(const base::Time& value) {
-    last_unidle_timestamp_ = value;
-  }
+  void SetLastUnIdleTimestamp(const base::Time& value);
+
+  void GetAdDiagnostics(GetAdDiagnosticsCallback callback) const;

 private:
  base::Value CollectDiagnostics() const;
  void CollectCatalogDiagnostics(base::Value* diagnostics) const;

-  bool ads_initialized_ = false;
  base::Time last_unidle_timestamp_;
 };

@@ -5,45 +5,53 @@

 #include "bat/ads/internal/ad_diagnostics/ad_diagnostics_entry.h"

+#include <utility>
+
 #include "base/strings/string_number_conversions.h"
+#include "base/values.h"

 namespace ads {

+namespace {
+
 const char kDiagnosticsEntryName[] = "name";
 const char kDiagnosticsEntryValue[] = "value";

+}  // namespace
+
 const char kDiagnosticsAdsEnabled[] = "Ads enabled";
-const char kDiagnosticsAdsInitialized[] = "Ads initialized";
 const char kDiagnosticsLocale[] = "Locale";
 const char kDiagnosticsCatalogId[] = "Catalog ID";
 const char kDiagnosticsCatalogLastUpdated[] = "Catalog last updated";
 const char kDiagnosticsLastUnIdleTimestamp[] = "Last unidle timestamp";

-template <>
-std::string ValueToString(bool value) {
-  return value ? "true" : "false";
-}
+void AddDiagnosticsEntry(const std::string& name,
+                         const std::string& value,
+                         base::Value* diagnostics) {
+  DCHECK(diagnostics);
+  DCHECK(diagnostics->is_list());

-template <>
-std::string ValueToString(std::string value) {
-  return value;
+  base::Value entry(base::Value::Type::DICTIONARY);
+  entry.SetStringKey(kDiagnosticsEntryName, name);
+  entry.SetStringKey(kDiagnosticsEntryValue, value);
+  diagnostics->Append(std::move(entry));
 }

 absl::optional<std::string> GetDiagnosticsEntry(const base::Value& diagnostics,
                                                const std::string& name) {
  DCHECK(diagnostics.is_list());

+  const std::string* value = nullptr;
  for (const base::Value& item : diagnostics.GetList()) {
    DCHECK(item.is_dict());
    const std::string* key = item.FindStringKey(kDiagnosticsEntryName);
-    if (!key || *key != name) {
-      continue;
+    if (key && *key == name) {
+      value = item.FindStringKey(kDiagnosticsEntryValue);
+      break;
    }
-    const std::string* value = item.FindStringKey(kDiagnosticsEntryValue);
-    return value ? *value : absl::optional<std::string>{};
  }

-  return absl::optional<std::string>{};
+  return value ? *value : absl::optional<std::string>();
 }

 }  // namespace ads
@@ -8,35 +8,23 @@

 #include <string>

-#include "base/values.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"

+namespace base {
+class Value;
+}
+
 namespace ads {

-extern const char kDiagnosticsEntryName[];
-extern const char kDiagnosticsEntryValue[];
 extern const char kDiagnosticsAdsEnabled[];
-extern const char kDiagnosticsAdsInitialized[];
 extern const char kDiagnosticsLocale[];
 extern const char kDiagnosticsCatalogId[];
 extern const char kDiagnosticsCatalogLastUpdated[];
 extern const char kDiagnosticsLastUnIdleTimestamp[];

-template <typename T>
-std::string ValueToString(T value);
-
-template <typename T>
 void AddDiagnosticsEntry(const std::string& name,
-                         T value,
-                         base::Value* diagnostics) {
-  DCHECK(diagnostics);
-  DCHECK(diagnostics->is_list());
-
-  base::Value entry(base::Value::Type::DICTIONARY);
-  entry.SetStringKey(kDiagnosticsEntryName, name);
-  entry.SetStringKey(kDiagnosticsEntryValue, ValueToString(value));
-  diagnostics->Append(std::move(entry));
-}
+                         const std::string& value,
+                         base::Value* diagnostics);

 absl::optional<std::string> GetDiagnosticsEntry(const base::Value& diagnostics,
                                                const std::string& name);
@@ -5,6 +5,8 @@

 #include "bat/ads/internal/ad_diagnostics/ad_diagnostics.h"

+#include <string>
+
 #include "base/i18n/time_formatting.h"
 #include "base/json/json_reader.h"
 #include "bat/ads/internal/ad_diagnostics/ad_diagnostics_entry.h"
@@ -39,8 +41,6 @@ TEST_F(AdDiagnosticsTest, CheckAdsInitialized) {

    EXPECT_EQ("false",
              GetDiagnosticsEntry(*json_value, kDiagnosticsAdsEnabled));
-    EXPECT_EQ("false",
-              GetDiagnosticsEntry(*json_value, kDiagnosticsAdsInitialized));
    EXPECT_EQ("en-US", GetDiagnosticsEntry(*json_value, kDiagnosticsLocale));
  });

@@ -55,8 +55,6 @@ TEST_F(AdDiagnosticsTest, CheckAdsInitialized) {
    ASSERT_TRUE(json_value);

    EXPECT_EQ("true", GetDiagnosticsEntry(*json_value, kDiagnosticsAdsEnabled));
-    EXPECT_EQ("true",
-              GetDiagnosticsEntry(*json_value, kDiagnosticsAdsInitialized));
    EXPECT_EQ("en-US", GetDiagnosticsEntry(*json_value, kDiagnosticsLocale));
  });
 }
@@ -216,7 +216,7 @@ void AdsImpl::OnUnIdle(const int idle_time, const bool was_locked) {
    return;
  }

-  ad_diagnostics_->set_last_unidle_timestamp(base::Time::Now());
+  AdDiagnostics::Get()->SetLastUnIdleTimestamp(base::Time::Now());

  MaybeUpdateIdleTimeThreshold();

@@ -408,8 +408,7 @@ void AdsImpl::GetAccountStatement(GetAccountStatementCallback callback) {
 }

 void AdsImpl::GetAdDiagnostics(GetAdDiagnosticsCallback callback) {
-  ad_diagnostics_->set_ads_initialized(IsInitialized());
-  ad_diagnostics_->GetAdDiagnostics(std::move(callback));
+  AdDiagnostics::Get()->GetAdDiagnostics(std::move(callback));
 }

 AdContentInfo::LikeAction AdsImpl::ToggleAdThumbUp(