Closed Bug 1404243 Opened 7 years ago Closed 7 years ago

Refactor for StyleShapeSource to support shape-outside: <image>

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

Bug 1404243 Part 1 - Move StyleShapeSource's large methods to nsStyleStruct.cpp.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1404243 Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1404243 Part 3 - Remove refcount for StyleBasicShape, and use UniquePtr to hold it.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1404243 Part 4 - Change StyleShapeSource's URL value storage by using nsStyleImage.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Servo PR #18700
41 bytes, text/x-github-pull-request
Details | Review 
To support shape-outside: <image> in bug 1404222. I'll need to do some refactor to StyleShapeSource. The patch series might be large enough to land.
Comment on attachment 8913928 [details]
Bug 1404243 Part 1 - Move StyleShapeSource's large methods to nsStyleStruct.cpp.

https://reviewboard.mozilla.org/r/184972/#review190352
Attachment #8913928 - Flags: review?(cam) → review+
Comment on attachment 8913929 [details]
Bug 1404243 Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals().

https://reviewboard.mozilla.org/r/184974/#review190354

r=me with that.

::: layout/style/nsStyleStruct.cpp:1078
(Diff revision 1)
>    }
>    return *this;
>  }
>  
>  bool
> +StyleShapeSource::operator==(const StyleShapeSource& aOther) const

I think I would prefer to keep this method named DefinitelyEquals.  When I see operator== I think it is really checking for exact equality, but since this isn't, I feel like we should use a different name.
Attachment #8913929 - Flags: review?(cam) → review+
Comment on attachment 8913930 [details]
Bug 1404243 Part 3 - Remove refcount for StyleBasicShape, and use UniquePtr to hold it.

https://reviewboard.mozilla.org/r/184976/#review190356
Attachment #8913930 - Flags: review?(cam) → review+
Comment on attachment 8913931 [details]
style: Remove refcount for StyleBasicShape

https://reviewboard.mozilla.org/r/184978/#review190358
Attachment #8913931 - Flags: review?(cam) → review+
Comment on attachment 8913932 [details]
Bug 1404243 Part 4 - Change StyleShapeSource's URL value storage by using nsStyleImage.

https://reviewboard.mozilla.org/r/184980/#review190360

::: layout/style/nsStyleStruct.cpp:1045
(Diff revision 1)
>  
>  // --------------------
>  // StyleShapeSource
>  
>  StyleShapeSource::StyleShapeSource(const StyleShapeSource& aSource)
>    : StyleShapeSource()

We don't need this delegated constructor call any more.
Attachment #8913932 - Flags: review?(cam) → review+
Comment on attachment 8913933 [details]
style: Change URL value storage in StyleShapeSource.

https://reviewboard.mozilla.org/r/184982/#review190362
Attachment #8913933 - Flags: review?(cam) → review+
Comment on attachment 8913929 [details]
Bug 1404243 Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals().

https://reviewboard.mozilla.org/r/184974/#review190354

> I think I would prefer to keep this method named DefinitelyEquals.  When I see operator== I think it is really checking for exact equality, but since this isn't, I feel like we should use a different name.

I agree with you. However, after supporting `shape-outside: <image>`, I'll remove the `DefinitelyEqualURIs` and deligate the comparison to  `nsStyleImage::operator==`. By then, `StyleShapeSource::operator==` won't have any implementation detail related to `DefinitelyEquals`.
Attachment #8913931 - Attachment is obsolete: true
Attachment #8913933 - Attachment is obsolete: true
Attached file Servo PR #18700 — 

    
    

    
  
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d8e44508ed0
Part 1 - Move StyleShapeSource's large methods to nsStyleStruct.cpp. r=heycam
https://hg.mozilla.org/integration/autoland/rev/84fba2528a6c
Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/64f6005d9a5c
Part 3 - Remove refcount for StyleBasicShape, and use UniquePtr to hold it. r=heycam
https://hg.mozilla.org/integration/autoland/rev/1a7862326a70
Part 4 - Change StyleShapeSource's URL value storage by using nsStyleImage. r=heycam

    
    

    
  
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02c27216914e
Some more heap write analysis tweaks. r=me

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: