Bug 39339 – Move enum ReasonForCallingCanExecuteScripts to header ScriptControllerBase.h

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

The enum ReasonForCallingCanExecuteScripts is present in both the JSC and V8 versions of ScriptController.h, and is used in ScriptControllerBase.cpp.  We should move this enum into a new header, say ScriptControllerBase.h, so that it is defined in one file. This will also be useful in the patch for Bug #39249, which proposes adding an enum to be used in ScriptController.

[Daniel Bates]

Created attachment 56448 [details]
Patch

No functionality was changed, so no new tests.

[Darin Adler]

Comment on attachment 56448 [details]
Patch

Seems OK to move this to a separate file. I'm not so fond of the name ScriptControllerBase.h for this.

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 56448 [details])
> Seems OK to move this to a separate file. I'm not so fond of the name ScriptControllerBase.h for this.

I am open to suggestions. 

There will be at least two enums in this file: ReasonForCallingCanExecuteScripts, and ShouldAllowXSS (will update patch for Bug #39249).

[Daniel Bates]

Committed r60017: <http://trac.webkit.org/changeset/60017>

[Daniel Bates]

This broke the Windows and Qt bots. Committed revert in 60019  <http://trac.webkit.org/changeset/60019> while I look into this some more.

[Csaba Osztrogonác]

The problem with Qt build was the missing include path.
You can fix it easyily, I think the second one is the nicer.

1.)
diff --git a/WebCore/bindings/js/ScriptController.h b/WebCore/bindings/js/ScriptController.h
-#include "ScriptControllerBase.h"
+#include "../ScriptControllerBase.h"

2.)
diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
index 6fbeffc..034e45f 100644
--- a/WebCore/WebCore.pro
+++ b/WebCore/WebCore.pro
@@ -170,6 +170,7 @@ include(WebCore.pri)
 INCLUDEPATH = \
     $$PWD \
     $$PWD/accessibility \
+    $$PWD/bindings \
     $$PWD/bindings/js \
     $$PWD/bridge \
     $$PWD/bridge/c \

[Daniel Bates]

(In reply to comment #6)
> The problem with Qt build was the missing include path.
> You can fix it easyily, I think the second one is the nicer.
> 
> 1.)
> diff --git a/WebCore/bindings/js/ScriptController.h b/WebCore/bindings/js/ScriptController.h
> -#include "ScriptControllerBase.h"
> +#include "../ScriptControllerBase.h"
> 
> 2.)
> diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
> index 6fbeffc..034e45f 100644
> --- a/WebCore/WebCore.pro
> +++ b/WebCore/WebCore.pro
> @@ -170,6 +170,7 @@ include(WebCore.pri)
>  INCLUDEPATH = \
>      $$PWD \
>      $$PWD/accessibility \
> +    $$PWD/bindings \
>      $$PWD/bindings/js \
>      $$PWD/bridge \
>      $$PWD/bridge/c \

Thanks Csaba. I also prefer the second option of adding "$$PWD/bindings \" to WebCore.pri. Will give this a try.

[Shinichiro Hamaji]

Comment on attachment 56448 [details]
Patch

Clearing r+ flag to remove this from pending-commit.

[Daniel Bates]

Created attachment 61145 [details]
Patch

Updated patch based on Csaba's suggestion and fixed Windows build issues.

[Adam Barth]

Comment on attachment 61145 [details]
Patch

There's probably more we can move into that header too.  I think some of the build systems are unhappy with this change.

WebCore/bindings/ScriptControllerBase.h:35
 +      enum ReasonForCallingCanExecuteScripts {
No namespace indent.

[Daniel Bates]

(In reply to comment #10)
> (From update of attachment 61145 [details])
> There's probably more we can move into that header too.  I think some of the build systems are unhappy with this change.
> 
> WebCore/bindings/ScriptControllerBase.h:35
>  +      enum ReasonForCallingCanExecuteScripts {
> No namespace indent.

Will fix when I land.

This change built on my Mac and Windows builds. The EWS bots don't show any descriptive error messages and/or other error output.

[Daniel Bates]

Committed r63050: <http://trac.webkit.org/changeset/63050>