Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
338283003:
Add HTMLTextFormControlElement unit test (Closed)
DescriptionAdd HTMLTextFormControlElement unit test
To confirm sanity.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176385
Patch Set 1 #
Total comments: 20
Patch Set 2 : Update #
Messages
Total messages: 11 (0 generated)
Do we really need this test in gtest rather than layout test? It seemsWebFormControlElement::setSelectionRange() calls this too. Add tkent@ for Forms team and API owner opinion.
https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... File Source/core/html/HTMLTextFormControlElementTest.cpp (right): https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:27: RefPtr<HTMLTextFormControlElement> m_textarea; nit: RefPtrWillBeRawPtr? not sure... https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:34: ASSERT(document); nit: We don't need this ASSERT. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:35: document->documentElement()->setInnerHTML(String::fromUTF8("<body><div id=div></div><textarea id=\"textarea\"></textarea></body>"), ASSERT_NO_EXCEPTION); nit: I think we don't need to have String::fromUTF8. There is String(const char*) constructor. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:38: ASSERT(m_textarea); nit: We don't need this ASSERT.
On 2014/06/17 08:40:31, yosi wrote: > Do we really need this test in gtest rather than layout test? > It seemsWebFormControlElement::setSelectionRange() calls this too. > > Add tkent@ for Forms team and API owner opinion. I'm fine with having unit tests in addition to layout tests.
https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... File Source/core/html/HTMLTextFormControlElementTest.cpp (right): https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:22: HTMLTextFormControlElement& textform() const { return *m_textarea; } nit: The function name should be textForm(). https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:27: RefPtr<HTMLTextFormControlElement> m_textarea; 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. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:34: ASSERT(document); 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. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:35: document->documentElement()->setInnerHTML(String::fromUTF8("<body><div id=div></div><textarea id=\"textarea\"></textarea></body>"), ASSERT_NO_EXCEPTION); nit: It's strange to use an unquoted attribute value (id=div) and a quoted value (id="textarea") at the same time. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:38: ASSERT(m_textarea); Same as line 34, since we are sure that getElementById returns a non-null object. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:56: } nit: There is a blank line after "namespace {" (line 17), so it seems natural to have one before the ending brace as well.
https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... File Source/core/html/HTMLTextFormControlElementTest.cpp (right): https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:22: HTMLTextFormControlElement& textform() const { return *m_textarea; } 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'. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:27: RefPtr<HTMLTextFormControlElement> m_textarea; 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<>.
https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... File Source/core/html/HTMLTextFormControlElementTest.cpp (right): https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:22: HTMLTextFormControlElement& textform() const { return *m_textarea; } 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. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:27: RefPtr<HTMLTextFormControlElement> m_textarea; 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. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:34: ASSERT(document); On 2014/06/17 08:45:12, yosi wrote: > nit: We don't need this ASSERT. Done. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:34: ASSERT(document); 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. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:35: document->documentElement()->setInnerHTML(String::fromUTF8("<body><div id=div></div><textarea id=\"textarea\"></textarea></body>"), ASSERT_NO_EXCEPTION); 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. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:35: document->documentElement()->setInnerHTML(String::fromUTF8("<body><div id=div></div><textarea id=\"textarea\"></textarea></body>"), ASSERT_NO_EXCEPTION); 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. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:38: ASSERT(m_textarea); On 2014/06/17 08:45:12, yosi wrote: > nit: We don't need this ASSERT. Done. https://codereview.chromium.org/338283003/diff/1/Source/core/html/HTMLTextFor... Source/core/html/HTMLTextFormControlElementTest.cpp:56: } 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.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/338283003/20001
Message was sent while issue was closed.
Change committed as 176385 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
