Summary: | Remove Document#selectedStylesheetSet/preferredStylesheetSet | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Nardi <cnardi> | ||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | ap, berto, cdumez, cgarcia, commit-queue, darin, dbates, esprehn+autocc, ews-watchlist, gustavo, kangil.han, kondapallykalyan, mcatanzaro, rniwa, sam, simon.fraser, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=185684 | ||||||||||||||||||
Attachments: |
|
Description
Chris Nardi
2018-05-07 10:02:53 PDT
Created attachment 340132 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Created attachment 340133 [details]
Patch
Created attachment 340137 [details]
Patch
Comment on attachment 340137 [details]
Patch
I removed methods from the GTK/Mac APIs -- not sure if I should've done that or just left them as empty methods.
Comment on attachment 340137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340137&action=review > Source/WebKitLegacy/mac/DOM/DOMDocument.h:84 > -@property (readonly, copy) NSString *preferredStylesheetSet WEBKIT_AVAILABLE_MAC(10_5); > -@property (copy) NSString *selectedStylesheetSet WEBKIT_AVAILABLE_MAC(10_5); > @property (readonly, strong) DOMElement *activeElement WEBKIT_AVAILABLE_MAC(10_6); This is public Mac API. We cannot remove these without deprecated them first. <https://developer.apple.com/documentation/webkit/domdocument/1494907-selectedstylesheetset?language=objc> <https://developer.apple.com/documentation/webkit/domdocument/1494857-preferredstylesheetset?language=objc> (In reply to Daniel Bates from comment #6) > Comment on attachment 340137 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340137&action=review > > > Source/WebKitLegacy/mac/DOM/DOMDocument.h:84 > > -@property (readonly, copy) NSString *preferredStylesheetSet WEBKIT_AVAILABLE_MAC(10_5); > > -@property (copy) NSString *selectedStylesheetSet WEBKIT_AVAILABLE_MAC(10_5); > > @property (readonly, strong) DOMElement *activeElement WEBKIT_AVAILABLE_MAC(10_6); > > This is public Mac API. We cannot remove these without deprecated them first. > > <https://developer.apple.com/documentation/webkit/domdocument/1494907- > selectedstylesheetset?language=objc> > <https://developer.apple.com/documentation/webkit/domdocument/1494857- > preferredstylesheetset?language=objc> What would be the protocol for removal then? Would we just remove the web-facing attributes (IDL), mark the API functions as deprecated, and then wait a cycle to actually remove them (or something of that nature)? Comment on attachment 340137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340137&action=review > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:-134 > - DOM_DOCUMENT_PROP_PREFERRED_STYLESHEET_SET, > - DOM_DOCUMENT_PROP_SELECTED_STYLESHEET_SET, For GTK, the public methods and properties can never be removed. But since it's quite likely that no applications have ever used these, and they are already deprecated, it would be fine to make them do nothing and print a runtime warning. In the unlikely event it breaks an application, we can deal with it. > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:-185 > - case DOM_DOCUMENT_PROP_SELECTED_STYLESHEET_SET: > - webkit_dom_document_set_selected_stylesheet_set(self, g_value_get_string(value)); > - break; case DOM_DOCUMENT_PROP_SELECTED_STYLESHEET_SET: g_warning("%s: The selected-stylesheet-set property has been removed and no longer works.", __func__); break; > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:-298 > - case DOM_DOCUMENT_PROP_PREFERRED_STYLESHEET_SET: > - g_value_take_string(value, webkit_dom_document_get_preferred_stylesheet_set(self)); > - break; > - case DOM_DOCUMENT_PROP_SELECTED_STYLESHEET_SET: > - g_value_take_string(value, webkit_dom_document_get_selected_stylesheet_set(self)); > - break; Ditto. > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:-700 > - g_object_class_install_property( > - gobjectClass, > - DOM_DOCUMENT_PROP_PREFERRED_STYLESHEET_SET, > - g_param_spec_string( > - "preferred-stylesheet-set", > - "Document:preferred-stylesheet-set", > - "read-only gchar* Document:preferred-stylesheet-set", > - "", > - WEBKIT_PARAM_READABLE)); > - > - g_object_class_install_property( > - gobjectClass, > - DOM_DOCUMENT_PROP_SELECTED_STYLESHEET_SET, > - g_param_spec_string( > - "selected-stylesheet-set", > - "Document:selected-stylesheet-set", > - "read-write gchar* Document:selected-stylesheet-set", > - "", > - WEBKIT_PARAM_READWRITE)); > - Leave these. > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:-1794 > - WebCore::JSMainThreadNullState state; > - g_return_val_if_fail(WEBKIT_DOM_IS_DOCUMENT(self), 0); > - WebCore::Document* item = WebKit::core(self); > - gchar* result = convertToUTF8String(item->preferredStylesheetSet()); > - return result; { g_warning("%s: this function has been removed and does nothing", __func__); return nullptr; } > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:-1814 > -gchar* webkit_dom_document_get_selected_stylesheet_set(WebKitDOMDocument* self) > -{ > - WebCore::JSMainThreadNullState state; > - g_return_val_if_fail(WEBKIT_DOM_IS_DOCUMENT(self), 0); > - WebCore::Document* item = WebKit::core(self); > - gchar* result = convertToUTF8String(item->selectedStylesheetSet()); > - return result; > -} > - > -void webkit_dom_document_set_selected_stylesheet_set(WebKitDOMDocument* self, const gchar* value) > -{ > - WebCore::JSMainThreadNullState state; > - g_return_if_fail(WEBKIT_DOM_IS_DOCUMENT(self)); > - g_return_if_fail(value); > - WebCore::Document* item = WebKit::core(self); > - WTF::String convertedValue = WTF::String::fromUTF8(value); > - item->setSelectedStylesheetSet(convertedValue); > -} Ditto. > Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/docs/webkitdomgtk-4.0-sections.txt:-501 > -webkit_dom_document_get_preferred_stylesheet_set > -webkit_dom_document_get_selected_stylesheet_set > -webkit_dom_document_set_selected_stylesheet_set Leave these. Created attachment 340369 [details]
Do not remove API methods
Created attachment 340371 [details]
Remove extra return
Hopefully this new patch looks better for both the Mac and GTK APIs. Instead of removing the methods, I simply made them return a warning (as suggested for GTK) or an empty value (for Mac). Comment on attachment 340371 [details] Remove extra return Attachment 340371 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7683281 New failing tests: css3/filters/backdrop/add-remove-add-backdrop-filter.html Created attachment 340381 [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 340371 [details] Remove extra return View in context: https://bugs.webkit.org/attachment.cgi?id=340371&action=review This is fine. Eventually we would also want to deprecate the Mac API, but someone else can do that later. > Source/WebKitLegacy/mac/DOM/DOMDocument.mm:339 > + return; No need for this line of code. Created attachment 340383 [details]
Mark methods as deprecated
(In reply to Darin Adler from comment #14) > Comment on attachment 340371 [details] > Remove extra return > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340371&action=review > > This is fine. Eventually we would also want to deprecate the Mac API, but > someone else can do that later. > > > Source/WebKitLegacy/mac/DOM/DOMDocument.mm:339 > > + return; > > No need for this line of code. I removed the extra return and (hopefully) marked the methods as deprecated (I couldn't exactly tell what the macro was but I just followed the example of the class macro above). Comment on attachment 340383 [details]
Mark methods as deprecated
GTK changes are good!
Comment on attachment 340383 [details] Mark methods as deprecated Clearing flags on attachment: 340383 Committed r231849: <https://trac.webkit.org/changeset/231849> All reviewed patches have been landed. Closing bug. Comment on attachment 340383 [details] Mark methods as deprecated View in context: https://bugs.webkit.org/attachment.cgi?id=340383&action=review > Source/WebKitLegacy/mac/DOM/DOMDocument.h:85 > -@property (readonly, copy) NSString *preferredStylesheetSet WEBKIT_AVAILABLE_MAC(10_5); > -@property (copy) NSString *selectedStylesheetSet WEBKIT_AVAILABLE_MAC(10_5); > +@property (readonly, copy) NSString *preferredStylesheetSet WEBKIT_DEPRECATED_MAC(10_5, 10_14); > +@property (copy) NSString *selectedStylesheetSet WEBKIT_DEPRECATED_MAC(10_5, 10_14); I just realized this change is unnecessary. The entire class is deprecated so there’s no reason to also deprecate individual functions. Someone should post a patch and roll this back. (In reply to Darin Adler from comment #21) > > Source/WebKitLegacy/mac/DOM/DOMDocument.h:85 > > -@property (readonly, copy) NSString *preferredStylesheetSet WEBKIT_AVAILABLE_MAC(10_5); > > -@property (copy) NSString *selectedStylesheetSet WEBKIT_AVAILABLE_MAC(10_5); > > +@property (readonly, copy) NSString *preferredStylesheetSet WEBKIT_DEPRECATED_MAC(10_5, 10_14); > > +@property (copy) NSString *selectedStylesheetSet WEBKIT_DEPRECATED_MAC(10_5, 10_14); > > I just realized this change is unnecessary. The entire class is deprecated > so there’s no reason to also deprecate individual functions. Someone should > post a patch and roll this back. This was addressed in the fix for bug #185684. |