Bug 145106 - Organize event handlers a bit
Summary: Organize event handlers a bit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-17 10:37 PDT by Sam Weinig
Modified: 2015-05-19 22:41 PDT (History)
3 users (show)

See Also:


Attachments
Patch (59.50 KB, patch)
2015-05-17 10:52 PDT, Sam Weinig
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (661.13 KB, application/zip)
2015-05-17 11:31 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2015-05-17 10:37:02 PDT
Organize event handlers a bit
Comment 1 Sam Weinig 2015-05-17 10:52:46 PDT
Created attachment 253285 [details]
Patch
Comment 2 Build Bot 2015-05-17 11:31:25 PDT
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
Comment 3 Build Bot 2015-05-17 11:31:28 PDT
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
Comment 4 Darin Adler 2015-05-19 08:43:44 PDT
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.
Comment 5 Sam Weinig 2015-05-19 14:12:20 PDT
(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.
Comment 6 Sam Weinig 2015-05-19 22:41:19 PDT
Committed r184616: <http://trac.webkit.org/changeset/184616>