Closed Bug 1322330 Opened 9 years ago Closed 9 years ago

Remove EffectProperties::GetFirstMaskFrame

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: u459114, Assigned: u459114)

Details

Attachments

(3 files)

Bug 1322330 - Part 1. Remove EffectProperties::GetFirstMaskFrame and implement EffectProperties::HasInvalidMaskOrClipPathResource.
58 bytes, text/x-review-board-request
longsonr
: review+
Details
Bug 1322330 - Part 2. Implement EffectProperties::HasInvalidClipPath.
58 bytes, text/x-review-board-request
longsonr
: review+
Details
Bug 1322330 - Part 3. Implement EffectProperties::HasInvalidMask.
58 bytes, text/x-review-board-request
longsonr
: review+
Details
According to Bug 1228280 comment 33, Remove EffectProperties::GetFirstFrame().
Comment on attachment 8817128 [details] Bug 1322330 - Part 1. Remove EffectProperties::GetFirstMaskFrame and implement EffectProperties::HasInvalidMaskOrClipPathResource. https://reviewboard.mozilla.org/r/97574/#review97848 ::: layout/painting/nsDisplayList.cpp:7466 (Diff revision 1) > aTo += "clip(basic-shape)"; > first = false; > } > > - if (effectProperties.GetFirstMaskFrame()) { > + nsTArray<nsSVGMaskFrame*> masks = effectProperties.GetMaskFrames(); > + if (masks.Length() > 0 && masks[0]) { if (!masks.IsEmpty()) { ::: layout/svg/nsSVGUtils.cpp:751 (Diff revision 1) > } > > + bool isOK = effectProperties.HasNoFilterOrHasValidFilter(); > + nsSVGClipPathFrame *clipPathFrame = effectProperties.GetClipPathFrame(&isOK); > + nsTArray<nsSVGMaskFrame*> masks = effectProperties.GetMaskFrames(); > + nsSVGMaskFrame *maskFrame = masks.Length() > 0 ? masks[0] : nullptr; nsSVGMaskFrame *maskFrame = masks.IsEmpty() ? nullptr ? masks[0];
Attachment #8817134 - Flags: review?(longsonr)
Attachment #8817150 - Flags: review?(longsonr)
Comment on attachment 8817150 [details] Bug 1322330 - Part 3. Implement EffectProperties::HasInvalidMask. https://reviewboard.mozilla.org/r/97586/#review97878
Comment on attachment 8817128 [details] Bug 1322330 - Part 1. Remove EffectProperties::GetFirstMaskFrame and implement EffectProperties::HasInvalidMaskOrClipPathResource. https://reviewboard.mozilla.org/r/97574/#review97872 ::: layout/painting/nsDisplayList.cpp:7467 (Diff revision 5) > first = false; > } > > - if (effectProperties.GetFirstMaskFrame()) { > + nsTArray<nsSVGMaskFrame*> masks = effectProperties.GetMaskFrames(); > + if (!masks.IsEmpty() && masks[0]) { > if (!first) { Is the test for masks[0] required? ::: layout/svg/nsSVGEffects.cpp:672 (Diff revision 5) > return result; > } > > +bool > +nsSVGEffects::EffectProperties::HasInvalidEffects() > +{ Would be easier to reverse the logic and create a function called HasValidEffects I expect. ::: layout/svg/nsSVGEffects.cpp:676 (Diff revision 5) > +nsSVGEffects::EffectProperties::HasInvalidEffects() > +{ > + if (mClipPath) { > + bool ok = true; > + nsSVGClipPathFrame *frame = static_cast<nsSVGClipPathFrame *> > + (mClipPath->GetReferencedFrame(nsGkAtoms::svgClipPathFrame, &ok)); * should hug type ::: layout/svg/nsSVGEffects.cpp:694 (Diff revision 5) > + } > + } > + } > + > + if (!HasNoFilterOrHasValidFilter()) { > + return true; return !HasNoFilterOrHasValidFilter();
Attachment #8817128 - Flags: review?(longsonr)
Comment on attachment 8817128 [details] Bug 1322330 - Part 1. Remove EffectProperties::GetFirstMaskFrame and implement EffectProperties::HasInvalidMaskOrClipPathResource. https://reviewboard.mozilla.org/r/97574/#review97872 > Is the test for masks[0] required? yes, it's required. After we introduce image mask, we may append empty items into mMask
Component: Layout → SVG
Comment on attachment 8817128 [details] Bug 1322330 - Part 1. Remove EffectProperties::GetFirstMaskFrame and implement EffectProperties::HasInvalidMaskOrClipPathResource. https://reviewboard.mozilla.org/r/97574/#review98280
Attachment #8817128 - Flags: review?(longsonr) → review+
Comment on attachment 8817134 [details] Bug 1322330 - Part 2. Implement EffectProperties::HasInvalidClipPath. https://reviewboard.mozilla.org/r/97576/#review97876 ::: layout/svg/nsSVGEffects.h:479 (Diff revision 6) > * @return the clip-path frame, or null if there is no clip-path frame > - * @param aOK if a clip-path was specified and the designated element > - * exists but is an element of the wrong type, *aOK is set to false. > - * Otherwise *aOK is untouched. > */ > - nsSVGClipPathFrame *GetClipPathFrame(bool* aOK); > + nsSVGClipPathFrame *GetClipPathFrame(); * should hug type
Attachment #8817134 - Flags: review?(longsonr) → review+
Comment on attachment 8817150 [details] Bug 1322330 - Part 3. Implement EffectProperties::HasInvalidMask. https://reviewboard.mozilla.org/r/97586/#review98282 ::: layout/svg/nsSVGEffects.cpp:674 (Diff revision 4) > bool > nsSVGEffects::EffectProperties::HasNoOrValidEffects() > { > - if (HasInvalidClipPath()) { > - return false; > + return HasNoOrValidClipPath() && HasNoOrValidMask() && > + HasNoFilterOrHasValidFilter(); > - } > +} maybe create a follow up bug to rename HasNoFilterOrHasValidFilter to be consistent with the other two i.e. to become HasNoOrHasValidFilter
Attachment #8817150 - Flags: review?(longsonr) → review+
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/377972df07e0 Part 1. Remove EffectProperties::GetFirstMaskFrame and implement EffectProperties::HasInvalidMaskOrClipPathResource. r=longsonr+218550 https://hg.mozilla.org/integration/autoland/rev/32b11d935bba Part 2. Implement EffectProperties::HasInvalidClipPath. r=longsonr+218550 https://hg.mozilla.org/integration/autoland/rev/848bba1099dc Part 3. Implement EffectProperties::HasInvalidMask. r=longsonr+218550
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: