Bug 185278 – Remove dead exception in MediaList.appendMedium

• Component: CSS
• Status: RESOLVED FIXED

[Chris Nardi]

Remove dead exception in MediaList.appendMedium

[Chris Nardi]

Created attachment 339487 [details]
Patch

[Chris Nardi]

Created attachment 339493 [details]
Modify deprecated caller

[Chris Nardi]

Created attachment 339496 [details]
Modify legacy caller

[Darin Adler]

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]

(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]

Created attachment 339557 [details]
Add back GError parameter

[Chris Nardi]

(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 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]

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 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]

Darin, do you think this change is good to land?

[Chris Dumez]

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]

Created attachment 340944 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 340944 [details]
Patch for landing

Clearing flags on attachment: 340944

Committed r232044: <https://trac.webkit.org/changeset/232044>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40439771>