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 module framework (#1066). #1096

Merged
merged 9 commits into from
Aug 3, 2019
Merged

Add module framework (#1066). #1096

merged 9 commits into from
Aug 3, 2019

Conversation

skynavga
Copy link
Collaborator

Closes #1066.

@skynavga skynavga added this to the 2ED-FPWD milestone May 25, 2019
@skynavga skynavga self-assigned this May 25, 2019
Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

This pull request introduces both a constraint on where elements can be defined within W3C namespaces and the concept of, and requirement to use, a module registry. It's not clear to me that we have consensus for either of those introduced concepts at this time.

I would argue that the constraint on defining vocabulary within a W3C namespace exists independently of the introduction of modules and does not need to be repeated or duplicated.

I would argue that the fact that a module is registered or not registered in some external document is orthogonal to the specification and we don't need to introduce the text about it. All that matters is that, somehow, the module is considered in scope when the document is being processed. The obvious way to bring external modules into scope is by specifying within a profile document the features that depend on the module.

@skynavga how would you feel about simplifying this pull request to remove the namespace constraints and the registration/non-registration aspects, and to add that profile features may depend on externally defined modules?

spec/ttml2.xml Outdated Show resolved Hide resolved
@skynavga
Copy link
Collaborator Author

@nigelmegitt you do realize that these changes are exactly what we have already accepted and merged into TTML3, and that making a change here would create an inconsistency with TTML3? see w3c/ttml3#30 for confirmation.

@nigelmegitt
Copy link
Contributor

@skynavga I seem to recall there was disquiet about those changes in TTML3 also. In my view it may be that the best approach is to fix it here and then port these changes as incremental changes onto TTML3 also, or it may be that we think TTML3 has a different set of constraints on modules than TTML2. Either way, my comments still stand.

@skynavga
Copy link
Collaborator Author

@nigelmegitt can we merge this PR and deal with your comments by way of another issue? since it proposes an additional follow-on substantive change that would affect both this spec and TTML3?

Copy link
Contributor

@palemieux palemieux left a comment

Choose a reason for hiding this comment

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

The introduction of a

spec/ttml2.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@cconcolato cconcolato left a comment

Choose a reason for hiding this comment

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

See comment #1066 (comment)

@skynavga
Copy link
Collaborator Author

I have attempted to address various comments in 2f67bbd, and, in particular, I have changed the adjectives internal/external to core/extension w.r.t. to characterizing modules and simplified language.

I have also removed the definition and references to a TTWG controlled Module Registry, which I still think is a good idea that will assist interoperability and implementation activity.

The net effect of removing the registry is that anyone may define an extension module that adds a content element or an attribute that it wishes to be treated as a style property regardless of whether it is in a TTWG controlled namespace or not.

I firmly believe this will lead to interoperability problems in the future, but we shall see.

spec/ttml2.xml Show resolved Hide resolved
@skynavga skynavga requested a review from palemieux July 12, 2019 05:54
@palemieux
Copy link
Contributor

@skynavga I have approved the PR. Cyril and Nigel had much stronger opinions on this since they are writing "modules", so I encourage you to check with them.

@skynavga
Copy link
Collaborator Author

@cconcolato @nigelmegitt please check

@skynavga
Copy link
Collaborator Author

@palemieux please mark your conversation #1096 (review) as resolved, or ask me to do so

@skynavga skynavga added semantics Changes processing semantics. syntax Changes document syntax. labels Jul 20, 2019
@skynavga
Copy link
Collaborator Author

Even though this PR impacts both document syntax and processing semantics, it is not interoperably testable as such, since whether or not a given implementation implements a feature defining module is dependent upon that implementation, and, if it does not implement it, then the default pruning rules will prune any elements or attributes added by a given module specification. Note, however, that element and/or attribute syntax associated with a given module specification could be tested for validity on an implementation that does support the module. Such tests would also need to specify a profile that requires support for any feature defined by the associated module that is being tested.

@skynavga skynavga added untestable Not interoperably testable. enhancement and removed enhancement labels Jul 20, 2019
Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

Thanks for the extensive work on this one @skynavga , I think we've ended up with a good result.

spec/ttml2.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@cconcolato cconcolato left a comment

Choose a reason for hiding this comment

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

The PR has been improved quite a bit. Thank you. I'm still not convinced that the formal definition of modules was necessary, and the current definition of modules mixes everything from syntactical elements (attributes) to specifications, which is strange to me. Also, I'm not sure the 'clarification' about the difference between module and profile is needed. But in the interest of making progress, I'm approving this PR.

@skynavga
Copy link
Collaborator Author

skynavga commented Aug 3, 2019

@cconcolato I've tried to address your comment in 7c3ef1c:

the current definition of modules mixes everything from syntactical elements (attributes) to specifications, which is strange to me

there are no other outstanding, actionable comments at this time, so based on the current three approvals, I'm proceeding with merging

@skynavga skynavga merged commit 69630a1 into master Aug 3, 2019
@skynavga skynavga deleted the issue-1066-module-framework branch August 3, 2019 09:24
@skynavga skynavga removed their assignment Aug 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semantics Changes processing semantics. substantive syntax Changes document syntax. untestable Not interoperably testable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add hooks to TTML2 to simplify the creation of external modules.
4 participants