Issue 6923 - Only parse metadata from special comments at the top of the file

lib/synchronizer.js

Index: lib/synchronizer.js
===================================================================
--- a/lib/synchronizer.js
+++ b/lib/synchronizer.js
@@ -160,16 +160,18 @@
       version: null,
       expires: null
     };
-    for (let i = 0; i < lines.length; i++)
+    for (let i = 1; i < lines.length; i++)
     {
-      let match = /^\s*!\s*(\w+)\s*:\s*(.*)/.exec(lines[i]);
-      if (match)
+      let match = /^\s*!\s*(?:(\w+)\s*:\s*(.*)|\S)/.exec(lines[i]);

[Manish Jethani, 2018/09/05 18:10:08]

How about this one: `/^\s*!\s*([^\s:]*)\s*(:)?\s*(.*)/`

It seems simpler to me but maybe it performs worse, I'd have to check. Anyway,
so with this we can check for `match[1]` being truthy.

[Sebastian Noack, 2018/09/06 00:56:03]

Done. Not sure if it's any easier to read through.
Nope, we have to check for match[2] instead.

[Manish Jethani, 2018/09/06 05:53:53]

I stand corrected, I meant `match[2]`

A couple of things to note with this regular expression:

 1.  Empty keys are valid, but of course they would get ignored in the code that
follows
 2.  Keys can now contain non-ASCII characters; if we stick with this, perhaps
we should document it; values could already contain non-ASCII characters, there
should be no reason to disallow them from the keys
 3.  This will continue even after empty comments; if you disagree, we need to
update it (I think it's fine to continue parsing after empty comments but you're
aware of the full context of this issue)

[Sebastian Noack, 2018/09/06 14:31:36]

This is just a stop gap, what is eventually considered a valid key is defined by
the contents of the params object. So whether the regexp allows non-ascii or not
doesn't matter, as long as we keep looking for more metadata after an invalid
key (regardless of whether it contained non-ascii or whitespaces, or simply
isn't given in the params object).
Opps, I missed that. Went back to my regular expression now.

As I said, look at EasyList for example, the metadata follow some non-metadata
comments separated by an empty comment. That is why I want to stop looking for
more metadata after an empty comment.

[Manish Jethani, 2018/09/10 07:24:49]

Acknowledged.

+      if (!match)
+        break;
+
+      if (match[1] != undefined)

[Manish Jethani, 2018/09/05 18:10:08]

We never compare with `undefined` in core. `undefined` is a global variable
(unlike `null`, which is a language keyword) and must be looked up in each
scope. It is slow. Normally we do `typeof foo != "undefined"` all over the
place.

[Sebastian Noack, 2018/09/06 00:56:03]

Actually, I cannot redefine undefined, neither through assignment nor through
Object.defineProperty(). But still it's different from null (trying to assign to
it is a syntax error), while undefined seems to be a read-only property on the
global object. Technically both could be optimized similarly, but not idea if
they are.

Just in case, also note that null == undefined, which might come handy, if this
should be a concern, and you don't want to treat actual null (if the value can
be) differently.

[Manish Jethani, 2018/09/06 05:53:53]

You can just shadow `undefined` in the local scope, which you cannot do with
`null`.
Acknowledged.

       {
         let keyword = match[1].toLowerCase();
-        let value = match[2];
-        if (keyword in params)
+        if (params.hasOwnProperty(keyword))
         {
-          params[keyword] = value;
+          params[keyword] = match[2];
           lines.splice(i--, 1);
         }
       }