Bug 160115 – Adding a new WebCore JavaScript built-in source file does not trigger rebuild of WebCoreJSBuiltins*

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Darin Adler]

Addng a new WebCore JavaScript built-in source file does not trigger rebuild of WebCoreJSBuiltins*

[Darin Adler]

Created attachment 284405 [details]
Patch

[Darin Adler]

I ran into this when I checked out the latest sources but they failed to link because XMLHttpRequest was not included in WebCoreJSBuiltins.cpp.

[youenn fablet]

Comment on attachment 284405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284405&action=review

Thanks!

> Source/WebCore/DerivedSources.make:1334
> +	$(PYTHON) $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py '$(WebCore_BUILTINS_SOURCES)' $@

Why is this needed?

> Source/WebCore/DerivedSources.make:1339
> +$(firstword $(WebCore_BUILTINS_WRAPPERS)): WebCore_BUILTINS_SOURCES_LIST $(BUILTINS_GENERATOR_SCRIPTS) WebCore_BUILTINS_DEPENDENCIES_LIST

Cannot we just use $(WebCore_BUILTINS_SOURCES) here?

[youenn fablet]

Created attachment 284444 [details]
Patch for landing

[youenn fablet]

(In reply to comment #4)
> Created attachment 284444 [details]
> Patch for landing

Updated bug title in log.

[WebKit Commit Bot]

Comment on attachment 284444 [details]
Patch for landing

Clearing flags on attachment: 284444

Committed r203661: <http://trac.webkit.org/changeset/203661>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 284405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284405&action=review

>> Source/WebCore/DerivedSources.make:1334
>> +	$(PYTHON) $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py '$(WebCore_BUILTINS_SOURCES)' $@
> 
> Why is this needed?

Because generating the wrappers depends on the *list* of JS files, not on the contents of those files, and further it needs to depend on removals as well as additions to that list. This technique gives us a file that changes when the filenames in the list change.

>> Source/WebCore/DerivedSources.make:1339
>> +$(firstword $(WebCore_BUILTINS_WRAPPERS)): WebCore_BUILTINS_SOURCES_LIST $(BUILTINS_GENERATOR_SCRIPTS) WebCore_BUILTINS_DEPENDENCIES_LIST
> 
> Cannot we just use $(WebCore_BUILTINS_SOURCES) here?

No. Doing that would rebuild:

1) Too often. Any time we changed the contents of one of the sources.
2) Not often enough. Would not trigger a rebuild if we removed a file from the list.

[youenn fablet]

(In reply to comment #8)
> Comment on attachment 284405 [details]
> Patch
> 
> >> Source/WebCore/DerivedSources.make:1339
> >> +$(firstword $(WebCore_BUILTINS_WRAPPERS)): WebCore_BUILTINS_SOURCES_LIST $(BUILTINS_GENERATOR_SCRIPTS) WebCore_BUILTINS_DEPENDENCIES_LIST
> > 
> > Cannot we just use $(WebCore_BUILTINS_SOURCES) here?
> 
> No. Doing that would rebuild:
> 
> 1) Too often. Any time we changed the contents of one of the sources.

I see the point.
That said, even if sources are changed, like adding a function, the header/source files will remain totally unchanged (lazy writer). So the perf loss is only the time spent to run the python generator. It does not trigger any compilation.

In the future, if we remove the use of macros, we might want to update wrapper cpp files according source file content. We can revisit this if we ever go down that path

Also, it might be good to think of the best way to limit the recompilation of cpp files in case of adding/removing a function.

> 2) Not often enough. Would not trigger a rebuild if we removed a file from
> the list.

OK