Bug 191583 - Various compiler warnings/errors fixes.
Summary: Various compiler warnings/errors fixes.
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: Charlie Turner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-13 02:08 PST by Charlie Turner
Modified: 2018-11-13 06:36 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.80 KB, patch)
2018-11-13 02:11 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (6.80 KB, patch)
2018-11-13 03:40 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (5.48 KB, patch)
2018-11-13 03:46 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch for landing (5.48 KB, patch)
2018-11-13 05:58 PST, Charlie Turner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2018-11-13 02:08:09 PST
Various compiler warnings/errors fixes.
Comment 1 Charlie Turner 2018-11-13 02:11:29 PST
Created attachment 354661 [details]
Patch
Comment 2 Charlie Turner 2018-11-13 02:13:15 PST
The build errors came from https://bugs.webkit.org/show_bug.cgi?id=90342, in release+logging configurations, the others were some warnings I noticed in a wayland upgrade.
Comment 3 Charlie Turner 2018-11-13 02:24:26 PST
I had a dirty jhbuild for the wayland related changes, rebuilding that here to check the EWS failure.
Comment 4 Frédéric Wang (:fredw) 2018-11-13 02:29:03 PST
Comment on attachment 354661 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:654
>  #endif

I think the !LOG_DISABLED is for compositingLogEnabled() and ENABLE(TREE_DEBUGGING) for showPaintOrderTree.
Does ENABLE(TREE_DEBUGGING) imply !LOG_DISABLED?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:730
>          showPaintOrderTree(m_renderView.layer());

Ditto.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:765
> +#if ENABLE(TREE_DEBUGGING)

Ditto.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:788
> +#endif

Shouldn't it be

#if !LOG_DISABLED
  if (compositingLogEnabled())
    LOG(Compositing, ...)
#endif

?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1003
> +#endif

Ditto.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1019
> +#endif

Ditto.

> Tools/wpe/backends/WindowViewBackend.cpp:384
> +    [](void*, struct wl_touch*, int32_t, wl_fixed_t) { }

The WPE bot does not seem to like this... Apparently there was a trailing comma in the previous code.
Comment 5 Charlie Turner 2018-11-13 02:48:15 PST
Comment on attachment 354661 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:654
>>  #endif
> 
> I think the !LOG_DISABLED is for compositingLogEnabled() and ENABLE(TREE_DEBUGGING) for showPaintOrderTree.
> Does ENABLE(TREE_DEBUGGING) imply !LOG_DISABLED?

TREE_DEBUGGING -> !NDEBUG -> !ASSERTIONS_DISABLED_DEFAULT -> !LOG_DISABLED from my analysis.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:788
>> +#endif
> 
> Shouldn't it be
> 
> #if !LOG_DISABLED
>   if (compositingLogEnabled())
>     LOG(Compositing, ...)
> #endif
> 
> ?

That seems more robust. I was relying on compositingState.depth only being defined in TREE_DEBUGGING mode, so this checked seemed reasonable. Will update to your suggestion.

>> Tools/wpe/backends/WindowViewBackend.cpp:384
>> +    [](void*, struct wl_touch*, int32_t, wl_fixed_t) { }
> 
> The WPE bot does not seem to like this... Apparently there was a trailing comma in the previous code.

Strange that my compiler didn't mind it :/ seems other inline structures in this file don't need the trailing comma, will double check here.
Comment 6 Charlie Turner 2018-11-13 03:36:44 PST
Comment on attachment 354661 [details]
Patch

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

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:788
>>> +#endif
>> 
>> Shouldn't it be
>> 
>> #if !LOG_DISABLED
>>   if (compositingLogEnabled())
>>     LOG(Compositing, ...)
>> #endif
>> 
>> ?
> 
> That seems more robust. I was relying on compositingState.depth only being defined in TREE_DEBUGGING mode, so this checked seemed reasonable. Will update to your suggestion.

Actually, that doesn't work properly. The issue is RenderLayerCompositor::CompositingState.depth is not defined when not building in TREE_DEBUGGING, so accessing it without this guard is incorrect.
Comment 7 Charlie Turner 2018-11-13 03:40:49 PST
Created attachment 354662 [details]
Patch

Add trailing comma to see if it helps the EWS bots in the wayland part of the patch. Can't reproduce that build failure locally with an updated jhbuild.
Comment 8 Charlie Turner 2018-11-13 03:46:18 PST
Created attachment 354663 [details]
Patch

There must be something out of sync with the trunk jhbuild and the wpe bot's jhbuild, rolling out the local warning fix.
Comment 9 Charlie Turner 2018-11-13 05:58:28 PST
Created attachment 354664 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-11-13 06:35:30 PST
Comment on attachment 354664 [details]
Patch for landing

Clearing flags on attachment: 354664

Committed r238130: <https://trac.webkit.org/changeset/238130>
Comment 11 WebKit Commit Bot 2018-11-13 06:35:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-11-13 06:36:22 PST
<rdar://problem/46026592>