[webkit-reviews] review denied: [Bug 42432] Enable Web Timing for GTK : [Attachment 95285] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 42432: Enable Web Timing for GTK
https://bugs.webkit.org/show_bug.cgi?id=42432

Attachment 95285: Patch
https://bugs.webkit.org/attachment.cgi?id=95285&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95285&action=review

I'm still concerned about Sergio's comment. Seems that this question should be
resolved before we can land this:

> This seems correct but the problem is that you are handling the
"connection-created" signal of any request issued to the SoupSession. Take into
account that this callback will be called whenever the SoupSession needs to
create a connection (and not only for this particular ResourceHandle). There
are some other problems too, see below in the g_signal_connect() part

> LayoutTests/platform/gtk/Skipped:303
> +# Navigation Timing http tests that are still failing
> +# Redirect stats are not zeroed out after a cross-origin redirect,
> +# plus the third iframe does not load at all

Do you mind opening a bug for this?

> LayoutTests/platform/gtk/Skipped:306
> +# The load is not slow enough - sleeping for two seconds instead
> +# of one makes the test succeed.

Here as well? Please try to fix this test as a followup.

> LayoutTests/platform/gtk/Skipped:310
>  http/tests/misc/webtiming-ssl.php

Perhaps this should be filed as a bug in upstream?

> Source/WebCore/GNUmakefile.am:583
> +if ENABLE_WEB_TIMING
> +FEATURE_DEFINES += ENABLE_WEB_TIMING=1
> +webcore_cppflags += -DENABLE_WEB_TIMING=1
> +endif  # END ENABLE_WEB_TIMING

What affect does this have on runtime performance. Do other ports enable it by
default or only to run tests?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:416
> +    g_signal_handlers_disconnect_matched(handle->defaultSession(),
G_SIGNAL_MATCH_DATA,
> +					    0, 0, 0, 0, handle);

This can be one line.