Closed Bug 1452080 Opened 6 years ago Closed 6 years ago

Remove ComputedStyle::PresContext.

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
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

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1452080: Remove ComputedStyle::PresContext usage in animation code.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1452080: Remove ComputedStyle::PresContext usage from layout and canvas code.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1452080: Rename ComputedStyle::PresContext to PresContextForFrame.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Interdiff to fix OSX-only menubar stuff.
1.11 KB, patch
xidorn
: review+
Details | Diff | Splinter Review 
In a world where ComputedStyle is able to be generated without a pres-context, there's no way to guarantee one.

After this patch the only remaining consumers of it are ComputedStyle itself (with FinishStyle), and nsIFrame::PresContext().
Comment on attachment 8965664 [details]
Bug 1452080: Remove ComputedStyle::PresContext usage in animation code.

https://reviewboard.mozilla.org/r/234516/#review240352

Thanks!
r=me with adding argument names in declarations.

::: dom/animation/KeyframeEffectReadOnly.h:168
(Diff revision 1)
>    void NotifyAnimationTimingUpdated();
>    void RequestRestyle(EffectCompositor::RestyleType aRestyleType);
>    void SetAnimation(Animation* aAnimation) override;
>    void SetKeyframes(JSContext* aContext, JS::Handle<JSObject*> aKeyframes,
>                      ErrorResult& aRv);
> -  void SetKeyframes(nsTArray<Keyframe>&& aKeyframes,
> +  void SetKeyframes(nsTArray<Keyframe>&& aKeyframes, const ComputedStyle*);

I don't have any strong opinions but we do usually define all argument names in declarations?  As far as I know we have been doing in dom/animation.
Attachment #8965664 - Flags: review?(hikezoe) → review+
Comment on attachment 8965665 [details]
Bug 1452080: Remove ComputedStyle::PresContext usage from layout and canvas code.

https://reviewboard.mozilla.org/r/234518/#review240438

::: dom/svg/SVGContentUtils.cpp:306
(Diff revision 1)
>  
>  float
> -SVGContentUtils::GetFontSize(ComputedStyle *aComputedStyle)
> +SVGContentUtils::GetFontSize(ComputedStyle* aComputedStyle,
> +                             nsPresContext* aPresContext)
>  {
> -  MOZ_ASSERT(aComputedStyle, "NULL ComputedStyle in GetFontSize");
> +  MOZ_ASSERT(aComputedStyle && aPresContext);

Maybe keeping the two assertions separate helps analysing in some cases, but it's probably not a big deal.

::: layout/generic/nsTextFrame.cpp:2308
(Diff revision 1)
>    }
>    // ContinueTextRunAcrossFrames guarantees that it doesn't matter which
>    // frame's style is used, so we use a mixture of the first frame and
>    // last frame's style
>    flags |= nsLayoutUtils::GetTextRunFlagsForStyle(lastComputedStyle,
> -      fontStyle, textStyle, LetterSpacing(firstFrame, textStyle));
> +      presContext, fontStyle, textStyle, LetterSpacing(firstFrame, textStyle));

Maybe just `firstFrame->PresContext()` so that you don't need to try catching a pres context in the loop.

::: layout/svg/nsSVGOuterSVGFrame.cpp:73
(Diff revision 1)
>    , mCallingReflowSVG(false)
> -  , mFullZoom(aStyle->PresContext()->GetFullZoom())
>    , mViewportInitialized(false)
>    , mIsRootContent(false)
>  {
> +  mFullZoom = PresContext()->GetFullZoom();

It is not clear why this needs to be in the body of constructor rather than in the initializer list. Should keeping it in the list still work?
Attachment #8965665 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8965666 [details]
Bug 1452080: Rename ComputedStyle::PresContext to PresContextForFrame.

https://reviewboard.mozilla.org/r/234520/#review240440

This patch itself looks fine, but it's not clear to me how would you remove mPresContext from ComputedStyle given those references?
Attachment #8965666 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> Comment on attachment 8965666 [details]
> Bug 1452080: Rename ComputedStyle::PresContext to PresContextForFrame.
> 
> https://reviewboard.mozilla.org/r/234520/#review240440
> 
> This patch itself looks fine, but it's not clear to me how would you remove
> mPresContext from ComputedStyle given those references?

We can't right now. Need to fix bug 1440305 at least, and then figure out what to do about counter styles.

But afterwards we'd "just" need to move the reference to the frame instead of the ComputedStyle instance, which seems doable. I just don't want to add new dependencies on this.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bffebe8aa254
Remove ComputedStyle::PresContext usage in animation code. r=hiro
https://hg.mozilla.org/integration/autoland/rev/806a9c95a243
Remove ComputedStyle::PresContext usage from layout and canvas code. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/7f5104c7a242
Rename ComputedStyle::PresContext to PresContextForFrame. r=xidorn
Arg, osx-only code :(
Flags: needinfo?(emilio)
Attachment #8966125 - Attachment is patch: true
Attachment #8966125 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/231e55e4683e
Remove ComputedStyle::PresContext usage in animation code. r=hiro
https://hg.mozilla.org/integration/autoland/rev/9e1a485cfb22
Remove ComputedStyle::PresContext usage from layout and canvas code. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/433416ba2a3a
Rename ComputedStyle::PresContext to PresContextForFrame. r=xidorn
Regressions: 1575926
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: