1449321 - Remove the IsServo and IsGecko methods from the style system code

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Jonathan Watt [:jwatt]]

Remove the IsServo and IsGecko methods from the style system code.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962898 [details] [diff] [review]
patch

Review of attachment 8962898 [details] [diff] [review]:
-----------------------------------------------------------------

(Stealing per Xidorn's request).

r=me with the change list bit fixed and followups to remove the dead code on file, thanks!

::: dom/base/nsIDocument.h
@@ +1553,2 @@
>          union {
>            nsCSSSelectorList* mGecko;

This can already be removed, as with the other selector cache and such, but I'll take care of that in bug 1449502.

::: editor/libeditor/HTMLEditor.cpp
@@ +3013,5 @@
>    nsCOMPtr<nsIDocument> document = GetDocument();
>    sheet->SetAssociatedDocument(document, StyleSheet::NotOwnedByDocument);
>  
> +  // XXXheycam ServoStyleSheets don't support being enabled/disabled yet.
> +  NS_ERROR("stylo: ServoStyleSheets can't be disabled yet");

Hmm, this is kind of a lie. The old style system was doing sheet->SetDisabled(false), which should work for servo sheets too since long ago.

But looks like all this code (nsIEditorStyleSheets) is unused. Mind filing a bug about this code being unused in editor and reference it from here like:

// FIXME: This used to do sheet->SetDisabled(false), figure out if we can just remove all
// this code in bug XXX, since it seems unused.

If it turns out that this code is useful (somewhat doubtful), we can fix this. Otherwise we can kill nsIEditorStyleSheets.

::: layout/base/PresShell.cpp
@@ +602,5 @@
>  #ifdef DEBUG
>  static void
>  VerifyStyleTree(nsPresContext* aPresContext, nsFrameManager* aFrameManager)
>  {
>    if (nsFrame::GetVerifyStyleTreeEnable()) {

We should just remove all these. Servo has a version of this function in ServoRestyleManager. Probably followup bug?

::: layout/base/RestyleManager.cpp
@@ +630,5 @@
>  RestyleManager::DebugVerifyStyleTree(nsIFrame* aFrame)
>  {
> +  // XXXheycam For now, we know that we don't use the same inheritance
> +  // hierarchy for certain cases, so just skip these assertions until
> +  // we work out what we want to assert (bug 1322570).

Well, we cannot assert what this code used to assert since I removed the useless parent pointers in bug 1384542... Oh well.


The ServoRestyleManager assertions on the DOM (VerifyFlatTree) should be equivalent though.

@@ +1745,5 @@
>  {
>    // We update the animation generation at start of each call to
>    // ProcessPendingRestyles so we should ignore any subsequent (redundant)
>    // calls that occur while we are still processing restyles.
> +  if (mInStyleRefresh) {

Just:

if (!mStyleRefresh) {
   ++mAnimationGeneration;
}

?

::: layout/base/nsCSSFrameConstructor.cpp
@@ +472,5 @@
>    // We reparent frames for two reasons: to put them inside ::first-line, and to
>    // put them inside some wrapper anonymous boxes.
>    //
>    // The latter shouldn't affect any styles in practice, so only needs style
>    // context reparenting in the Gecko backend.

This part of the comment should go.

@@ +474,5 @@
>    //
>    // The latter shouldn't affect any styles in practice, so only needs style
>    // context reparenting in the Gecko backend.
>    //
>    // FIXME(emilio): Remove old Gecko stuff.

And this one too.

::: layout/base/nsStyleChangeList.cpp
@@ +62,3 @@
>  #endif
> +  } else {
> +    // Filter out all other changes for same content for Gecko (Servo asserts against this

This change is not idempotent. This used to be:

if (Reconstruct) {
  if (IsServo()) {
    // ...
  } else {
    RemoveElementsBy(..);
  }
}

Now you turned it into:

if (Reconstruct) {
  // ...
} else {
  RemoveElementsBy(..)
}

Which is different and also incorrect (I'd be surprised f tests pass).

Anyway, TL;DR: this whole else branch should go away.

::: layout/base/nsStyleChangeList.h
@@ +40,5 @@
>    using base_type::Clear;
>    using base_type::Length;
>    using base_type::operator[];
>  
> +  explicit nsStyleChangeList() {

no need for explicit now.

::: layout/style/GenericSpecifiedValuesInlines.h
@@ +28,5 @@
>  {
> +  // Servo handles this during cascading.
> +  //
> +  // FIXME(emilio): We should eventually move it to the document though.
> +  return false;

Can you file a bug to remove this function and reference it from the comment? Feel free to assign to me, can take care of it.

::: layout/style/Loader.cpp
@@ +2091,5 @@
>  {
>    LOG(("css::Loader::LoadChildSheet"));
>    NS_PRECONDITION(aURL, "Must have a URI to load");
>    NS_PRECONDITION(aParentSheet, "Must have a parent sheet");
> +  MOZ_ASSERT(!aGeckoParentRule, "TODO remove this param");

File bug, reference from here?

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> ::: layout/style/GenericSpecifiedValuesInlines.h
> @@ +28,5 @@
> >  {
> > +  // Servo handles this during cascading.
> > +  //
> > +  // FIXME(emilio): We should eventually move it to the document though.
> > +  return false;
> 
> Can you file a bug to remove this function and reference it from the
> comment? Feel free to assign to me, can take care of it.

Bug 1449566.

> ::: layout/style/Loader.cpp
> @@ +2091,5 @@
> >  {
> >    LOG(("css::Loader::LoadChildSheet"));
> >    NS_PRECONDITION(aURL, "Must have a URI to load");
> >    NS_PRECONDITION(aParentSheet, "Must have a parent sheet");
> > +  MOZ_ASSERT(!aGeckoParentRule, "TODO remove this param");
> 
> File bug, reference from here?

Bug 1449565.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/763277b299e4
Remove the IsServo and IsGecko methods from the style system code. r=emilio

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eee0f48055b
Backed out 3 changesets (bug 1449321, bug 1449566, bug 1449565) as per developer request.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26afc0176590
Remove the IsServo and IsGecko methods from the style system code. r=emilio

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/26afc0176590