Bug 173444 - [WebIDL] Remove custom bindings for HTMLDocument
Summary: [WebIDL] Remove custom bindings for HTMLDocument
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-15 16:04 PDT by Sam Weinig
Modified: 2018-07-04 02:49 PDT (History)
10 users (show)

See Also:


Attachments
Patch (28.31 KB, patch)
2017-06-15 16:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (29.92 KB, patch)
2017-06-15 16:23 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (32.64 KB, patch)
2017-06-15 17:05 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.12 MB, application/zip)
2017-06-15 18:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.28 MB, application/zip)
2017-06-15 18:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (932.99 KB, application/zip)
2017-06-15 18:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.77 MB, application/zip)
2017-06-15 18:47 PDT, Build Bot
no flags Details
Patch (54.13 KB, patch)
2017-06-16 09:37 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (640.35 KB, application/zip)
2017-06-16 10:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (585.58 KB, application/zip)
2017-06-16 10:44 PDT, Build Bot
no flags Details
Patch (56.13 KB, patch)
2017-06-16 10:57 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (56.08 KB, patch)
2017-06-16 11:32 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (491.11 KB, application/zip)
2017-06-16 12:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-elcapitan (234.34 KB, application/zip)
2017-06-16 12:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (201.79 KB, application/zip)
2017-06-16 12:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (463.23 KB, application/zip)
2017-06-16 12:39 PDT, Build Bot
no flags Details
Patch (56.16 KB, patch)
2017-06-16 13:05 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (56.15 KB, patch)
2017-06-16 13:57 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.01 MB, application/zip)
2017-06-16 15:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.90 MB, application/zip)
2017-06-16 15:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.15 MB, application/zip)
2017-06-16 15:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (13.65 MB, application/zip)
2017-06-16 15:35 PDT, Build Bot
no flags Details
Patch (61.35 KB, patch)
2017-06-16 16:01 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1001.11 KB, application/zip)
2017-06-16 17:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.08 MB, application/zip)
2017-06-16 17:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.75 MB, application/zip)
2017-06-16 17:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (923.79 KB, application/zip)
2017-06-16 17:38 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-06-15 16:04:49 PDT
[WebIDL] Remove custom bindings for HTMLDocument
Comment 1 Sam Weinig 2017-06-15 16:18:05 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2017-06-15 16:20:42 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 2017-06-15 16:23:43 PDT Comment hidden (obsolete)
Comment 4 Sam Weinig 2017-06-15 17:05:40 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-06-15 18:12:05 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-06-15 18:12:07 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-06-15 18:18:45 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-06-15 18:18:46 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-06-15 18:44:14 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-06-15 18:44:16 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-06-15 18:47:40 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-06-15 18:47:41 PDT Comment hidden (obsolete)
Comment 13 Sam Weinig 2017-06-16 09:37:04 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-06-16 10:40:16 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-06-16 10:40:18 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2017-06-16 10:44:32 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-06-16 10:44:34 PDT Comment hidden (obsolete)
Comment 18 Sam Weinig 2017-06-16 10:57:45 PDT Comment hidden (obsolete)
Comment 19 Sam Weinig 2017-06-16 11:32:23 PDT Comment hidden (obsolete)
Comment 20 Build Bot 2017-06-16 12:20:44 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2017-06-16 12:20:45 PDT Comment hidden (obsolete)
Comment 22 Build Bot 2017-06-16 12:28:09 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2017-06-16 12:28:10 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2017-06-16 12:30:56 PDT Comment hidden (obsolete)
Comment 25 Build Bot 2017-06-16 12:30:57 PDT Comment hidden (obsolete)
Comment 26 Build Bot 2017-06-16 12:39:00 PDT Comment hidden (obsolete)
Comment 27 Build Bot 2017-06-16 12:39:01 PDT Comment hidden (obsolete)
Comment 28 Sam Weinig 2017-06-16 13:05:52 PDT Comment hidden (obsolete)
Comment 29 Sam Weinig 2017-06-16 13:57:48 PDT
Created attachment 313128 [details]
Patch
Comment 30 Darin Adler 2017-06-16 14:34:57 PDT
Comment on attachment 313128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313128&action=review

Would be nice to line up our terminology with the specification even closer. Can we use the term "responsible document" for example?

> Source/WebCore/ChangeLog:109
> +2017-06-15  Sam Weinig  <sam@webkit.org>

Double change log.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5344
> +        AddToImplIncludes("Document.h");

Not sure I understand why this is required.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5352
> +        AddToImplIncludes("Document.h");

Ditto.

> Source/WebCore/dom/Document.cpp:2663
> +    return;

Should not add this. I assume it’s a leftover from refactoring.

> Source/WebCore/dom/Document.cpp:7257
> +void Document::clear()
> +{
> +    // Per https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-clear, this method does nothing.
> +}
> +
> +void Document::captureEvents()
> +{
> +    // Per https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-captureevents, this method does nothing.
> +}
> +
> +void Document::releaseEvents()
> +{
> +    // Per https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-releaseevents, this method does nothing.
> +}

inline?

> Source/WebCore/dom/Document.h:601
>      WEBCORE_EXPORT void open(Document* ownerDocument = nullptr);

Maybe rename this at some point? Even possible at that time that we might want to rename openForBindings back to open.

> Source/WebCore/dom/Document.h:605
>      WEBCORE_EXPORT void close();

Maybe rename this at some point? Even possible at that time that we might want to rename closeForBindings back to close.

> Source/WebCore/dom/Document.h:1335
> +    WEBCORE_EXPORT const AtomicString& linkColorForBindings() const;
> +    WEBCORE_EXPORT void setLinkColorForBindings(const String&);

Do we really need the existing linkColor and setLinkColor? Can we rename those instead?
Comment 31 Build Bot 2017-06-16 15:11:14 PDT Comment hidden (obsolete)
Comment 32 Build Bot 2017-06-16 15:11:16 PDT Comment hidden (obsolete)
Comment 33 Build Bot 2017-06-16 15:18:10 PDT Comment hidden (obsolete)
Comment 34 Build Bot 2017-06-16 15:18:11 PDT Comment hidden (obsolete)
Comment 35 Build Bot 2017-06-16 15:27:39 PDT Comment hidden (obsolete)
Comment 36 Build Bot 2017-06-16 15:27:41 PDT Comment hidden (obsolete)
Comment 37 Build Bot 2017-06-16 15:35:20 PDT Comment hidden (obsolete)
Comment 38 Build Bot 2017-06-16 15:35:22 PDT Comment hidden (obsolete)
Comment 39 Sam Weinig 2017-06-16 15:58:47 PDT
(In reply to Darin Adler from comment #30)
> Comment on attachment 313128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313128&action=review
> 
> Would be nice to line up our terminology with the specification even closer.
> Can we use the term "responsible document" for example?

I renamed CallerDocument to ResponsibleDocument and added a more clear comment.

> 
> > Source/WebCore/ChangeLog:109
> > +2017-06-15  Sam Weinig  <sam@webkit.org>
> 
> Double change log.

Fixed.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5344
> > +        AddToImplIncludes("Document.h");
> 
> Not sure I understand why this is required.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5352
> > +        AddToImplIncludes("Document.h");
> 
> Ditto.

They shouldn't be. Removed.

> 
> > Source/WebCore/dom/Document.cpp:2663
> > +    return;
> 
> Should not add this. I assume it’s a leftover from refactoring.

Yup. Removed.

> 
> > Source/WebCore/dom/Document.cpp:7257
> > +void Document::clear()
> > +{
> > +    // Per https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-clear, this method does nothing.
> > +}
> > +
> > +void Document::captureEvents()
> > +{
> > +    // Per https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-captureevents, this method does nothing.
> > +}
> > +
> > +void Document::releaseEvents()
> > +{
> > +    // Per https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-releaseevents, this method does nothing.
> > +}
> 
> inline?

Inlined.

> 
> > Source/WebCore/dom/Document.h:601
> >      WEBCORE_EXPORT void open(Document* ownerDocument = nullptr);
> 
> Maybe rename this at some point? Even possible at that time that we might
> want to rename openForBindings back to open.
> 
> > Source/WebCore/dom/Document.h:605
> >      WEBCORE_EXPORT void close();
> 
> Maybe rename this at some point? Even possible at that time that we might
> want to rename closeForBindings back to close.

Added comments. Follow up.

> 
> > Source/WebCore/dom/Document.h:1335
> > +    WEBCORE_EXPORT const AtomicString& linkColorForBindings() const;
> > +    WEBCORE_EXPORT void setLinkColorForBindings(const String&);
> 
> Do we really need the existing linkColor and setLinkColor? Can we rename
> those instead?

I tried to think of an alternate name for the existing linkColor and came up blank. If I think of one I'll come back and fix it.

Thanks for the review!
Comment 40 Sam Weinig 2017-06-16 16:01:07 PDT Comment hidden (obsolete)
Comment 41 Build Bot 2017-06-16 17:11:55 PDT Comment hidden (obsolete)
Comment 42 Build Bot 2017-06-16 17:11:56 PDT Comment hidden (obsolete)
Comment 43 Build Bot 2017-06-16 17:18:30 PDT Comment hidden (obsolete)
Comment 44 Build Bot 2017-06-16 17:18:32 PDT Comment hidden (obsolete)
Comment 45 Build Bot 2017-06-16 17:30:05 PDT Comment hidden (obsolete)
Comment 46 Build Bot 2017-06-16 17:30:07 PDT Comment hidden (obsolete)
Comment 47 Build Bot 2017-06-16 17:38:43 PDT Comment hidden (obsolete)
Comment 48 Build Bot 2017-06-16 17:38:45 PDT Comment hidden (obsolete)
Comment 49 Sam Weinig 2017-06-16 18:19:31 PDT
Committed r218437: <http://trac.webkit.org/changeset/218437>
Comment 50 Frédéric Wang (:fredw) 2018-07-04 02:49:35 PDT
Committed r233503: <https://trac.webkit.org/changeset/233503>