Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Delta Between Two Patch Sets: compiled/Filter.cpp

Issue 29333474: Issue 4125 - [emscripten] Convert filter classes to C++ (Closed)
Left Patch Set: Optimized hash lookup performance a bit Created Feb. 8, 2016, 7:11 p.m.
Right Patch Set: Addressed comments from Patch Set 28 Created March 21, 2017, 10:04 a.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « compiled/Filter.h ('k') | compiled/InvalidFilter.h » ('j') | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 #include "Filter.h" 1 #include "Filter.h"
2 #include "CommentFilter.h" 2 #include "CommentFilter.h"
3 #include "InvalidFilter.h" 3 #include "InvalidFilter.h"
4 #include "RegExpFilter.h" 4 #include "RegExpFilter.h"
5 #include "BlockingFilter.h"
5 #include "WhitelistFilter.h" 6 #include "WhitelistFilter.h"
6 #include "ElemHideBase.h" 7 #include "ElemHideBase.h"
7 #include "ElemHideFilter.h" 8 #include "ElemHideFilter.h"
8 #include "ElemHideException.h" 9 #include "ElemHideException.h"
9 #include "CSSPropertyFilter.h" 10 #include "CSSPropertyFilter.h"
10 #include "StringMap.h" 11 #include "StringMap.h"
11 12
12 namespace 13 namespace
13 { 14 {
14 StringMap<Filter*> knownFilters(8192); 15 StringMap<Filter*> knownFilters(8192);
(...skipping 32 matching lines...) Expand 10 before | Expand all | Expand 10 after
47 // Remove trailing spaces 48 // Remove trailing spaces
48 for (; end > 0; end--) 49 for (; end > 0; end--)
49 if (text[end - 1] != ' ') 50 if (text[end - 1] != ' ')
50 break; 51 break;
51 52
52 // Set new string boundaries 53 // Set new string boundaries
53 text.reset(text, start, end - start); 54 text.reset(text, start, end - start);
54 } 55 }
55 } 56 }
56 57
57 Filter::Filter(const String& text) 58 Filter::Filter(Type type, const String& text)
58 : ref_counted(), mText(text) 59 : mType(type), mText(text)
sergei 2016/02/17 12:54:37 `ref_counted()` is not necessary here.
Wladimir Palant 2016/02/18 16:06:41 I preferred to spell this out explicitly neverthel
59 { 60 {
60 annotate_address(this, "Filter"); 61 annotate_address(this, "Filter");
61 } 62 }
62 63
63 Filter::~Filter() 64 Filter::~Filter()
64 { 65 {
65 // TODO: This should be removing from knownFilters 66 knownFilters.erase(mText);
66 } 67 }
67 68
68 OwnedString Filter::Serialize() const 69 OwnedString Filter::Serialize() const
69 { 70 {
70 OwnedString result(u"[Filter]\ntext="_str); 71 OwnedString result(u"[Filter]\ntext="_str);
71 result.append(mText); 72 result.append(mText);
72 result.append(u'\n'); 73 result.append(u'\n');
73 return std::move(result); 74 return result;
sergei 2016/02/17 12:54:36 std::move is not necessary here. BTW, in C++, here
Wladimir Palant 2016/02/18 16:06:42 Done.
74 } 75 }
75 76
76 Filter* Filter::FromText(DependentString& text) 77 Filter* Filter::FromText(DependentString& text)
77 { 78 {
78 NormalizeWhitespace(text); 79 NormalizeWhitespace(text);
sergei 2016/02/17 12:54:33 For me personally, the approach here is really con
Wladimir Palant 2016/02/18 16:06:43 I've been through multiple iterations here, and th
sergei 2016/02/22 12:45:36 Clear. BTW, can we avoid copying of strings here
Wladimir Palant 2016/02/23 12:37:20 Reading that file will eventually move into C++ so
sergei 2016/02/23 15:07:24 Acknowledged.
79 if (text.empty()) 80 if (text.empty())
80 return nullptr; 81 return nullptr;
81 82
82 // Parsing also normalizes the filter text, so it has to be done before the 83 // Parsing also normalizes the filter text, so it has to be done before the
83 // lookup in knownFilters. 84 // lookup in knownFilters.
84 union 85 union
85 { 86 {
86 RegExpFilterData regexp; 87 RegExpFilterData regexp;
87 ElemHideData elemhide; 88 ElemHideData elemhide;
88 } data; 89 } data;
89 OwnedString error; 90 DependentString error;
90 91
91 Filter::Type type = CommentFilter::Parse(text); 92 Filter::Type type = CommentFilter::Parse(text);
92 if (type == Filter::Type::UNKNOWN) 93 if (type == Filter::Type::UNKNOWN)
93 type = ElemHideBase::Parse(text, data.elemhide); 94 type = ElemHideBase::Parse(text, data.elemhide);
94 if (type == Filter::Type::UNKNOWN) 95 if (type == Filter::Type::UNKNOWN)
95 type = RegExpFilter::Parse(text, error, data.regexp); 96 type = RegExpFilter::Parse(text, error, data.regexp);
96 97
97 FilterPtr filter(GetKnownFilter(text)); 98 auto knownFilter = knownFilters.find(text);
98 if (filter) 99 if (knownFilter)
99 return filter; 100 {
101 knownFilter->second->AddRef();
102 return knownFilter->second;
103 }
100 104
105 FilterPtr filter;
101 switch (type) 106 switch (type)
102 { 107 {
103 case Filter::Type::COMMENT: 108 case Filter::Type::COMMENT:
104 filter = new CommentFilter(text); 109 filter = new CommentFilter(text);
105 break; 110 break;
106 case Filter::Type::INVALID: 111 case Filter::Type::INVALID:
107 filter = new InvalidFilter(text, error); 112 filter = new InvalidFilter(text, error);
108 break; 113 break;
109 case Filter::Type::BLOCKING: 114 case Filter::Type::BLOCKING:
110 filter = new RegExpFilter(text, data.regexp); 115 filter = new BlockingFilter(text, data.regexp);
111 break; 116 break;
112 case Filter::Type::WHITELIST: 117 case Filter::Type::WHITELIST:
113 filter = new WhitelistFilter(text, data.regexp); 118 filter = new WhitelistFilter(text, data.regexp);
114 break; 119 break;
115 case Filter::Type::ELEMHIDE: 120 case Filter::Type::ELEMHIDE:
116 filter = new ElemHideFilter(text, data.elemhide); 121 filter = new ElemHideFilter(text, data.elemhide);
117 break; 122 break;
118 case Filter::Type::ELEMHIDEEXCEPTION: 123 case Filter::Type::ELEMHIDEEXCEPTION:
119 filter = new ElemHideException(text, data.elemhide); 124 filter = new ElemHideException(text, data.elemhide);
120 break; 125 break;
121 case Filter::Type::CSSPROPERTY: 126 case Filter::Type::CSSPROPERTY:
122 filter = new CSSPropertyFilter(text, data.elemhide); 127 filter = new CSSPropertyFilter(text, data.elemhide);
123 if (reinterpret_cast<CSSPropertyFilter*>(filter.get())->IsGeneric()) 128 if (static_cast<CSSPropertyFilter*>(filter.get())->IsGeneric())
sergei 2016/02/17 12:54:34 it's better to use `static_cast` here.
Wladimir Palant 2016/02/18 16:06:44 Done.
124 filter = new InvalidFilter(text, 129 filter = new InvalidFilter(text, u"filter_cssproperty_nodomain"_str);
125 u"No active domain specified for CSS property filter"_str);
126 break; 130 break;
127 default: 131 default:
128 // This should never happen but just in case 132 // This should never happen but just in case
129 return nullptr; 133 return nullptr;
130 } 134 }
131 135
136 // This is a hack: we looked up the entry using text but create it using
137 // filter->mText. This works because both are equal at this point. However,
138 // text refers to a temporary buffer which will go away.
132 enter_context("Adding to known filters"); 139 enter_context("Adding to known filters");
133 knownFilters[filter->mText] = filter.get(); 140 knownFilter.assign(filter->mText, filter.get());
134 exit_context(); 141 exit_context();
135 142
136 // TODO: We intentionally leak the filter here - currently it won't be used 143 return filter.release();
137 // for anything and would be deleted immediately.
138 filter->AddRef();
139
140 return filter;
141 } 144 }
142
143 Filter* Filter::GetKnownFilter(const String& text)
144 {
145 auto it = knownFilters.find(text);
146 if (it != knownFilters.end())
147 return it->second;
148 else
149 return nullptr;
150 }
LEFTRIGHT

Powered by Google App Engine
This is Rietveld