Bug 185381

Summary: Remove Document#selectedStylesheetSet/preferredStylesheetSet
Product: WebKit Reporter: Chris Nardi <cnardi>
Component: CSSAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Do not remove API methods
none
Remove extra return
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Mark methods as deprecated none

Description Chris Nardi 2018-05-07 10:02:53 PDT
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.
Comment 1 Chris Nardi 2018-05-10 14:39:28 PDT
Created attachment 340132 [details]
Patch
Comment 2 EWS Watchlist 2018-05-10 14:41:17 PDT
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
Comment 3 Chris Nardi 2018-05-10 14:51:57 PDT
Created attachment 340133 [details]
Patch
Comment 4 Chris Nardi 2018-05-10 15:35:35 PDT
Created attachment 340137 [details]
Patch
Comment 5 Chris Nardi 2018-05-10 16:08:44 PDT
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 6 Daniel Bates 2018-05-10 19:50:06 PDT
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>
Comment 7 Chris Nardi 2018-05-10 19:59:31 PDT
(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 8 Michael Catanzaro 2018-05-11 07:06:06 PDT
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.
Comment 9 Chris Nardi 2018-05-14 15:47:59 PDT
Created attachment 340369 [details]
Do not remove API methods
Comment 10 Chris Nardi 2018-05-14 16:04:34 PDT
Created attachment 340371 [details]
Remove extra return
Comment 11 Chris Nardi 2018-05-14 16:58:47 PDT
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 12 EWS Watchlist 2018-05-14 17:09:49 PDT
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
Comment 13 EWS Watchlist 2018-05-14 17:09:51 PDT
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 14 Darin Adler 2018-05-14 17:53:22 PDT
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.
Comment 15 Chris Nardi 2018-05-14 18:05:59 PDT
Created attachment 340383 [details]
Mark methods as deprecated
Comment 16 Chris Nardi 2018-05-14 18:07:37 PDT
(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 17 Michael Catanzaro 2018-05-14 18:59:37 PDT
Comment on attachment 340383 [details]
Mark methods as deprecated

GTK changes are good!
Comment 18 WebKit Commit Bot 2018-05-16 09:20:33 PDT
Comment on attachment 340383 [details]
Mark methods as deprecated

Clearing flags on attachment: 340383

Committed r231849: <https://trac.webkit.org/changeset/231849>
Comment 19 WebKit Commit Bot 2018-05-16 09:20:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-05-16 09:21:27 PDT
<rdar://problem/40295866>
Comment 21 Darin Adler 2018-05-16 09:25:33 PDT
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.
Comment 22 Daniel Bates 2018-05-16 11:55:44 PDT
(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.