Closed Bug 1447476 Opened 6 years ago Closed 6 years ago

Cleanup the frame constructor a bit.

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1447476: Remove TreeMatchContext.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1447476: Simplify ResolveStyleContext.
59 bytes, text/x-review-board-request
xidorn
: review+
bzbarsky
: review+
Details
Bug 1447476: Inline ResolveServoStyle.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Now that non-stylo is gone, we can do some due cleanup here.
Comment on attachment 8960828 [details]
Do LazyFC for anonymous nodes too, and cleanup MaybeConstructLazily.

Actually I change my mind, since you can insert a range that contains both XUL elements and text nodes, and then the assertion could be hit. It doesn't happen on our automation, but I think it's not worth doing in this bug. I'll move this patch to bug 1447506.
Attachment #8960828 - Flags: review?(xidorn+moz)
Attachment #8960828 - Attachment is obsolete: true
I suppose this depends on the code removal in bug 1447358.
Depends on: 1447358
Comment on attachment 8960811 [details]
Bug 1447476: Remove TreeMatchContext.

https://reviewboard.mozilla.org/r/229544/#review235306

::: layout/base/nsCSSFrameConstructor.cpp:6545
(Diff revision 2)
>            aContent->IsNodeOfType(nsINode::ePROCESSING_INSTRUCTION)) {
>          // Comments and processing instructions never have frames, so we
>          // should not try to generate style contexts for them.
>          return false;
>        }
> -      // XXXbz when this code is killed, the state argument to
> +      // FIXME(emilio): This is buggy some times, see bug 1424656.

I don't think it is a good idea to mention a non-public security bug in code comment...

::: layout/style/StyleSetHandle.h:121
(Diff revision 2)
> -                    LazyComputeBehavior aMayCompute,
> -                    TreeMatchContext* aTreeMatchContext);
> +    // TODO(emilio): This might be nicer (albeit a bit slower) if we just grab
> +    // the style from the parent in ServoStyleSet.
> +    //
> +    // It may be faster if we account not having to pass it around in
> +    // nsCSSFrameConstructor though.

I don't quite understand this comment... Could you probably file a bug and link it here instead, rather than trying to putting more details in comment?
Attachment #8960811 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8960812 [details]
Bug 1447476: Simplify ResolveStyleContext.

https://reviewboard.mozilla.org/r/229546/#review235312

It looks fine to me, but I'm not super confident on reviewing this, so probably better ask bz to have another look.

::: layout/base/nsCSSFrameConstructor.cpp:6454
(Diff revision 3)
>        nsIFrame* styleParent;
>        aSibling->GetParentStyleContext(&styleParent);
>        if (!styleParent) {
>          styleParent = aSibling->GetParent();
>        }
>        if (!styleParent) {
>          NS_NOTREACHED("Shouldn't happen");
>          return false;
>        }

You should be able to remove this piece of code as well since you removed the only usage of `styleParent`.
Attachment #8960812 - Flags: review?(xidorn+moz) → review+
Attachment #8960812 - Flags: review?(bzbarsky)
Comment on attachment 8960813 [details]
Bug 1447476: Inline ResolveServoStyle.

https://reviewboard.mozilla.org/r/229548/#review235338

TBH I'm no very happy about this... I've tried hard to avoid including ServoBindings.h in non-inlines header files, so that we don't need to rebuild tons of unrelated files when just touching a single binding function...
Attachment #8960813 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8960811 [details]
Bug 1447476: Remove TreeMatchContext.

https://reviewboard.mozilla.org/r/229544/#review235384

::: layout/style/StyleSetHandle.h:121
(Diff revision 2)
> -                    LazyComputeBehavior aMayCompute,
> -                    TreeMatchContext* aTreeMatchContext);
> +    // TODO(emilio): This might be nicer (albeit a bit slower) if we just grab
> +    // the style from the parent in ServoStyleSet.
> +    //
> +    // It may be faster if we account not having to pass it around in
> +    // nsCSSFrameConstructor though.

Yeah, I just meant that ResolveStyleForText doesn't really need the aParentContext pointer, and can just look at the DOM. I guess for restyling is nicer to not look repeteadly.
Comment on attachment 8960812 [details]
Bug 1447476: Simplify ResolveStyleContext.

https://reviewboard.mozilla.org/r/229546/#review235482

r=me.  Nice to see all the red.  ;)

::: commit-message-72c35:3
(Diff revision 3)
> +Bug 1447476: Simplify ResolveStyleContext. r?xidorn
> +
> +We don't need the parent style context, nor the pseudo-element dance or anything

More precisely, all end up at Servo_ResolveStyle, which is what ResolveServoStyle does, right?  Saying it that way makes the pseudo-element changes a lot clearer.

::: layout/base/nsCSSFrameConstructor.cpp:5040
(Diff revision 3)
> -  if (RestyleManager()->IsGecko()) {
> -    MOZ_CRASH("old style system disabled");
> -  }
>  
> -  return result.forget();
> +  // FIXME(emilio): We can't use ResolveServoStyle properly because this text
> +  // node can come from non-lazy frame construction, in which case our style can

"our style" being the style of the parent element?  And the point is we want the out-of-date style here?

Might be worth clarifying this a bit.
Attachment #8960812 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: