Bug 66096 – Add a way to extend DOM objects in garden-o-matic.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dimitri Glazkov (Google)]

Add a way to extend DOM objects in garden-o-matic.

[Dimitri Glazkov (Google)]

Created attachment 103668 [details]
Patch

[Sam Weinig]

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?

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 103668 [details]
Patch

This looks fine, assuming ther eare no licensing problems and you address Sam and my comments above.

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

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.

[Adam Barth]

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".

[Erik Arvidsson]

(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

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

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