Emilio Cobos Álvarez (:emilio) Assignee
	Description • 7 years ago

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

	Code Review Bot [:reviewbot]
	Comment 3 • 7 years ago
mozreview-review

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]

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 7 years ago
mozreview-review

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 7 years ago
mozreview-review

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?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

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.

	Code Review Bot [:reviewbot]
	Comment 10 • 7 years ago
mozreview-review

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]

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 7 years ago
mozreview-review

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

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 :)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 7 years ago
mozreview-review

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

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 14 • 7 years ago
mozreview-review-reply

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...

	Pulsebot
	Comment 18 • 7 years ago

Pushed by ecoal95@gmail.com:
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

	Code Review Bot [:reviewbot]
	Comment 19 • 7 years ago
mozreview-review

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]

	Pulsebot
	Comment 20 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c6463d58d766
followup: Add a missing forward declaration on a CLOSED TREE. r=me

	Pulsebot
	Comment 21 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d1d69957a5b3
followup: Add another missing include on a CLOSED TREE. r=me

	Pulsebot
	Comment 22 • 7 years ago

Backout by csabou@mozilla.com:
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

	Cosmin Sabou [:CosminS]
	Comment 23 • 7 years ago

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

	Pulsebot
	Comment 24 • 7 years ago

Pushed by ecoal95@gmail.com:
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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 25 • 7 years ago

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 :)

	Andreea Pavel [:apavel]
	Comment 26 • 7 years ago
bugherder

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