Summary: | [Chromium] Separate WebKit initialization and V8 initialization in chromium port | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dmitry Lomov <dslomov> | ||||||||||||||||
Component: | WebKit Misc. | Assignee: | Dmitry Lomov <dslomov> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, tonyg, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Dmitry Lomov
2011-09-07 14:25:51 PDT
Created attachment 106643 [details]
This patch adds initializeV8 function
+fishd for WebKit API review. 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?
(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? (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. 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?
Created attachment 106899 [details]
Obsolete sig removed
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 Created attachment 106912 [details]
Fixed botched build.
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. (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. > 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.
Created attachment 107080 [details]
Default argument instead if overloads
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. 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.
(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 :) Created attachment 107202 [details]
Style
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. (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? (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.
> 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 :)
Created attachment 107697 [details]
CR feedback
Comment on attachment 107697 [details]
CR feedback
R=me
(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. (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 on attachment 107697 [details] CR feedback Clearing flags on attachment: 107697 Committed r95368: <http://trac.webkit.org/changeset/95368> All reviewed patches have been landed. Closing bug. (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. Maciej affectionally calls this the "you ain't gonna need it" principle: http://en.wikipedia.org/wiki/You_ain't_gonna_need_it (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. |