Bug 110131

Summary: Add event dispatch class for the new calendar picker
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 109439, 110132    
Attachments:
Description Flags
Patch
none
Patch none

Description Keishi Hattori 2013-02-18 09:16:13 PST
We need to add an event dispatch class that will be used in the new calendar picker
Comment 1 Keishi Hattori 2013-02-18 09:26:44 PST
Created attachment 188904 [details]
Patch
Comment 2 Build Bot 2013-02-19 15:42:22 PST
Comment on attachment 188904 [details]
Patch

Attachment 188904 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16637067
Comment 3 Kent Tamura 2013-02-19 23:25:06 PST
Comment on attachment 188904 [details]
Patch

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

> Source/WebCore/Resources/pagepopups/calendarPicker.js:690
> +    if (!this._callbacks)
> +        return;
> +    var callbacksForType = this._callbacks[type];
> +    if (!callbacksForType)
> +        return;

The function does nothing in a case that 'callback' is not registered.  Should we warn in such case, or should we accept such call sites?

> Source/WebCore/Resources/pagepopups/calendarPicker.js:691
> +    callbacksForType.splice(callbacksForType.indexOf(callback), 1);

What should happen if 'callback' is not a member of 'callbacksForType'?
Comment 4 Keishi Hattori 2013-02-20 00:16:19 PST
(In reply to comment #3)
> (From update of attachment 188904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188904&action=review
> 
> > Source/WebCore/Resources/pagepopups/calendarPicker.js:690
> > +    if (!this._callbacks)
> > +        return;
> > +    var callbacksForType = this._callbacks[type];
> > +    if (!callbacksForType)
> > +        return;
> 
> The function does nothing in a case that 'callback' is not registered.  Should we warn in such case, or should we accept such call sites?
> 
> > Source/WebCore/Resources/pagepopups/calendarPicker.js:691
> > +    callbacksForType.splice(callbacksForType.indexOf(callback), 1);
> 
> What should happen if 'callback' is not a member of 'callbacksForType'?

Like Element.removeEventListener, if the callback is not registered there will be no effect.

I kind of feel that this is better because of its simplicity. We usually don't care if the callback exists or not and we just want it gone. For example when calling CalendarPicker.cleanup() you don't know which callbacks have been set (it might be in the middle of an animation) but you know you want to remove them.
Comment 5 Kent Tamura 2013-02-20 03:51:38 PST
Comment on attachment 188904 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +
> +        No new tests. Code is not yet used.

Please mention that this patch is a part of Bug 109439.
Comment 6 Keishi Hattori 2013-02-21 01:54:57 PST
Created attachment 189477 [details]
Patch
Comment 7 WebKit Review Bot 2013-02-21 02:18:16 PST
Comment on attachment 189477 [details]
Patch

Clearing flags on attachment: 189477

Committed r143576: <http://trac.webkit.org/changeset/143576>
Comment 8 WebKit Review Bot 2013-02-21 02:18:20 PST
All reviewed patches have been landed.  Closing bug.