Bug 67733 – [Chromium] Separate WebKit initialization and V8 initialization in chromium port

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Dmitry Lomov]

Adding V8 initialization to WebKit::initialize had an unfortunate side effect that V8 is initialized in Chromium browser process.
V8 initialization should be factored out of WebKit initialization.

[Dmitry Lomov]

Created attachment 106643 [details]
This patch adds initializeV8 function

[Adam Barth]

+fishd for WebKit API review.

[Darin Fisher (:fishd, Google)]

Comment on attachment 106643 [details]
This patch adds initializeV8 function

Hmm, this could also be addressed by adding a way to pass options to the "initialize" method.  Had you considered that approach instead?

The current solution has the side-effect that it makes it possible to do work after webkit is initialized but before v8 is initialized.  Do we require that kind of flexibility?

[Dmitry Lomov]

(In reply to comment #3)
> (From update of attachment 106643 [details])
> Hmm, this could also be addressed by adding a way to pass options to the "initialize" method.  Had you considered that approach instead?

I did - I do not have a strong opinion, but I feel this way we will have a nice separation of concerns. WebKit and V8 are (semi) independent subsystems.  

> 
> The current solution has the side-effect that it makes it possible to do work after webkit is initialized but before v8 is initialized.  Do we require that kind of flexibility?

We do not require it at the moment, but WenKit is certainly usable and useful without initialized V8, and we might find a need to lazily initialize V8 in some scenarios.

Why do you think having an option on initialize method is preferable?

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> Why do you think having an option on initialize method is preferable?

I'm concerned that callers will forget to initialize V8.  I'd prefer a mechanism that forces callers to opt-in to not initializing V8 as that seems like the uncommon case.

[Dmitry Lomov]

Created attachment 106896 [details]
This patch adds a parameter to WebKit::initialize instead.

What do you think about this variant?
I have added an overload with the old signature too (that initialized V8 by default).

This other overload is needed anyway until corresponding changes in chromium are done. After that it can be kept or removed. I am leaning towards keeping it and using non-default only in special cases, but then we'll have an equivalent of a parameter with default argument, which is not good. What do you think is the right call?

[Dmitry Lomov]

Created attachment 106899 [details]
Obsolete sig removed

[WebKit Review Bot]

Comment on attachment 106899 [details]
Obsolete sig removed

Attachment 106899 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9625740

[Dmitry Lomov]

Created attachment 106912 [details]
Fixed botched build.

[Adam Barth]

Comment on attachment 106912 [details]
Fixed botched build.

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

> Source/WebKit/chromium/public/WebKit.h:42
> +    INITIALIZE_V8,
> +    DO_NOT_INITIALIZE_V8

These names should be in CamelCase.

> Source/WebKit/chromium/public/WebKit.h:53
> +// INITIALIZE_V8 should be passed if V8 is planned to be used.
> +WEBKIT_EXPORT void initialize(WebKitPlatformSupport*, InitializeV8);
> +
> +// Version of initialize function that initializes V8.
>  WEBKIT_EXPORT void initialize(WebKitPlatformSupport*);

Why not use a default value for the argument?  That seems more self-documenting.

[Dmitry Lomov]

(In reply to comment #10)
> (From update of attachment 106912 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106912&action=review
> 
> > Source/WebKit/chromium/public/WebKit.h:42
> > +    INITIALIZE_V8,
> > +    DO_NOT_INITIALIZE_V8
> 
> These names should be in CamelCase.
> 
> > Source/WebKit/chromium/public/WebKit.h:53
> > +// INITIALIZE_V8 should be passed if V8 is planned to be used.
> > +WEBKIT_EXPORT void initialize(WebKitPlatformSupport*, InitializeV8);
> > +
> > +// Version of initialize function that initializes V8.
> >  WEBKIT_EXPORT void initialize(WebKitPlatformSupport*);
> 
> Why not use a default value for the argument?  That seems more self-documenting.

My understanding was that the use of default arguments is frowned upon, but I certainly agree.

[Adam Barth]

> My understanding was that the use of default arguments is frowned upon, but I certainly agree.

I believe they are frowned upon in Google style but not in WebKit style.  The Chromium WebKit API uses WebKit style.

[Dmitry Lomov]

Created attachment 107080 [details]
Default argument instead if overloads

[Adam Barth]

Comment on attachment 107080 [details]
Default argument instead if overloads

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

> Source/WebKit/chromium/public/WebKit.h:49
> +// INITIALIZE_V8 should be passed if V8 is planned to be used.

nit: INITIALIZE_V8 => InitializeV8.

LGTM, but we'll need fishd to review.

[WebKit Review Bot]

Attachment 107080 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/public/WebKit.h:50:  The parameter name "initializeV8" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Lomov]

(In reply to comment #12)
> > My understanding was that the use of default arguments is frowned upon, but I certainly agree.
> 
> I believe they are frowned upon in Google style but not in WebKit style.  The Chromium WebKit API uses WebKit style.

Right -  I was worried that since WebKit API is exposed to chromium, folks will not be happy about it. Apparently this is not the case :)

[Dmitry Lomov]

Created attachment 107202 [details]
Style

[Darin Fisher (:fishd, Google)]

Comment on attachment 107202 [details]
Style

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

> Source/WebKit/chromium/public/WebKit.h:40
> +enum ShouldInitializeV8 {

nit: We use a specific naming convention for enums in the WebKit API:

  enum Foo {
     FooA,
     FooB
  };

The values are prefixed with the type name in other words.

Also, we prefix types with "Web" unless they are defined within a class.

However, this suggests something fairly awkward (WebKitV8InitializationOptions), which makes me sad.

Perhaps a better function is to have two functions:

  WEBKIT_EXPORT void initialize(WebKitPlatformSupport*);
  WEBKIT_EXPORT void initializeWithoutV8(WebKitPlatformSupport*);

Then, in the implementation we can have initialize call initializeWithoutV8 followed by the code
to actually initialize V8.

[Dmitry Lomov]

(In reply to comment #18)
> (From update of attachment 107202 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107202&action=review

> Perhaps a better function is to have two functions:
> 
>   WEBKIT_EXPORT void initialize(WebKitPlatformSupport*);
>   WEBKIT_EXPORT void initializeWithoutV8(WebKitPlatformSupport*);
> 
> Then, in the implementation we can have initialize call initializeWithoutV8 followed by the code
> to actually initialize V8.

Hmm if we have another subsystem with optional initialization (let's call it Veder), how will this scale?
initialize
initializeWithoutV8
initializeWithoutV8andVeder
initializeWithoutV8AndVeder
?

I'd rather go with flags, even if that means WebKitV8InitializationOptions. Thoughts?

[Darin Fisher (:fishd, Google)]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 107202 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=107202&action=review
> 
> > Perhaps a better function is to have two functions:
> > 
> >   WEBKIT_EXPORT void initialize(WebKitPlatformSupport*);
> >   WEBKIT_EXPORT void initializeWithoutV8(WebKitPlatformSupport*);
> > 
> > Then, in the implementation we can have initialize call initializeWithoutV8 followed by the code
> > to actually initialize V8.
> 
> Hmm if we have another subsystem with optional initialization (let's call it Veder), how will this scale?
> initialize
> initializeWithoutV8
> initializeWithoutV8andVeder
> initializeWithoutV8AndVeder

We can change this when that becomes true, no?  When that becomes true, I would prefer that we have an Options struct (e.g., WebURLLoaderOptions).

> I'd rather go with flags, even if that means WebKitV8InitializationOptions. Thoughts?

But that flags type is not extensible.  It only deals with the V8.  I would go with initializeWithoutV8 for now, and worry about extensibility later when it matters.

[Dmitry Lomov]

> But that flags type is not extensible.  It only deals with the V8.  I would go with initializeWithoutV8 for now, and worry about extensibility later when it matters.

If you say so sir :)

[Dmitry Lomov]

Created attachment 107697 [details]
CR feedback

[Darin Fisher (:fishd, Google)]

Comment on attachment 107697 [details]
CR feedback

R=me

[Darin Fisher (:fishd, Google)]

(In reply to comment #21)
> > But that flags type is not extensible.  It only deals with the V8.  I would go with initializeWithoutV8 for now, and worry about extensibility later when it matters.
> 
> If you say so sir :)

It sounds like I didn't convince you?  My intention is not to strong-arm you.

[Dmitry Lomov]

(In reply to comment #24)
> (In reply to comment #21)
> > > But that flags type is not extensible.  It only deals with the V8.  I would go with initializeWithoutV8 for now, and worry about extensibility later when it matters.
> > 
> > If you say so sir :)
> 
> It sounds like I didn't convince you?  My intention is not to strong-arm you.

Well I think it is better to put the right and sustainable pattern into the code upfront, so I'd rather go with flags or even the whole options struct. But I really just want to move on with this patch

[WebKit Review Bot]

Comment on attachment 107697 [details]
CR feedback

Clearing flags on attachment: 107697

Committed r95368: <http://trac.webkit.org/changeset/95368>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

(In reply to comment #25)
> > It sounds like I didn't convince you?  My intention is not to strong-arm you.
> 
> Well I think it is better to put the right and sustainable pattern into the code upfront, so I'd rather go with flags or even the whole options struct. But I really just want to move on with this patch

Yeah... it is just common practice in WebKit to generalization until it is necessary or foresee-ably necessary.  We don't know what other options we might need if ever.

[Adam Barth]

Maciej affectionally calls this the "you ain't gonna need it" principle:

http://en.wikipedia.org/wiki/You_ain't_gonna_need_it

[Dmitry Lomov]

(In reply to comment #29)
> Maciej affectionally calls this the "you ain't gonna need it" principle:
> 
> http://en.wikipedia.org/wiki/You_ain't_gonna_need_it

I respectfully disagree - I do already need initialization options, I might as well get them right from the start, and not leave the debt to whoever comes next.