Bug 110131 - Add event dispatch class for the new calendar picker
Summary: Add event dispatch class for the new calendar picker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks: 109439 110132
  Show dependency treegraph
 
Reported: 2013-02-18 09:16 PST by Keishi Hattori
Modified: 2013-02-21 02:18 PST (History)
2 users (show)

See Also:


Attachments
Patch (3.04 KB, patch)
2013-02-18 09:26 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (3.13 KB, patch)
2013-02-21 01:54 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.