Bug 153270

Created attachment 269344 [details] Patch

Attachment 269344 [details] did not pass style-queue: ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:96: The parameter name "acceleration" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 7 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 269431 [details] Patch

Created attachment 269442 [details] Patch

Attachment 269442 [details] did not pass style-queue: ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:96: The parameter name "acceleration" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 7 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 269448 [details] Patch

Attachment 269448 [details] did not pass style-queue: ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:96: The parameter name "acceleration" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 7 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 269450 [details] Patch

Attachment 269450 [details] did not pass style-queue: ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:96: The parameter name "acceleration" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 269451 [details] Patch

Attachment 269451 [details] did not pass style-queue: ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:86: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/dom/DeviceMotionData.h:96: The parameter name "acceleration" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: The parameter name "rotationRate" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/DeviceMotionData.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 7 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 269451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269451&action=review > Source/WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:192 > wrapped().initDeviceMotionEvent(type, bubbles, cancelable, deviceMotionData.get()); I think all these parameters should be RefPtr&& instead of a pointers, and all parameters could be WTFMoved into this call. > Source/WebCore/dom/DeviceMotionData.cpp:72 > +Ref<DeviceMotionData> DeviceMotionData::create(Acceleration* acceleration, Acceleration* accelerationIncludingGravity, > + RotationRate* rotationRate, bool canProvideInterval, double interval) Ditto. And these should be on the same line to make the style bot happy. > Source/WebCore/dom/DeviceMotionData.cpp:84 > +DeviceMotionData::DeviceMotionData(Acceleration* acceleration, Acceleration* accelerationIncludingGravity, > + RotationRate* rotationRate, bool canProvideInterval, double interval) ditto. And in the header > Source/WebCore/dom/Document.cpp:4832 > +void Document::pushCurrentScript(HTMLScriptElement* newCurrentScript) I don't think this is called anywhere. Could it be removed? Same with popCurrentScript and m_currentScriptStack. > Source/WebCore/dom/Document.h:927 > + void setBodyOrFrameset(RefPtr<HTMLElement>&&, ExceptionCode&); I don't think this is called. Remove if not. Also in header. > Source/WebCore/dom/MouseEvent.cpp:256 > + return WTFMove(clonedMouseEvent); I don't think you need WTFMove with return.

Created attachment 269553 [details] Patch

Comment on attachment 269451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269451&action=review >> Source/WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:192 >> wrapped().initDeviceMotionEvent(type, bubbles, cancelable, deviceMotionData.get()); > > I think all these parameters should be RefPtr&& instead of a pointers, and all parameters could be WTFMoved into this call. I thought that nobody hands DeviceMotionData ownership. But in this case it might be good to hand ownership to DeviceMotionData. Fixed. >> Source/WebCore/dom/DeviceMotionData.cpp:72 >> + RotationRate* rotationRate, bool canProvideInterval, double interval) > > Ditto. And these should be on the same line to make the style bot happy. Done. >> Source/WebCore/dom/Document.cpp:4832 >> +void Document::pushCurrentScript(HTMLScriptElement* newCurrentScript) > > I don't think this is called anywhere. Could it be removed? Same with popCurrentScript and m_currentScriptStack. No, pushCurrentScript() is being used by CurrentScriptIncrementer class. class CurrentScriptIncrementer { WTF_MAKE_NONCOPYABLE(CurrentScriptIncrementer); public: CurrentScriptIncrementer(Document& document, Element* element) : m_document(document) , m_isHTMLScriptElement(is<HTMLScriptElement>(*element)) { if (m_isHTMLScriptElement) m_document.pushCurrentScript(downcast<HTMLScriptElement>(element)); } >> Source/WebCore/dom/Document.h:927 >> + void setBodyOrFrameset(RefPtr<HTMLElement>&&, ExceptionCode&); > > I don't think this is called. Remove if not. Also in header. I find where it is used. It is used by DerivedSources/WebCore/JSDocument.cpp. DerivedSources/WebCore/JSDocument.cpp:3178:10: error: ‘class WebCore::Document’ has no member named ‘setBodyOrFrameset’ impl.setBodyOrFrameset(nativeValue, ec); >> Source/WebCore/dom/MouseEvent.cpp:256 >> + return WTFMove(clonedMouseEvent); > > I don't think you need WTFMove with return. done.

Created attachment 269554 [details] Patch

Comment on attachment 269554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269554&action=review > Source/WebCore/dom/MouseEvent.cpp:256 > + return clonedMouseEvent.releaseNonNull(); clonedMouseEvent should be a Ref, not a RefPtr, then you don't need NonNull. Just return the ref.

(In reply to comment #14) > No, pushCurrentScript() is being used by CurrentScriptIncrementer class. CurrentScriptIncrementer.h is not in the Xcode project, so searching for it yielded no results :( It should be added. > I find where it is used. It is used by DerivedSources/WebCore/JSDocument.cpp. Ok, also not searchable, but for a good reason.

Comment on attachment 269554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269554&action=review > Source/WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:191 > + RefPtr<DeviceMotionData> deviceMotionData = DeviceMotionData::create(WTFMove(acceleration), WTFMove(accelerationIncludingGravity), WTFMove(rotationRate), intervalProvided, interval); This should be going into a Ref, not a RefPtr. I suggest using auto. > Source/WebCore/dom/Document.cpp:2586 > +void Document::setBodyOrFrameset(RefPtr<HTMLElement>&& newBody, ExceptionCode& ec) I don’t think this is quite right. The code below assigns a new value to newBody, which could modify the argument passed in. We don’t want that to happen. > Source/WebCore/dom/Document.cpp:3757 > +bool Document::setFocusedElement(RefPtr<Element>&& newFocusedElement, FocusDirection direction) This function takes no advantage of being passed a RefPtr&& and doesn’t take ownership of the argument; I suggest we consider changing the argument type to a raw pointer. I worry about object lifetime problems, though, if we remove the protection of being in a RefPtr from the argument. The code calls a lot of arbitrary JavaScript that could delete the last reference to the object. So probably want to use a raw pointer but have a local variable of type RefPtr (or Ref) inside the function.

(In reply to comment #16) > Comment on attachment 269554 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269554&action=review > > > Source/WebCore/dom/MouseEvent.cpp:256 > > + return clonedMouseEvent.releaseNonNull(); > > clonedMouseEvent should be a Ref, not a RefPtr, then you don't need NonNull. > Just return the ref. (In reply to comment #16) > Comment on attachment 269554 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269554&action=review > > > Source/WebCore/dom/MouseEvent.cpp:256 > > + return clonedMouseEvent.releaseNonNull(); > > clonedMouseEvent should be a Ref, not a RefPtr, then you don't need NonNull. > Just return the ref. When *clonedMouseEvent* is a Ref, there was a build break on GTK port. It seems we need to use WTFMove() explicitly in this case. Failed to run "['Tools/Scripts/build-webkit', '--release', '--gtk', '--update-gtk', '--makeargs="-j16"']" exit_code: 1 -Wundef -Wwrite-strings -fPIC -MMD -MT Source/WebCore/CMakeFiles/WebCore.dir/dom/MouseEvent.cpp.o -MF Source/WebCore/CMakeFiles/WebCore.dir/dom/MouseEvent.cpp.o.d -o Source/WebCore/CMakeFiles/WebCore.dir/dom/MouseEvent.cpp.o -c ../../Source/WebCore/dom/MouseEvent.cpp ../../Source/WebCore/dom/MouseEvent.cpp: In member function 'virtual WTF::Ref<WebCore::Event> WebCore::MouseEvent::cloneFor(WebCore::HTMLIFrameElement*) const': ../../Source/WebCore/dom/MouseEvent.cpp:256:12: error: use of deleted function 'WTF::Ref<T>::Ref(const WTF::Ref<U>&) [with U = WebCore::MouseEvent; T = WebCore::Event]' In file included from ../../Source/WTF/wtf/PassRefPtr.h:25:0, from ../../Source/WTF/wtf/RefPtr.h:30, from ../../Source/WebCore/dom/RegisteredEventListener.h:28, from ../../Source/WebCore/dom/EventListenerMap.h:36, from ../../Source/WebCore/dom/EventTarget.h:34, from ../../Source/WebCore/page/DOMWindow.h:31, from ../../Source/WebCore/dom/UIEvent.h:27, from ../../Source/WebCore/dom/UIEventWithKeyState.h:27, from ../../Source/WebCore/dom/MouseRelatedEvent.h:28, from ../../Source/WebCore/dom/MouseEvent.h:27, from ../../Source/WebCore/dom/MouseEvent.cpp:24: ../../Source/WTF/wtf/Ref.h:67:26: note: declared here

Created attachment 269640 [details] Patch

Created attachment 269666 [details] Patch

Comment on attachment 269666 [details] Patch Attachment 269666 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/729048 New failing tests: imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html

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

Created attachment 269670 [details] Patch

Comment on attachment 269670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269670&action=review > Source/WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:192 > + wrapped().initDeviceMotionEvent(type, bubbles, cancelable, &deviceMotionData.get()); deviceMotionData.ptr() is the better idiom here instead of &deviceMotionData.get()

Created attachment 269716 [details] Patch for landing

Comment on attachment 269716 [details] Patch for landing Clearing flags on attachment: 269716 Committed r195524: <http://trac.webkit.org/changeset/195524>

All reviewed patches have been landed. Closing bug.