Bug 66096

Created attachment 103668 [details] Patch

Comment on attachment 103668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103668&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:222 > +// arv r0x0r 4eva. I am not sure future readers of this code will understand this comment. > Tools/ChangeLog:6 > + This is loosely based on http://ajaxian.com/archives/web-ninja-interview-erik-arvidsson. Did you copy it? Does it have a license?

Comment on attachment 103668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103668&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:232 > + function f() { > + var el = typeof base == 'string' ? document.createElement(base) : base.call(this); > + f.prototype.__proto__ = el.__proto__; > + el.__proto__ = f.prototype; > + el.init && el.init(); > + return el; > + } f and el are pretty lame variable names. Also, this construction leaks the implementation detail "f" in the API. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base_unittests.js:223 > + init: function() > + { We've been putting the { on the same line in these sorts of declarations. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base_unittests.js:226 > + method: function(msg) msg => message ? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base_unittests.js:241 > + ok(document.body.lastChild.innerHTML == "awesome"); > + ok(document.body.lastChild.method() == 42); These should use equals > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base_unittests.js:247 > + ok(document.body.lastChild.innerHTML == "awesome"); > + ok(document.body.lastChild.method() == 42); > + ok(document.body.lastChild.className == "like"); Same here.

Comment on attachment 103668 [details] Patch This looks fine, assuming ther eare no licensing problems and you address Sam and my comments above.

(In reply to comment #2) > (From update of attachment 103668 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103668&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:222 > > +// arv r0x0r 4eva. > > I am not sure future readers of this code will understand this comment. I'll make it more readable :) > > > Tools/ChangeLog:6 > > + This is loosely based on http://ajaxian.com/archives/web-ninja-interview-erik-arvidsson. > > Did you copy it? Does it have a license? I made wild extrapolations of the initial idea. I hope Arv blesses this himself.

Comment on attachment 103668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103668&action=review >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:232 >> + } > > f and el are pretty lame variable names. Also, this construction leaks the implementation detail "f" in the API. I'll rename. Though I don't see a way of not leaking the f. That's the whole point of the trick. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base_unittests.js:223 >> + { > > We've been putting the { on the same line in these sorts of declarations. Really? That seems to be different from the WebKit style. I'll change.

Comment on attachment 103668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103668&action=review >>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:232 >>> + } >> >> f and el are pretty lame variable names. Also, this construction leaks the implementation detail "f" in the API. > > I'll rename. Though I don't see a way of not leaking the f. That's the whole point of the trick. Just change: function f() { to var f = function () { Just like magic, we no longer leak the name "f".

(In reply to comment #2) > (From update of attachment 103668 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103668&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:222 > > +// arv r0x0r 4eva. > > I am not sure future readers of this code will understand this comment. > > > Tools/ChangeLog:6 > > + This is loosely based on http://ajaxian.com/archives/web-ninja-interview-erik-arvidsson. > > Did you copy it? Does it have a license? The original code was written for Chromium so it has a BSD-style license: http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/shared/js/cr/ui.js&type=cs

(In reply to comment #8) > (In reply to comment #2) > > (From update of attachment 103668 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=103668&action=review > > > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:222 > > > +// arv r0x0r 4eva. > > > > I am not sure future readers of this code will understand this comment. > > > > > Tools/ChangeLog:6 > > > + This is loosely based on http://ajaxian.com/archives/web-ninja-interview-erik-arvidsson. > > > > Did you copy it? Does it have a license? > > The original code was written for Chromium so it has a BSD-style license: > > http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/shared/js/cr/ui.js&type=cs I'll update the link in ChangeLog and mention this in the comment.

Committed r92972: <http://trac.webkit.org/changeset/92972>