Bug 121467 - [Windows] Clean up WinLauncher by using smart pointers
Summary: [Windows] Clean up WinLauncher by using smart pointers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-16 21:08 PDT by Brent Fulgham
Modified: 2013-09-16 21:22 PDT (History)
0 users

See Also:


Attachments
Patch (19.86 KB, patch)
2013-09-16 21:11 PDT, Brent Fulgham
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2013-09-16 21:08:36 PDT
Get rid of a lot of ugly (and error-prone) boilerplate by switching to smart pointers for the various data types in the program.
Comment 1 Brent Fulgham 2013-09-16 21:11:23 PDT
Created attachment 211861 [details]
Patch
Comment 2 Anders Carlsson 2013-09-16 21:15:18 PDT
Comment on attachment 211861 [details]
Patch

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

> Tools/WinLauncher/WinLauncher.cpp:82
> +IWebInspectorPtr gInspector;
> +IWebViewPtr gWebView;
> +IWebViewPrivatePtr gWebViewPrivate;
> +IWebPreferencesPtr gStandardPreferences;
> +IWebPreferencesPrivatePtr gPrefsPrivate;

I don't think these need to be smart pointers, and I think that making them smart pointers will cause global constructors.

> Tools/WinLauncher/WinLauncher.cpp:88
> +std::vector<IWebHistoryItemPtr> gHistoryItems;

Same thing here. Maybe you should just create an application class/struct?

> Tools/WinLauncher/WinLauncher.cpp:537
> +        static _bstr_t defaultHTML(TEXT("<p style=\"background-color: #00FF00\">Testing</p><img id=\"webkit logo\" src=\"http://webkit.org/images/icon-gold.png\" alt=\"Face\"><div style=\"border: solid blue; background: white;\" contenteditable=\"true\">div with blue border</div><ul><li>foo<li>bar<li>baz</ul>"));

I don't think this needs to be static.

> Tools/WinLauncher/WinLauncher.cpp:915
> +    static _bstr_t methodBStr(TEXT("GET"));

Same thing here.
Comment 3 Brent Fulgham 2013-09-16 21:21:30 PDT
Comment on attachment 211861 [details]
Patch

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

Thanks for looking this over. I'll make some more adjustments in a follow-up patch.

>> Tools/WinLauncher/WinLauncher.cpp:82
>> +IWebPreferencesPrivatePtr gPrefsPrivate;
> 
> I don't think these need to be smart pointers, and I think that making them smart pointers will cause global constructors.

Hmm. I have some other cleanup to do; I'll revise this when I make those changes.

>> Tools/WinLauncher/WinLauncher.cpp:88
>> +std::vector<IWebHistoryItemPtr> gHistoryItems;
> 
> Same thing here. Maybe you should just create an application class/struct?

Agreed. I'll do that next.

>> Tools/WinLauncher/WinLauncher.cpp:537
>> +        static _bstr_t defaultHTML(TEXT("<p style=\"background-color: #00FF00\">Testing</p><img id=\"webkit logo\" src=\"http://webkit.org/images/icon-gold.png\" alt=\"Face\"><div style=\"border: solid blue; background: white;\" contenteditable=\"true\">div with blue border</div><ul><li>foo<li>bar<li>baz</ul>"));
> 
> I don't think this needs to be static.

Done.

>> Tools/WinLauncher/WinLauncher.cpp:915
>> +    static _bstr_t methodBStr(TEXT("GET"));
> 
> Same thing here.

Done.
Comment 4 Brent Fulgham 2013-09-16 21:22:12 PDT
Committed r155929: <http://trac.webkit.org/changeset/155929>