Bug 67087 - Prepare SVG image intrinsic size negotiation: Stop sharing SVGImages
Summary: Prepare SVG image intrinsic size negotiation: Stop sharing SVGImages
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 47156
  Show dependency tree / graph
 
Reported: 2011-08-27 01:22 PDT by Nikolas Zimmermann
Modified: 2011-10-05 06:38 PDT (History)
9 users (show)

See Also:

Attachments
Patch (14.07 KB, patch)
2011-08-27 02:33 PDT, Nikolas Zimmermann 		webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (13.98 KB, patch)
2011-08-30 01:58 PDT, Nikolas Zimmermann 		no flags Details | Formatted Diff | Diff
Patch v3 (14.02 KB, patch)
2011-08-30 02:17 PDT, Nikolas Zimmermann 		no flags Details | Formatted Diff | Diff
Patch v4 (14.73 KB, patch)
2011-08-30 02:58 PDT, Nikolas Zimmermann 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2011-08-27 01:22:10 PDT 
Prepare SVG image intrinsic size negotiation: Stop sharing SVGImages.
This is needed as SVG image will soon depend on the place where its used.
Size negotiation has to be implemented for border-image/mask-image/background-image/html:img/svg:image.
This is all done already, I'm splitting up my master patch into smaller pieces, this is part one.
Comment 1 Nikolas Zimmermann 2011-08-27 02:33:30 PDT 
Created attachment 105433 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-27 03:13:41 PDT 
Comment on attachment 105433 [details]
Patch

Attachment 105433 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9547407

New failing tests:
http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html
http/tests/inspector/network/network-cachedresources-with-same-urls.html
Comment 3 Nikolas Zimmermann 2011-08-30 01:58:53 PDT 
Created attachment 105595 [details]
Patch v2

Fix regressions, should now pass cr-linux-ews again.
Comment 4 Nikolas Zimmermann 2011-08-30 02:00:31 PDT 
Comment on attachment 105595 [details]
Patch v2

Oops, uploaded wrong version.
Comment 5 Nikolas Zimmermann 2011-08-30 02:17:05 PDT 
Created attachment 105601 [details]
Patch v3
Comment 6 Nikolas Zimmermann 2011-08-30 02:58:08 PDT 
Created attachment 105604 [details]
Patch v4

Heh, Patch v3 still contained a missing change to RenderImage.cpp, sorry for the patch spam. This time I got merging everything right.
Comment 7 Dirk Schulze 2011-08-30 09:07:47 PDT 
The patch looks sane to me. But I'd suggest that you ask someone with more experiences in CachedResources. The sentence about the "less dangerous way" scares me :P
Comment 8 Alexey Proskuryakov 2011-08-31 17:13:10 PDT 
CC'ing some experts, this is probably for Antti to review.
Comment 9 Antti Koivisto 2011-09-06 03:07:07 PDT 
Why is this needed in the first place? Isn't SVG scaling just a paint/hit test time transform?

Anyway, I don't think this is the right way to go. Repeatedly re-requesting a resource from network so we can render it in different sizes is not good and we shouldn't complicate the cache code further by introducing the strange concept of "unique cached resource".

Eventually we probably want to separate the cache storage layer (which holds the bits) and the interoperation layer (the decoded forms of the resource). That would make things easier here too.

For now, if you need to cache SVG trees in multiple sizes then that's exactly what you should do. HashMap<IntSize, SVGImage> and all that.
Comment 10 Antti Koivisto 2011-09-06 03:09:26 PDT 
s/interoperation/interpretation/
Comment 11 Nikolas Zimmermann 2011-10-05 06:38:35 PDT 
As discussed with Antti, this approach is bad - I've come up with a new concept, that's going to be discussed in a new bug report, see bug 69416.