Bug 74457 – Change adoptPtr(new ...) to ...::create in Page.cpp

• Component: New Bugs
• Status: RESOLVED FIXED

[Greg Billock]

Change adoptPtr(new ...) to ...::create in Page.cpp

[Greg Billock]

Created attachment 119098 [details]
Patch

[Darin Adler]

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?

[Greg Billock]

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.

[Greg Billock]

Created attachment 119250 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 119250 [details]
Patch

Attachment 119250 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10873339

[Greg Billock]

Created attachment 119283 [details]
Patch

[Greg Billock]

Took out misfire edit of WebInspectorFrontendClient.

[Greg Billock]

Re-tickle cq?

[WebKit Review Bot]

Comment on attachment 119283 [details]
Patch

Clearing flags on attachment: 119283

Committed r103365: <http://trac.webkit.org/changeset/103365>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.