Source/core/html/HTMLTextFormControlElementTest.cpp - Issue 338283003: Add HTMLTextFormControlElement unit test

Side by Side Diff: Source/core/html/HTMLTextFormControlElementTest.cpp

Issue 338283003: Add HTMLTextFormControlElement unit test (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Created 6 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 // Copyright 2014 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #include "config.h"

	6 #include "core/html/HTMLTextFormControlElement.h"

	7

	8 #include "core/frame/FrameView.h"

	9 #include "core/html/HTMLDocument.h"

	10 #include "core/testing/DummyPageHolder.h"

	11 #include "wtf/OwnPtr.h"

	12 #include <gtest/gtest.h>

	13

	14 using namespace WebCore;

	15

	16 namespace {

	17

	18 class HTMLTextFormControlElementTest : public ::testing::Test {

	19 protected:

	20 virtual void SetUp() OVERRIDE;

	21

	22 HTMLTextFormControlElement& textform() const { return *m_textarea; }
	Yuta Kitamura 2014/06/17 09:07:50 nit: The function name should be textForm(). nit: The function name should be textForm(). tkent 2014/06/18 00:06:38 textControl() is better, IMO. This is not a 'form Show quoted text On 2014/06/17 09:07:50, Yuta Kitamura wrote: > nit: The function name should be textForm(). textControl() is better, IMO. This is not a 'form'. yoichio 2014/06/18 01:36:47 Done. Show quoted text On 2014/06/18 00:06:38, tkent wrote: > On 2014/06/17 09:07:50, Yuta Kitamura wrote: > > nit: The function name should be textForm(). > > textControl() is better, IMO. This is not a 'form'. Done.
	23

	24 private:

	25 OwnPtr<DummyPageHolder> m_dummyPageHolder;

	26

	27 RefPtr<HTMLTextFormControlElement> m_textarea;
	yosin_UTC9 2014/06/17 08:45:12 nit: RefPtrWillBeRawPtr? not sure... nit: RefPtrWillBeRawPtr? not sure... Yuta Kitamura 2014/06/17 09:07:50 Well, I'm not sure either, but RefPtrWillBeRawPtr Show quoted text On 2014/06/17 08:45:12, yosi wrote: > nit: RefPtrWillBeRawPtr? not sure... Well, I'm not sure either, but RefPtrWillBeRawPtr looks wrong to me. I think the answer will differ depending on whether the test fixtures are allocated on stack or heap, which I don't know off-hand. tkent 2014/06/18 00:06:38 This should be RefPtrWillBePersistent<>. Show quoted text On 2014/06/17 09:07:50, Yuta Kitamura wrote: > On 2014/06/17 08:45:12, yosi wrote: > > nit: RefPtrWillBeRawPtr? not sure... > > Well, I'm not sure either, but RefPtrWillBeRawPtr looks > wrong to me. > > I think the answer will differ depending on whether > the test fixtures are allocated on stack or heap, > which I don't know off-hand. This should be RefPtrWillBePersistent<>. yoichio 2014/06/18 01:36:46 Done. Show quoted text On 2014/06/18 00:06:38, tkent wrote: > On 2014/06/17 09:07:50, Yuta Kitamura wrote: > > On 2014/06/17 08:45:12, yosi wrote: > > > nit: RefPtrWillBeRawPtr? not sure... > > > > Well, I'm not sure either, but RefPtrWillBeRawPtr looks > > wrong to me. > > > > I think the answer will differ depending on whether > > the test fixtures are allocated on stack or heap, > > which I don't know off-hand. > > This should be RefPtrWillBePersistent<>. Done.
	28 };

	29

	30 void HTMLTextFormControlElementTest::SetUp()

	31 {

	32 m_dummyPageHolder = DummyPageHolder::create(IntSize(800, 600));

	33 HTMLDocument* document = toHTMLDocument(&m_dummyPageHolder->document());

	34 ASSERT(document);
	yosin_UTC9 2014/06/17 08:45:12 nit: We don't need this ASSERT. nit: We don't need this ASSERT. Yuta Kitamura 2014/06/17 09:07:50 This ASSERT is meaningless, because toHTMLDocument This ASSERT is meaningless, because toHTMLDocument() never returns null if the argument is not null (it does not return null if the argument is not an instance of HTMLDocument. Instead, it ASSERTs that the argument is an object of the desired type). Or better yet, you can receive \|document\| as a reference, because there is an overloaded version of toXXX() functions that takes a reference and returns a reference. yoichio 2014/06/18 01:36:46 Done. Show quoted text On 2014/06/17 08:45:12, yosi wrote: > nit: We don't need this ASSERT. Done. yoichio 2014/06/18 01:36:47 Done. Show quoted text On 2014/06/17 09:07:50, Yuta Kitamura wrote: > This ASSERT is meaningless, because toHTMLDocument() never > returns null if the argument is not null (it does not return > null if the argument is not an instance of HTMLDocument. > Instead, it ASSERTs that the argument is an object of > the desired type). > > Or better yet, you can receive \|document\| as a reference, > because there is an overloaded version of toXXX() functions > that takes a reference and returns a reference. Done.
	35 document->documentElement()->setInnerHTML(String::fromUTF8("<body><div id=di v></div><textarea id=\"textarea\"></textarea></body>"), ASSERT_NO_EXCEPTION);
	yosin_UTC9 2014/06/17 08:45:12 nit: I think we don't need to have String::fromUTF nit: I think we don't need to have String::fromUTF8. There is String(const char) constructor. Yuta Kitamura* 2014/06/17 09:07:50 nit: It's strange to use an unquoted attribute val nit: It's strange to use an unquoted attribute value (id=div) and a quoted value (id="textarea") at the same time. yoichio 2014/06/18 01:36:46 Done. Show quoted text On 2014/06/17 08:45:12, yosi wrote: > nit: I think we don't need to have String::fromUTF8. There is String(const > char) constructor. Done. yoichio* 2014/06/18 01:36:47 Done. Show quoted text On 2014/06/17 09:07:50, Yuta Kitamura wrote: > nit: It's strange to use an unquoted attribute value > (id=div) and a quoted value (id="textarea") at the > same time. Done.
	36 document->view()->updateLayoutAndStyleIfNeededRecursive();

	37 m_textarea = toHTMLTextFormControlElement(document->getElementById("textarea "));

	38 ASSERT(m_textarea);
	yosin_UTC9 2014/06/17 08:45:12 nit: We don't need this ASSERT. nit: We don't need this ASSERT. Yuta Kitamura 2014/06/17 09:07:50 Same as line 34, since we are sure that getElement Same as line 34, since we are sure that getElementById returns a non-null object. yoichio 2014/06/18 01:36:47 Done. Show quoted text On 2014/06/17 08:45:12, yosi wrote: > nit: We don't need this ASSERT. Done.
	39 m_textarea->focus();

	40 }

	41

	42 TEST_F(HTMLTextFormControlElementTest, SetSelectionRange)

	43 {

	44 HTMLTextFormControlElement& form = textform();

	45 EXPECT_EQ(0, form.selectionStart());

	46 EXPECT_EQ(0, form.selectionEnd());

	47

	48 form.setInnerTextValue("Hello, text form.");

	49 EXPECT_EQ(0, form.selectionStart());

	50 EXPECT_EQ(0, form.selectionEnd());

	51

	52 form.setSelectionRange(1, 3);

	53 EXPECT_EQ(1, form.selectionStart());

	54 EXPECT_EQ(3, form.selectionEnd());

	55 }

	56 }
	Yuta Kitamura 2014/06/17 09:07:50 nit: There is a blank line after "namespace {" (li nit: There is a blank line after "namespace {" (line 17), so it seems natural to have one before the ending brace as well. yoichio 2014/06/18 01:36:46 Done. Show quoted text On 2014/06/17 09:07:50, Yuta Kitamura wrote: > nit: There is a blank line after "namespace {" (line 17), > so it seems natural to have one before the ending brace > as well. Done.
OLD	NEW

« no previous file with comments | « Source/core/core.gypi ('k') | no next file » | no next file with comments »