WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
45056
Clean up issues from
r66428
.
https://bugs.webkit.org/show_bug.cgi?id=45056
Summary
Clean up issues from r66428.
Lei Zhang
Reported
2010-09-01 13:35:24 PDT
Created
attachment 66255
[details]
first attempt to address concerns from
bug 43658
. This patch attempts to address issues raised in
https://bugs.webkit.org/show_bug.cgi?id=43658#c16
(In reply to
bug 43658 comment #16
)
> I see multiple problems with this patch: > > It’s not good how this patch spreads the feature over two classes. The code in both FrameLoader and DOMWindow is inelegant. All the code should be in one class, with the other class calling over as needed. That way you would not have to expose a printDeferred function.
I moved more of the logic out of DOMWindow and into MainResourceLoader.
> The name printDeferred is not a good boolean function or data member name. It sounds like a verb "print deferred". I would name it "should print when finished loading": > > m_shouldPrintWhenFinishedLoading
Done.
> Also, I think FrameLoader::finishedLoading may not be the right place to call this. That's the function called when the main resources is loaded, not when the entire page is loaded.
I looked into this - in MainResourceLoader::didFinishLoading(), we have: frameLoader()->finishedLoading(); ResourceLoader::didFinishLoading(); If I try to handle the deferred print after ResourceLoader::didFinishLoading(), it's too late because the data structures we need have been destroyed. Doing it before frameLoader()->finishedLoading() is too early since FrameLoader::isLoading() still returns true. So either we should do it between frameLoader()->finishedLoading() and ResourceLoader::didFinishLoading(), or I need to do it inside frameLoader()->finishedLoading(). I am currently handling it between these two calls, but if you feel it belongs inside frameLoader()->finishedLoading(), I'll look in there to see what's the right place.
Attachments
first attempt to address concerns from bug 43658.
(5.96 KB, patch)
2010-09-01 13:35 PDT
,
Lei Zhang
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug