Bug 31393 - externalRepresentation should take Frame as the argument
Summary: externalRepresentation should take Frame as the argument
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-11 23:57 PST by Shinichiro Hamaji
Modified: 2009-11-12 18:49 PST (History)
1 user (show)

See Also:

Attachments
Patch v1 (6.81 KB, patch)
2009-11-12 00:00 PST, Shinichiro Hamaji 		no flags Details | Formatted Diff | Diff
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 Shinichiro Hamaji 2009-11-11 23:57:15 PST 
As suggested in Bug 30555, WebCore::externalRepresentation should take Frame* instead of RenderObject*.
Comment 1 Shinichiro Hamaji 2009-11-12 00:00:41 PST 
Created attachment 43036 [details]
Patch v1
Comment 2 Darin Adler 2009-11-12 09:22:44 PST 
Comment on attachment 43036 [details]
Patch v1

This is a good change. But you're missing an opportunity to fix a major bug in externalRepresentation.

The layout needs to be moved to the top of the function, before the SVG call to writeRenderResources.

And the RenderObject should be fetched only after the call to layout.

I'll say r=me assuming that you'll do those fixes in a subsequent patch, because those are really the motivation for making this change.
Comment 3 Shinichiro Hamaji 2009-11-12 18:37:27 PST 
Comment on attachment 43036 [details]
Patch v1

Clearing flags on attachment: 43036

Committed r50923: <http://trac.webkit.org/changeset/50923>
Comment 4 Shinichiro Hamaji 2009-11-12 18:37:36 PST 
All reviewed patches have been landed.  Closing bug.
Comment 5 Shinichiro Hamaji 2009-11-12 18:49:22 PST 
Yeah, I'll fix it, but I wanted to avoid changing both WebCore interface and WebCore implementation in one patch. An interface change requires change of multiple platform code, which has the risk of build breakage. So, I want to keep the risk as possible. I'll post another patch after I confirm this change doesn't break anything in build bots. I think I should have described this plan, sorry.

Thanks for the review.