Bug 153270 – Reduce PassRefPtr uses in dom - 4

• Component: DOM
• Status: RESOLVED FIXED

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 269344 [details]
Patch

[WebKit Commit Bot]

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.

[Gyuyoung Kim]

Created attachment 269431 [details]
Patch

[Gyuyoung Kim]

Created attachment 269442 [details]
Patch

[WebKit Commit Bot]

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.

[Gyuyoung Kim]

Created attachment 269448 [details]
Patch

[WebKit Commit Bot]

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.

[Gyuyoung Kim]

Created attachment 269450 [details]
Patch

[WebKit Commit Bot]

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.

[Gyuyoung Kim]

Created attachment 269451 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

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.

[Gyuyoung Kim]

Created attachment 269553 [details]
Patch

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 269554 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

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

[Darin Adler]

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.

[Gyuyoung Kim]

(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

[Gyuyoung Kim]

Created attachment 269640 [details]
Patch

[Gyuyoung Kim]

Created attachment 269666 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Gyuyoung Kim]

Created attachment 269670 [details]
Patch

[Darin Adler]

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

[Gyuyoung Kim]

Created attachment 269716 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 269716 [details]
Patch for landing

Clearing flags on attachment: 269716

Committed r195524: <http://trac.webkit.org/changeset/195524>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.