WebKitWebSource and WebKitTextCombiner both use g_object_emit. g_object_emit_by_psec would be more efficient, and not really any more complicated. I haven't done much of a search, so there may be other cases as well.
I think I meant we should replace g_object_notify with g_object_notify_by_pspec. It might also be worth replacing g_signal_emit_by_name with g_signal_emit where we can, but I haven't looked into that more.
Created attachment 221273 [details] Patch
Attachment 221273 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:49: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:50: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 221273 [details] Patch There are a lot of other places where we could make similar changes, but since they're not media-related, I'm not sure how effectively I could test them.
The style failure is because GStreamer enums don't match WebKit's style.
Comment on attachment 221273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221273&action=review > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:216 > - g_object_notify(G_OBJECT(pad), "tags"); > + g_object_notify_by_pspec(G_OBJECT(pad), padProperties[PROP_PAD_TAGS]); This specific change doesn't seem really worth to me. Pad events don't occur that often (I think) and g_object_notify() for this object should already be quite efficient since the class has only one property. It's true that _by_pspec is advised but I think it depends how often that code path would be used and also on the number of properties of the class. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:861 > - g_object_notify(G_OBJECT(src), "iradio-name"); > + g_object_notify_by_pspec(G_OBJECT(src), properties[PROP_IRADIO_NAME]); About this and the following ones, I think I agree with the changes. How often is handleResponseReceived called though?
(In reply to comment #6) > > Source/WebCore/platform/graphics/gstreamer/TextCombinerGStreamer.cpp:216 > > - g_object_notify(G_OBJECT(pad), "tags"); > > + g_object_notify_by_pspec(G_OBJECT(pad), padProperties[PROP_PAD_TAGS]); > > This specific change doesn't seem really worth to me. Pad events don't occur that often (I think) and g_object_notify() for this object should already be quite efficient since the class has only one property. > > It's true that _by_pspec is advised but I think it depends how often that code path would be used and also on the number of properties of the class. That makes sense. The GLib documentation indicated that you should always use g_object_notify_by_pspec if you can, but I guess it doesn't really matter much in this case. We actually do get quite a few tag events in a row while the video is loading, but the string comparison in g_object_notify probably isn't a bottleneck. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:861 > > - g_object_notify(G_OBJECT(src), "iradio-name"); > > + g_object_notify_by_pspec(G_OBJECT(src), properties[PROP_IRADIO_NAME]); > > About this and the following ones, I think I agree with the changes. How often is handleResponseReceived called though? I'm not really sure. I'll do some tests to see.
(In reply to comment #6) > About this and the following ones, I think I agree with the changes. How often is handleResponseReceived called though? It only gets called once, near the beginning of the stream, apparently.
I can just close this if you don't think it's worthwhile.
Yes, I don't think the benefit would be worth it.