Bug 67733 - [Chromium] Separate WebKit initialization and V8 initialization in chromium port
Summary: [Chromium] Separate WebKit initialization and V8 initialization in chromium port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-07 14:25 PDT by Dmitry Lomov
Modified: 2011-09-19 09:39 PDT (History)
5 users (show)

See Also:


Attachments
This patch adds initializeV8 function (2.31 KB, patch)
2011-09-07 14:47 PDT, Dmitry Lomov
fishd: review-
Details | Formatted Diff | Diff
This patch adds a parameter to WebKit::initialize instead. (3.07 KB, patch)
2011-09-09 12:01 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff
Obsolete sig removed (2.96 KB, patch)
2011-09-09 12:07 PDT, Dmitry Lomov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fixed botched build. (2.83 KB, patch)
2011-09-09 13:40 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff
Default argument instead if overloads (2.79 KB, patch)
2011-09-12 13:54 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff
Style (2.78 KB, patch)
2011-09-13 11:39 PDT, Dmitry Lomov
fishd: review-
Details | Formatted Diff | Diff
CR feedback (2.77 KB, patch)
2011-09-16 12:05 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2011-09-07 14:25:51 PDT
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.
Comment 1 Dmitry Lomov 2011-09-07 14:47:56 PDT
Created attachment 106643 [details]
This patch adds initializeV8 function
Comment 2 Adam Barth 2011-09-07 14:52:09 PDT
+fishd for WebKit API review.
Comment 3 Darin Fisher (:fishd, Google) 2011-09-08 00:07:56 PDT
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?
Comment 4 Dmitry Lomov 2011-09-08 10:32:58 PDT
(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?
Comment 5 Darin Fisher (:fishd, Google) 2011-09-09 09:40:58 PDT
(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.
Comment 6 Dmitry Lomov 2011-09-09 12:01:35 PDT
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?
Comment 7 Dmitry Lomov 2011-09-09 12:07:59 PDT
Created attachment 106899 [details]
Obsolete sig removed
Comment 8 WebKit Review Bot 2011-09-09 12:32:54 PDT
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
Comment 9 Dmitry Lomov 2011-09-09 13:40:44 PDT
Created attachment 106912 [details]
Fixed botched build.
Comment 10 Adam Barth 2011-09-09 17:05:59 PDT
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.
Comment 11 Dmitry Lomov 2011-09-12 12:59:27 PDT
(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.
Comment 12 Adam Barth 2011-09-12 13:27:12 PDT
> 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.
Comment 13 Dmitry Lomov 2011-09-12 13:54:09 PDT
Created attachment 107080 [details]
Default argument instead if overloads
Comment 14 Adam Barth 2011-09-12 13:56:09 PDT
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.
Comment 15 WebKit Review Bot 2011-09-12 13:57:19 PDT
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.
Comment 16 Dmitry Lomov 2011-09-13 10:15:43 PDT
(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 :)
Comment 17 Dmitry Lomov 2011-09-13 11:39:07 PDT
Created attachment 107202 [details]
Style
Comment 18 Darin Fisher (:fishd, Google) 2011-09-13 20:43:29 PDT
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.
Comment 19 Dmitry Lomov 2011-09-14 09:34:43 PDT
(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?
Comment 20 Darin Fisher (:fishd, Google) 2011-09-15 23:48:06 PDT
(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.
Comment 21 Dmitry Lomov 2011-09-16 08:55:20 PDT
> 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 :)
Comment 22 Dmitry Lomov 2011-09-16 12:05:33 PDT
Created attachment 107697 [details]
CR feedback
Comment 23 Darin Fisher (:fishd, Google) 2011-09-16 14:53:03 PDT
Comment on attachment 107697 [details]
CR feedback

R=me
Comment 24 Darin Fisher (:fishd, Google) 2011-09-16 14:54:19 PDT
(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.
Comment 25 Dmitry Lomov 2011-09-16 15:07:55 PDT
(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
Comment 26 WebKit Review Bot 2011-09-16 22:29:21 PDT
Comment on attachment 107697 [details]
CR feedback

Clearing flags on attachment: 107697

Committed r95368: <http://trac.webkit.org/changeset/95368>
Comment 27 WebKit Review Bot 2011-09-16 22:29:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Darin Fisher (:fishd, Google) 2011-09-16 22:32:17 PDT
(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.
Comment 29 Adam Barth 2011-09-16 22:38:39 PDT
Maciej affectionally calls this the "you ain't gonna need it" principle:

http://en.wikipedia.org/wiki/You_ain't_gonna_need_it
Comment 30 Dmitry Lomov 2011-09-19 09:39:12 PDT
(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.