RESOLVED FIXED 185278
Remove dead exception in MediaList.appendMedium
https://bugs.webkit.org/show_bug.cgi?id=185278
Summary Remove dead exception in MediaList.appendMedium
Chris Nardi
Reported 2018-05-03 16:52:48 PDT
Remove dead exception in MediaList.appendMedium
Attachments
Patch (3.96 KB, patch)
2018-05-03 16:58 PDT, Chris Nardi
no flags
Modify deprecated caller (7.13 KB, patch)
2018-05-03 17:18 PDT, Chris Nardi
no flags
Modify legacy caller (8.34 KB, patch)
2018-05-03 17:46 PDT, Chris Nardi
no flags
Add back GError parameter (6.99 KB, patch)
2018-05-04 10:00 PDT, Chris Nardi
no flags
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.91 MB, application/zip)
2018-05-04 10:51 PDT, EWS Watchlist
no flags
Patch for landing (6.96 KB, patch)
2018-05-21 19:18 PDT, Chris Nardi
no flags
Chris Nardi
Comment 1 2018-05-03 16:58:52 PDT
Chris Nardi
Comment 2 2018-05-03 17:18:29 PDT
Created attachment 339493 [details] Modify deprecated caller
Chris Nardi
Comment 3 2018-05-03 17:46:30 PDT
Created attachment 339496 [details] Modify legacy caller
Darin Adler
Comment 4 2018-05-03 23:55:36 PDT
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.
Michael Catanzaro
Comment 5 2018-05-04 09:39:03 PDT
(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.
Chris Nardi
Comment 6 2018-05-04 10:00:55 PDT
Created attachment 339557 [details] Add back GError parameter
Chris Nardi
Comment 7 2018-05-04 10:02:29 PDT
(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!
EWS Watchlist
Comment 8 2018-05-04 10:51:47 PDT
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
EWS Watchlist
Comment 9 2018-05-04 10:51:49 PDT
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
Michael Catanzaro
Comment 10 2018-05-04 14:01:57 PDT
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!
Chris Nardi
Comment 11 2018-05-21 18:15:07 PDT
Darin, do you think this change is good to land?
Chris Dumez
Comment 12 2018-05-21 18:45:41 PDT
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;
Chris Nardi
Comment 13 2018-05-21 19:18:07 PDT
Created attachment 340944 [details] Patch for landing
WebKit Commit Bot
Comment 14 2018-05-21 20:11:35 PDT
Comment on attachment 340944 [details] Patch for landing Clearing flags on attachment: 340944 Committed r232044: <https://trac.webkit.org/changeset/232044>
WebKit Commit Bot
Comment 15 2018-05-21 20:11:37 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2018-05-21 20:12:21 PDT
Note You need to log in before you can comment on or make changes to this bug.