Issue 2658 - Got rid of try..catch for filter validation

lib/filterValidation.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /** @module filterValidation */
 
 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.
  *
  * @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 strictly speaking we return an
object containing an error. But fair enough.

+ * @return {ParsedFilters}
  */
 exports.parseFilters = function(text, ignoreHeaders)
 {
   let lines = text.split("\n");
   let filters = [];
 
   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 we return above.

       filters.push(filter);
   }
 
-  return filters;
+  return {filters: filters};
 };