1454236 - Remove nsINode::eDOCUMENT_FRAGMENT

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1454236: Introduce nsINode::{IsDocumentFragment, AsDocumentFragment}.

I wonder if we should use some macros to reduce a bit the copy pasta in the As*
functions...

Also whether they should start returning references... In any case, those are
followups.

Review commit: https://reviewboard.mozilla.org/r/236736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236736/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1454236: Remove nsINode::eDOCUMENT_FRAGMENT.

Review commit: https://reviewboard.mozilla.org/r/236738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236738/

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8968046 [details]
Bug 1454236: Remove nsINode::eDOCUMENT_FRAGMENT.

https://reviewboard.mozilla.org/r/236738/#review242466


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: parser/html/nsHtml5TreeOperation.cpp:218
(Diff revision 1)
>  IsElementOrTemplateContent(nsINode* aNode)
>  {
>    if (aNode) {
>      if (aNode->IsElement()) {
>        return true;
> -    } else if (aNode->NodeType() == nsINode::DOCUMENT_FRAGMENT_NODE) {
> +    } else if (aNode->IsDocumentFragment()) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968045 [details]
Bug 1454236: Introduce nsINode::{IsDocumentFragment, AsDocumentFragment}.

https://reviewboard.mozilla.org/r/236736/#review243862

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968046 [details]
Bug 1454236: Remove nsINode::eDOCUMENT_FRAGMENT.

https://reviewboard.mozilla.org/r/236738/#review243864

::: editor/libeditor/HTMLEditorDataTransfer.cpp:273
(Diff revision 1)
>                             nodeList,
>                             streamStartParent, streamStartOffset,
>                             streamEndParent, streamEndOffset);

Fix the weird preexisting indent here?

::: parser/html/nsParserUtils.cpp:167
(Diff revision 1)
>      rv = nsContentUtils::ParseFragmentHTML(
>        aFragment, fragment, nsGkAtoms::body, kNameSpaceID_XHTML, false, true);
>    }
>    if (fragment) {
>      nsTreeSanitizer sanitizer(aFlags);
> -    sanitizer.Sanitize(fragment);
> +    sanitizer.Sanitize(fragment->AsDocumentFragment());

This will look a bit saner once you merge on top of bug 1452183 (which is already landed).  In particular, `fragment` is a DocumentFragment after that, so the `sanitizer.Sanitize(fragment)` can stay as-is.  As in, please merge and take this change out.  ;)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67ddc6418466
Introduce nsINode::{IsDocumentFragment, AsDocumentFragment}. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/658bffdfcc1d
Remove nsINode::eDOCUMENT_FRAGMENT. r=bz

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/67ddc6418466
https://hg.mozilla.org/mozilla-central/rev/658bffdfcc1d