Bug 109076 - Fix CompactHTMLToken's copy ctor to copy all fields
Summary: Fix CompactHTMLToken's copy ctor to copy all fields
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 106127
  Show dependency tree / graph
 
Reported: 2013-02-06 11:30 PST by Tony Gentilcore
Modified: 2013-02-06 13:51 PST (History)
4 users (show)

See Also:

Attachments
Patch (1.68 KB, patch)
2013-02-06 11:31 PST, Tony Gentilcore 		no flags Details | Formatted Diff | Diff
Patch for landing (2.12 KB, patch)
2013-02-06 13:03 PST, Tony Gentilcore 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2013-02-06 11:30:06 PST 
Fix CompactHTMLToken's copy ctor to copy all fields
Comment 1 Tony Gentilcore 2013-02-06 11:31:08 PST 
Created attachment 186883 [details]
Patch
Comment 2 Darin Adler 2013-02-06 12:32:33 PST 
Comment on attachment 186883 [details]
Patch

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

> Source/WebCore/html/parser/CompactHTMLToken.cpp:91
>  CompactHTMLToken::CompactHTMLToken(const CompactHTMLToken& other)

Sure would be nice if this could have been left to the compiler to generate. All we’d need would be a version of OwnPtr that knows how to clone, but I suppose that’s a lot to ask.

> Source/WebCore/html/parser/CompactHTMLToken.cpp:92
>      : m_type(other.type())

Seems to me that it’s an anti-pattern to use getter functions in copy constructors like this one.
Comment 3 Tony Gentilcore 2013-02-06 13:03:07 PST 
Created attachment 186904 [details]
Patch for landing
Comment 4 Tony Gentilcore 2013-02-06 13:03:52 PST 
(In reply to comment #2)
> (From update of attachment 186883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186883&action=review
> 
> > Source/WebCore/html/parser/CompactHTMLToken.cpp:91
> >  CompactHTMLToken::CompactHTMLToken(const CompactHTMLToken& other)
> 
> Sure would be nice if this could have been left to the compiler to generate. All we’d need would be a version of OwnPtr that knows how to clone, but I suppose that’s a lot to ask.

Good point, but I'll leave that for another day if you don't mind.

> 
> > Source/WebCore/html/parser/CompactHTMLToken.cpp:92
> >      : m_type(other.type())
> 
> Seems to me that it’s an anti-pattern to use getter functions in copy constructors like this one.

Fixed.
Comment 5 WebKit Review Bot 2013-02-06 13:51:26 PST 
Comment on attachment 186904 [details]
Patch for landing

Clearing flags on attachment: 186904

Committed r142029: <http://trac.webkit.org/changeset/142029>
Comment 6 WebKit Review Bot 2013-02-06 13:51:30 PST 
All reviewed patches have been landed.  Closing bug.