[webkit-reviews] review denied: [Bug 8462] Add WebKit detection scripts to WebKit site for webdevelopers : [Attachment 7812] Patch v2

[bugzilla-request-daemon at opendarwin.org]

Geoffrey Garen <ggaren at apple.com> has denied Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 8462: Add WebKit detection scripts to WebKit site for webdevelopers
http://bugzilla.opendarwin.org/show_bug.cgi?id=8462

Attachment 7812: Patch v2
http://bugzilla.opendarwin.org/attachment.cgi?id=7812&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
I don't understand the lines of this form:

+  if (matches) {
+    var webkit_version = parse_webkit_version(matches[1]);    
+  } 
+  return {major: webkit_version['major'], minor: webkit_version['minor'],
is_nightly: webkit_version['is_nightly']};

If matches is false webkit_version will be undefined, so I think this code will
throw an exception in non-webkit browsers. If matches is true, webkit_version
will be defined as a local variable that immediately goes out of scope after
the if statement, which is also not what we want.

I don't understand the reason for differentiating parse_webkit_version and
get_webkit_version inside webkit_version.js.

We'd really like this to be a simple bit of code that web developers can lift
without thinking, so some of the fancy stuff here, like the XL script, might be
counter-productive. You could fix that by having the site highlight the simple
version, but also point to how to do other, more advanced detection.

It would be nice to have a simple "how to detect safari/webkit" in addition to
"how to detect safari/webkit's version number."

r- for the first comment.