1426494 - Share more code between nsIDocument and ShadowRoot.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

Now we have a common class for them (StyleScope), we should be able to share a bit more code among them.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1426494: Devirtualize StyleScope::AsNode.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1426494: Share more code among Document / ShadowRoot.

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

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8938135 [details]
Bug 1426494: Devirtualize StyleScope::AsNode.

https://reviewboard.mozilla.org/r/208852/#review214586


C/C++ static analysis found 2 defects in this patch.

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

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


::: dom/base/StyleScope.cpp:13
(Diff revision 1)
>  #include "mozilla/dom/StyleSheetList.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> +StyleScope::StyleScope(mozilla::dom::ShadowRoot& aShadowRoot)

Error: Bad implicit conversion constructor for 'stylescope' [clang-tidy: mozilla-implicit-constructor]

::: dom/base/StyleScope.cpp:13
(Diff revision 1)
>  #include "mozilla/dom/StyleSheetList.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> +StyleScope::StyleScope(mozilla::dom::ShadowRoot& aShadowRoot)

Error: Bad implicit conversion constructor for 'stylescope' [clang-tidy: mozilla-implicit-constructor]

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8938135 [details]
Bug 1426494: Devirtualize StyleScope::AsNode.

https://reviewboard.mozilla.org/r/208852/#review214594

::: dom/base/StyleScope.h:30
(Diff revision 1)
>   * TODO(emilio, bug 1418159): In the future this should hold most of the
>   * relevant style state, this should allow us to fix bug 548397.
>   */
>  class StyleScope
>  {
> +  enum class Kind {

Nit, { goes to its own line.

::: dom/base/StyleScope.cpp:13
(Diff revision 1)
>  #include "mozilla/dom/StyleSheetList.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> +StyleScope::StyleScope(mozilla::dom::ShadowRoot& aShadowRoot)

Fix the implicit conversion errors.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8938136 [details]
Bug 1426494: Share more code among Document / ShadowRoot.

https://reviewboard.mozilla.org/r/208854/#review214598

::: dom/base/nsIDocument.h:2837
(Diff revision 1)
> -  GetElementsByTagName(const nsAString& aTagName)
> -  {
> -    return NS_GetContentList(this, kNameSpaceID_Unknown, aTagName);
> -  }
> -  already_AddRefed<nsContentList>
>      GetElementsByTagNameNS(const nsAString& aNamespaceURI,

Keeping this ErrorResult version of GetElementsByTagNameNS is a bit confusing. Could we not have it?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1426494: s/StyleScope/DocumentOrShadowRoot.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938135 [details]
Bug 1426494: Devirtualize StyleScope::AsNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208852/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938136 [details]
Bug 1426494: Share more code among Document / ShadowRoot.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208854/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938136 [details]
Bug 1426494: Share more code among Document / ShadowRoot.

https://reviewboard.mozilla.org/r/208854/#review214598

> Keeping this ErrorResult version of GetElementsByTagNameNS is a bit confusing. Could we not have it?

Moved to StyleScope before the rename, and implemented the non-ErrorResult one in terms of it.

Comment 10

[Code Review Bot [:reviewbot]]

Comment on attachment 8938135 [details]
Bug 1426494: Devirtualize StyleScope::AsNode.

https://reviewboard.mozilla.org/r/208852/#review214620


C/C++ static analysis found 2 defects in this patch.

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

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


::: dom/base/StyleScope.cpp:13
(Diff revision 2)
>  #include "mozilla/dom/StyleSheetList.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> +StyleScope::StyleScope(mozilla::dom::ShadowRoot& aShadowRoot)

Error: Bad implicit conversion constructor for 'stylescope' [clang-tidy: mozilla-implicit-constructor]

::: dom/base/StyleScope.cpp:13
(Diff revision 2)
>  #include "mozilla/dom/StyleSheetList.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> +StyleScope::StyleScope(mozilla::dom::ShadowRoot& aShadowRoot)

Error: Bad implicit conversion constructor for 'stylescope' [clang-tidy: mozilla-implicit-constructor]

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8938170 [details]
Bug 1426494: s/StyleScope/DocumentOrShadowRoot.

https://reviewboard.mozilla.org/r/208884/#review214616

This should use hg mv, and not copy and delete, but I guess since StyleScope is so new it doesn't really matter.

::: dom/base/DocumentOrShadowRoot.h:41
(Diff revision 1)
> +    ShadowRoot,
> +  };
> +
> +public:
> +  explicit DocumentOrShadowRoot(nsIDocument&);
> +  explicit DocumentOrShadowRoot(mozilla::dom::ShadowRoot&);

Why these ctors don't have argument names when all the other methods do have. For consistency aDoc and aShadowRoot would be nice.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938135 [details]
Bug 1426494: Devirtualize StyleScope::AsNode.

https://reviewboard.mozilla.org/r/208852/#review214620

> Error: Bad implicit conversion constructor for 'stylescope' [clang-tidy: mozilla-implicit-constructor]

Both constructors are marked explicit, I really have no idea how I'm supposed to address this :)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8938136 [details]
Bug 1426494: Share more code among Document / ShadowRoot.

https://reviewboard.mozilla.org/r/208854/#review214628

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938170 [details]
Bug 1426494: s/StyleScope/DocumentOrShadowRoot.

https://reviewboard.mozilla.org/r/208884/#review214616

> Why these ctors don't have argument names when all the other methods do have. For consistency aDoc and aShadowRoot would be nice.

Because the other methods do use them, while this is only a declaration. In general if the argument name doesn't say anything useful about it, I don't tend to include it.

I don't see anything in the coding style page in MDN, though it may be worth to decide about it, we do use this idiom in a couple places, though putting the argument name is more frequent it seems...

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938135 [details]
Bug 1426494: Devirtualize StyleScope::AsNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208852/diff/2-3/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938136 [details]
Bug 1426494: Share more code among Document / ShadowRoot.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208854/diff/2-3/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938170 [details]
Bug 1426494: s/StyleScope/DocumentOrShadowRoot.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208884/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07c4aa18a0b6
Devirtualize StyleScope::AsNode. r=smaug
https://hg.mozilla.org/integration/autoland/rev/74a8ebb0f5d3
Share more code among Document / ShadowRoot. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8d07cb1ef232
s/StyleScope/DocumentOrShadowRoot. r=smaug

Comment 19

[Code Review Bot [:reviewbot]]

Comment on attachment 8938135 [details]
Bug 1426494: Devirtualize StyleScope::AsNode.

https://reviewboard.mozilla.org/r/208852/#review214640


C/C++ static analysis found 2 defects in this patch.

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

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


::: dom/base/StyleScope.cpp:13
(Diff revision 3)
>  #include "mozilla/dom/StyleSheetList.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> +StyleScope::StyleScope(mozilla::dom::ShadowRoot& aShadowRoot)

Error: Bad implicit conversion constructor for 'stylescope' [clang-tidy: mozilla-implicit-constructor]

::: dom/base/StyleScope.cpp:13
(Diff revision 3)
>  #include "mozilla/dom/StyleSheetList.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> +StyleScope::StyleScope(mozilla::dom::ShadowRoot& aShadowRoot)

Error: Bad implicit conversion constructor for 'stylescope' [clang-tidy: mozilla-implicit-constructor]

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6463d58d766
followup: Add a missing forward declaration on a CLOSED TREE. r=me

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1d69957a5b3
followup: Add another missing include on a CLOSED TREE. r=me

Comment 22

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8fc361b0f24
Backed out 3 changesets for build bustages on dom/base/FuzzingFunctions.h:25:44 r=backout on a CLOSED TREE

Comment 23

[Cosmin Sabou [:CosminS]]

Backed out for build bustages on dom/base/FuzzingFunctions.h:25:44

Treeherder link: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8d07cb1ef2323f3033e10f4eeb0c4e6a69b8a9ae&filter-resultStatus=busted&selectedJob=152754636

Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8d07cb1ef2323f3033e10f4eeb0c4e6a69b8a9ae&filter-resultStatus=busted&selectedJob=152754636

Backout link: https://hg.mozilla.org/integration/autoland/rev/a8fc361b0f24025fa0db2fb838d074317b6b55c2

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f28babf65d72
Devirtualize StyleScope::AsNode. r=smaug
https://hg.mozilla.org/integration/autoland/rev/abba86fa9a5b
Share more code among Document / ShadowRoot. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d00cfb672a3f
s/StyleScope/DocumentOrShadowRoot. r=smaug

Comment 25

[Emilio Cobos Álvarez (:emilio)]

windows.h #defines GetLocaleInfo, fun stuff to figure out...

Thanks rillian / dmajor for helping me track this down in #build, it would've been so fun to debug given I don't use windows :)

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/f28babf65d72
https://hg.mozilla.org/mozilla-central/rev/abba86fa9a5b
https://hg.mozilla.org/mozilla-central/rev/d00cfb672a3f