Summary: | [GTK] improve platformSetDefersLoading | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dan Winship <danw> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mrobinson, pnormand, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 71611 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Dan Winship
2011-11-13 10:42:45 PST
Created attachment 114861 [details]
Fix platformDefersLoading
Comment on attachment 114861 [details] Fix platformDefersLoading View in context: https://bugs.webkit.org/attachment.cgi?id=114861&action=review I really like how this patch removes a lot of hacks for using soup_session_(un)pause_message. A couple comments follow. > Source/WebCore/ChangeLog:9 > + No new tests. (OOPS!) You'll need to remove the OOPS here for the cq to land this patch. You can just say something like, "No new tests. This is covered by existing tests." > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:564 > return; It seems like there's nothing now to do in this method if defersLoading == true? If so, you can do an early return right away and avoid all the other checking later. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:567 > + // Requests that haven't yet been sent > + if (!d->m_cancellable) { I think it makes sense to actually remove the comment and add a helper that makes this check explicit. You might do something like static bool requestStarted(...). It makes sense to have a canonical idea of when the request has started. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:578 > + // Do not pause or unpause operations that have not reached sendRequestCallback yet. > + // If m_defersLoading is true at that point, we'll pause. > if (!d->m_inputStream) > return; This check existed only because calling soup_session_pause_message at the wrong time led to CRITICAL warnings. I'm pretty sure you can remove it competely now. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:582 > + GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult; > + d->m_deferredResult = 0; I think you can just do: RefPtr<GAsyncResult> asyncResult = adoptGRef(d->m_deferredResult.leakRef()); Philippe can you verify that "emulating" a message pause still fulfills the needs of the GStreamer backend. (In reply to comment #3) > Philippe can you verify that "emulating" a message pause still fulfills the needs of the GStreamer backend. I tested the patches, haven't found any regression media playback-wise, I did some functional testing and tried the layout tests as well. (In reply to comment #2) > > + No new tests. (OOPS!) > > You'll need to remove the OOPS here for the cq to land this patch. You can just say something like, "No new tests. This is covered by existing tests." Yeah, that was intentional: > This patch also makes defersLoading work for non-http URIs, which > didn't work before... I looked through the skipped tests list, but > I couldn't find any that looked like they were being skipped because > of that. Maybe we need a new one? > It seems like there's nothing now to do in this method if defersLoading == true? > This check existed only because calling soup_session_pause_message at the wrong time led to CRITICAL warnings. I'm pretty sure you can remove it competely now. Ah, yeah. Cruft from earlier versions of the patch. > > + GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult; > > + d->m_deferredResult = 0; > > I think you can just do: RefPtr<GAsyncResult> asyncResult = adoptGRef(d->m_deferredResult.leakRef()); OK. But is that actually nicer? :) Is http://www.webkit.org/coding/RefPtr.html out of date? Or does GRefPtr intentionally not work like RefPtr? I originally tried GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult.release(); but GRefPtr doesn't have release() (and the docs on the RefPtr page say that leakRef() only exists on PassRefPtr) (In reply to comment #5) > Is http://www.webkit.org/coding/RefPtr.html out of date? Or does GRefPtr intentionally not work like RefPtr? I originally tried > > GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult.release(); > > but GRefPtr doesn't have release() (and the docs on the RefPtr page say that leakRef() only exists on PassRefPtr) Oh. I think that GRefPtr is missing the release method? That you needed it here indicates that it should probably have it. Do you mind adding it with your patch and using it? GRefPtr just lags behind RefPtr feature-wise. (In reply to comment #6) > Oh. I think that GRefPtr is missing the release method? That you needed it here indicates that it should probably have it. Do you mind adding it with your patch and using it? GRefPtr just lags behind RefPtr feature-wise. Hm... RefPtr.release() returns a PassRefPtr, but there's no GPassRefPtr. The docs say "If ownership and lifetime are guaranteed, a local variable can be a raw pointer." So I just did that (with leakRef). Created attachment 115169 [details]
updated patch
oh, that's rebased to master btw, so it's landable Comment on attachment 115169 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=115169&action=review Looks good to me, except that I think you are leaking the asyncResult. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:750 > + // We only need to take action here to UN-defer loading Nit: Missing a period. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:759 > return; Is it actually possible for m_cancellable to be non-null and d->m_soupRequest to be null? If so it'd be good to leave a small comment here explaining when and if not, it'd be better to ASSERT(d->m_soupRequest) instead. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:763 > + GAsyncResult* asyncResult = d->m_deferredResult.leakRef(); I think you actually need to call adoptGRef here because the naked assignment adds another reference and the pointer leaks. Sorry for the confusion. Created attachment 115382 [details]
updated updated patch
updated for previous comments in bugzilla and on IRC. (I ended up going with your original suggestion for the leakref thing.)
Comment on attachment 115382 [details] updated updated patch Clearing flags on attachment: 115382 Committed r100466: <http://trac.webkit.org/changeset/100466> All reviewed patches have been landed. Closing bug. |