Change in pdfium[master]: Start CSS parser unit tests
dsinclair (Gerrit)
dsinclair uploaded this change for review.
Start CSS parser unit tests Start adding unit tests for the css parser. Change-Id: Id863d9cd5f13ab82626bc7b945de925253c88d43 --- M BUILD.gn A xfa/fde/css/cfde_cssstylesheet_unittest.cpp M xfa/fde/css/fde_cssdeclaration.cpp M xfa/fde/css/fde_cssdeclaration.h 4 files changed, 122 insertions(+), 0 deletions(-)
diff --git a/BUILD.gn b/BUILD.gn
index 5e0a630..c69e6e4 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -1746,6 +1746,7 @@
if (pdf_enable_xfa) {
sources += [
"xfa/fde/cfde_txtedtbuf_unittest.cpp",
+ "xfa/fde/css/cfde_cssstylesheet_unittest.cpp",
"xfa/fde/css/fde_cssdatatable_unittest.cpp",
"xfa/fde/xml/fde_xml_imp_unittest.cpp",
"xfa/fxbarcode/pdf417/BC_PDF417HighLevelEncoder_unittest.cpp",
diff --git a/xfa/fde/css/cfde_cssstylesheet_unittest.cpp b/xfa/fde/css/cfde_cssstylesheet_unittest.cpp
new file mode 100644
index 0000000..1edc4b5
--- /dev/null
+++ b/xfa/fde/css/cfde_cssstylesheet_unittest.cpp
@@ -0,0 +1,112 @@
+// Copyright 2017 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
+
+#include "xfa/fde/css/fde_cssstylesheet.h"
+
+#include <memory>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/base/ptr_util.h"
+
+class CFDE_CSSStyleSheetTest : public testing::Test {
+ public:
+ void SetUp() override {
+ sheet_ = pdfium::MakeUnique<CFDE_CSSStyleSheet>();
+ decl_ = nullptr;
+ }
+
+ void TearDown() override { decl_ = nullptr; }
+
+ void LoadAndVerifyDecl(const FX_WCHAR* buf, size_t decl_count) {
+ ASSERT(sheet_);
+
+ EXPECT_TRUE(sheet_->LoadFromBuffer(buf, FXSYS_wcslen(buf)));
+ EXPECT_EQ(1, sheet_->CountRules());
+
+ CFDE_CSSRule* rule = sheet_->GetRule(0);
+ EXPECT_EQ(FDE_CSSRuleType::Style, rule->GetType());
+
+ CFDE_CSSStyleRule* style = static_cast<CFDE_CSSStyleRule*>(rule);
+ decl_ = style->GetDeclaration();
+ EXPECT_EQ(decl_count, decl_->PropertyCountForTesting());
+ }
+
+ void VerifyFloat(FDE_CSSProperty prop, float val, FDE_CSSPrimitiveType type) {
+ ASSERT(decl_);
+
+ bool important;
+ CFDE_CSSValue* v = decl_->GetProperty(prop, important);
+ CFDE_CSSPrimitiveValue* pval = static_cast<CFDE_CSSPrimitiveValue*>(v);
+ EXPECT_EQ(type, pval->GetPrimitiveType());
+ EXPECT_EQ(val, pval->GetFloat());
+ }
+
+ void VerifyEnum(FDE_CSSProperty prop, FDE_CSSPropertyValue val) {
+ ASSERT(decl_);
+
+ bool important;
+ CFDE_CSSValue* v = decl_->GetProperty(prop, important);
+ CFDE_CSSPrimitiveValue* pval = static_cast<CFDE_CSSPrimitiveValue*>(v);
+ EXPECT_EQ(FDE_CSSPrimitiveType::Enum, pval->GetPrimitiveType());
+ EXPECT_EQ(val, pval->GetEnum());
+ }
+
+ std::unique_ptr<CFDE_CSSStyleSheet> sheet_;
+ CFDE_CSSDeclaration* decl_;
+};
+
+TEST_F(CFDE_CSSStyleSheetTest, ParseBorder) {
+ LoadAndVerifyDecl(L"a { border: 5px; }", 4);
+ VerifyFloat(FDE_CSSProperty::BorderLeftWidth, 5.0,
+ FDE_CSSPrimitiveType::Pixels);
+ VerifyFloat(FDE_CSSProperty::BorderRightWidth, 5.0,
+ FDE_CSSPrimitiveType::Pixels);
+ VerifyFloat(FDE_CSSProperty::BorderTopWidth, 5.0,
+ FDE_CSSPrimitiveType::Pixels);
+ VerifyFloat(FDE_CSSProperty::BorderBottomWidth, 5.0,
+ FDE_CSSPrimitiveType::Pixels);
+}
+
+TEST_F(CFDE_CSSStyleSheetTest, ParseBorderFull) {
+ LoadAndVerifyDecl(L"a { border: 5px solid red; }", 4);
+ VerifyFloat(FDE_CSSProperty::BorderLeftWidth, 5.0,
+ FDE_CSSPrimitiveType::Pixels);
+ VerifyFloat(FDE_CSSProperty::BorderRightWidth, 5.0,
+ FDE_CSSPrimitiveType::Pixels);
+ VerifyFloat(FDE_CSSProperty::BorderTopWidth, 5.0,
+ FDE_CSSPrimitiveType::Pixels);
+ VerifyFloat(FDE_CSSProperty::BorderBottomWidth, 5.0,
+ FDE_CSSPrimitiveType::Pixels);
+}
+
+TEST_F(CFDE_CSSStyleSheetTest, ParseBorderLeft) {
+ LoadAndVerifyDecl(L"a { border-left: 2.5pc; }", 1);
+ VerifyFloat(FDE_CSSProperty::BorderLeftWidth, 2.5,
+ FDE_CSSPrimitiveType::Picas);
+}
+
+TEST_F(CFDE_CSSStyleSheetTest, ParseBorderLeftThick) {
+ LoadAndVerifyDecl(L"a { border-left: thick; }", 1);
+ VerifyEnum(FDE_CSSProperty::BorderLeftWidth, FDE_CSSPropertyValue::Thick);
+}
+
+TEST_F(CFDE_CSSStyleSheetTest, ParseBorderRight) {
+ LoadAndVerifyDecl(L"a { border-right: 2.5pc; }", 1);
+ VerifyFloat(FDE_CSSProperty::BorderRightWidth, 2.5,
+ FDE_CSSPrimitiveType::Picas);
+}
+
+TEST_F(CFDE_CSSStyleSheetTest, ParseBorderTop) {
+ LoadAndVerifyDecl(L"a { border-top: 2.5pc; }", 1);
+ VerifyFloat(FDE_CSSProperty::BorderTopWidth, 2.5,
+ FDE_CSSPrimitiveType::Picas);
+}
+
+TEST_F(CFDE_CSSStyleSheetTest, ParseBorderBottom) {
+ LoadAndVerifyDecl(L"a { border-bottom: 2.5pc; }", 1);
+ VerifyFloat(FDE_CSSProperty::BorderBottomWidth, 2.5,
+ FDE_CSSPrimitiveType::Picas);
+}
diff --git a/xfa/fde/css/fde_cssdeclaration.cpp b/xfa/fde/css/fde_cssdeclaration.cpp
index cf5e73e..1d9e855 100644
--- a/xfa/fde/css/fde_cssdeclaration.cpp
+++ b/xfa/fde/css/fde_cssdeclaration.cpp
@@ -642,3 +642,10 @@
}
return true;
}
+
+size_t CFDE_CSSDeclaration::PropertyCountForTesting() const {
+ size_t ret = 0;
+ for (const FDE_CSSPropertyHolder* p = m_pFirstProperty; p; p = p->pNext)
+ ret++;
+ return ret;
+}
diff --git a/xfa/fde/css/fde_cssdeclaration.h b/xfa/fde/css/fde_cssdeclaration.h
index 2dbd7a0..cef98c7 100644
--- a/xfa/fde/css/fde_cssdeclaration.h
+++ b/xfa/fde/css/fde_cssdeclaration.h
@@ -58,6 +58,8 @@
const FX_WCHAR* pszValue,
int32_t iValueLen);
+ size_t PropertyCountForTesting() const;
+
protected:
bool ParseFontProperty(const FDE_CSSPropertyArgs* pArgs,
const FX_WCHAR* pszValue,
To view, visit this change. To unsubscribe, visit settings.
dsinclair (Gerrit)
dsinclair uploaded patch set #3 to this change.
Start CSS parser unit tests Start adding unit tests for the css parser. Fixup memory leaks that are exposed by the tests. Change-Id: Id863d9cd5f13ab82626bc7b945de925253c88d43 --- M BUILD.gn A xfa/fde/css/cfde_cssstylesheet_unittest.cpp M xfa/fde/css/cfde_cssvalue.cpp M xfa/fde/css/cfde_cssvalue.h M xfa/fde/css/fde_cssdatatable.cpp M xfa/fde/css/fde_cssdatatable.h M xfa/fde/css/fde_cssdeclaration.cpp M xfa/fde/css/fde_cssdeclaration.h M xfa/fde/css/fde_cssstyleselector.cpp M xfa/fde/css/fde_cssstylesheet.cpp M xfa/fde/css/fde_cssstylesheet.h 11 files changed, 505 insertions(+), 377 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
dsinclair (Gerrit)
dsinclair posted comments on this change.
Patch set 3:Commit-Queue +1
PTAL.
Nicolás Peña (Gerrit)
Nicolás Peña posted comments on this change.
Patch set 3:
(14 comments)
File xfa/fde/css/cfde_cssstylesheet_unittest.cpp:
I imagine this 1 is because all your tests so far have only 1 rule
File xfa/fde/css/cfde_cssvalue.h:
Patch Set #3, Line 13:
publicCan we have the constructor declared in the header?
Patch Set #3, Line 18:
void Retain() { ++m_nCount; }Is it possible to use CFX_RetainPtr or something like this, to avoid adding this to more classes?
File xfa/fde/css/fde_cssdeclaration.h:
Patch Set #3, Line 40:
const_iteratorMaybe use |const_prop_iterator| instead? Or capitalize the c.
File xfa/fde/css/fde_cssdeclaration.cpp:
Followup: this always returns true
Patch Set #3, Line 231:
CFDE_CSSPrimitiveValueIf you are several doing several of CFX_RetainPtr<Class>(new Class()), maybe instead add a Create method in Class?
Patch Set #3, Line 639:
PropertyCountForTestingThis, or make the test class a friend. Up to you
File xfa/fde/css/fde_cssstyleselector.cpp:
Patch Set #3, Line 291:
bImportantFollowup: Pass bImportant by pointer instead of non-const ref
File xfa/fde/css/fde_cssstylesheet.cpp:
This variable still exists, but you erased this line. I'm a bit confused
Nit: no {}'s
Patch Set #3, Line 235:
pdfium::WrapUniquepush_back(std::move(rule)) does not work? Otherwise, |rule| is unneeded variable, right?
Patch Set #3, Line 287:
rule.releaseditto
Patch Set #3, Line 408:
pdfium::WrapUnique<CFDE_CSSSelector>(m_pNext.release())return std::move(m_pNext);
Nit: not {}'s
To view, visit this change. To unsubscribe, visit settings.
dsinclair (Gerrit)
dsinclair posted comments on this change.
File xfa/fde/css/cfde_cssstylesheet_unittest.cpp:
Patch Set #3, Line 27:
EXPECT_EQ(1, sheet_->CountRules());
I imagine this 1 is because all your tests so far have only 1 rule
Yea, will probably have to change in the future, but works for now.
File xfa/fde/css/cfde_cssvalue.h:
Patch Set #3, Line 13:
public:Can we have the constructor declared in the header?
See line 25.
Patch Set #3, Line 18:
void Retain() { ++m_nCount; }Is it possible to use CFX_RetainPtr or something like this, to avoid adding
These are used by CFX_RetainPtr.
File xfa/fde/css/fde_cssdeclaration.h:
Patch Set #3, Line 40:
using const_iterator =
Maybe use |const_prop_iterator| instead? Or capitalize the c.
Done
File xfa/fde/css/fde_cssdeclaration.cpp:
Patch Set #3, Line 201:
return true;
Followup: this always returns true
Done
Patch Set #3, Line 231:
return CFX_RetainPtr<CFDE_CSSPrimitiveValue>(
If you are several doing several of CFX_RetainPtr<Class>(new Class()), mayb
Possibly, will leave for a potential followup.
Patch Set #3, Line 639:
size_t CFDE_CSSDeclaration::PropertyCountForTesting() const {
This, or make the test class a friend. Up to you
I like this, I'm not a fan of friends, heh.
File xfa/fde/css/fde_cssstyleselector.cpp:
Patch Set #3, Line 291:
ppDeclArray[i]->GetProperty(FDE_CSSProperty::FontSize, bImportant);
Followup: Pass bImportant by pointer instead of non-const ref
This variable still exists, but you erased this line. I'm a bit confused
Yup, I messed it up, it's actually fixed in the next CL where I add a test for this. The var gets removed and the size is used directly.
Patch Set #3, Line 28:
if (IsCSSChar(wch) || wch == ':') {Nit: no {}'s
Done
Patch Set #3, Line 235:
pdfium::WrapUnique<CFDE_CSSRule>(rule.release()));
push_back(std::move(rule)) does not work? Otherwise, |rule| is unneeded var
rule is needed so we can assign rule.get() to pStyleRule. Not sure why I didn't use std::move ....
Patch Set #3, Line 287:
pdfium::WrapUnique<CFDE_CSSRule>(rule.release()));ditto
Done
Patch Set #3, Line 408:
return pdfium::WrapUnique<CFDE_CSSSelector>(m_pNext.release());return std::move(m_pNext);
Done
Patch Set #3, Line 477:
if (iNameLen == 0) {Nit: not {}'s
Done
To view, visit this change. To unsubscribe, visit settings.
dsinclair (Gerrit)
dsinclair posted comments on this change.
Patch set 5:Commit-Queue +1
I'd prefer to keep the tests and RetainPtr changes together as the tests are what tell me the RetainPtr code is covering all the leaks for those given tests.
(1 comment)
File xfa/fde/css/cfde_cssvalue.h:
Patch Set #4, Line 12:
class CFDE_CSSValue : public CFX_Retainable {Done.
I also fixed to use pdfium::MakeRetain where needed.
To view, visit change 2180. To unsubscribe, visit settings.
dsinclair (Gerrit)
dsinclair posted comments on this change.
Patch set 6:
(3 comments)
File xfa/fde/css/fde_cssdatatable.h:
Patch Set #4, Line 44:
std::unique_ptr<CFDE_CSSFunction> m_pFunction;Will create a followup to split the class.
File xfa/fde/css/fde_cssdatatable.cpp:
Patch Set #4, Line 541:
: CFDE_CSSValue(FDE_CSSVALUETYPE_List), m_ppList(std::move(list)) {}Dropped the const, skipped the && as per conversation. (Making this && causes pdfium::MakeRetain to no longer work correctly.)
File xfa/fde/css/fde_cssdeclaration.cpp:
Patch Set #4, Line 54:
return pdfium::MakeRetain<CFDE_CSSPrimitiveValue>(eUnit, fValue);Done
To view, visit change 2180. To unsubscribe, visit settings.