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.

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

Add portals (defer). #345

Merged
merged 4 commits into from
May 30, 2020
Merged

Add portals (defer). #345

merged 4 commits into from
May 30, 2020

Conversation

dbaron
Copy link
Contributor

@dbaron dbaron commented May 23, 2020

Closes #157.

@dbaron dbaron requested a review from annevk May 23, 2020 00:13
@dbaron dbaron self-assigned this May 23, 2020
Copy link
Member

@tantek tantek left a comment

Choose a reason for hiding this comment

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

LGTM. Reviewed issue #157 as well and everything seems consistent.

@mozilla mozilla deleted a comment May 25, 2020
@mozilla mozilla deleted a comment May 25, 2020
Copy link
Contributor

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Looks good modulo nit.

activities.json Outdated Show resolved Hide resolved
@domenic
Copy link
Contributor

domenic commented May 26, 2020

Hey folks!

I wanted to chime in here to express a bit of surprise at this position. Although I appreciate the open-ended sentence at the end, I'm not sure what to make of the rest. The portals proposal has extensive discussion of the differences between portals and iframes. And, at least the current draft fully specifies the interaction with all web platform features except session history. (Well, it actually fully specifies the interaction with session history, by using the top-level browsing context concept. But this creates too much implementer work for figuring out how to map session history to UI elements such as the back button, so we've had a long-standing acknowledged issue to detail that mapping in a way that makes it easier for implementers to give their users a good user experience.) Certainly, there is no confusion with the origin model in the current draft; it's not modified at all. Finally, although judgments of what are "significant" are subjective, I was under the impression from our meeting with @annevk in Berlin that we'd managed to keep the complexity pretty low, at least in his estimation. The spec is, after all, pretty short.

All that said! The real reason that I'd push for a "defer" instead of "harmful" is that the portals proposal is undergoing significant changes, in responses to the shifting tides over the last year or so in terms of storage partitioning efforts in browsers. As such, the current draft does not reflect what we intend to implement in Chromium. That's more captured in the explainer. And the explainer itself has a lot of gaps: if you squint at the various "TODO" sections I've added, you can get a sense of where we are heading, but it's not something I'd really encourage anyone to spend time on; we know there's work to do in laying out the proposed modifications to the storage restrictions and partitioning model, permissions, and rendering in a digestible format, first as an explainer, and then as a spec. (These will, unavoidably, increase the complexity of the spec a bit, perhaps even to "significant" levels, but we think this will be worth it for the privacy benefits.)

So, if it's possible under your process, we'd love to withdraw the current portals proposal from consideration. It's too much under-construction at the moment to be fairly judged. We're hopeful that the direction we're heading will make it more interesting to Mozilla. (That direction is, roughly, laser-focused on privacy-preserving prerendering, including accommodations for modern storage partitioning efforts.) It's certainly up to you whether you'd want to mark portals as "harmful" in the meantime, but I think it would be a bit strange, as the position is being taken on an older draft of the spec/explainer, and as mentioned above, seems to have some inaccuracies in how it read that spec/explainer.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@dbaron dbaron marked this pull request as draft May 27, 2020 22:23
@dbaron dbaron changed the title Add portals (harmful). Add portals (defer). May 29, 2020
@dbaron dbaron marked this pull request as ready for review May 29, 2020 19:39
@dbaron dbaron requested review from annevk, bholley and tantek May 29, 2020 19:39
Copy link
Collaborator

@bholley bholley left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but I wonder if we should link directly to Domenic's comment from the position?

Copy link
Member

@tantek tantek left a comment

Choose a reason for hiding this comment

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

Agree with @bholley's suggestion. In addition we should make explicit about defer vs harmful current state. E.g. something like:

s/While this proposal is in the early stages of development and it is too early to evaluate fully, there are concerns/Marking ‘defer’ because per Dominic's comment(link) this proposal is in the early stages of development and it is too early to evaluate fully. However in it's current state it would be marked ‘harmful’ since there are concerns

@dbaron dbaron requested review from tantek and bholley May 29, 2020 22:24
Copy link
Member

@tantek tantek left a comment

Choose a reason for hiding this comment

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

LGTM. This is a good way of phrasing it, noting explicitly the existence of harmful things, necessary to fix (but may not be sufficient, depending on how it evolves).

@annevk annevk merged commit f0d220a into mozilla:master May 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTML portal element (Portals)
5 participants