Bug 66096 - Add a way to extend DOM objects in garden-o-matic.
Summary: Add a way to extend DOM objects in garden-o-matic.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 64188 66144
  Show dependency treegraph
 
Reported: 2011-08-11 14:01 PDT by Dimitri Glazkov (Google)
Modified: 2011-08-12 10:34 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.29 KB, patch)
2011-08-11 14:02 PDT, Dimitri Glazkov (Google)
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-08-11 14:01:27 PDT
Add a way to extend DOM objects in garden-o-matic.
Comment 1 Dimitri Glazkov (Google) 2011-08-11 14:02:27 PDT
Created attachment 103668 [details]
Patch
Comment 2 Sam Weinig 2011-08-11 15:35:46 PDT
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 3 Adam Barth 2011-08-11 15:42:40 PDT
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 4 Adam Barth 2011-08-11 15:43:35 PDT
Comment on attachment 103668 [details]
Patch

This looks fine, assuming ther eare no licensing problems and you address Sam and my comments above.
Comment 5 Dimitri Glazkov (Google) 2011-08-11 20:56:48 PDT
(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 6 Dimitri Glazkov (Google) 2011-08-11 21:01:25 PDT
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 7 Adam Barth 2011-08-11 21:22:17 PDT
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".
Comment 8 Erik Arvidsson 2011-08-12 10:27:11 PDT
(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
Comment 9 Dimitri Glazkov (Google) 2011-08-12 10:30:37 PDT
(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.
Comment 10 Dimitri Glazkov (Google) 2011-08-12 10:34:23 PDT
Committed r92972: <http://trac.webkit.org/changeset/92972>