Bug 83159

Summary: [chromium] Only shut down V8 if we actually initialized it
Product: WebKit Reporter: jochen
Component: New BugsAssignee: jochen
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adamk, dglazkov, fishd, jamesr, rafaelw, tkent, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description jochen 2012-04-04 07:18:42 PDT
[chromium] Only shut down V8 if we actually initialized it
Comment 1 jochen 2012-04-04 07:19:25 PDT
Created attachment 135585 [details]
Patch
Comment 2 Yury Semikhatsky 2012-04-04 07:40:12 PDT
Comment on attachment 135585 [details]
Patch

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

> Source/WebKit/chromium/src/WebKit.cpp:82
> +static bool s_V8Initialized = false;

Pleas guard it with #ifndef NDEBUG
Comment 3 jochen 2012-04-04 07:57:45 PDT
Created attachment 135589 [details]
Patch
Comment 4 WebKit Review Bot 2012-04-04 07:59:17 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 jochen 2012-04-04 07:59:54 PDT
(In reply to comment #2)
> (From update of attachment 135585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135585&action=review
> 
> > Source/WebKit/chromium/src/WebKit.cpp:82
> > +static bool s_V8Initialized = false;
> 
> Pleas guard it with #ifndef NDEBUG

Pavel suggested to introduce shutdownWithoutV8 as a counterpart to initializeWithoutV8 instead
Comment 6 Darin Fisher (:fishd, Google) 2012-04-04 13:09:24 PDT
Hmm... I think the first patch might be nicer because it is less error prone.  Embedders will not need to keep track of how they initialized WebKit.

I'd probably modify the first patch to name the static variable more specifically, like: "s_addedV8CallCompletedCallback"
Comment 7 Darin Fisher (:fishd, Google) 2012-04-04 13:12:46 PDT
It also seems like we could more tightly couple the call to AddCallCompletedCallback with the existence of s_endOfTaskRunner.  Then, we could just have WebKit::shutdown() null-test s_endOfTaskRunner to determine if it should call RemoveCallCompletedCallback.  They really do go hand-in-hand.
Comment 8 jochen 2012-04-05 00:48:24 PDT
Created attachment 135770 [details]
Patch
Comment 9 Pavel Feldman 2012-04-05 00:52:29 PDT
Comment on attachment 135770 [details]
Patch

Rubber stamping since I think you've done what Darin was suggesting.
Comment 10 WebKit Review Bot 2012-04-05 01:53:20 PDT
Comment on attachment 135770 [details]
Patch

Clearing flags on attachment: 135770

Committed r113301: <http://trac.webkit.org/changeset/113301>
Comment 11 WebKit Review Bot 2012-04-05 01:53:26 PDT
All reviewed patches have been landed.  Closing bug.