Bug 164479 - Fix various --minimal build issue
Summary: Fix various --minimal build issue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-07 06:12 PST by Csaba Osztrogonác
Modified: 2016-11-14 08:06 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.80 KB, patch)
2016-11-07 06:13 PST, Csaba Osztrogonác
darin: review+
Details | Formatted Diff | Diff
patch for landing (3.44 KB, patch)
2016-11-14 06:21 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
patch for landing (3.23 KB, patch)
2016-11-14 06:30 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2016-11-07 06:12:44 PST
1.) Source/WebCore/dom/Node.cpp should include ExceptionCode.h
HIERARCHY_REQUEST_ERR, NOT_FOUND_ERR, NOT_SUPPORTED_ERR, ... are defined in ExceptionCode.h
In normal WebKit build ExceptionCode.h is included indirectly, but --minimal build fails.

2.) Source/WebCore/page/Page.cpp shouldn't call the undefined PlatformMediaSessionManager::updateNowPlayingInfoIfNecessary()
It is defined on Mac always and on !Mac inside ENABLE(VIDEO) || ENABLE(WEB_AUDIO) guards.

3.) emptyString() can't be converted to ExceptionOr<String> in Source/WebCore/testing/Internals.cpp
../../Source/WebCore/testing/Internals.cpp: In member function 'WebCore::ExceptionOr<WTF::String> WebCore::Internals::captionsStyleSheetOverride()':
../../Source/WebCore/testing/Internals.cpp:2511:23: error: could not convert 'WTF::emptyString()' from 'const WTF::String' to 'WebCore::ExceptionOr<WTF::String>'
Comment 1 Csaba Osztrogonác 2016-11-07 06:13:45 PST
Created attachment 294058 [details]
Patch
Comment 2 Darin Adler 2016-11-07 23:12:39 PST
Comment on attachment 294058 [details]
Patch

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

I am OK with these build fixes, but the ones in Page.cpp could be done a little better.

> Source/WebCore/page/Page.cpp:1450
> +#if PLATFORM(MAC) || ENABLE(VIDEO) || ENABLE(WEB_AUDIO)

If we keep this conditional, there is no reason for it to mention #if PLATFORM(MAC). Just ENABLE(VIDEO) || ENABLE(WEB_AUDIO) should do.

> Source/WebCore/page/Page.cpp:1473
> +#if PLATFORM(MAC) || ENABLE(VIDEO) || ENABLE(WEB_AUDIO)
>      if (wasVisibleAndActive != isVisibleAndActive())
>          PlatformMediaSessionManager::updateNowPlayingInfoIfNecessary();
> +#endif

Ditto.

But maybe this conditional is not needed. Instead, perhaps PlatformMediaSessionManager::updateNowPlayingInfoIfNecessary should be defined even when ENABLE(VIDEO) || ENABLE(WEB_AUDIO) is false, in PlatformMediaSessionManager.cpp. Since the header is not conditional, it seems to me that the function body shouldn’t be either. It‘s not like we need to optimize this case to avoid the function call.

> Source/WebCore/testing/Internals.cpp:2511
> -    return emptyString();
> +    return String();

If we want to return an empty string and not null, then it would be better to return:

    return String { emptyString() };

But I guess it’s OK your way.
Comment 3 Csaba Osztrogonác 2016-11-14 06:21:09 PST
Created attachment 294699 [details]
patch for landing
Comment 4 Csaba Osztrogonác 2016-11-14 06:30:23 PST
Created attachment 294701 [details]
patch for landing
Comment 5 WebKit Commit Bot 2016-11-14 07:42:52 PST
Comment on attachment 294701 [details]
patch for landing

Clearing flags on attachment: 294701

Committed r208682: <http://trac.webkit.org/changeset/208682>