Bug 20620 - Wrap uses of KJS in core classes with USE(JSC)
Summary: Wrap uses of KJS in core classes with USE(JSC)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 20619
  Show dependency treegraph
 
Reported: 2008-09-03 04:22 PDT by Eric Seidel (no email)
Modified: 2008-09-03 18:36 PDT (History)
1 user (show)

See Also:


Attachments
Add #if USE(JAVASCRIPTCORE_BINDINGS) around KJS dependencies (5.54 KB, patch)
2008-09-03 04:35 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Clean up Platform.h and add PLATFORM(CHROMIUM) and USE(V8_BINDINGS) (6.07 KB, patch)
2008-09-03 05:16 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Clean up Platform.h and add PLATFORM(CHROMIUM) and USE(V8_BINDINGS) (6.18 KB, patch)
2008-09-03 05:28 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add #if USE(JAVASCRIPTCORE_BINDINGS) around KJS dependencies (6.14 KB, patch)
2008-09-03 14:44 PDT, Eric Seidel (no email)
sam: review+
Details | Formatted Diff | Diff
Clean up Platform.h and add PLATFORM(CHROMIUM), PLATFORM(SKIA) and USE(V8_BINDINGS) (9.61 KB, patch)
2008-09-03 14:44 PDT, Eric Seidel (no email)
sam: review-
Details | Formatted Diff | Diff
Add #if USE(JSC) around KJS dependencies (13.88 KB, patch)
2008-09-03 17:38 PDT, Eric Seidel (no email)
sam: review+
Details | Formatted Diff | Diff
Clean up Platform.h and add PLATFORM(CHROMIUM), PLATFORM(SKIA) and USE(V8_BINDINGS) (8.08 KB, patch)
2008-09-03 17:38 PDT, Eric Seidel (no email)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-09-03 04:22:24 PDT
Wrap uses of KJS in core classes with USE(JAVASCRIPTCORE_BINDINGS)

We can also possibly add a USE(V8_BINDINGS) as Chromium has.
Comment 1 Eric Seidel (no email) 2008-09-03 04:35:22 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(-)
Comment 2 Eric Seidel (no email) 2008-09-03 04:40:01 PDT
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!
Comment 3 Eric Seidel (no email) 2008-09-03 05:16:40 PDT
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(-)
Comment 4 Eric Seidel (no email) 2008-09-03 05:27:55 PDT
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).
Comment 5 Eric Seidel (no email) 2008-09-03 05:28:41 PDT
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(-)
Comment 6 Sam Weinig 2008-09-03 10:13:15 PDT
Eric, please include changelogs with your patches (at least to set a good example).
Comment 7 Eric Seidel (no email) 2008-09-03 14:44:54 PDT
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(-)
Comment 8 Eric Seidel (no email) 2008-09-03 14:44:59 PDT
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 9 Sam Weinig 2008-09-03 14:48:11 PDT
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 10 Sam Weinig 2008-09-03 14:58:23 PDT
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 11 Sam Weinig 2008-09-03 14:58:54 PDT
Comment on attachment 23148 [details]
Add #if USE(JAVASCRIPTCORE_BINDINGS) around KJS dependencies

r=me if you switch to use USE(JSC)
Comment 12 Eric Seidel (no email) 2008-09-03 15:42:36 PDT
(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.
Comment 13 Mark Rowe (bdash) 2008-09-03 15:58:39 PDT
> > 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.
Comment 14 Eric Seidel (no email) 2008-09-03 16:45:23 PDT
(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?
Comment 15 Eric Seidel (no email) 2008-09-03 16:58:34 PDT
(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.
Comment 16 Eric Seidel (no email) 2008-09-03 17:38:14 PDT
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(-)
Comment 17 Eric Seidel (no email) 2008-09-03 17:38:17 PDT
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 18 Sam Weinig 2008-09-03 18:05:22 PDT
Comment on attachment 23152 [details]
Add #if USE(JSC) around KJS dependencies

Needs an updated changelog.