Summary: | Change adoptPtr(new ...) to ...::create in Page.cpp | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Greg Billock <gbillock> | ||||||||
Component: | New Bugs | Assignee: | Greg Billock <gbillock> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | gustavo, japhet, levin, webkit.review.bot, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Greg Billock
2011-12-13 15:33:02 PST
Created attachment 119098 [details]
Patch
Comment on attachment 119098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119098&action=review This looks fine, but does not go far enough. In most of these cases, the constructors of these classes should be made private at the same time. I’d rather see a patch that does fewer classes at a time, but makes the constructors private for those classes. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Can’t land a patch with that line in it. > Source/WebCore/loader/ProgressTracker.h:31 > +#include <wtf/PassOwnPtr.h> If we just have a return type of PassOwnPtr, I think we can do with just <wtf/Forward.h>. > Source/WebCore/notifications/NotificationController.h:32 > +#include <wtf/PassOwnPtr.h> Ditto. > Source/WebCore/page/SpeechInput.h:39 > +#include <wtf/PassOwnPtr.h> I’m surprised this include is needed. Doesn’t Forward.h forward-declare PassOwnPtr? Comment on attachment 119098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119098&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > Can’t land a patch with that line in it. Definitely. :-) My plan was to ask David about the intended fate of the constructors, but you're too quick. :-) It sounds like they should become private. I'll get some guidance about writing tests. These changes should be invisible for layout tests, right? >> Source/WebCore/loader/ProgressTracker.h:31 >> +#include <wtf/PassOwnPtr.h> > > If we just have a return type of PassOwnPtr, I think we can do with just <wtf/Forward.h>. Done. >> Source/WebCore/notifications/NotificationController.h:32 >> +#include <wtf/PassOwnPtr.h> > > Ditto. Done. > Source/WebCore/page/PageGroup.h:52 > + static PassOwnPtr<PageGroup> create(Page*); Should I also make a factory for PageGroup(const String&)? >> Source/WebCore/page/SpeechInput.h:39 >> +#include <wtf/PassOwnPtr.h> > > I’m surprised this include is needed. Doesn’t Forward.h forward-declare PassOwnPtr? Moved to .cpp file. Created attachment 119250 [details]
Patch
Comment on attachment 119250 [details] Patch Attachment 119250 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10873339 Created attachment 119283 [details]
Patch
Took out misfire edit of WebInspectorFrontendClient. Re-tickle cq? Comment on attachment 119283 [details] Patch Clearing flags on attachment: 119283 Committed r103365: <http://trac.webkit.org/changeset/103365> All reviewed patches have been landed. Closing bug. |