Closed Bug 1322330 Opened 8 years ago Closed 8 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: