Bug 190028 – Use enum class in createMarkup arguments

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

createMarkup takes a bunch of enums and a boolean. Make them enum class.

[Ryosuke Niwa]

Created attachment 350941 [details]
WIP

[EWS Watchlist]

Attachment 350941 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:77:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 350944 [details]
WIP2

[EWS Watchlist]

Attachment 350944 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:77:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 350947 [details]
WIP3

[EWS Watchlist]

Attachment 350947 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:77:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 350948 [details]
WIP4

[EWS Watchlist]

Attachment 350948 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:77:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 350950 [details]
Cleanup

[EWS Watchlist]

Attachment 350950 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:77:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 350952 [details]
Fixed Windows builds

[EWS Watchlist]

Attachment 350952 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:77:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 350953 [details]
Fix Windows build for real

[EWS Watchlist]

Attachment 350953 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:77:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

iOS EWS appears to be simply broken...

raceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/xcode/simulated_device.py", line 446, in tear_down
    device.platform_device._tear_down(deadline - time.time())
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/xcode/simulated_device.py", line 522, in _tear_down
    self._shut_down(deadline - time.time())
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/xcode/simulated_device.py", line 508, in _shut_down
    raise RuntimeError('Timed out while waiting for {} to shut down'.format(self.udid))
RuntimeError: Timed out while waiting for 3FDF5418-1774-4710-B947-9952835E2563 to shut down

[EWS Watchlist]

Comment on attachment 350953 [details]
Fix Windows build for real

Attachment 350953 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9366821

New failing tests:
fast/workers/worker-exception-during-navigation.html

[EWS Watchlist]

Created attachment 350958 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Wenson Hsieh]

Comment on attachment 350953 [details]
Fix Windows build for real

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

> Source/WebCore/editing/markup.h:58
> +enum class URLsToResolve { None, AllURLs, ExcludeLocalFiles };

It looks like this one is actually stored somewhere, so maybe it's worth making the enum class narrower? (uint8_t)

[Darin Adler]

Comment on attachment 350953 [details]
Fix Windows build for real

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

This patch is fine, but since it’s all about naming and clarity I do have some comments and things for you to ponder, but no "required" changes.

My preference when possible is to use separately named functions rather than configuration/policy arguments, even if those arguments are named well rather than being mysterious booleans, but in cases like this where that would cause a combinatorial explosion of functions, I think this is required. Another pattern I like is that if there are many of these policy type arguments, a structure is often better than a list of separate arguments, or in other cases, an OptionSet.

We should look into the Windows test failure. I suspect it’s real. But surprising since this should just be a naming change, with no effect on behavior.

> Source/WebCore/editing/HTMLInterchange.h:41
> -enum EAnnotateForInterchange { DoNotAnnotateForInterchange, AnnotateForInterchange };
> +enum class AnnotateForInterchange { No, Yes };

Despite long history with this code, I can’t remember precisely what this means. Seems like this needs a comment defining what it means to annotate for interchange.

> Source/WebCore/editing/markup.cpp:1103
> +// Used by [DOMNode markupString] in WebKitLegacy.
> +String serializeFragmentWithDocType(const Node& node)
>  {
> -    // FIXME: This is never "for interchange". Is that right?
> -    String markupString = createMarkup(node, IncludeNode, 0);
> +    String markupString = serializeFragment(node, FragmentSerializationRoot::Self);

Instead of adding this comment, how about moving this small, otherwise-mysterious function into WebKitLegacy and exporting the two functions it uses instead. The way it does things is strange, and probably needs to be kept this way for compatibility -- a "legacy" concern -- so it’s nicer to just move the code there?

> Source/WebCore/editing/markup.cpp:1116
> +// Used by [DOMRange markupString] in WebKitLegacy.
>  String createFullMarkup(const Range& range)
>  {
> -    // FIXME: This is always "for interchange". Is that right?
> -    return documentTypeString(range.startContainer().document()) + createMarkup(range, 0, AnnotateForInterchange);
> +    return documentTypeString(range.startContainer().document()) + createMarkup(range, nullptr, AnnotateForInterchange::Yes);
>  }

Ditto.

> Source/WebCore/editing/markup.h:58
> +enum class URLsToResolve { None, AllURLs, ExcludeLocalFiles };

The type name here sounds like it would be a collection of URLs, not a policy about URL resolution. For more consistency with other similar enumeration names that use verbs and yes/no you could consider naming a little more like this:

    ResolveURLs::No, ResolveURLs::Yes, ResolveURLs::NonLocalFileURLs

Also would be good to have a name that makes it slightly clearer why "ResolveNonLocalURLs" is an important option to offer. It’s not all that clear that it is a security-policy-related feature. I wish we could find a way to work in the word "allow" or something else that alludes to security concerns.

> Source/WebCore/editing/markup.h:72
> +enum class ConvertBlocksToInlines { No, Yes };

This long-standing concept seems mysterious and not self-explanatory. How can one convert an arbitrary block to an inline: what would a <p> be turned into? And block and inline are layout and style concepts, not markup concepts, so whatever this does, it must be some crude approximation of those things. I think it’s worth considering what this really means so we can give it an even better name.

> Source/WebCore/editing/markup.h:77
> +enum class FragmentSerializationRoot { Self, Children };
> +enum class FragmentSerializationMode { HTML, XML };

- The concept of choosing HTML vs. XML is not specific to *fragment* serialization. So could omit the word "fragment" from the name. We could call it "syntax", since that’s what the HTML specification calls it <https://html.spec.whatwg.org/multipage/syntax.html>. SerializationSyntax::HTML.
- It’s imprecise to call the children the "root" of serialization, since by definition a set of multiple nodes is not a single "root". I think of this as "omit root" vs. "include root" as opposed to say the children *become* the root. On the other hand, "children" is a pleasant familiar word, so maybe we don’t need to change it. I’m not so sure about "self" as the word for "the object you passed as the first argument". Two separate named functions might be better: serializeSubtree, serializeSubtreeOmittingRoot? Not sure those names are better.
- Perhaps consider omitting the word "Fragment" from *both* of these type names, simply for brevity, since it’s already in the function name and these aren’t ambiguous without it.

> Source/WebCore/editing/markup.h:78
> +String serializeFragment(const Node&, FragmentSerializationRoot, Vector<Node*>* = nullptr, URLsToResolve = URLsToResolve::None, Vector<QualifiedName>* tagNamesToSkip = nullptr, FragmentSerializationMode = FragmentSerializationMode::HTML);

I’m not sure I love the new function name.

This name uses the term "fragment" when there is a node type called a "document fragment". If this function really was specific to document fragments, there would be no need to specify whether to serialize the root, since a document fragment itself would not be serialized.

I’m not entirely sure that a subtree of a document should be called a "fragment" when it’s still in the tree. Seems like it’s just a subtree then. The term "fragment" is more apropos for something that is no longer inside a document, has been broken from the tree. On the other hand, once it’s serialized it has been broken from the tree so I suppose the result can be considered a "serialized fragment". Not sure I’m right, just thinking out loud.

This name doesn’t make it clear that the serialization will leave out style, which is what you chose to emphasize in the change log comment. Might even need a comment explaining the purpose of this function, and writing that comment might help us come up with an event better name.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #19)
> Comment on attachment 350953 [details]
> Fix Windows build for real
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350953&action=review
> 
> This patch is fine, but since it’s all about naming and clarity I do have
> some comments and things for you to ponder, but no "required" changes.
> 
> My preference when possible is to use separately named functions rather than
> configuration/policy arguments, even if those arguments are named well
> rather than being mysterious booleans, but in cases like this where that
> would cause a combinatorial explosion of functions, I think this is
> required. Another pattern I like is that if there are many of these policy
> type arguments, a structure is often better than a list of separate
> arguments, or in other cases, an OptionSet.

Yeah. The challenge here is that each enum class is supported / used by a slightly different set of functions & classes. I think this code needs quite a bit of help to get to a clean state.

> We should look into the Windows test failure. I suspect it’s real. But
> surprising since this should just be a naming change, with no effect on
> behavior.

I *think* it's an unrelated failure since the failing test is fast/workers/worker-exception-during-navigation.html

> 
> > Source/WebCore/editing/HTMLInterchange.h:41
> > -enum EAnnotateForInterchange { DoNotAnnotateForInterchange, AnnotateForInterchange };
> > +enum class AnnotateForInterchange { No, Yes };
> 
> Despite long history with this code, I can’t remember precisely what this
> means. Seems like this needs a comment defining what it means to annotate
> for interchange.

It was added by https://trac.webkit.org/changeset/8087/webkit. I *think* the flag adds a special new line at the end which is removed upon paste. We also convert space to space/nbsp patterns using convertHTMLTextToInterchangeFormat.

Will add the following comment:
// Controls whether a special BR which is removed upon paste in ReplaceSelectionCommand needs to be inserted
// and making sequence of spaces not collapsible by inserting non-breaking spaces.
// See https://trac.webkit.org/r8087 and https://trac.webkit.org/r8096.

> > Source/WebCore/editing/markup.cpp:1103
> > +// Used by [DOMNode markupString] in WebKitLegacy.
> > +String serializeFragmentWithDocType(const Node& node)
> >  {
> > -    // FIXME: This is never "for interchange". Is that right?
> > -    String markupString = createMarkup(node, IncludeNode, 0);
> > +    String markupString = serializeFragment(node, FragmentSerializationRoot::Self);
> 
> Instead of adding this comment, how about moving this small,
> otherwise-mysterious function into WebKitLegacy and exporting the two
> functions it uses instead. The way it does things is strange, and probably
> needs to be kept this way for compatibility -- a "legacy" concern -- so it’s
> nicer to just move the code there?

Yeah, I was considering to do that as a follow up. Will fix in this patch instead.

> > Source/WebCore/editing/markup.h:58
> > +enum class URLsToResolve { None, AllURLs, ExcludeLocalFiles };
> 
> The type name here sounds like it would be a collection of URLs, not a
> policy about URL resolution. For more consistency with other similar
> enumeration names that use verbs and yes/no you could consider naming a
> little more like this:
> 
>     ResolveURLs::No, ResolveURLs::Yes, ResolveURLs::NonLocalFileURLs

Haha, funny. I had that exact enum class earlier and changed it to this. Will use that instead.

> Also would be good to have a name that makes it slightly clearer why
> "ResolveNonLocalURLs" is an important option to offer. It’s not all that
> clear that it is a security-policy-related feature. I wish we could find a
> way to work in the word "allow" or something else that alludes to security
> concerns.

Okay, I'd go with ResolveURLs::No, ResolveURLs::Yes, and ResolveURLs::YesExcludingLocalFileURLsForPrivacy then.

> > Source/WebCore/editing/markup.h:72
> > +enum class ConvertBlocksToInlines { No, Yes };
> 
> This long-standing concept seems mysterious and not self-explanatory. How
> can one convert an arbitrary block to an inline: what would a <p> be turned
> into? And block and inline are layout and style concepts, not markup
> concepts, so whatever this does, it must be some crude approximation of
> those things. I think it’s worth considering what this really means so we
> can give it an even better name.

Oh, we add "display: inline" when ConvertBlocksToInlines::Yes is set in CompositeEditCommand::moveParagraphs. It comes from https://trac.webkit.org/changeset/20346 and https://trac.webkit.org/changeset/20542.

It's possible we don't really need it anymore. It's a strange transformation to be performing...

> > Source/WebCore/editing/markup.h:77
> > +enum class FragmentSerializationRoot { Self, Children };
> > +enum class FragmentSerializationMode { HTML, XML };
> 
> - The concept of choosing HTML vs. XML is not specific to *fragment*
> serialization. So could omit the word "fragment" from the name. We could
> call it "syntax", since that’s what the HTML specification calls it
> <https://html.spec.whatwg.org/multipage/syntax.html>.
> SerializationSyntax::HTML.

Okay, will do. BTW, the latest spec for serialization is at: https://www.w3.org/TR/DOM-Parsing/#serializing

> - It’s imprecise to call the children the "root" of serialization, since by
> definition a set of multiple nodes is not a single "root". I think of this
> as "omit root" vs. "include root" as opposed to say the children *become*
> the root. On the other hand, "children" is a pleasant familiar word, so
> maybe we don’t need to change it. I’m not so sure about "self" as the word
> for "the object you passed as the first argument". Two separate named
> functions might be better: serializeSubtree, serializeSubtreeOmittingRoot?
> Not sure those names are better.

Perhaps serializeSubtree and serializeChildren if we're going with two function route?
But I do like the fact the name "serializeFragment" is more directly relatable to
"the fragment serialization algorithm", which is the name used in various specs.

I'd go with SerializedNodes::SubtreeIncludingNode and SerializedNodes::SubtreesOfChildren.

> - Perhaps consider omitting the word "Fragment" from *both* of these type
> names, simply for brevity, since it’s already in the function name and these
> aren’t ambiguous without it.

Will do.

> > Source/WebCore/editing/markup.h:78
> > +String serializeFragment(const Node&, FragmentSerializationRoot, Vector<Node*>* = nullptr, URLsToResolve = URLsToResolve::None, Vector<QualifiedName>* tagNamesToSkip = nullptr, FragmentSerializationMode = FragmentSerializationMode::HTML);
> 
> I’m not sure I love the new function name.
> 
> This name uses the term "fragment" when there is a node type called a
> "document fragment". If this function really was specific to document
> fragments, there would be no need to specify whether to serialize the root,
> since a document fragment itself would not be serialized.

This is because the spec uses the term "fragment serialization algorithm":
https://w3c.github.io/DOM-Parsing/#dfn-fragment-serializing-algorithm

I agree it's not the ideal term but I think matching the terminology used in
HTML & related specifications is quite useful.

Eventually, it's probably good to rename MarkupAccumulator to FragmentSerialization.h
so that the relationship between our implementation & spec would be self-evident.

> This name doesn’t make it clear that the serialization will leave out style,
> which is what you chose to emphasize in the change log comment. Might even
> need a comment explaining the purpose of this function, and writing that
> comment might help us come up with an event better name.

Yeah, my plan is to rename createMarkup to something like createMarkupForRangeWithStyle in a follow up patch.
Perhaps I should have done it in this patch to make it more coherent...

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #18)
> Comment on attachment 350953 [details]
> Fix Windows build for real
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350953&action=review
> 
> > Source/WebCore/editing/markup.h:58
> > +enum class URLsToResolve { None, AllURLs, ExcludeLocalFiles };
> 
> It looks like this one is actually stored somewhere, so maybe it's worth
> making the enum class narrower? (uint8_t)

Yeah, I forgot to make all of them uint8_t. Will do.

[Ryosuke Niwa]

Created attachment 351022 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 351022 [details]
Patch for landing

Wait for EWS

[EWS Watchlist]

Attachment 351022 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:75:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 351034 [details]
Fixed GTK build

[EWS Watchlist]

Attachment 351034 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/markup.h:75:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radar WebKit Bug Importer]

<rdar://problem/44853864>

[Ryosuke Niwa]

Committed r236583: <https://trac.webkit.org/changeset/236583>