Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove registerContentHandler() and several friends #3027

Merged
merged 2 commits into from Sep 14, 2017
Merged

Remove registerContentHandler() and several friends #3027

merged 2 commits into from Sep 14, 2017

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 8, 2017

They have seen extremely poor adoption unfortunately and were not implemented in line with the standard.

Fixes #630.

@annevk
Remove registerContentHandler() and several friends
1dc7141 
They have seen extremely poor adoption unfortunately and were not implemented in line with the standard.

Fixes #630.
@annevk
Copy link
Member Author

annevk commented Sep 8, 2017

Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1398169.

@annevk
Copy link
Member Author

annevk commented Sep 8, 2017

Note that the algorithms for these methods could generally be much better too. I haven't invested in that for now. It seems better to get closer to the status quo first.

@annevk annevk added the removal/deprecation Removing or deprecating a feature label Sep 8, 2017
domenic
domenic requested changes Sep 13, 2017
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a couple leftovers, one noted inline. Then also, Ctrl+Fing "content handler" shows:

allows Web pages to register themselves as potential protocol and content handlers:

Sad to see these go, but always good to align with what's implemented...

source
@@ -92119,7 +91886,7 @@ interface <dfn>NavigatorContentUtils</dfn> {
<p><strong>Misleading titles.</strong> User agents should not rely wholly on the <var>title</var>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above this there is an example of people registering video/mpeg many times, which is no longer applicable.

source Outdated
@@ -92119,7 +91886,7 @@ interface <dfn>NavigatorContentUtils</dfn> {
<p><strong>Misleading titles.</strong> User agents should not rely wholly on the <var>title</var>
argument to the methods when presenting the registered handlers to the user, since sites could
easily lie. For example, a site <code data-x="">hostile.example.net</code> could claim that it was
registering the "Cuddly Bear Happy Content Handler". User agents should therefore use the
registering the "Cuddly Bear Happy Scheme Handler". User agents should therefore use the
handler's domain in any UI along with any title.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While here, maybe domain -> origin?

@annevk
Copy link
Member Author

annevk commented Sep 13, 2017

I'm rather happy to see registerContentHandler() go as its processing model was never really clear.

Nits addressed.

Once this lands I'd like to do some further cleanup and file bugs against browsers for the remainder.

@domenic domenic added the needs tests Moving the issue forward requires someone to write tests label Sep 13, 2017
domenic
domenic approved these changes Sep 13, 2017
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think we should add some historical.html tests and update any IDL tests, ideally before merging? Sounds like you have a specific plan with the cleanup first though, which I am happy to trust.

@domenic
Copy link
Member

domenic commented Sep 14, 2017

When you do your bug-filing, browsers might particularly appreciate you searching for feature-request bugs filed on them for implementing these methods, and commenting to say "this can be closed now as we've removed it from the spec".

annevk added a commit to web-platform-tests/wpt that referenced this pull request Sep 14, 2017
@annevk
registerContentHandler() and several friends are removed
6fc9a53 
See whatwg/html#3027.
@annevk annevk mentioned this pull request Sep 14, 2017
Merged
@annevk annevk merged commit b143dbc into master Sep 14, 2017
@annevk
Copy link
Member Author

annevk commented Sep 14, 2017

I filed #3043 for the follow-up work.

@annevk annevk deleted the annevk/remove-content-handlers branch September 14, 2017 11:16
bobholt pushed a commit to bobholt/web-platform-tests that referenced this pull request Sep 14, 2017
@annevk
registerContentHandler() and several friends are removed
f6559de 
See whatwg/html#3027.
@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Sep 14, 2017
rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this pull request Nov 8, 2017
@annevk @rachelandrew
registerContentHandler() and several friends are removed
ac41db9 
See whatwg/html#3027.
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
@annevk @jakearchibald
registerContentHandler() and several friends are removed
0ccb059 
See whatwg/html#3027.
@alecmev alecmev mentioned this pull request Dec 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic approved these changes

Assignees
No one assigned
Labels
removal/deprecation Removing or deprecating a feature
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@annevk @domenic