Change the TranslateDeclined() to only count decline if the explicit_closed is set to true

components/translate/core/browser/translate_ui_delegate_unittest.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/translate/core/browser/translate_ui_delegate.h"
+
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/macros.h"
+#include "base/strings/stringprintf.h"
+#include "build/build_config.h"
+#include "components/infobars/core/infobar.h"
+#include "components/pref_registry/testing_pref_service_syncable.h"
+#include "components/translate/core/browser/translate_client.h"
+#include "components/translate/core/browser/translate_driver.h"
+#include "components/translate/core/browser/translate_infobar_delegate.h"
+#include "components/translate/core/browser/translate_manager.h"
+#include "components/translate/core/browser/translate_prefs.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+using testing::Return;
+
+namespace translate {
+
+class MockTranslateDriver : public TranslateDriver {

[groby-ooo-7-16, 2016/01/28 21:17:28]

There's already a MockTranslateDriver in
components/translate/core/browser/language_state_unittest.cc

I'd suggest factoring that out.

[ftang, 2016/01/28 22:37:21]

Those are not using gmock. Do we have a directory to all the mock? If I
factoring that out, where should I place it?

[groby-ooo-7-16, 2016/01/29 20:09:16]

We usually place them right next to the header. So here, in
components/translate/core/browser. 

+ public:
+  MOCK_METHOD0(IsLinkNavigation, bool());
+  MOCK_METHOD0(OnTranslateEnabledChanged, void());
+  MOCK_METHOD0(OnIsPageTranslatedChanged, void());
+  MOCK_METHOD4(TranslatePage, void(int, const std::string&,
+                                   const std::string&, const std::string&));
+  MOCK_METHOD1(RevertTranslation, void(int));
+  MOCK_METHOD0(IsOffTheRecord, bool());
+  MOCK_METHOD0(GetContentsMimeType, const std::string&());
+  MOCK_METHOD0(GetLastCommittedURL, const GURL&());
+  MOCK_METHOD0(GetVisibleURL, const GURL&());
+  MOCK_METHOD0(HasCurrentPage, bool());
+  MOCK_METHOD1(OpenUrlInNewTab, void(const GURL&));
+};
+
+class MockTranslateClient : public TranslateClient {
+ public:
+  MOCK_METHOD0(GetTranslateDriver, TranslateDriver*());

[groby-ooo-7-16, 2016/01/28 21:17:28]

I'd have the mock take a driver in its ctor - and then just return that all the
time. Saves clients to do the EXPECT_CALL dance.

[ftang, 2016/01/28 22:37:22]

Acknowledged.

+  MOCK_METHOD0(GetPrefs, PrefService*());

[groby-ooo-7-16, 2016/01/28 21:17:28]

If you rely on prefs in any way, shouldn't this return |pref_service_| from the
test fixture? Either via a member passed in at ctor, or via EXPECT_CALL?

[ftang, 2016/01/28 22:37:21]

Acknowledged.

+  // MOCK_METHOD cannot mock a scoped_ptr argument.
+  MOCK_METHOD0(GetTranslatePrefsMock, TranslatePrefs*());

[groby-ooo-7-16, 2016/01/28 21:17:28]

I'm not sure why you're using a MOCK_METHOD here - doesn't it just return NULL?

[ftang, 2016/01/28 22:37:21]

gmock cannot mock scoped_ptr<class> so I follow some suggest pattern to deal the
scoped_ptr<> in parameters and return.

[groby-ooo-7-16, 2016/01/29 20:09:16]

Hm - is that suggested in Chrome, or by gmock? We usually just write stub
methods if it can't be created via MOCK_METHOD.

I.e. here

    scoped_ptr<TranslatePrefs> GetTranslatePrefs() {
      return make_scoped_ptr(new TranslatePrefs(prefs_));
    }

(As said below, you'll need to create a new object every time GetTranslatePrefs
is called)

[ftang, 2016/02/02 00:37:50]

Done.

+  scoped_ptr<TranslatePrefs> GetTranslatePrefs() {
+    return scoped_ptr<TranslatePrefs>(GetTranslatePrefsMock());
+  }
+  MOCK_METHOD0(GetTranslateAcceptLanguages, TranslateAcceptLanguages*());
+  MOCK_CONST_METHOD0(GetInfobarIconID, int());
+
+  MOCK_CONST_METHOD1(CreateInfoBarMock,
+                     infobars::InfoBar*(TranslateInfoBarDelegate*));
+  scoped_ptr<infobars::InfoBar> CreateInfoBar(
+      scoped_ptr<TranslateInfoBarDelegate> delegate) const {
+    return scoped_ptr<infobars::InfoBar>(
+        CreateInfoBarMock(delegate.get()));

[groby-ooo-7-16, 2016/01/28 21:17:28]

You want to do new TranslateInfoBar(std::move(delegate)) - otherwise, the
delegate will be deleted when you leave function scope.

[ftang, 2016/01/28 22:37:21]

Acknowledged.

+  }
+
+  MOCK_METHOD5(ShowTranslateUI, void(translate::TranslateStep,
+                                     const std::string&,
+                                     const std::string&,
+                                     TranslateErrors::Type,
+                                     bool));
+  MOCK_METHOD1(IsTranslatableURL, bool(const GURL&));
+  MOCK_METHOD1(ShowReportLanguageDetectionErrorUI, void(const GURL&));
+};
+
+class TranslateUIDelegateTest : public testing::Test {
+ public:
+  TranslateUIDelegateTest() : testing::Test() {}
+
+  void SetUp() override {
+    pref_service_.reset(new user_prefs::TestingPrefServiceSyncable());
+#if defined(OS_CHROMEOS)
+    const char* preferred_languages_prefs =
+        "settings.language.preferred_languages";
+#else
+    const char* preferred_languages_prefs = NULL;

[groby-ooo-7-16, 2016/01/28 21:17:28]

I think you can always just assume NULL for this.  You're not testing
CrOS-specific behavior.

[ftang, 2016/01/28 22:37:21]

but if I always assume NULL here. Won't the test break when running the test
under OS_CHROMEOS?

[groby-ooo-7-16, 2016/01/29 20:09:16]

You're right, it will. I forgot TranslatePrefs DCHECKs the value. :(

+#endif
+
+    prefs_ = new TranslatePrefs(pref_service_.get(),
+        "intl.accept_languages", preferred_languages_prefs);
+    TranslatePrefs::RegisterProfilePrefs(pref_service_->registry());
+
+    EXPECT_CALL(client_, GetTranslateDriver())
+        .WillRepeatedly(Return(&driver_));
+    EXPECT_CALL(client_, GetTranslatePrefsMock())
+        .WillRepeatedly(Return(prefs_));

[groby-ooo-7-16, 2016/01/28 21:17:28]

That won't work - when the first return value is out of scope, it will delete
|prefs_|. (scoped_ptr is similar to std::unique_ptr)

GetTranslatePrefs is actually a factory method for scoped_ptr<TranslatePrefs>
objects, so you'll need to allocate a new one on every call. 

[ftang, 2016/01/28 22:37:21]

Acknowledged.

+
+    manager_.reset(new TranslateManager(&client_, "hi"));
+    manager_->GetLanguageState().set_translation_declined(false);
+
+    delegate_.reset(new TranslateUIDelegate(
+        manager_->GetWeakPtr(), "ar", "fr"));
+
+    for (int i = 0; i < 10; i++) {
+       prefs_->IncrementTranslationAcceptedCount("ar");
+    }
+    prefs_->ResetTranslationDeniedCount("ar");
+    prefs_->IncrementTranslationDeniedCount("ar");

[groby-ooo-7-16, 2016/01/28 21:17:28]

Are you sure this should be in SetUp? It seems setup for your particular tests,
not UiDelegate tests in general. (And I don't think you need the Reset, because
each test case will receive new - and empty - preferences)

[ftang, 2016/01/28 22:37:22]

Acknowledged.

+
+    ASSERT_FALSE(prefs_->IsTooOftenDenied("ar"));
+  }
+
+  void TearDown() override {

[groby-ooo-7-16, 2016/01/28 21:17:28]

No need to override if it does nothing.

[ftang, 2016/01/28 22:37:22]

Acknowledged.

+  }
+
+  MockTranslateDriver driver_;
+  MockTranslateClient client_;
+  scoped_ptr<user_prefs::TestingPrefServiceSyncable> pref_service_;
+  TranslatePrefs* prefs_;

[groby-ooo-7-16, 2016/01/28 21:17:28]

You want this a scoped_ptr, I think. Otherwise, you'd leak it. Or you could just
have TranslatePrefs as a scoped object in SetUp - they're designed to be
short-lived.

[ftang, 2016/01/28 22:37:21]

Acknowledged.

+  scoped_ptr<TranslateManager> manager_;
+  scoped_ptr<TranslateUIDelegate> delegate_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TranslateUIDelegateTest);
+};
+
+
+TEST_F(TranslateUIDelegateTest, CheckDeclinedFalse) {
+  int accepted_count = prefs_->GetTranslationAcceptedCount("ar");
+  int denied_count = prefs_->GetTranslationDeniedCount("ar");
+
+  delegate_->TranslationDeclined(false);
+
+  ASSERT_EQ(accepted_count, prefs_->GetTranslationAcceptedCount("ar"));

[groby-ooo-7-16, 2016/01/28 21:17:28]

You probably want EXPECT_EQ/EXPECT_FALSE. No need to abort the tests if one
expectation fails.

[ftang, 2016/01/28 22:37:21]

Acknowledged.

+  ASSERT_EQ(denied_count, prefs_->GetTranslationDeniedCount("ar"));
+  ASSERT_FALSE(prefs_->IsTooOftenDenied("ar"));
+  ASSERT_FALSE(manager_->GetLanguageState().translation_declined());
+}
+
+TEST_F(TranslateUIDelegateTest, CheckDeclinedTrue) {
+  int denied_count = prefs_->GetTranslationDeniedCount("ar");
+
+  EXPECT_CALL(driver_, IsOffTheRecord())
+      .WillOnce(Return(false));
+
+  delegate_->TranslationDeclined(true);
+
+  ASSERT_EQ(0, prefs_->GetTranslationAcceptedCount("ar"));
+  ASSERT_EQ(denied_count + 1, prefs_->GetTranslationDeniedCount("ar"));
+  ASSERT_TRUE(manager_->GetLanguageState().translation_declined());
+}
+
+// TODO(ftang) Currently this file only test TranslationDeclined(), we

[groby-ooo-7-16, 2016/01/28 21:17:28]

You sure you want to take this on as your own TODO? :)

[ftang, 2016/01/28 22:37:22]

yes

[groby-ooo-7-16, 2016/01/29 20:09:16]

Thank you for volunteering, then!

[ftang, 2016/02/02 00:37:50]

Acknowledged.

+// need to add the test for other functions soon to increase the test
+// coverage.
+
+}  // namespace translate