Bug 177766 - [Settings] Convert more settings to be generated
Summary: [Settings] Convert more settings to be generated
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-02 12:55 PDT by Sam Weinig
Modified: 2017-10-18 21:42 PDT (History)
5 users (show)

See Also:


Attachments
Patch (195.52 KB, patch)
2017-10-02 13:10 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (195.45 KB, patch)
2017-10-02 20:44 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (29.96 KB, patch)
2017-10-18 14:16 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (29.98 KB, patch)
2017-10-18 14:43 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (29.95 KB, patch)
2017-10-18 18:11 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (29.97 KB, patch)
2017-10-18 18:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-10-02 12:55:31 PDT
[Settings] Convert more settings to be generated
Comment 1 Sam Weinig 2017-10-02 13:10:34 PDT
Created attachment 322414 [details]
Patch
Comment 2 Sam Weinig 2017-10-02 20:44:47 PDT
Created attachment 322489 [details]
Patch
Comment 3 Fujii Hironori 2017-10-03 01:32:42 PDT
GTK port EWS also failed even though it is green.
I tested with GTK port and reproduced the same error.

By removing the following outdated file, it succeeds to build:

WebKitBuild/Release/DerivedSources/ForwardingHeaders/WebCore/Settings.h
Comment 4 Sam Weinig 2017-10-03 07:00:34 PDT
(In reply to Fujii Hironori from comment #3)
> GTK port EWS also failed even though it is green.
> I tested with GTK port and reproduced the same error.
> 
> By removing the following outdated file, it succeeds to build:
> 
> WebKitBuild/Release/DerivedSources/ForwardingHeaders/WebCore/Settings.h

Any idea how to fix this? I'm not sure how to make the new generated Settings.h available as a forwarding header.
Comment 5 Fujii Hironori 2017-10-03 19:45:50 PDT
Generally speaking, it's impossible to remove old generating files.
For example, Foo.idl generated Foo.cpp, and Bar.cpp generated Bar.o.
Then, the source files Foo.idl and Bar.cpp are removed.
The old generated files Foo.cpp and Bar.o exist in the build directory.
It's impossible to remove these generated files because the new build script has no information about the files.

Old generated file is WebKitBuild/Release/DerivedSources/ForwardingHeaders/WebCore/Settings.h.
New generated file is WebKitBuild/Release/DerivedSources/WebCore/Settings.h

-IDerivedSources/ForwardingHeaders is given before -IDerivedSources.
Then, #include <WebCore/Settings.h> refers old one.

One solution is reordering the include directories.
But, I don't think this is a good idea.
Becuase opposite problems can happen, and this is only for the incremental build failure.

Another solution is using a different name, for example SettingsGenerated.h.
And, Preserve Source/WebCore/page/Settings.h with just two following lines:

> #pragma once
> #include "SettingsGenerated.h"
Comment 6 Sam Weinig 2017-10-18 14:16:06 PDT Comment hidden (obsolete)
Comment 7 Sam Weinig 2017-10-18 14:43:41 PDT Comment hidden (obsolete)
Comment 8 Sam Weinig 2017-10-18 18:11:06 PDT Comment hidden (obsolete)
Comment 9 Sam Weinig 2017-10-18 18:18:03 PDT
Created attachment 324191 [details]
Patch
Comment 10 Dean Jackson 2017-10-18 20:41:25 PDT
Comment on attachment 324191 [details]
Patch

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

> Source/WebCore/page/Settings.in:328
> +loadsImagesAutomatically initial=false, onChange=imagesEnabledChanged
> +imagesEnabled initial=true, getter=areImagesEnabled, onChange=imagesEnabledChanged

Should these have the same onChange?

Answer: yes, even though it looks like a typo.

> Source/WebCore/page/SettingsBase.cpp:478
>  
> +
> +

Lots of spaces
Comment 11 WebKit Commit Bot 2017-10-18 21:08:42 PDT
Comment on attachment 324191 [details]
Patch

Clearing flags on attachment: 324191

Committed r223653: <https://trac.webkit.org/changeset/223653>
Comment 12 WebKit Commit Bot 2017-10-18 21:08:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2017-10-18 21:09:26 PDT
<rdar://problem/35067339>
Comment 14 Ryan Haddad 2017-10-18 21:36:55 PDT
This change broke the Windows build:
C:\cygwin\home\buildbot\WebKit\Source\WebCore\page\SettingsBase.cpp(562): error C2039: 'setResourceUsageOverlayVisible': is not a member of 'WebCore::Page' [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
Comment 15 Sam Weinig 2017-10-18 21:40:20 PDT
(In reply to Ryan Haddad from comment #14)
> This change broke the Windows build:
> C:\cygwin\home\buildbot\WebKit\Source\WebCore\page\SettingsBase.cpp(562):
> error C2039: 'setResourceUsageOverlayVisible': is not a member of
> 'WebCore::Page'
> [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.
> vcxproj]

Working on a fix.
Comment 16 Sam Weinig 2017-10-18 21:42:39 PDT
Build fix in r223665.