Bug 142712 – Web Inspector: Rename ConsoleMessage and ConsoleMessageImpl to LegacyConsoleMessage and LegacyConsoleMessageImpl respectively

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

As a first step of ConsoleMessage refactoring (https://bugs.webkit.org/show_bug.cgi?id=142599), rename ConsoleMessage and ConsoleMessageImpl to LegacyConsoleMessage and LegacyConsoleMessageImpl respectively.

Rename WebInspector.ConsoleMessage class to WebInspector.LegacyConsoleMessage.
Rename WebInspector.ConsoleMessageImpl class to WebInspector.LegacyConsoleMessageImpl.
Rename ConsoleMessage.js file to LegacyConsoleMessage.js.
Rename ConsoleMessageImpl file to LegacyConsoleMessageImpl.js.

[Nikita Vasilyev]

Created attachment 248685 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 248685 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Main.html:309
> -    <script src="Views/ConsoleMessageImpl.js"></script>
> +    <script src="Views/LegacyConsoleMessageImpl.js"></script>

Technically this should be sorted, but it doesn't matter since it will be removed later.

> Source/WebInspectorUI/WebInspectorUI.vcxproj/WebInspectorUI.vcxproj:319
> +    <None Include="..\UserInterface\LegacyConsoleMessage.js" />
> +    <None Include="..\UserInterface\LegacyConsoleMessageImpl.js" />

Same here.

[WebKit Commit Bot]

Comment on attachment 248685 [details]
Patch

Clearing flags on attachment: 248685

Committed r181517: <http://trac.webkit.org/changeset/181517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Looks like you broke GTK+!

https://webkit-queues.appspot.com/results/5613271193223168

[Nikita Vasilyev]

I searched via: ack -Q 'ConsoleMessageImpl.js' and I couldn’t find any references to the old files inside the WebKit repository.

I’m not familiar with GTK build. Any idea what am I supposed to do fix this?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142718

[Joseph Pecoraro]

(In reply to comment #7)
> Re-opened since this is blocked by bug 142718

Actually, I think the GTK bot should just need a clean build. It is picking up a file in the derived sources that, after this patch, will no longer exist.

Unfortunately this did just get rolled out, so we can just roll it back in.

[Csaba Osztrogonác]

build error:
ninja: error: '../../Source/WebInspectorUI/UserInterface/Views/ConsoleMessageImpl.js', needed by 'DerivedSources/webkit2gtk/InspectorGResourceBundle.xml', missing and no known rule to make it

Ouch, you shouldn't have rolled out the patch, I already triggered 
clean build on the GTK bots long time ago and they were happy.

The problem is that the mechanism in GTK build system to determine
when should it rerun cmake to regenarate make/ninja files is buggy.
I noticed the same InspectorUI related incremental build issue 2-3
times in the near past. We should add make GTK build to run cmake
always if this part changes.

[Joseph Pecoraro]

> Ouch, you shouldn't have rolled out the patch, I already triggered 
> clean build on the GTK bots long time ago and they were happy.

Yeah, sorry. NVI came to me, I set the flag. We figured out what happened, I unset the flag, and moments later the commit-queue landed the patch. =(

[Csaba Osztrogonác]

(In reply to comment #9)
> build error:
> ninja: error:
> '../../Source/WebInspectorUI/UserInterface/Views/ConsoleMessageImpl.js',
> needed by 'DerivedSources/webkit2gtk/InspectorGResourceBundle.xml', missing
> and no known rule to make it
> 
> Ouch, you shouldn't have rolled out the patch, I already triggered 
> clean build on the GTK bots long time ago and they were happy.
> 
> The problem is that the mechanism in GTK build system to determine
> when should it rerun cmake to regenarate make/ninja files is buggy.
> I noticed the same InspectorUI related incremental build issue 2-3
> times in the near past. We should add make GTK build to run cmake
> always if this part changes.

I checked the bug in details. The problem is that the generated
InspectorGResourceBundle.xml depends on a non existent file (after rename).
In this case cmake should rerun to regenerate the makefile. Maybe an
additional check in webkitdirs.pm-shouldRemoveCMakeCache() could help.

http://trac.webkit.org/browser/trunk/Source/WebKit2/PlatformGTK.cmake#L559
http://trac.webkit.org/browser/trunk/Source/WebKit2/PlatformGTK.cmake#L406

[Carlos Garcia Campos]

(In reply to comment #9)
> build error:
> ninja: error:
> '../../Source/WebInspectorUI/UserInterface/Views/ConsoleMessageImpl.js',
> needed by 'DerivedSources/webkit2gtk/InspectorGResourceBundle.xml', missing
> and no known rule to make it
> 
> Ouch, you shouldn't have rolled out the patch, I already triggered 
> clean build on the GTK bots long time ago and they were happy.
> 
> The problem is that the mechanism in GTK build system to determine
> when should it rerun cmake to regenarate make/ninja files is buggy.
> I noticed the same InspectorUI related incremental build issue 2-3
> times in the near past. We should add make GTK build to run cmake
> always if this part changes.

Yes, the problem is the use of wildcard characters in cmake that I never liked. We could just list all the files in the mkefile, but then everytime a new file is added to the inspector and not explicitly added to the cmake files, we will miss it.

[Csaba Osztrogonác]

(In reply to comment #12)
> Yes, the problem is the use of wildcard characters in cmake that I never
> liked. We could just list all the files in the mkefile, but then everytime a
> new file is added to the inspector and not explicitly added to the cmake
> files, we will miss it.

Or we could extract files from Source/WebInspectorUI/UserInterface/Main.html
with a simple cmake script (with an additional sanity check if the file
really exist and we didn't parsed a wrong thing).

[WebKit Commit Bot]

Comment on attachment 248685 [details]
Patch

Rejecting attachment 248685 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 248685 [details]
Patch

Rejecting attachment 248685 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 248685 [details]
Patch

Rejecting attachment 248685 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 248685, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5388747482857472

[Nikita Vasilyev]

Created attachment 248788 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 248788 [details]
Patch

Clearing flags on attachment: 248788

Committed r181610: <http://trac.webkit.org/changeset/181610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

(In reply to comment #9)
> build error:
> ninja: error:
> '../../Source/WebInspectorUI/UserInterface/Views/ConsoleMessageImpl.js',
> needed by 'DerivedSources/webkit2gtk/InspectorGResourceBundle.xml', missing
> and no known rule to make it
> 
> Ouch, you shouldn't have rolled out the patch, I already triggered 
> clean build on the GTK bots long time ago and they were happy.

This just landed again, and GTK bots need the clean build. Could you trigger it? (Am I able to trigger it?)

[Joseph Pecoraro]

> This just landed again, and GTK bots need the clean build. Could you trigger
> it? (Am I able to trigger it?)

I think I just triggered it via the build pages:
https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29

I did a "Force Clean Build" on the Debug and Release bots.