Bug 185278 - Remove dead exception in MediaList.appendMedium
Summary: Remove dead exception in MediaList.appendMedium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-03 16:52 PDT by Chris Nardi
Modified: 2018-05-21 20:12 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.96 KB, patch)
2018-05-03 16:58 PDT, Chris Nardi
no flags Details | Formatted Diff | Diff
Modify deprecated caller (7.13 KB, patch)
2018-05-03 17:18 PDT, Chris Nardi
no flags Details | Formatted Diff | Diff
Modify legacy caller (8.34 KB, patch)
2018-05-03 17:46 PDT, Chris Nardi
no flags Details | Formatted Diff | Diff
Add back GError parameter (6.99 KB, patch)
2018-05-04 10:00 PDT, Chris Nardi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.91 MB, application/zip)
2018-05-04 10:51 PDT, Build Bot
no flags Details
Patch for landing (6.96 KB, patch)
2018-05-21 19:18 PDT, Chris Nardi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Nardi 2018-05-03 16:52:48 PDT
Remove dead exception in MediaList.appendMedium
Comment 1 Chris Nardi 2018-05-03 16:58:52 PDT
Created attachment 339487 [details]
Patch
Comment 2 Chris Nardi 2018-05-03 17:18:29 PDT
Created attachment 339493 [details]
Modify deprecated caller
Comment 3 Chris Nardi 2018-05-03 17:46:30 PDT
Created attachment 339496 [details]
Modify legacy caller
Comment 4 Darin Adler 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.
Comment 5 Michael Catanzaro 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.
Comment 6 Chris Nardi 2018-05-04 10:00:55 PDT
Created attachment 339557 [details]
Add back GError parameter
Comment 7 Chris Nardi 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!
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Michael Catanzaro 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!
Comment 11 Chris Nardi 2018-05-21 18:15:07 PDT
Darin, do you think this change is good to land?
Comment 12 Chris Dumez 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;
Comment 13 Chris Nardi 2018-05-21 19:18:07 PDT
Created attachment 340944 [details]
Patch for landing
Comment 14 WebKit Commit Bot 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>
Comment 15 WebKit Commit Bot 2018-05-21 20:11:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-05-21 20:12:21 PDT
<rdar://problem/40439771>