Bug 55732

Summary: WebKit2: Refactor classes in GenericCallback.h
Product: WebKit Reporter: Jeff Miller <jeffm>
Component: WebKit2Assignee: Jeff Miller <jeffm>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch darin: review+

Description Jeff Miller 2011-03-03 17:20:21 PST
Refactor classes in GenericCallback.h
Comment 1 Jeff Miller 2011-03-03 17:30:50 PST
Created attachment 84663 [details]
Patch
Comment 2 Darin Adler 2011-03-03 18:13:46 PST
Comment on attachment 84663 [details]
Patch

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

Change looks good. Perhaps we should put RefCounted in CallbackBase too?

> Source/WebKit2/ChangeLog:17
> +        Remove FIXME comment about making a version of GenericCallback with two arguments, and defining
> +        ComputedPagesCallback.  This isn't really feasible.  Also, the parameters are a const Vector<WebCore::IntRect>&
> +        and a double, and I don't think a callback with a class and POD parameter types is really worth a generic class.

I don’t understand why it isn’t feasible. I think we can make a template the works with both class and POD types.
Comment 3 Jeff Miller 2011-03-03 21:32:17 PST
(In reply to comment #2)
> (From update of attachment 84663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84663&action=review
> 
> Change looks good. Perhaps we should put RefCounted in CallbackBase too?

Good idea, I'll make this change before landing the patch.

> 
> > Source/WebKit2/ChangeLog:17
> > +        Remove FIXME comment about making a version of GenericCallback with two arguments, and defining
> > +        ComputedPagesCallback.  This isn't really feasible.  Also, the parameters are a const Vector<WebCore::IntRect>&
> > +        and a double, and I don't think a callback with a class and POD parameter types is really worth a generic class.
> 
> I don’t understand why it isn’t feasible. I think we can make a template the works with both class and POD types.

We certainly could, but it seems somewhat unlikely that it would be used more than once.  In any case, I'll restore the FIXME comment.
Comment 4 Jeff Miller 2011-03-03 21:36:49 PST
Committed r80322: <http://trac.webkit.org/changeset/80322>
Comment 5 WebKit Review Bot 2011-03-03 23:07:20 PST
http://trac.webkit.org/changeset/80322 might have broken Windows 7 Release (Tests)
The following tests are not passing:
http/tests/cookies/double-quoted-value-with-semi-colon.html
http/tests/cookies/simple-cookies-expired.html
http/tests/cookies/simple-cookies-max-age.html
http/tests/incremental/split-hex-entities.pl