Web Inspector: replace hand written InspectorBackendStub.js with generated one

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 64012 [details]
[patch] initial version.

[Yury Semikhatsky]

Comment on attachment 64012 [details]
[patch] initial version.

Please make sure that devtools.html and inspector.html won't miss link to the generated file.

[WebKit Review Bot]

Attachment 64012 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3733036

[Ilya Tikhonovsky]

Created attachment 64097 [details]
[patch] second iteration. just for check gtk build on trybots.

[WebKit Review Bot]

Attachment 64097 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3776051

[Ilya Tikhonovsky]

Created attachment 64143 [details]
[patch] third iteration. for trybots

[WebKit Review Bot]

Attachment 64143 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3756077

[Ilya Tikhonovsky]

Created attachment 64348 [details]
[patch] fourth iteration. Gtk tests were disabled. Qt tests were disabled. Cr-linux was fixed.

[Joseph Pecoraro]

Comment on attachment 64348 [details]
[patch] fourth iteration. Gtk tests were disabled. Qt tests were disabled. Cr-linux was fixed.

> +++ b/LayoutTests/platform/qt/Skipped
> +# [Qt] InspectorBackendStub.js was removed from the sources and replaced with generated one.
> +inspector/

Could use a bug, I see you made one for Gtk.


> +++ b/WebCore/ChangeLog
> [phone]  Ilya Tikhonovsky  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Web Inspector: replace hand written InspectorBackendStub.js by generated one.
> +        https://bugs.webkit.org/show_bug.cgi?id=43791
> +
> +        * GNUmakefile.am:
> +        * WebCore.gypi:
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * combine-javascript-resources:
> +        * inspector/CodeGeneratorInspector.pm:
> +        * inspector/front-end/InspectorBackendStub.js: Removed.
> +

Could mention that you're adding a new, optional, command
line switch to `combine-javascript-resources` to accomplish this.
I think it would be helpful!


> +++ b/WebCore/WebCore.gypi
> -            'inspector/front-end/InspectorBackendStub.js',

Should this file be removed from WebKit.qrc as well?

  WebCore/inspector/front-end/WebKit.qrc


> +++ b/WebCore/WebCore.xcodeproj/project.pbxproj
> @@ -20306,7 +20306,7 @@

The following (reformatted) still exists in the build phase:

>   # Don't do anything for Debug builds, so the Inspector is easier to debug.
>   if [[ ${CONFIGURATION:=Debug} == \"Debug\" ]]; then
>     exit
>   fi

If I understand this correctly, a release build would run the combine-javascript
script, but not a debug build. Does the generated javascript file get copied over
to the build directory elsewhere so this works in debug builds?


> +++ b/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp
> +    // FixMe: it is not working. It should be not current folder but subsolder of $(WEBKITOUTPUTDIR)/Programs.

NIT: Grammar: "It should not be the current directory, but a subdirectory of $(WEBKITOUTPUTDIR)/Programs."

r? concerning the Mac Debug build question.

[Eric Seidel (no email)]

Comment on attachment 64012 [details]
[patch] initial version.

Cleared Yury Semikhatsky's review+ from obsolete attachment 64012 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Ilya Tikhonovsky]

Created attachment 64487 [details]
[patch] next iteration.


(In reply to comment #9)
> (From update of attachment 64348 [details])
> > +++ b/LayoutTests/platform/qt/Skipped
> > +# [Qt] InspectorBackendStub.js was removed from the sources and replaced with generated one.
> > +inspector/
> 
> Could use a bug, I see you made one for Gtk.

https://bugs.webkit.org/show_bug.cgi?id=44042


> > +++ b/WebCore/ChangeLog
> > [phone]  Ilya Tikhonovsky  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Web Inspector: replace hand written InspectorBackendStub.js by generated one.
> > +        https://bugs.webkit.org/show_bug.cgi?id=43791
> > +
> > +        * GNUmakefile.am:
> > +        * WebCore.gypi:
> > +        * WebCore.xcodeproj/project.pbxproj:
> > +        * combine-javascript-resources:
> > +        * inspector/CodeGeneratorInspector.pm:
> > +        * inspector/front-end/InspectorBackendStub.js: Removed.
> > +
> 
> Could mention that you're adding a new, optional, command
> line switch to `combine-javascript-resources` to accomplish this.
> I think it would be helpful!

done

> > +++ b/WebCore/WebCore.gypi
> > -            'inspector/front-end/InspectorBackendStub.js',
> 
> Should this file be removed from WebKit.qrc as well?
> 
>   WebCore/inspector/front-end/WebKit.qrc

done 



> > +++ b/WebCore/WebCore.xcodeproj/project.pbxproj
> > @@ -20306,7 +20306,7 @@
> 
> The following (reformatted) still exists in the build phase:
> 
> >   # Don't do anything for Debug builds, so the Inspector is easier to debug.
> >   if [[ ${CONFIGURATION:=Debug} == \"Debug\" ]]; then
> >     exit
> >   fi
> 
> If I understand this correctly, a release build would run the combine-javascript
> script, but not a debug build. Does the generated javascript file get copied over
> to the build directory elsewhere so this works in debug builds?

done


> > +++ b/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp
> > +    // FixMe: it is not working. It should be not current folder but subsolder of $(WEBKITOUTPUTDIR)/Programs.
> 
> NIT: Grammar: "It should not be the current directory, but a subdirectory of $(WEBKITOUTPUTDIR)/Programs."

done

[Yury Semikhatsky]

Comment on attachment 64487 [details]
[patch] next iteration.

WebCore/WebCore.vcproj/WebCore.vcproj: 
 +  				CommandLine="mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\bindings"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\parser"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\runtime"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\masm"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\pcre"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\profiler"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wrec"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wtf"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wtf\text"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wtf\unicode"
mkdir 2>NUL "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wtf\unicode\icu"

xcopy /y /d "$(ProjectDir)..\config.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(WebKitOutputDir)\obj\WebCore\DerivedSources\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\accessibility\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\accessibility\win\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\inspector\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\loader\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\loader\appcache\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\loader\archive\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\loader\archive\cf\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\loader\icon\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\history\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\history\cf\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\html\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\notifications\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\css\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\animation\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\cf\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\graphics\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\graphics\cg\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\graphics\transforms\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\graphics\win\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\graphics\opentype\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\text\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\text\transcoder\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\win\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\network\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\network\cf\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\network\win\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\sql\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\platform\cairo\cairo\src\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\bindings\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\bindings\js\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\page\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\page\animation\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\page\win\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\bridge\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\bridge\jsc\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\plugins\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\plugins\win\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\rendering\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\rendering\style\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\editing\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\dom\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\xml\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\svg\animation\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\svg\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\storage\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\websockets\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)..\workers\*.h" "$(WebKitOutputDir)\include\WebCore"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\bindings\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\bindings"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\parser\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\parser"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\runtime\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\runtime"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\masm\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\masm"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\pcre\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\pcre"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\profiler\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\profiler"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\wrec\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wrec"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\wtf\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wtf"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\wtf\text\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wtf\text"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\wtf\unicode\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wtf\unicode"
xcopy /y /d "$(ProjectDir)\..\ForwardingHeaders\wtf\unicode\icu\*.h" "$(WebKitOutputDir)\include\WebCore\ForwardingHeaders\wtf\unicode\icu"

mkdir 2>NUL "$(WebKitOutputDir)\bin\WebKit.resources\inspector"
xcopy /y /d /s /exclude:xcopy.excludes "$(ProjectDir)\..\inspector\front-end\*" "$(WebKitOutputDir)\bin\WebKit.resources\inspector"
mkdir 2>NUL "$(WebKitOutputDir)\bin\WebKit.resources\en.lproj"
xcopy /y /d /s /exclude:xcopy.excludes "$(ProjectDir)\..\English.lproj\localizedStrings.js" "$(WebKitOutputDir)\bin\WebKit.resources\en.lproj"

if exist "$(WebKitOutputDir)\buildfailed" del "$(WebKitOutputDir)\buildfailed"
"
Would be nice to extract this command into external build file as you proposed offline. If you did that prior to this change it would make it more clear.

LayoutTests/platform/qt/Skipped:5437
 +  inspector/
Please coordinate this with Qt team before landing.

[Ilya Tikhonovsky]

Created attachment 64552 [details]
[patch] next iteration. rebaselined.

[Joseph Pecoraro]

Comment on attachment 64552 [details]
[patch] next iteration. rebaselined.

Thanks for addressing my comments Ilya!

> +    open(my $STUB, ">$outputDir/$backendJSStubName.js") || die "Couldn't open file $outputDir/$backendJSStubName.js";
> +    print $STUB join("\n", @backendStubJS);
> +    close($STUB);
> +    undef($STUB);

I'd recommend "JS_STUB". Or something with JS in the name. Even though the
filename is on the right, I still like the readability of the open() argument.
And who knows, in the future we might have other stubs.

Otherwise this looks good to me. I'll let someone who knows more about the
build systems have a look.

[Ilya Tikhonovsky]

Created attachment 64587 [details]
[patch] next iteration. With working Qt tests and disabled GTK tests.

[Yury Semikhatsky]

Comment on attachment 64587 [details]
[patch] next iteration. With working Qt tests and disabled GTK tests.

r+ given that Qt build works fine.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65500 might have broken Qt Windows 32-bit Release and Qt Windows 32-bit Debug

[Ilya Tikhonovsky]

Committed r65595
	M	GNUmakefile.am
	M	WebKit/chromium/WebKit.gyp
	M	WebKit/chromium/ChangeLog
	M	WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp
	M	WebKit/gtk/ChangeLog
	M	WebCore/WebCore.pri
	M	WebCore/WebCore.pro
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/copyInspectorFiles.cmd
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gyp/WebCore.gyp
	M	WebCore/WebCore.gypi
	M	WebCore/inspector/CodeGeneratorInspector.pm
	D	WebCore/inspector/front-end/InspectorBackendStub.js
	A	WebCore/inspector/front-end/InspectorBackendStub.qrc
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/combine-javascript-resources
	M	ChangeLog
	M	LayoutTests/platform/gtk/Skipped
	M	LayoutTests/ChangeLog
W: -empty_dir: trunk/WebCore/inspector/front-end/InspectorBackendStub.js
r65595 = 98068b3e7c4f021a5e6e6be9ba5aff8949b689ac (refs/remotes/trunk)

[WebKit Review Bot]

http://trac.webkit.org/changeset/65595 might have broken Qt Windows 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/65594
http://trac.webkit.org/changeset/65595
http://trac.webkit.org/changeset/65596
http://trac.webkit.org/changeset/65597

[WebKit Review Bot]

http://trac.webkit.org/changeset/65608 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release

[Csaba Osztrogonác]

Unfortunately directory separator is different on Windows and on Linux,
and $$PWD always contains slashes instead of $$QMAKE_DIR_SEP.

I propose you should use a code like this:

...
TEMP = $$PWD/$$INSPECTOR_BACKEND_STUB_QRC $${WC_GENERATED_SOURCES_DIR}/InspectorBackendStub.qrc
inspectorBackendStub.commands = $$QMAKE_COPY $$replace(TEMP, "/", $$QMAKE_DIR_SEP)
...

I tested it on Windows and on Linux and it works.

[Ilya Tikhonovsky]

The fix was rolled out and landed again 

Committed r65608
	M	GNUmakefile.am
	M	WebKit/chromium/WebKit.gyp
	M	WebKit/chromium/ChangeLog
	M	WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp
	M	WebKit/gtk/ChangeLog
	M	WebCore/WebCore.pri
	M	WebCore/WebCore.pro
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/copyInspectorFiles.cmd
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gyp/WebCore.gyp
	M	WebCore/WebCore.gypi
	M	WebCore/inspector/CodeGeneratorInspector.pm
	D	WebCore/inspector/front-end/InspectorBackendStub.js
	A	WebCore/inspector/front-end/InspectorBackendStub.qrc
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/combine-javascript-resources
	M	ChangeLog
	M	LayoutTests/platform/gtk/Skipped
	M	LayoutTests/ChangeLog
W: -empty_dir: trunk/WebCore/inspector/front-end/InspectorBackendStub.js
r65608 = a47f67210f35308e6571a84d16314e6cfd67a4da (refs/remotes/trunk)

qt for linux fix.
Committed r65609
	M	WebCore/WebCore.pri
	M	WebCore/ChangeLog
r65609 = 50a68d9e955199ab33c84123d775fbfbd06f8ce4 (refs/remotes/trunk)