Bug 105568 - Implement AutocompleteErrorEvent#reason
Summary: Implement AutocompleteErrorEvent#reason
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dan Beam
URL: http://lists.whatwg.org/htdig.cgi/wha...
Keywords:
Depends on:
Blocks: 100560
  Show dependency treegraph
 
Reported: 2012-12-20 12:37 PST by Dan Beam
Modified: 2013-01-10 14:41 PST (History)
20 users (show)

See Also:


Attachments
Patch (19.55 KB, patch)
2013-01-08 23:00 PST, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2013-01-08 23:43 PST, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (32.40 KB, patch)
2013-01-09 20:42 PST, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (33.44 KB, patch)
2013-01-09 21:15 PST, Dan Beam
no flags Details | Formatted Diff | Diff
Patch (33.51 KB, patch)
2013-01-10 01:07 PST, Dan Beam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Beam 2012-12-20 12:37:40 PST
As talked about on whatwg@whatwg.org [1], the reason for an autocompleteerror event being triggered should be passed along to handlers of the event.

Elliot Sprehn (esprehn@chromium.org) recommended that I subclass Event as AutocompleteErrorEvent and add a read-only string "reason" attribute to the interface. The WebIDL for this looks like:

  interface AutocompleteErrorEvent : Event {
    readonly attribute DOMString reason;
  }

so this is what I plan to implement.

[1] http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-December/038302.html
Comment 1 Ilya Sherman 2012-12-20 14:24:12 PST
If we use a string, developers will try to parse the string.  Let's use an enumerated value instead.
Comment 2 Elliott Sprehn 2012-12-20 14:27:18 PST
(In reply to comment #1)
> If we use a string, developers will try to parse the string.  Let's use an enumerated value instead.

This is an enumerated value. The platform in general is moving away from magic numbers and to strings for the enum like values. That's the reason why they're all lower case and all one word. :)
Comment 3 Ilya Sherman 2012-12-20 14:51:03 PST
(In reply to comment #2)
> (In reply to comment #1)
> > If we use a string, developers will try to parse the string.  Let's use an enumerated value instead.
> 
> This is an enumerated value. The platform in general is moving away from magic numbers and to strings for the enum like values. That's the reason why they're all lower case and all one word. :)

But with strings, the runtime has no hope of telling you that you've made a typo; whereas with named constants, at least in principle you could get a warning/error.  Why does JavaScript hate its users? :(
Comment 4 Elliott Sprehn 2012-12-20 15:00:54 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > If we use a string, developers will try to parse the string.  Let's use an enumerated value instead.
> > 
> > This is an enumerated value. The platform in general is moving away from magic numbers and to strings for the enum like values. That's the reason why they're all lower case and all one word. :)
> 
> But with strings, the runtime has no hope of telling you that you've made a typo; whereas with named constants, at least in principle you could get a warning/error.  Why does JavaScript hate its users? :(

If we had AutocompleteErrorEvent.REASON_INVALID it'd end up being a numeric value and developers would just write comparisons against the integer value anyway, we've seen this time and time again:

if (errorEvent.reason == 1) ...
else if (errorEvent.reason == 2) ...

Also since this is JS typing AutocompleteErrorEvent.REASON_INAAALID wouldn't be any kind of error, it would evaluate to undefined and developers would have no idea.

Symbols give you the power to force developers to type the named constant (I'm not sure developers really want that though...), but we'll have to wait a while before V8 or JSC supports them.
Comment 5 Dan Beam 2012-12-20 15:03:36 PST
Ilya Sherman: Additionally, JS developers can still fake this themselves, i.e.:

  /** @const @enum {string} */
  var AutocompleteErrorTypes = { REASON_INVALID: "invalid", ... };
Comment 6 Paul Irish 2012-12-20 18:31:16 PST
the string is probably the most developer-friendly approach here. As a data point, nearly all elem.nodeType checking is checking against integers rather than the enumerated values.
Comment 7 Dan Beam 2012-12-20 19:19:20 PST
Additionally, HTMLMediaElement#canPlayType() returns "no", "maybe", and "probably"
Comment 8 Dan Beam 2013-01-08 23:00:48 PST
Created attachment 181853 [details]
Patch
Comment 9 Dan Beam 2013-01-08 23:02:56 PST
Comment on attachment 181853 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

I will add a much more verbose :) changelog entry before I land.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

I will fix this before landing.

> Source/WebCore/ChangeLog:15
> +        * Target.pri:

I dont' think anything but WebCore.gypi was really needed here...

> Source/WebCore/ChangeLog:34
> +        * page/DOMWindow.idl:

I'm not sure if this event would have a "new GlobalEventStyle()" constuctor...
Comment 10 WebKit Review Bot 2013-01-08 23:06:41 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 11 Early Warning System Bot 2013-01-08 23:13:25 PST
Comment on attachment 181853 [details]
Patch

Attachment 181853 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15766326
Comment 12 Early Warning System Bot 2013-01-08 23:16:37 PST
Comment on attachment 181853 [details]
Patch

Attachment 181853 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15764364
Comment 13 Kent Tamura 2013-01-08 23:28:18 PST
Comment on attachment 181853 [details]
Patch

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

> Source/WebCore/DerivedSources.cpp:36
> +#if ENABLE(REQUEST_AUTOCOMPLETE)
> +#include "JSAutocompleteErrorEvent.cpp"
> +#endif

You don't need to wrap with ENABLE(REQUEST_AUTOCOMPLETE) because the IDL has Conditional=REQUEST_AUTOCOMPLETE.

> Source/WebCore/DerivedSources.make:202
> +    $(WebCore)/dom/AutocompleteErrorEvent.idl \

You need to add entries for the followings to WebCore/WebCore.pbxproj/project.pbxproj:
 AutocompleteErrorEvent.cpp
 AutocompleteErrorEvent.h
 JSAutocompleteErrorEvent.cpp
 JSAutocompleteErrorEvent.h
 DOMAutocompleteErrorEvent.mm
 DOMAutocompleteErrorEvent.h


and add the followings to WebCore/WebCore.vcproj/WebCore.vcproj:
 JSAutocompleteErrorEvent.cpp
 JSAutocompleteErrorEvent.h

> Source/WebCore/dom/AutocompleteErrorEvent.h:14
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS

This is not the standard copyright header for WebKit contribution.
This should follow Source/WebKit/LICENSE.

> Source/WebCore/dom/AutocompleteErrorEvent.h:30
> +#include "Event.h"

This file should be wrapped with #if ENABLE(REQUEST_AUTOCOMPLETE).

> Source/WebCore/dom/AutocompleteErrorEvent.h:38
> +    AutocompleteErrorEventInit()
> +    {
> +    };

Is this necessary?

> Source/WebCore/dom/DOMAllInOne.cpp:32
> +#if ENABLE(REQUEST_AUTOCOMPLETE)
> +#include "AutocompleteErrorEvent.cpp"
> +#endif

This is unnecessary.  We don't have AutocompleteErrorEvent.cpp.

> Source/WebCore/html/HTMLFormElement.cpp:65
> +#if ENABLE(REQUEST_AUTOCOMPLETE)
> +#include "AutocompleteErrorEvent.h"
> +#endif

You don't need to wrap with ENABLE(REQUEST_AUTOCOMPLETE) if the content of AutocompleteErrorEvent.h is wrapped with it.
Comment 14 Dan Beam 2013-01-08 23:43:28 PST
Created attachment 181860 [details]
Patch
Comment 15 Dan Beam 2013-01-08 23:43:39 PST
Comment on attachment 181853 [details]
Patch

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

>> Source/WebCore/DerivedSources.cpp:36
>> +#endif
> 
> You don't need to wrap with ENABLE(REQUEST_AUTOCOMPLETE) because the IDL has Conditional=REQUEST_AUTOCOMPLETE.

Removed.

>> Source/WebCore/DerivedSources.make:202
>> +    $(WebCore)/dom/AutocompleteErrorEvent.idl \
> 
> You need to add entries for the followings to WebCore/WebCore.pbxproj/project.pbxproj:
>  AutocompleteErrorEvent.cpp
>  AutocompleteErrorEvent.h
>  JSAutocompleteErrorEvent.cpp
>  JSAutocompleteErrorEvent.h
>  DOMAutocompleteErrorEvent.mm
>  DOMAutocompleteErrorEvent.h
> 
> 
> and add the followings to WebCore/WebCore.vcproj/WebCore.vcproj:
>  JSAutocompleteErrorEvent.cpp
>  JSAutocompleteErrorEvent.h

Talked to you about this - will try to get away with just WebCore.gypi for now...

>> Source/WebCore/dom/AutocompleteErrorEvent.h:14
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> 
> This is not the standard copyright header for WebKit contribution.
> This should follow Source/WebKit/LICENSE.

Done.

>> Source/WebCore/dom/AutocompleteErrorEvent.h:30
>> +#include "Event.h"
> 
> This file should be wrapped with #if ENABLE(REQUEST_AUTOCOMPLETE).

Done.

>> Source/WebCore/dom/AutocompleteErrorEvent.h:38
>> +    };
> 
> Is this necessary?

No, I just copied it.  Removed.

>> Source/WebCore/dom/DOMAllInOne.cpp:32
>> +#endif
> 
> This is unnecessary.  We don't have AutocompleteErrorEvent.cpp.

Removed.

>> Source/WebCore/html/HTMLFormElement.cpp:65
>> +#endif
> 
> You don't need to wrap with ENABLE(REQUEST_AUTOCOMPLETE) if the content of AutocompleteErrorEvent.h is wrapped with it.

Done.
Comment 16 Kent Tamura 2013-01-09 00:00:18 PST
Comment on attachment 181860 [details]
Patch

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

> Source/WebCore/dom/AutocompleteErrorEvent.h:2
> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

You wrote this.  This line should be "Copyright (C) 2013 Google Inc. All rights reserved."

> Source/WebCore/dom/AutocompleteErrorEvent.idl:2
> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Ditto.
Comment 17 Dan Beam 2013-01-09 20:42:42 PST
Created attachment 182054 [details]
Patch
Comment 18 Dan Beam 2013-01-09 20:43:03 PST
Comment on attachment 181860 [details]
Patch

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

>> Source/WebCore/dom/AutocompleteErrorEvent.h:2
>> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
> 
> You wrote this.  This line should be "Copyright (C) 2013 Google Inc. All rights reserved."

Done.

>> Source/WebCore/dom/AutocompleteErrorEvent.idl:2
>> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
> 
> Ditto.

Done.
Comment 19 Dan Beam 2013-01-09 20:43:30 PST
Added tests and ChangeLog entries.
Comment 20 Dan Beam 2013-01-09 20:57:19 PST
Comment on attachment 182054 [details]
Patch

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

> LayoutTests/fast/events/constructors/autocomplete-error-event-constructor-expected.txt:1
> +This tests the constructor for the AutocompleteErrorEvent DOM class.

Ah, this needs to be in platform/chromium/, fixing.
Comment 21 Dan Beam 2013-01-09 21:15:03 PST
Created attachment 182058 [details]
Patch
Comment 22 Dan Beam 2013-01-09 21:15:47 PST
Comment on attachment 182054 [details]
Patch

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

>> LayoutTests/fast/events/constructors/autocomplete-error-event-constructor-expected.txt:1
>> +This tests the constructor for the AutocompleteErrorEvent DOM class.
> 
> Ah, this needs to be in platform/chromium/, fixing.

Done.
Comment 23 Dan Beam 2013-01-10 01:07:38 PST
Created attachment 182084 [details]
Patch
Comment 24 Dan Beam 2013-01-10 01:11:56 PST
Comment on attachment 182084 [details]
Patch

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

> LayoutTests/fast/js/script-tests/global-constructors.js:42
> +        name == "FileReader" ||

The only difference between this patch and the last one is that there was an accidental syntax error in the last one fixed here.
Comment 25 Adam Barth 2013-01-10 14:04:55 PST
Comment on attachment 182084 [details]
Patch

It looks like you've addressed all of tkent's feedback and this patch looks good to me too.
Comment 26 WebKit Review Bot 2013-01-10 14:41:40 PST
Comment on attachment 182084 [details]
Patch

Clearing flags on attachment: 182084

Committed r139372: <http://trac.webkit.org/changeset/139372>
Comment 27 WebKit Review Bot 2013-01-10 14:41:47 PST
All reviewed patches have been landed.  Closing bug.