Bug 126069 - Start refactoring Filter code to reuse CachedSVGDocument for clipPath
Summary: Start refactoring Filter code to reuse CachedSVGDocument for clipPath
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-12-20 07:58 PST by Dirk Schulze
Modified: 2023-08-31 15:24 PDT (History)
15 users (show)

See Also:


Attachments
Patch (3.82 KB, patch)
2013-12-20 08:02 PST, Dirk Schulze
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (3.76 KB, patch)
2013-12-20 10:51 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-02 for mac-mountainlion (474.96 KB, application/zip)
2013-12-20 12:18 PST, WebKit Commit Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (594.15 KB, application/zip)
2013-12-20 21:55 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2013-12-20 07:58:29 PST
The current code in StyleResolver needs some refactoring so that clipPath can use SVG document caching as well.
Comment 1 Dirk Schulze 2013-12-20 08:02:38 PST
Created attachment 219759 [details]
Patch
Comment 2 Andreas Kling 2013-12-20 08:24:46 PST
Comment on attachment 219759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219759&action=review

> Source/WebCore/ChangeLog:12
> +        Smaller refactoring of the CSS filter style resolver code. Previously the code
> +        requested the FilterOperations list from RenderStyle and compared the content
> +        in this list with an internal map. Then the resource loading was triggered.
> +        With the refactoring we do not request the list from RenderStyle anymore but
> +        rely on the hash map data entirely.

Why is this not necessary anymore?

> Source/WebCore/css/StyleResolver.cpp:3418
> +    PendingSVGDocumentMap::iterator end = state.pendingSVGDocuments().end();
> +    for (PendingSVGDocumentMap::iterator it = state.pendingSVGDocuments().begin(); it != end; ++it) {

I'd shorten this down a bit with auto:

for (auto it = state.pendingSVGDocuments().begin(), end = state.pendingSVGDocuments().end(); it != end; ++it) {
Comment 3 Dirk Schulze 2013-12-20 10:40:12 PST
(In reply to comment #2)
> (From update of attachment 219759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219759&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Smaller refactoring of the CSS filter style resolver code. Previously the code
> > +        requested the FilterOperations list from RenderStyle and compared the content
> > +        in this list with an internal map. Then the resource loading was triggered.
> > +        With the refactoring we do not request the list from RenderStyle anymore but
> > +        rely on the hash map data entirely.
> 
> Why is this not necessary anymore?

The HashMap has a pointer to the FilterOperation that needs the cached resource. The function currently has access to this pointer but instead uses RenderStyle to get the operator again. Even worst, we now need to iterate through the whole list of FilterOperations instead just taking the FilterOperations we know that they need resources.


> 
> > Source/WebCore/css/StyleResolver.cpp:3418
> > +    PendingSVGDocumentMap::iterator end = state.pendingSVGDocuments().end();
> > +    for (PendingSVGDocumentMap::iterator it = state.pendingSVGDocuments().begin(); it != end; ++it) {
> 
> I'd shorten this down a bit with auto:
> 
> for (auto it = state.pendingSVGDocuments().begin(), end = state.pendingSVGDocuments().end(); it != end; ++it) {


Will do.
Comment 4 Dirk Schulze 2013-12-20 10:51:41 PST
Created attachment 219769 [details]
Patch
Comment 5 Andreas Kling 2013-12-20 11:42:16 PST
Comment on attachment 219769 [details]
Patch

r=me
Comment 6 WebKit Commit Bot 2013-12-20 12:18:28 PST
Comment on attachment 219769 [details]
Patch

Rejecting attachment 219769 [details] from commit-queue.

New failing tests:
editing/pasteboard/copy-image-with-alt-text.html
Full output: http://webkit-queues.appspot.com/results/50948252
Comment 7 WebKit Commit Bot 2013-12-20 12:18:31 PST
Created attachment 219780 [details]
Archive of layout-test-results from webkit-cq-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2013-12-20 21:55:36 PST
Comment on attachment 219759 [details]
Patch

Attachment 219759 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/44438132

New failing tests:
editing/inserting/insert-br-009.html
Comment 9 Build Bot 2013-12-20 21:55:38 PST
Created attachment 219837 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 10 Dirk Schulze 2013-12-21 22:31:14 PST
Comment on attachment 219769 [details]
Patch

Looking at the tests, they seem unrelated to filters. Resubmitting.
Comment 11 WebKit Commit Bot 2013-12-21 22:58:35 PST
Comment on attachment 219769 [details]
Patch

Clearing flags on attachment: 219769

Committed r160973: <http://trac.webkit.org/changeset/160973>
Comment 12 Ahmad Saleem 2023-08-31 15:23:56 PDT
Landed and didn't seem to back-out:

https://github.com/WebKit/WebKit/commit/2283bb6745b292b6c68784385b7284e724cae818
Comment 13 Radar WebKit Bug Importer 2023-08-31 15:24:16 PDT
<rdar://problem/114791414>