Closed
Bug 1452080
Opened 7 years ago
Closed 7 years ago
Remove ComputedStyle::PresContext.
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-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 6•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•7 years ago
|
||
(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
Comment 9•7 years ago
|
||
Backed out 3 changesets (bug 1452080) for bustage in /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp on a CLOSED TREE
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7f5104c7a2425167959f725550c14719cb0c6274&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running
Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=172626081&repo=autoland&lineNumber=16844
Backout:
https://hg.mozilla.org/integration/autoland/rev/28a81bb4a0bd1fc67709038c799da980326752f8
Flags: needinfo?(emilio)
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8966125 -
Flags: review?(xidorn+moz)
Assignee | ||
Updated•7 years ago
|
Attachment #8966125 -
Attachment is patch: true
Updated•7 years ago
|
Attachment #8966125 -
Flags: review?(xidorn+moz) → review+
Comment 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/231e55e4683e
https://hg.mozilla.org/mozilla-central/rev/9e1a485cfb22
https://hg.mozilla.org/mozilla-central/rev/433416ba2a3a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•