Bug 157883 – Implement operator== for WeakPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Rawinder Singh]

To be able to check whether the references held by WeakPtrs are equal when using a container (e.g. WTF::HashSet), the operator== needs to be implemented.

[Rawinder Singh]

Created attachment 279346 [details]
Patch

[Chris Dumez]

Comment on attachment 279346 [details]
Patch

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

> Source/WTF/ChangeLog:8
> +        Implement operator== for WeakPtr so that it can be used with containers (e.g. WTF::HashSet).

While adding an operator==() to WeakPtr makes sense, I don't think having a HashSet<WeakPtr<X>> would be possible anyway.

[Rawinder Singh]

(In reply to comment #2)
> Comment on attachment 279346 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279346&action=review
> 
> > Source/WTF/ChangeLog:8
> > +        Implement operator== for WeakPtr so that it can be used with containers (e.g. WTF::HashSet).
> 
> While adding an operator==() to WeakPtr makes sense,

> I don't think having a HashSet<WeakPtr<X>> would be possible anyway.

Ok.

Should I upload it again without the comment?

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 279346 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=279346&action=review
> > 
> > > Source/WTF/ChangeLog:8
> > > +        Implement operator== for WeakPtr so that it can be used with containers (e.g. WTF::HashSet).
> > 
> > While adding an operator==() to WeakPtr makes sense,
> 
> > I don't think having a HashSet<WeakPtr<X>> would be possible anyway.
> 
> Ok.
> 
> Should I upload it again without the comment?

Yes but also, it'd be great if you used this operator in your patch as well. If you look at EventHandler.cpp, I believe you'll find places where it would be useful (e.g. m_lastScrollbarUnderMouse is weak and pointer compared in several places). Also look at EventHandler::platformCompleteWheelEvent() in EventHandlerMac.mm.

[Rawinder Singh]

Created attachment 279465 [details]
Patch

[Chris Dumez]

Comment on attachment 279465 [details]
Patch

r=me

[Build Bot]

Comment on attachment 279465 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279466 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Rawinder Singh]

(In reply to comment #6)
> Comment on attachment 279465 [details]
> Patch
> 
> r=me

Do you have any ideas why this patch is failing for mac?

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #6)
> > Comment on attachment 279465 [details]
> > Patch
> > 
> > r=me
> 
> Do you have any ideas why this patch is failing for mac?

Mac ews has been flaky recently:/

[WebKit Commit Bot]

Comment on attachment 279465 [details]
Patch

Clearing flags on attachment: 279465

Committed r201213: <http://trac.webkit.org/changeset/201213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change appears to have cause an API test failure on Mac and iOS"

FAIL WTF_WeakPtr.MultipleFactories

/Volumes/Data/slave/elcapitan-release/build/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:86
Value of: weakPtr1 != weakPtr2
  Actual: false
Expected: true

<https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/6206/steps/run-api-tests/logs/stdio>

[Chris Dumez]

(In reply to comment #13)
> This change appears to have cause an API test failure on Mac and iOS"
> 
> FAIL WTF_WeakPtr.MultipleFactories
> 
> /Volumes/Data/slave/elcapitan-release/build/Tools/TestWebKitAPI/Tests/WTF/
> WeakPtr.cpp:86
> Value of: weakPtr1 != weakPtr2
>   Actual: false
> Expected: true
> 
> <https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/6206/steps/run-api-
> tests/logs/stdio>

Fixed in <http://trac.webkit.org/changeset/201214>.