Bug 74457 - Change adoptPtr(new ...) to ...::create in Page.cpp
Summary: Change adoptPtr(new ...) to ...::create in Page.cpp
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: Greg Billock
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-13 15:33 PST by Greg Billock
Modified: 2011-12-20 16:24 PST (History)
5 users (show)

See Also:


Attachments
Patch (25.11 KB, patch)
2011-12-13 15:33 PST, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (30.81 KB, patch)
2011-12-14 10:56 PST, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (29.98 KB, patch)
2011-12-14 13:23 PST, Greg Billock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Billock 2011-12-13 15:33:02 PST
Change adoptPtr(new ...) to ...::create in Page.cpp
Comment 1 Greg Billock 2011-12-13 15:33:43 PST
Created attachment 119098 [details]
Patch
Comment 2 Darin Adler 2011-12-13 15:40:03 PST
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 3 Greg Billock 2011-12-14 10:53:43 PST
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.
Comment 4 Greg Billock 2011-12-14 10:56:27 PST
Created attachment 119250 [details]
Patch
Comment 5 Gustavo Noronha (kov) 2011-12-14 12:20:51 PST
Comment on attachment 119250 [details]
Patch

Attachment 119250 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10873339
Comment 6 Greg Billock 2011-12-14 13:23:48 PST
Created attachment 119283 [details]
Patch
Comment 7 Greg Billock 2011-12-14 13:26:35 PST
Took out misfire edit of WebInspectorFrontendClient.
Comment 8 Greg Billock 2011-12-20 13:46:06 PST
Re-tickle cq?
Comment 9 WebKit Review Bot 2011-12-20 16:24:07 PST
Comment on attachment 119283 [details]
Patch

Clearing flags on attachment: 119283

Committed r103365: <http://trac.webkit.org/changeset/103365>
Comment 10 WebKit Review Bot 2011-12-20 16:24:11 PST
All reviewed patches have been landed.  Closing bug.