WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
,
EWS Watchlist
no flags
Details
Patch for landing
(6.96 KB, patch)
2018-05-21 19:18 PDT
,
Chris Nardi
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Chris Nardi
Comment 1
2018-05-03 16:58:52 PDT
Created
attachment 339487
[details]
Patch
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
<
rdar://problem/40439771
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug