Bug 145106 – Organize event handlers a bit

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Organize event handlers a bit

[Sam Weinig]

Created attachment 253285 [details]
Patch

[Build Bot]

Comment on attachment 253285 [details]
Patch

Attachment 253285 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5902866543804416

New failing tests:
platform/mac/fast/text/font-weights.html

[Build Bot]

Created attachment 253288 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Darin Adler]

Comment on attachment 253285 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253285&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2304
> +                if ($attribute->signature->extendedAttributes->{"WindowEventHandler"} && $interfaceName ne "DOMWindow") {

It’s a real shame to hard-code interface names like this. I was proud that I avoided that last time. Is there some other way to do this?

> Source/WebCore/dom/GlobalEventHandlers.idl:35
> +    // [NotEnumerable] attribute EventHandler oncancel;

I think you need a comment to explain the meaning of the commented out handlers. I believe the concept you have in mind is that they are from some specification that we want to implement and so should be implemented some day, but I think it’s worth a comment at the top explaining that.

> Source/WebCore/dom/GlobalEventHandlers.idl:96
> +    // Non-standard additions

I think “non-standard” is possible too strong for these. Maybe “not yet standard” would be more accurate, unless we think none of these will ever be on the standard track.

> Source/WebCore/page/DOMWindow.idl:249
>  DOMWindow implements WindowTimers;
>  DOMWindow implements WindowBase64;
> +DOMWindow implements WindowEventHandlers;
> +DOMWindow implements GlobalEventHandlers;

Maybe alphabetical order?

> Source/WebCore/page/WindowEventHandlers.idl:31
> +    // [NotEnumerable, WindowEventHandler] attribute EventHandler onafterprint;.

Same comment about comments.

> Source/WebCore/page/WindowEventHandlers.idl:46
> +    // Non-standard additions

Same comment about terminology.

[Sam Weinig]

(In reply to comment #4)
> Comment on attachment 253285 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253285&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2304
> > +                if ($attribute->signature->extendedAttributes->{"WindowEventHandler"} && $interfaceName ne "DOMWindow") {
> 
> It’s a real shame to hard-code interface names like this. I was proud that I
> avoided that last time. Is there some other way to do this?

There actually is.  I can add an overload of windowEventHandlerAttribute/set WindowEventHandlerAttribute that does the right thing. I'll do that.

> 
> > Source/WebCore/dom/GlobalEventHandlers.idl:35
> > +    // [NotEnumerable] attribute EventHandler oncancel;
> 
> I think you need a comment to explain the meaning of the commented out
> handlers. I believe the concept you have in mind is that they are from some
> specification that we want to implement and so should be implemented some
> day, but I think it’s worth a comment at the top explaining that.

Will do.

> 
> > Source/WebCore/dom/GlobalEventHandlers.idl:96
> > +    // Non-standard additions
> 
> I think “non-standard” is possible too strong for these. Maybe “not yet
> standard” would be more accurate, unless we think none of these will ever be
> on the standard track.

Will do.

> 
> > Source/WebCore/page/DOMWindow.idl:249
> >  DOMWindow implements WindowTimers;
> >  DOMWindow implements WindowBase64;
> > +DOMWindow implements WindowEventHandlers;
> > +DOMWindow implements GlobalEventHandlers;
> 
> Maybe alphabetical order?

Will do.

[Sam Weinig]

Committed r184616: <http://trac.webkit.org/changeset/184616>