Bug 134417

Summary:

Add test to ensure that SVGDocumentExtensions::rebuildElements() doesn't rebuild invalid elements

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

WebCore Misc.

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, bunhere, cdumez, commit-queue, darin, ddkilzer, d-r, fmalita, gyuyoung.kim, je_julie.kim, pdr, sam, schenney, sergio

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

Unspecified

Bug Depends on:

134500

Bug Blocks:

Attachments:

Description	Flags
Patch and layout test	none
Layout test	none

Description Daniel Bates 2014-06-27 15:23:32 PDT

We should use Vector::swap() instead of std::move() to explicitly swap Vector objects in SVGDocumentExtensions::rebuildElements(). Currently we use std::move() to implicitly swap Vector objects. This is error prone as a const lvalue reference can bind to an rvalue so calling T t = std::move(t_1) <=> T t = t_1 <=> T t(t_1) where t_1 is const T&&.

Comment 1 Daniel Bates 2014-06-27 15:24:00 PDT

<rdar://problem/17479854>

Comment 2 Daniel Bates 2014-06-27 15:27:30 PDT

Created attachment 234024 [details]
Patch and layout test

Comment 3 Darin Adler 2014-06-27 16:50:17 PDT

Comment on attachment 234024 [details]
Patch and layout test

I don’t think this is an improvement.

Comment 4 Darin Adler 2014-06-27 16:51:17 PDT

(In reply to comment #0)
> Currently we use std::move() to implicitly swap Vector objects.

No, we use it to move a vector. There is no swapping going on; we move the vector from the object into a local variable.

Comment 5 Daniel Bates 2014-06-30 17:14:48 PDT

Comment on attachment 234024 [details]
Patch and layout test

Clearing review flag. After talking with Darin Adler and Anders Carlsson today (06/30), a better approach is to come up with a WTF::move() function that asserts we aren't called with a const value.

Comment 6 Daniel Bates 2014-07-01 16:27:44 PDT

(In reply to comment #5)
> (From update of attachment 234024 [details])
> Clearing review flag. After talking with Darin Adler and Anders Carlsson today (06/30), a better approach is to come up with a WTF::move() function that asserts we aren't called with a const value.

Filed bug #134500 to add WTF::move(). I'll repurpose this bug for adding the layout test, included in attachment #234024 [details].

Comment 7 Daniel Bates 2014-07-03 16:26:54 PDT

Created attachment 234378 [details]
Layout test

Comment 8 David Kilzer (:ddkilzer) 2014-07-03 18:26:30 PDT

Comment on attachment 234378 [details]
Layout test

r=me

Comment 9 Daniel Bates 2014-07-07 10:52:54 PDT

Comment on attachment 234378 [details]
Layout test

Clearing flags on attachment: 234378

Committed r170847: <http://trac.webkit.org/changeset/170847>

Comment 10 Daniel Bates 2014-07-07 10:53:01 PDT

All reviewed patches have been landed.  Closing bug.