[Webkit-unassigned] [Bug 109305] [WK2] Page reloading will crash UIProcess after WebProcess was killed

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=109305





--- Comment #26 from Benjamin Poulain <benjamin at webkit.org>  2013-02-11 23:56:31 PST ---
(From update of attachment 187768)
View in context: https://bugs.webkit.org/attachment.cgi?id=187768&action=review

Some comments:

> Source/WebKit2/ChangeLog:9
> +        Re-initialize pointer to WebInspectorProxy object before calling
> +        initializeWebPage().

You should also explain why you make the change, not only what the change is.

> Tools/ChangeLog:14
> +        * TestWebKitAPI/Tests/WebKit2/CrashReload.cpp: Added.

I'd argue the name CrashReload is a little too generic. It is hard to pinpoint what is tested in there.

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1091
>  				2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */,
>  				2E7765CF16C4D81100BA2BB1 /* mainMac.mm in Sources */,
> +				8A3AF93B16C9ED2700D248C1 /* CrashReload.cpp in Sources */,

The build section should be sorted alphabetically

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:35
> +static bool first = false;
> +static bool second = false;

Those names are too generic.
First, could they be made local instead of being global?
Second, they should be named in a way take make their purpose obvious.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:39
> +    // Ok, this is the first load.

Comments like this are not helping.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:43
> +    if (!first) {
> +        first = true;
> +        return;
> +    }

Should't this also check second is false? If second is true here

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:45
> +    // Second time is the charm.

Comments like this are not helping, comments should explain the "Why" of the code.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:74
> +    // Load a blank page and next kills it

Period at the end of the sentence.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.