Remove dead exception in MediaList.appendMedium
Created attachment 339487 [details] Patch
Created attachment 339493 [details] Modify deprecated caller
Created attachment 339496 [details] Modify legacy caller
Comment on attachment 339496 [details] Modify legacy caller View in context: https://bugs.webkit.org/attachment.cgi?id=339496&action=review > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMMediaList.h:84 > - * @error: #GError > * > * > * Deprecated: 2.22: Use JavaScriptCore API instead > **/ > WEBKIT_DEPRECATED void > -webkit_dom_media_list_append_medium(WebKitDOMMediaList* self, const gchar* newMedium, GError** error); > +webkit_dom_media_list_append_medium(WebKitDOMMediaList* self, const gchar* newMedium); I don’t think this is a backwards compatible change for the GTK port. Instead we should keep the error argument for now even if *error is never going to be set to something non-null. One of the maintainers of the GTK port should weigh in on this.
(In reply to Darin Adler from comment #4) > I don’t think this is a backwards compatible change for the GTK port. > Instead we should keep the error argument for now even if *error is never > going to be set to something non-null. One of the maintainers of the GTK > port should weigh in on this. Yes, that's exactly right. That's public API that must not be changed. Thanks for noticing this, Darin! Chris, you can just keep the error parameter, restore the g_return_if_fail(!error || !*error), and then do nothing else with it. It's fine to have an error parameter for legacy reasons, even if it will no longer be set.
Created attachment 339557 [details] Add back GError parameter
(In reply to Michael Catanzaro from comment #5) > (In reply to Darin Adler from comment #4) > > I don’t think this is a backwards compatible change for the GTK port. > > Instead we should keep the error argument for now even if *error is never > > going to be set to something non-null. One of the maintainers of the GTK > > port should weigh in on this. > > Yes, that's exactly right. That's public API that must not be changed. > Thanks for noticing this, Darin! > > Chris, you can just keep the error parameter, restore the > g_return_if_fail(!error || !*error), and then do nothing else with it. It's > fine to have an error parameter for legacy reasons, even if it will no > longer be set. I'll be sure to keep this in mind for future reference, I couldn't tell based on past patches to the GTK API. Thanks for the confirmation!
Comment on attachment 339557 [details] Add back GError parameter Attachment 339557 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7565660 New failing tests: fast/mediastream/delayed-permission-allowed.html
Created attachment 339564 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 339557 [details] Add back GError parameter View in context: https://bugs.webkit.org/attachment.cgi?id=339557&action=review > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMMediaList.cpp:197 > - auto result = item->appendMedium(convertedNewMedium); > - if (result.hasException()) { > - auto description = WebCore::DOMException::description(result.releaseException().code()); > - g_set_error_literal(error, g_quark_from_string("WEBKIT_DOM"), description.legacyCode, description.name); > - } > + item->appendMedium(convertedNewMedium); GTK change is good!
Darin, do you think this change is good to land?
Comment on attachment 339557 [details] Add back GError parameter View in context: https://bugs.webkit.org/attachment.cgi?id=339557&action=review r=me with nit. > Source/WebCore/css/MediaList.cpp:211 > + bool added = m_mediaQueries->add(medium); I do not think we need this local. Could be: if (!m_mediaQueries->add(medium)) return;
Created attachment 340944 [details] Patch for landing
Comment on attachment 340944 [details] Patch for landing Clearing flags on attachment: 340944 Committed r232044: <https://trac.webkit.org/changeset/232044>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40439771>