Bug 185381 – Remove Document#selectedStylesheetSet/preferredStylesheetSet

• Component: CSS
• Status: RESOLVED FIXED

[Chris Nardi]

These attributes are non-standard, and only implemented in WebKit and Blink. Blink is considering removing them, see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/w1Bv7YZxAco and https://crbug.com/690609.

If this change is supported, I can upload a patch as well.

[Chris Nardi]

Created attachment 340132 [details]
Patch

[EWS Watchlist]

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

[Chris Nardi]

Created attachment 340133 [details]
Patch

[Chris Nardi]

Created attachment 340137 [details]
Patch

[Chris Nardi]

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.

[Daniel Bates]

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>

[Chris Nardi]

(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)?

[Michael Catanzaro]

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.

[Chris Nardi]

Created attachment 340369 [details]
Do not remove API methods

[Chris Nardi]

Created attachment 340371 [details]
Remove extra return

[Chris Nardi]

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).

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

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.

[Chris Nardi]

Created attachment 340383 [details]
Mark methods as deprecated

[Chris Nardi]

(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).

[Michael Catanzaro]

Comment on attachment 340383 [details]
Mark methods as deprecated

GTK changes are good!

[WebKit Commit Bot]

Comment on attachment 340383 [details]
Mark methods as deprecated

Clearing flags on attachment: 340383

Committed r231849: <https://trac.webkit.org/changeset/231849>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40295866>

[Darin Adler]

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.

[Daniel Bates]

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