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

Unified Diff: lib/filterValidation.js

Issue 5655474749833216: Issue 2658 - Got rid of try..catch for filter validation (Closed)
Patch Set: Created June 7, 2015, 2:47 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « background.js ('k') | options.js » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: lib/filterValidation.js
===================================================================
--- a/lib/filterValidation.js
+++ b/lib/filterValidation.js
@@ -19,58 +19,75 @@
let {Filter, InvalidFilter, ElemHideBase} = require("filterClasses");
+function isValidCSSSelector(selector)
+{
+ let style = document.createElement("style");
+ document.documentElement.appendChild(style);
+ let sheet = style.sheet;
+ document.documentElement.removeChild(style);
+
+ try
+ {
+ document.querySelector(selector);
+ sheet.insertRule(selector + "{}", 0);
+ }
+ catch (e)
+ {
+ return false;
+ }
+ return true;
+}
+
+/**
+ * @typedef ParsedFilter
+ * @property {?Filter} [filter] The parsed filter is it is valid. Or null if
kzar 2015/06/08 11:28:04 Nit: Typo "The parsed filter_if_ it is valid"
Sebastian Noack 2015/06/08 12:00:08 Done.
+ * the given string is empty or a filter list header.
+ * @property {string} [error] An error message indicated that the filter cannot
+ * be parsed or contains an invalid CSS selector.
+ */
+
+let parseFilter =
/**
* Parses and validates a filter given by the user.
*
* @param {string} text
- * @param {Boolean} [ignoreHeaders=false] If true, no exception is raised
+ * @param {Boolean} [ignoreHeaders=false] If true, no error is produced
* for filter list headers, instead
- * the function will return null.
- * @return {Filter}
- * @throws Will throw an exception if filter cannot be
- * parsed or contains an invalid CSS selector.
- * @static
+ * {filter: null} is returned.
+ * @return {ParsedFilter}
*/
-function parseFilter(text, ignoreHeaders)
+exports.parseFilter = function(text, ignoreHeaders)
{
+ let filter = null;
text = Filter.normalize(text);
- if (!text)
- return null;
- if (text[0] == "[")
+ if (text)
{
- if (ignoreHeaders)
- return null;
+ if (text[0] != "[")
+ {
+ filter = Filter.fromText(text);
- throw ext.i18n.getMessage("unexpected_filter_list_header");
- }
+ if (filter instanceof InvalidFilter)
+ return {error: filter.reason};
- let filter = Filter.fromText(text);
-
- if (filter instanceof InvalidFilter)
- throw filter.reason;
-
- if (filter instanceof ElemHideBase)
- {
- let style = document.createElement("style");
- document.documentElement.appendChild(style);
- let sheet = style.sheet;
- document.documentElement.removeChild(style);
-
- try
+ if (filter instanceof ElemHideBase && !isValidCSSSelector(filter.selector))
+ return {error: ext.i18n.getMessage("invalid_css_selector", "'" + filter.selector + "'")};
+ }
+ else if (!ignoreHeaders)
{
- document.querySelector(filter.selector);
- sheet.insertRule(filter.selector + "{}", 0);
- }
- catch (error)
- {
- throw ext.i18n.getMessage("invalid_css_selector", "'" + filter.selector + "'");
+ return {error: ext.i18n.getMessage("unexpected_filter_list_header")};
}
}
- return filter;
-}
-exports.parseFilter = parseFilter;
+ return {filter: filter};
+};
+
+/**
+ * @typedef ParsedFilters
+ * @property {Filter[]} [filters] The parsed filters if all of them are valid.
+ * @property {string} [error] An error message indicated that any filter cannot
+ * be parsed or contains an invalid CSS selector.
+ */
/**
* Parses and validates a newline-separated list of filters given by the user.
@@ -78,10 +95,8 @@
* @param {string} text
* @param {Boolean} [ignoreHeaders=false] If true, filter list headers
* will be stripped instead of
- * raising an exception.
- * @return {Filter[]}
- * @throws Will throw an exception if one of the filters cannot
- be parsed or contains an invalid CSS selector.
+ * producing an error.
kzar 2015/06/08 11:28:04 Nit: "returning"?
Sebastian Noack 2015/06/08 12:00:08 I did not say to "return" an error, because strict
+ * @return {ParsedFilters}
*/
exports.parseFilters = function(text, ignoreHeaders)
{
@@ -90,19 +105,14 @@
for (let i = 0; i < lines.length; i++)
{
- let filter;
- try
- {
- filter = parseFilter(lines[i], ignoreHeaders);
- }
- catch (error)
- {
- throw ext.i18n.getMessage("line", (i + 1).toString()) + ": " + error;
- }
+ let {filter, error} = parseFilter(lines[i], ignoreHeaders);
+
+ if (error)
+ return {error: ext.i18n.getMessage("line", (i + 1).toString()) + ": " + error};
if (filter)
kzar 2015/06/08 11:28:04 Nit: Should be `else if (filter)` IMO
Sebastian Noack 2015/06/08 12:00:08 No, it shouldn't. The else would be redundant, as
filters.push(filter);
}
- return filters;
+ return {filters: filters};
};
« no previous file with comments | « background.js ('k') | options.js » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld