Bug 157985 - Purge PassRefPtr from TouchList
Summary: Purge PassRefPtr from TouchList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nael Ouedraogo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-23 02:49 PDT by Nael Ouedraogo
Modified: 2016-05-25 04:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.28 KB, patch)
2016-05-23 03:01 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (1.32 MB, application/zip)
2016-05-23 04:12 PDT, Build Bot
no flags Details
Patch (4.04 KB, patch)
2016-05-24 02:59 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (4.30 KB, patch)
2016-05-25 03:22 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (4.30 KB, patch)
2016-05-25 03:39 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nael Ouedraogo 2016-05-23 02:49:09 PDT
Purge PassRefPtr from TouchList.
Comment 1 Nael Ouedraogo 2016-05-23 03:01:16 PDT
Created attachment 279558 [details]
Patch
Comment 2 Build Bot 2016-05-23 04:12:25 PDT
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
Comment 3 Build Bot 2016-05-23 04:12:29 PDT
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
Comment 4 Gyuyoung Kim 2016-05-23 05:16:06 PDT
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>&& ?
Comment 5 Nael Ouedraogo 2016-05-23 06:40:59 PDT
(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.
Comment 6 Nael Ouedraogo 2016-05-23 07:58:01 PDT
(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.
Comment 7 Chris Dumez 2016-05-23 08:41:52 PDT
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()).
Comment 8 Nael Ouedraogo 2016-05-24 02:59:28 PDT
Created attachment 279637 [details]
Patch
Comment 9 WebKit Commit Bot 2016-05-24 03:01:43 PDT
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.
Comment 10 Nael Ouedraogo 2016-05-24 05:16:53 PDT
> 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.
Comment 11 Darin Adler 2016-05-24 08:49:45 PDT
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.
Comment 12 Nael Ouedraogo 2016-05-25 03:22:07 PDT
Created attachment 279752 [details]
Patch
Comment 13 Nael Ouedraogo 2016-05-25 03:31:57 PDT
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.
Comment 14 Nael Ouedraogo 2016-05-25 03:39:17 PDT
Created attachment 279754 [details]
Patch
Comment 15 WebKit Commit Bot 2016-05-25 04:12:24 PDT
Comment on attachment 279754 [details]
Patch

Clearing flags on attachment: 279754

Committed r201377: <http://trac.webkit.org/changeset/201377>
Comment 16 WebKit Commit Bot 2016-05-25 04:12:29 PDT
All reviewed patches have been landed.  Closing bug.