Bug 20620 – Wrap uses of KJS in core classes with USE(JSC)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Wrap uses of KJS in core classes with USE(JAVASCRIPTCORE_BINDINGS)

We can also possibly add a USE(V8_BINDINGS) as Chromium has.

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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!

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

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(-)

[Sam Weinig]

Eric, please include changelogs with your patches (at least to set a good example).

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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(-)

[Sam Weinig]

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.

[Sam Weinig]

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

[Sam Weinig]

Comment on attachment 23148 [details]
Add #if USE(JAVASCRIPTCORE_BINDINGS) around KJS dependencies

r=me if you switch to use USE(JSC)

[Eric Seidel (no email)]

(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.

[Mark Rowe (bdash)]

> > 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.

[Eric Seidel (no email)]

(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?

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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(-)

[Sam Weinig]

Comment on attachment 23152 [details]
Add #if USE(JSC) around KJS dependencies

Needs an updated changelog.