Bug 174784 – [WinCairo] Fix build with AllInOnes disabled

• Component: Accessibility
• Status: RESOLVED FIXED

[Yoshiaki Jitsukawa]

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.

[Yoshiaki Jitsukawa]

Created attachment 316284 [details]
Patch

[Build Bot]

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

[Yoshiaki Jitsukawa]

Created attachment 316288 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/33488914>

[Alex Christensen]

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.

[Yoshiaki Jitsukawa]

(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.

[Alex Christensen]

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?

[Yoshiaki Jitsukawa]

(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

[Yoshiaki Jitsukawa]

(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

[Yoshiaki Jitsukawa]

Created attachment 316343 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Yoshiaki Jitsukawa]

Created attachment 316353 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316353 [details]
Patch

Clearing flags on attachment: 316353

Committed r219869: <http://trac.webkit.org/changeset/219869>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.