Bug 157985 – Purge PassRefPtr from TouchList

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nael Ouedraogo]

Purge PassRefPtr from TouchList.

[Nael Ouedraogo]

Created attachment 279558 [details]
Patch

[Build Bot]

Comment on attachment 279558 [details]
Patch

Attachment 279558 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1368778

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 279560 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Gyuyoung Kim]

Comment on attachment 279558 [details]
Patch

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

> Source/WebCore/dom/TouchList.h:51
> +    void append(RefPtr<Touch> touch) { m_values.append(WTFMove(touch)); }

RefPtr<Touch>&& ?

[Nael Ouedraogo]

(In reply to comment #4)
> Comment on attachment 279558 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279558&action=review
> 
> > Source/WebCore/dom/TouchList.h:51
> > +    void append(RefPtr<Touch> touch) { m_values.append(WTFMove(touch)); }
> 
> RefPtr<Touch>&& ?

The append() method is passed a :
- RefPtr<Touch>& in EventHandler::handleTouchEvent() which cannot be moved.
- Ref<Touch> in EventPath.cpp
- and Touch* in JS binding code.

The alternative is to implement two append functions.

[Nael Ouedraogo]

(In reply to comment #2)
> Comment on attachment 279558 [details]
> Patch
> 
> Attachment 279558 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/1368778
> 
> New failing tests:
> http/tests/media/hls/video-controls-live-stream.html

Failure observed in http/tests/media/hls on mac-yosemite in debug seems not related to the patch.
Same test on mac-elcapitan in debug successfully passed locally.

[Chris Dumez]

Comment on attachment 279558 [details]
Patch

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

>>> Source/WebCore/dom/TouchList.h:51
>>> +    void append(RefPtr<Touch> touch) { m_values.append(WTFMove(touch)); }
>> 
>> RefPtr<Touch>&& ?
> 
> The append() method is passed a :
> - RefPtr<Touch>& in EventHandler::handleTouchEvent() which cannot be moved.
> - Ref<Touch> in EventPath.cpp
> - and Touch* in JS binding code.
> 
> The alternative is to implement two append functions.

Gyuyoung is right. This function should take a RefPtr<Touch>&&. Fix the call sites if necessary (using copyRef() if you cannot WTFMove()).

[Nael Ouedraogo]

Created attachment 279637 [details]
Patch

[WebKit Commit Bot]

Attachment 279637 [details] did not pass style-queue:


ERROR: Source/WebCore/page/EventHandler.cpp:3912:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/EventHandler.cpp:3913:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/EventHandler.cpp:3914:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nael Ouedraogo]

> Gyuyoung is right. This function should take a RefPtr<Touch>&&. Fix the call
> sites if necessary (using copyRef() if you cannot WTFMove()).

OK. I fixed in uploaded patch.

[Darin Adler]

Comment on attachment 279637 [details]
Patch

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

Change is OK. A few things I would have done differently.

When touching a header file, we are changing each to use #pragma once instead of #ifndef.

> Source/WebCore/dom/TouchList.h:51
> +    void append(RefPtr<Touch>&& touch) { m_values.append(WTFMove(touch)); }

Since there can’t be any null values, this should take a Ref&&, not RefPtr&&.

> Source/WebCore/dom/TouchList.h:56
> +    Vector<RefPtr<Touch>> m_values;

Since there can’t be any null values, this should be Vector<Ref>, not Vector<RefPtr>.

> Source/WebCore/page/EventHandler.cpp:3914
> +        Ref<Touch> touch = Touch::create(targetFrame, touchTarget.get(), point.id(),
> +                                         point.screenPos().x(), point.screenPos().y(),
> +                                         adjustedPageX, adjustedPageY,
> +                                         point.radiusX(), point.radiusY(), point.rotationAngle(), point.force());

WebKit code style guidelines discourage this kind of "indenting to match the (" style. Strangely, I can’t find it in document, though.

Also, we normally would use "auto" instead of writing Ref<Touch> in new code.

[Nael Ouedraogo]

Created attachment 279752 [details]
Patch

[Nael Ouedraogo]

I fixed the code style and #pragma issue in the uploaded patch.

> > Source/WebCore/dom/TouchList.h:51
> > +    void append(RefPtr<Touch>&& touch) { m_values.append(WTFMove(touch)); }
> 
> Since there can’t be any null values, this should take a Ref&&, not RefPtr&&.
> 
> > Source/WebCore/dom/TouchList.h:56
> > +    Vector<RefPtr<Touch>> m_values;
> 
> Since there can’t be any null values, this should be Vector<Ref>, not
> Vector<RefPtr>.

Use of Ref&& instead of RefPtr&& is more complex since append method is passed a Touch* in JS binding code.  

I'll investigate it further.

[Nael Ouedraogo]

Created attachment 279754 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279754 [details]
Patch

Clearing flags on attachment: 279754

Committed r201377: <http://trac.webkit.org/changeset/201377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.