Bug 153903

Created attachment 270713 [details] Patch

<rdar://problem/24518146>

Created attachment 270714 [details] Patch

Attachment 270714 [details] did not pass style-queue: ERROR: Source/WebCore/dom/UIEventWithKeyState.h:72: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:73: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:74: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:75: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:76: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:77: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:82: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:83: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:84: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:85: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:86: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/UIEventWithKeyState.h:87: Wrong number of spaces before statement. (expected: 28) [whitespace/indent] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:73: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:75: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:77: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:78: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:90: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:92: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:94: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:95: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:108: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:142: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:143: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:144: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:145: Wrong number of spaces before statement. (expected: 27) [whitespace/indent] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:145: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:146: Wrong number of spaces before statement. (expected: 27) [whitespace/indent] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:146: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:148: Wrong number of spaces before statement. (expected: 27) [whitespace/indent] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:148: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:151: Wrong number of spaces before statement. (expected: 27) [whitespace/indent] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:152: Wrong number of spaces before statement. (expected: 27) [whitespace/indent] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:153: Wrong number of spaces before statement. (expected: 27) [whitespace/indent] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:146: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:155: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 37 in 80 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 270714 [details] Patch Attachment 270714 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/784196 New failing tests: http/tests/eventsource/eventsource-parse-event-stream.html http/tests/eventsource/eventsource-eof.html

Created attachment 270716 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 270714 [details] Patch Attachment 270714 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/784199 New failing tests: http/tests/eventsource/eventsource-parse-event-stream.html http/tests/eventsource/eventsource-eof.html

Created attachment 270717 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 270714 [details] Patch Attachment 270714 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/784204 New failing tests: http/tests/eventsource/eventsource-parse-event-stream.html http/tests/eventsource/eventsource-eof.html

Created attachment 270718 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 270782 [details] Patch

Attachment 270782 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:73: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:75: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:77: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:78: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:90: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:92: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:94: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:95: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/MouseEvent.cpp:139: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:140: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:142: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:140: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 12 in 82 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 270783 [details] Patch

Attachment 270783 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:135: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:137: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] Total errors found: 2 in 82 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 270784 [details] Patch

Attachment 270784 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:135: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:137: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] Total errors found: 2 in 82 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 270862 [details] Patch

Created attachment 270873 [details] Patch

Attachment 270873 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:135: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:137: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] Total errors found: 2 in 83 files If any of these errors are false positives, please file a bug against check-webkit-style.

File Bug 153999 to track the style checking issue.

Comment on attachment 270873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270873&action=review > Source/WebCore/Modules/encryptedmedia/MediaKeyMessageEvent.h:48 > + static Ref<MediaKeyMessageEvent> create(const AtomicString& type, RefPtr<Uint8Array> message, const String& destinationURL) The argument type here should not be RefPtr. If the caller is handing off ownership, then it should be Ref<Uint8Array>&& or RefPtr<Uint8Array>&&. If the caller is not handing off ownership, it should be Uint8Array& or Uint8Array*. > Source/WebCore/Modules/encryptedmedia/MediaKeyMessageEvent.h:70 > + MediaKeyMessageEvent(const AtomicString& type, RefPtr<Uint8Array> message, const String& destinationURL); Ditto. > Source/WebCore/Modules/encryptedmedia/MediaKeyNeededEvent.h:46 > + static Ref<MediaKeyNeededEvent> create(const AtomicString& type, RefPtr<Uint8Array> initData) Ditto. > Source/WebCore/Modules/encryptedmedia/MediaKeyNeededEvent.h:67 > + MediaKeyNeededEvent(const AtomicString& type, RefPtr<Uint8Array> initData); Ditto. > Source/WebCore/Modules/speech/SpeechSynthesisEvent.h:38 > static Ref<SpeechSynthesisEvent> create(const AtomicString& type, unsigned long charIndex, float elapsedTime, const String& name); Not new to this patch, but the type here is wrong. It should be "unsigned", not "unsigned long". This is a common error made when people are working with IDL, since the IDL syntax for this type *is* "unsigned long". > Source/WebCore/bindings/objc/DOM.mm:521 > + RefPtr<KeyboardEvent> key = KeyboardEvent::createForBindings(); Does not make sense to use createForBindings here. > Source/WebCore/bindings/objc/DOM.mm:531 > + RefPtr<KeyboardEvent> key = KeyboardEvent::createForBindings(); Ditto. > Source/WebCore/dom/MouseEvent.cpp:39 > + : clientX(0) > + , clientY(0) > + , button(0) To avoid repeating these it’s better to initialize data members in the struct definition itself rather than in the constructors. > Source/WebCore/dom/OverflowEvent.cpp:-41 > OverflowEvent::OverflowEvent() > - : Event(eventNames().overflowchangedEvent, false, false) Why is this change OK? > Source/WebCore/html/HTMLMediaElement.cpp:2234 > + RefPtr<Event> event = MediaKeyEvent::create(eventNames().webkitkeyaddedEvent, keySystem, sessionId, nullptr, nullptr, "", nullptr, 0); Should use emptyString(), not "". Return type should be Ref, not RefPtr. > Source/WebCore/html/HTMLMediaElement.cpp:2263 > + RefPtr<Event> event = MediaKeyEvent::create(eventNames().webkitkeyerrorEvent, keySystem, sessionId, nullptr, nullptr, "", MediaKeyError::create(mediaKeyErrorCode), systemCode); Ditto. > Source/WebCore/html/MediaKeyEvent.h:52 > + static Ref<MediaKeyEvent> create(const AtomicString& type, const String& keySystem, const String& sessionId, RefPtr<Uint8Array> initData, RefPtr<Uint8Array> message, const String& defaultURL, RefPtr<MediaKeyError> errorCode, uint32_t systemCode) Arguments should not be RefPtr. They should be RefPtr&& or Ref&& or a reference or a pointer. > Source/WebCore/html/MediaKeyEvent.h:79 > + MediaKeyEvent(const AtomicString& type, const String& keySystem, const String& sessionId, RefPtr<Uint8Array> initData, RefPtr<Uint8Array> message, const String& defaultURL, RefPtr<MediaKeyError> errorCode, uint32_t systemCode); Ditto. > Source/WebCore/html/track/TrackEvent.cpp:45 > +TrackEvent::TrackEvent(const AtomicString& type, bool canBubble, bool cancelable, RefPtr<TrackBase> track) Ditto. > Source/WebCore/html/track/TrackEvent.h:46 > + static Ref<TrackEvent> create(const AtomicString& type, bool canBubble, bool cancelable, RefPtr<TrackBase> track) Ditto. > Source/WebCore/page/EventSource.cpp:416 > + Ref<MessageEvent> event = MessageEvent::create(m_eventName.isEmpty() ? eventNames().messageEvent : AtomicString(m_eventName), false, false, SerializedScriptValue::create(String::adopt(m_data)), m_eventStreamOrigin, m_lastEventId); > return event; No need for the local variable. It was there before so we could call init. Please just use return directly. > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1616 > + RefPtr<ScriptCallStack> stack = createScriptCallStack(JSMainThreadExecState::currentState(), 2); Should be Ref instead of RefPtr.

Comment on attachment 270873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270873&action=review >> Source/WebCore/bindings/objc/DOM.mm:521 >> + RefPtr<KeyboardEvent> key = KeyboardEvent::createForBindings(); > > Does not make sense to use createForBindings here. It looks like that this KeyboardEvent is never used. I have examined all the paths that the event is passed to. None of them seems to use the event. Even if I could miss something, this event is useless without KeyboardEvent::initKeyboardEvent. There is no information inside it. Therefore, it could be another bug. >> Source/WebCore/dom/OverflowEvent.cpp:-41 >> - : Event(eventNames().overflowchangedEvent, false, false) > > Why is this change OK? Normally, the default constructor (default createForBindings method) is used with init*Event in order to create an event. Therefore, the event type and flags should be set by OverflowEvent::initOverflowEvent. Unfortunately, this class have done it completely wrong as OverflowEvent:: initOverflowEvent doesn't call the Event::initEvent to set the members. Instead, it relies on the constructor. I will double check if there is any other class which does the same thing.

Comment on attachment 270873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270873&action=review >>> Source/WebCore/bindings/objc/DOM.mm:521 >>> + RefPtr<KeyboardEvent> key = KeyboardEvent::createForBindings(); >> >> Does not make sense to use createForBindings here. > > It looks like that this KeyboardEvent is never used. > I have examined all the paths that the event is passed to. None of them seems to use the event. Even if I could miss something, this event is useless without KeyboardEvent::initKeyboardEvent. There is no information inside it. Therefore, it could be another bug. I don't have a good solution for it. First, there is already a default constructor which is for bindings. It is not possible to have another. However, I am not sure if we should fake one for it.

Created attachment 270977 [details] Patch for committing

Attachment 270977 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:119: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:121: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] Total errors found: 2 in 83 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 270873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270873&action=review >>>> Source/WebCore/bindings/objc/DOM.mm:521 >>>> + RefPtr<KeyboardEvent> key = KeyboardEvent::createForBindings(); >>> >>> Does not make sense to use createForBindings here. >> >> It looks like that this KeyboardEvent is never used. >> I have examined all the paths that the event is passed to. None of them seems to use the event. Even if I could miss something, this event is useless without KeyboardEvent::initKeyboardEvent. There is no information inside it. Therefore, it could be another bug. > > I don't have a good solution for it. First, there is already a default constructor which is for bindings. It is not possible to have another. However, I am not sure if we should fake one for it. We can easily have two constructors that don’t need to take any arguments. We simply create a placeholder argument with a value that is ignored for the second constructor. One of the many examples of this idiom in WebKit is the String(WTF::HashTableDeletedValueType) constructor.

Comment on attachment 270873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270873&action=review >>>>> Source/WebCore/bindings/objc/DOM.mm:521 >>>>> + RefPtr<KeyboardEvent> key = KeyboardEvent::createForBindings(); >>>> >>>> Does not make sense to use createForBindings here. >>> >>> It looks like that this KeyboardEvent is never used. >>> I have examined all the paths that the event is passed to. None of them seems to use the event. Even if I could miss something, this event is useless without KeyboardEvent::initKeyboardEvent. There is no information inside it. Therefore, it could be another bug. >> >> I don't have a good solution for it. First, there is already a default constructor which is for bindings. It is not possible to have another. However, I am not sure if we should fake one for it. > > We can easily have two constructors that don’t need to take any arguments. We simply create a placeholder argument with a value that is ignored for the second constructor. > > One of the many examples of this idiom in WebKit is the String(WTF::HashTableDeletedValueType) constructor. I think the right thing to do here is to add a new function other than createForBindings that is called here. It could be called createDummyEvent or something like that. And you can have a comment about the fact that you’d like to return and delete that function. By giving it a special name you make it easy to search for callers, and make it unlikely that later programmers will be tempted to call it. That’s better than using createForBindings here.

Created attachment 271029 [details] Patch for committing v2

Attachment 271029 [details] did not pass style-queue: ERROR: Source/WebCore/dom/MouseEvent.cpp:119: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/MouseEvent.cpp:121: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] Total errors found: 2 in 75 files If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r196400: <http://trac.webkit.org/changeset/196400>

Bug 154094 is filed to keep tracking of Cleanup KeyboardEvent::createForDummy().

Also, <rdar://problem/24596445> is filed to cleanup GestureEvent and TouchEvent in the Internal repository.

Fixed bindings test failure after this change with <https://trac.webkit.org/r196407>