Bug 105568 – Implement AutocompleteErrorEvent#reason

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Dan Beam]

As talked about on [e-mail] [1], the reason for an autocompleteerror event being triggered should be passed along to handlers of the event.

Elliot Sprehn ([e-mail]) 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

[Ilya Sherman]

If we use a string, developers will try to parse the string.  Let's use an enumerated value instead.

[Elliott Sprehn]

(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. :)

[Ilya Sherman]

(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? :(

[Elliott Sprehn]

(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.

[Dan Beam]

Ilya Sherman: Additionally, JS developers can still fake this themselves, i.e.:

  /** @const @enum {string} */
  var AutocompleteErrorTypes = { REASON_INVALID: "invalid", ... };

[Paul Irish]

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.

[Dan Beam]

Additionally, HTMLMediaElement#canPlayType() returns "no", "maybe", and "probably"

[Dan Beam]

Created attachment 181853 [details]
Patch

[Dan Beam]

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...

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Early Warning System Bot]

Comment on attachment 181853 [details]
Patch

Attachment 181853 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15766326

[Early Warning System Bot]

Comment on attachment 181853 [details]
Patch

Attachment 181853 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15764364

[Kent Tamura]

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.

[Dan Beam]

Created attachment 181860 [details]
Patch

[Dan Beam]

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.

[Kent Tamura]

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.

[Dan Beam]

Created attachment 182054 [details]
Patch

[Dan Beam]

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.

[Dan Beam]

Added tests and ChangeLog entries.

[Dan Beam]

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.

[Dan Beam]

Created attachment 182058 [details]
Patch

[Dan Beam]

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.

[Dan Beam]

Created attachment 182084 [details]
Patch

[Dan Beam]

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.

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 182084 [details]
Patch

Clearing flags on attachment: 182084

Committed r139372: <http://trac.webkit.org/changeset/139372>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.