Aside: what do you think about using mock gerrit patches for interface/architecture discussion in the future? Inline gerrit comments (with their "mark as resolved" option) seem like a better way of keeping track of many small threads of discussion than Flow or wikitext.
SlotRoleHandler::getRawHtml:
> If [SlotRoleHandler::getRawHtml] does [modify the HTML], constructing yet another ParserOutput, with the need to copy over complex state (which is quite tricky, as it turns out), seems overkill. It would make the interface prettier, but it would make the code more complicated, both in inside the method and in the caller.
If getRawHtml were replaced with getParserOutput, it would not need to merge anything AFICS, just clone the ParserOutput it gets from the ContentHandler and modify the HTML. And the caller can just fetch the raw HTML from it and be no worse off than using getRawHtml. And the method would be able to add ResourceLoader modules and whatnot, which might be easily a requirement (and might be different for different roles using the same content type so cannot be punted to the content handler).
> SlotParserOutputProvider is passed in because it serves as an in-place cache. I added an explanation to the spec.
The spec says "This follows the in-place caching approach of the current architecture." but I don't get what that refers to. Is the current equivalent $content->getParserOutput()->getRaw()? I don't see any caching in that. I would expect the new classes to behave similarly - if you want caching, use getRenderedRevision::getParserOutput, not the SlotRoleHandler directly.
> "raw" here refers to the fact that the modifications that ParserOutput::getText() applies have not been applied.
So the assumption is that those transformations will always be applied to the combined slot, and consequently the same options apply to all slots (e.g. no disabling section edit links for one slot but not the others)? That seems reasonable, just want to make it explicit.
Anyway my original point was that we need both a way to get the "naked" HTML (for visual diffs and maybe action=render&slot=foo) and to get the wrapped one.
> Perhaps getRawHtml should not exist, and all of this should be left to RevisionRenderer?
I don't think we can avoid having a method for slot-specific wrapping of the raw content HTML, for adding things like the documentation header (and possibly associated styles/scripts). I can't think of a use case for slot-specific generation of the raw content HTML, I think it's fine to have callers use the Content for that. (This is the kind of limitation that should be called out in the RfC though, to see if someone can spot a use case it would break.)
SlotRoleRegistry::getRoleHandler:
The spec says "Note that the same role may have different handlers depending on the page title" and here you say "defineRole() should fail for a role that is already defined." which seems contradictory. I agree that the name "SlotRoleHandler" kind of implies you get one for each role; but as it is specified now, that seems problematic.
SlotRoleHandler::getRawHtml gets a SlotRecord, so it doesn't make much difference whether different role handlers are returned for different content types, or always the same one and it adapts behavior to different content types via some internal behavior (proxying to a subhandler or whatever). That is not the case for getSlotHeadingText/getOutputPlacementHints. I'm not sure about the latter but changing the heading text / wrapping logic based on content type (or title, if that approach is prefered) seems like a reasonable use case. So either that should be passed to all methods, or it should be passed to getRoleHandler (and then instead of singleton-like handler objects we could have a separate one for each revision slot and it could hold the SlotRecord or LinkTarget as an internal variable).
As for content model agnosticism: using a SlotRecord instead of a LinkTarget is more generic as you can always get a LinkTarget from a SlotRecord (in theory it might require a DB lookup, but it's hard to imagine a situation where the slot record is loaded but the revision record isn't). And whatever use case you have in mind for behavior splitting, it seems like a fragile assumption that it will always be deterministic based on the title.
(Unless your goal is to discourage/disallow using the content model as "mutable" state for a slot. If that's the case, it should be more explicit as other parts of the system will have to be adapted - e.g. what will happen to the content type when the page gets renamed? Plus, you need to special-case the main slot again. )
supported content model:
As I said above, I see two options:
- content model is expected to be a deterministic option of the role name and title (with special casing for things like Flow in the main slot). No need for isSupportedContentModel as it's basically the same thing as getDefaultContentModel, but there should be something like updateContentOnPageMove($oldTitle, $newTitle) which converts the old content into a new one that has the expected content model.
- content model is sort of a parameter to the role which acts as mutable state (so an editor can change the slot from documentation<wikitext> to documentation<markdown>). We'll need getSupportedConentModels (not isSupportedConentModel) to tell users trying to change the content model what they can change it to.
The difference between the two options is not huge - you can achieve the same thing with having separate documentation-wikitext and documentation-markdown roles. It will result in different diff behavior though, so IMO the question is, what behavior do we want there? Having a single role (so that diffs work through content type changes) seems like the more user-friendly option to me.
getAllowedRoles/getRequiredRoles:
> In my mind, more advanced control over the edit form should reside in the page type handler.
Anything that can be customizable per role must be in the role handlers, as the page type handler cannot know what roles are added by extensions it is not aware of.
> By the logic that the role handler only depends on the role and the title, the allowed/required roles only depend on the title, not the concrete model.
That is unrelated. Allowed/required flags of course can't depend on their role slot's content model (since these flags are only relevant for roles that do not exist in the current revision) but they could depend on the main content model and on the existence/nonexistence of other roles. None of the use cases listed on the RfC seem to require that, but in general it's not hard to imagine situations in which it is useful.
Also, if these flags depend on the title, what happens when the page is moved and existing slots become forbidden / missing slots become required? The spec addresses that but only superficially. IMO it's one of the questions where we should involve UX experts and consider what kind of user interactions would be needed. Shouldrole handlers be able to prevent a page move when the slot does not make sense under the new title? Do we need some role handler function that provides a nice error message when that happens? (Or not prevent, but show a warning?) Should they be able to do some kind of auto-conversion?
getRenderedRevision:
> In particular, when passing no user for audience checks (that is, the public audience), we still need a way to say what user should be used when resolving ~~~~.
Only for PST. Is there any use case when we pass an unsaved revision to the renderer, and the audience is not the user who created it?
