| Summary: | Wrap uses of KJS in core classes with USE(JSC) | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> |
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | pinkerton |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Mac | ||
| OS: | OS X 10.5 | ||
| Bug Depends on: | |||
| Bug Blocks: | 20619 | ||
| Attachments: | |||
|
Description
Eric Seidel (no email)
2008-09-03 04:22:24 PDT
Created attachment 23135 [details]
Add #if USE(JAVASCRIPTCORE_BINDINGS) around KJS dependencies
WebCore/platform/text/AtomicString.cpp | 4 ++++
WebCore/platform/text/AtomicString.h | 6 ++++++
WebCore/platform/text/PlatformString.h | 8 ++++++++
WebCore/platform/text/String.cpp | 5 ++++-
WebCore/platform/text/StringImpl.cpp | 4 ----
WebCore/platform/text/StringImpl.h | 1 -
6 files changed, 22 insertions(+), 6 deletions(-)
This is not all of them, and this is actually more changes than we've made locally (due to MSVC being smart enough to strip away functions we don't use), but this gets rid of at least 3 more forked files for us! Created attachment 23137 [details]
Clean up Platform.h and add PLATFORM(CHROMIUM) and USE(V8_BINDINGS)
.../Configurations/JavaScriptCore.xcconfig | 2 +-
JavaScriptCore/wtf/Platform.h | 22 +++++++++++--------
WebCore/Configurations/WebCore.xcconfig | 2 +-
WebCore/config.h | 19 ++++++++---------
WebKit/mac/Configurations/WebKit.xcconfig | 2 +-
5 files changed, 25 insertions(+), 22 deletions(-)
I'm slightly re-defining what USE(JAVASCRIPTCORE_BINDINGS) means in this patch. Mostly because I don't see when/why anyone would ever want to depend on JSC but not use the plugin bindings (which seems to be on for all platforms). Created attachment 23139 [details]
Clean up Platform.h and add PLATFORM(CHROMIUM) and USE(V8_BINDINGS)
.../Configurations/JavaScriptCore.xcconfig | 2 +-
JavaScriptCore/wtf/Platform.h | 22 +++++++++++--------
WebCore/Configurations/WebCore.xcconfig | 2 +-
WebCore/config.h | 21 ++++++++++---------
WebKit/mac/Configurations/WebKit.xcconfig | 2 +-
5 files changed, 27 insertions(+), 22 deletions(-)
Eric, please include changelogs with your patches (at least to set a good example). Created attachment 23148 [details]
Add #if USE(JAVASCRIPTCORE_BINDINGS) around KJS dependencies
WebCore/ChangeLog | 13 +++++++++++++
WebCore/platform/text/AtomicString.cpp | 4 ++++
WebCore/platform/text/AtomicString.h | 6 ++++++
WebCore/platform/text/PlatformString.h | 8 ++++++++
WebCore/platform/text/String.cpp | 5 ++++-
WebCore/platform/text/StringImpl.cpp | 4 ----
WebCore/platform/text/StringImpl.h | 1 -
7 files changed, 35 insertions(+), 6 deletions(-)
Created attachment 23149 [details]
Clean up Platform.h and add PLATFORM(CHROMIUM), PLATFORM(SKIA) and USE(V8_BINDINGS)
JavaScriptCore/ChangeLog | 10 +++++
.../Configurations/JavaScriptCore.xcconfig | 2 +-
JavaScriptCore/wtf/ASCIICType.h | 1 +
JavaScriptCore/wtf/Platform.h | 37 +++++++++++++-------
WebCore/ChangeLog | 9 +++++
WebCore/Configurations/WebCore.xcconfig | 2 +-
WebCore/config.h | 21 ++++++-----
WebKit/mac/ChangeLog | 9 +++++
WebKit/mac/Configurations/WebKit.xcconfig | 2 +-
WebKit/mac/WebKitPrefix.h | 5 +++
10 files changed, 72 insertions(+), 26 deletions(-)
Comment on attachment 23148 [details]
Add #if USE(JAVASCRIPTCORE_BINDINGS) around KJS dependencies
I am not a fan of reusing USE(JAVASCRIPTCORE_BINDINGS) due to it's muddled past. Perhaps USE(JSC) would be cleaner.
Comment on attachment 23149 [details]
Clean up Platform.h and add PLATFORM(CHROMIUM), PLATFORM(SKIA) and USE(V8_BINDINGS)
This comment is not necessary.
+/* Chromium runs WebKit multi-process/sandboxed, and provides its own */
+/* OS-level abstractions to work in this environment. */
Will this break windows builds that use CG?
+/* Makes PLATFORM(WIN) default to PLATFORM(CAIRO) */
+#if !PLATFORM(MAC) && !PLATFORM(QT) && !PLATFORM(WX)
Why not do this in Platform.h
+#if !defined(WTF_USE_JAVASCRIPTCORE_BINDINGS) && !defined(WTF_USE_V8_BINDINGS)
+/* Currently Chromium is the only platform which uses V8 by default */
+#if PLATFORM(CHROMIUM)
+#define WTF_USE_JAVASCRIPTCORE_BINDINGS 0
+#define WTF_USE_V8_BINDINGS 1
+#else
+#define WTF_USE_JAVASCRIPTCORE_BINDINGS 1
+#define WTF_USE_V8_BINDINGS 0
+#endif /* PLATFORM(CHROMIUM) */
+#endif /* !defined(WTF_USE_JAVASCRIPTCORE_BINDINGS) && !defined(WTF_USE_V8_BINDINGS) */
Why is this change ok?
-#define ENABLE_NETSCAPE_PLUGIN_API 0
+#define ENABLE_NETSCAPE_PLUGIN_API 1
Comment on attachment 23148 [details]
Add #if USE(JAVASCRIPTCORE_BINDINGS) around KJS dependencies
r=me if you switch to use USE(JSC)
(In reply to comment #10) > (From update of attachment 23149 [details] [edit]) > This comment is not necessary. > +/* Chromium runs WebKit multi-process/sandboxed, and provides its own */ > +/* OS-level abstractions to work in this environment. */ OK, I figured all the other platforms had (vacuous) comments, I should add one too. :) > Will this break windows builds that use CG? > +/* Makes PLATFORM(WIN) default to PLATFORM(CAIRO) */ > +#if !PLATFORM(MAC) && !PLATFORM(QT) && !PLATFORM(WX) Nah, this is kinda strange, but the Apple windows build fixes up its defines in WebCore/config.h Basically we need a better way to define what you're building. Window-Apple, Windows-Cairo, Windows-Chromium, etc. > Why not do this in Platform.h > +#if !defined(WTF_USE_JAVASCRIPTCORE_BINDINGS) && !defined(WTF_USE_V8_BINDINGS) > +/* Currently Chromium is the only platform which uses V8 by default */ > +#if PLATFORM(CHROMIUM) > +#define WTF_USE_JAVASCRIPTCORE_BINDINGS 0 > +#define WTF_USE_V8_BINDINGS 1 > +#else > +#define WTF_USE_JAVASCRIPTCORE_BINDINGS 1 > +#define WTF_USE_V8_BINDINGS 0 > +#endif /* PLATFORM(CHROMIUM) */ > +#endif /* !defined(WTF_USE_JAVASCRIPTCORE_BINDINGS) && > !defined(WTF_USE_V8_BINDINGS) */ Cause Platform.h is part of JavaScriptCore, and this stuff had been part of WebCore in the past. > Why is this change ok? > -#define ENABLE_NETSCAPE_PLUGIN_API 0 > +#define ENABLE_NETSCAPE_PLUGIN_API 1 Yup. All platforms had it on. Platforms can still turn it off by defining than as part of their build system. > > Why is this change ok?
> > -#define ENABLE_NETSCAPE_PLUGIN_API 0
> > +#define ENABLE_NETSCAPE_PLUGIN_API 1
>
> Yup. All platforms had it on. Platforms can still turn it off by defining
> than as part of their build system.
What do you mean by "All platforms had it on"? Wx doesn't support NPAPI, nor does Gtk on Windows.
(In reply to comment #13) > > > Why is this change ok? > > > -#define ENABLE_NETSCAPE_PLUGIN_API 0 > > > +#define ENABLE_NETSCAPE_PLUGIN_API 1 > > > > Yup. All platforms had it on. Platforms can still turn it off by defining > > than as part of their build system. > > What do you mean by "All platforms had it on"? Wx doesn't support NPAPI, nor > does Gtk on Windows. Then it seems like a blacklist is more appropriate than the whitelist we had. These would be: PLATFORM(WX) and PLATFORM(GTK) && PLATFORM(WIN) I assume? (In reply to comment #14) > (In reply to comment #13) > > > > Why is this change ok? > > > > -#define ENABLE_NETSCAPE_PLUGIN_API 0 > > > > +#define ENABLE_NETSCAPE_PLUGIN_API 1 > > > > > > Yup. All platforms had it on. Platforms can still turn it off by defining > > > than as part of their build system. > > > > What do you mean by "All platforms had it on"? Wx doesn't support NPAPI, nor > > does Gtk on Windows. It seems that Platform.h from ToT doesn't agree with you: #if (PLATFORM(MAC) || PLATFORM(GTK) || PLATFORM(SYMBIAN) || PLATFORM(WIN) || PLATFORM(WX)) && !defined(ENABLE_NETSCAPE_PLUGIN_API) #define ENABLE_NETSCAPE_PLUGIN_API 1 #endif My change is correct at least for the two examples you posted. Created attachment 23152 [details]
Add #if USE(JSC) around KJS dependencies
WebCore/ChangeLog | 13 ++++++++
WebCore/bindings/js/JSPluginElementFunctions.cpp | 35 ----------------------
WebCore/config.h | 22 +++++++------
WebCore/html/HTMLAppletElement.cpp | 2 -
WebCore/html/HTMLAppletElement.h | 2 -
WebCore/html/HTMLEmbedElement.cpp | 6 ----
WebCore/html/HTMLEmbedElement.h | 2 -
WebCore/html/HTMLObjectElement.cpp | 5 ---
WebCore/html/HTMLObjectElement.h | 2 -
WebCore/html/HTMLPlugInElement.cpp | 6 ++--
WebCore/html/HTMLPlugInElement.h | 8 ++--
WebCore/platform/text/AtomicString.cpp | 4 ++
WebCore/platform/text/AtomicString.h | 6 ++++
WebCore/platform/text/PlatformString.h | 8 +++++
WebCore/platform/text/String.cpp | 5 ++-
WebCore/platform/text/StringImpl.cpp | 4 --
WebCore/platform/text/StringImpl.h | 1 -
17 files changed, 54 insertions(+), 77 deletions(-)
Created attachment 23153 [details]
Clean up Platform.h and add PLATFORM(CHROMIUM), PLATFORM(SKIA) and USE(V8_BINDINGS)
JavaScriptCore/ChangeLog | 10 ++++++
.../Configurations/JavaScriptCore.xcconfig | 2 +-
JavaScriptCore/wtf/ASCIICType.h | 1 +
JavaScriptCore/wtf/Platform.h | 34 ++++++++++++-------
WebCore/ChangeLog | 9 +++++
WebCore/Configurations/WebCore.xcconfig | 2 +-
WebKit/mac/ChangeLog | 9 +++++
WebKit/mac/Configurations/WebKit.xcconfig | 2 +-
WebKit/mac/WebKitPrefix.h | 5 +++
9 files changed, 58 insertions(+), 16 deletions(-)
Comment on attachment 23152 [details]
Add #if USE(JSC) around KJS dependencies
Needs an updated changelog.
|