Summary: | Add a way to extend DOM objects in garden-o-matic. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||
Component: | Tools / Tests | Assignee: | Dimitri Glazkov (Google) <dglazkov> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, arv | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 64188, 66144 | ||||||
Attachments: |
|
Description
Dimitri Glazkov (Google)
2011-08-11 14:01:27 PDT
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> |