Bug 174784

Summary: [WinCairo] Fix build with AllInOnes disabled
Product: WebKit Reporter: Yoshiaki Jitsukawa <yoshiaki.jitsukawa>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, commit-queue, don.olmstead, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch none

Description Yoshiaki Jitsukawa 2017-07-24 05:19:15 PDT
If ENABLE_ALLINONE_BUILD=FALSE is given, compilation errors occurs, 
which are hidden in the all-in-one build. 

I wonder if we should maintain builds without all-in-one, but let me submit a 
patch to fix the errors.
Comment 1 Yoshiaki Jitsukawa 2017-07-24 05:33:34 PDT
Created attachment 316284 [details]
Patch
Comment 2 Build Bot 2017-07-24 05:36:23 PDT
Comment on attachment 316284 [details]
Patch

Attachment 316284 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/4178261

New failing tests:
(JS) JSTestObj.cpp
Comment 3 Yoshiaki Jitsukawa 2017-07-24 06:36:16 PDT
Created attachment 316288 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2017-07-24 09:24:58 PDT
<rdar://problem/33488914>
Comment 5 Alex Christensen 2017-07-24 11:21:25 PDT
Comment on attachment 316288 [details]
Patch

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

It's strange that we need to include DOMWindow.h in so many places.  Other platforms don't build with the all-in-one files.  This leads me to believe that there might be something wrong with this approach.

> Source/WebCore/PlatformWin.cmake:152
> +if (NOT ENABLE_ALLINONE_BUILD)

We shouldn't need to check this.  IIRC, the all-in-one build removes files from WebCore_SOURCES.
Comment 6 Yoshiaki Jitsukawa 2017-07-24 14:39:36 PDT
(In reply to Alex Christensen from comment #5)

Thank you for reviewing!

> It's strange that we need to include DOMWindow.h in so many places.  Other
> platforms don't build with the all-in-one files.  This leads me to believe
> that there might be something wrong with this approach.

The DOMWindow class is forward-declared in "JSDOMWindowBase.h" and the JSDOMWindowBase 
class has a member "RefPtr<DOMWindow> m_wrapped;" so it seems that the sources
which include "JSDomWindowBase.h" need the definition of the DOMWindow class
somewhere in the compilation unit.

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/bindings/js/JSDOMWindowBase.h#L28

Another solution would be to include "DOMWindow.h" in "JSDomWindowBase.h".

> > Source/WebCore/PlatformWin.cmake:152
> > +if (NOT ENABLE_ALLINONE_BUILD)
> 
> We shouldn't need to check this.  IIRC, the all-in-one build removes files
> from WebCore_SOURCES.

Let me check.
Comment 7 Alex Christensen 2017-07-24 17:02:15 PDT
Comment on attachment 316288 [details]
Patch

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

We are trying to reduce the number of headers that include other headers, so adding #include "DOMWindow.h" to more headers would be easy, but it would slow down the build.  This is better than that.

> Source/WebCore/rendering/RenderMediaControls.h:30
> +#include "MediaControlElementTypes.h"

This entire header and cpp could probably be removed.

> Source/WebCore/testing/MemoryInfo.h:34
> +#include "DOMWindow.h"

Could we add #include "DOMWindow.h" to some cpp files instead of this?
Comment 8 Yoshiaki Jitsukawa 2017-07-24 18:26:07 PDT
(In reply to Alex Christensen from comment #7)
> Comment on attachment 316288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316288&action=review
> 
> We are trying to reduce the number of headers that include other headers, so
> adding #include "DOMWindow.h" to more headers would be easy, but it would
> slow down the build.  This is better than that.

I understand.

> > Source/WebCore/rendering/RenderMediaControls.h:30
> > +#include "MediaControlElementTypes.h"
> 
> This entire header and cpp could probably be removed.
> 
> > Source/WebCore/testing/MemoryInfo.h:34
> > +#include "DOMWindow.h"
> 
> Could we add #include "DOMWindow.h" to some cpp files instead of this?

Probably we could remove #include "JSDOMWindow.h" from Source/WebCore/testing/MemoryInfo.h
Comment 9 Yoshiaki Jitsukawa 2017-07-24 18:31:22 PDT
(In reply to Alex Christensen from comment #5)
> > Source/WebCore/PlatformWin.cmake:152
> > +if (NOT ENABLE_ALLINONE_BUILD)
> 
> We shouldn't need to check this.  IIRC, the all-in-one build removes files
> from WebCore_SOURCES.

PROCESS_ALLINONE_FILE() for WebCore_SOURCES is called before including PlatformWin.cmake so RenderThemeWin.cpp is not removed by the macro.

Like other ports, it would be more simple to exclude platform RenderTheme from RenderingAllInOne.cpp
Comment 10 Yoshiaki Jitsukawa 2017-07-24 20:22:57 PDT
Created attachment 316343 [details]
Patch
Comment 11 Build Bot 2017-07-24 21:56:28 PDT
Comment on attachment 316343 [details]
Patch

Attachment 316343 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4182277

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open.html
Comment 12 Build Bot 2017-07-24 21:56:29 PDT
Created attachment 316352 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 13 Yoshiaki Jitsukawa 2017-07-24 22:33:02 PDT
Created attachment 316353 [details]
Patch
Comment 14 WebKit Commit Bot 2017-07-25 09:49:53 PDT
Comment on attachment 316353 [details]
Patch

Clearing flags on attachment: 316353

Committed r219869: <http://trac.webkit.org/changeset/219869>
Comment 15 WebKit Commit Bot 2017-07-25 09:49:55 PDT
All reviewed patches have been landed.  Closing bug.