[webkit-reviews] review granted: [Bug 107192] Rework NetworkProcess resource load identifiers : [Attachment 183567] Patch v1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 107192: Rework NetworkProcess resource load identifiers
https://bugs.webkit.org/show_bug.cgi?id=107192

Attachment 183567: Patch v1
https://bugs.webkit.org/attachment.cgi?id=183567&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183567&action=review


Looks like a good improvement logically.

One thing I'm quite unhappy about is making some classes RefCounted. It is
always better to have a concrete ownership policy than to hope that RefPtrs
will balance out in the end somehow. I'm particularly unhappy about
complicating ownership in NetworkProcess code, which is very multithreaded, and
RefPtrs to shared objects add unpredictability to it. It's especially bad
because we have debug assertions for RefPtr threading errors disabled now.

> Source/WebCore/loader/FrameLoader.cpp:2590
> +	       unsigned long identifier = 0;
> +	       if (m_frame->page())
> +		   identifier =
m_frame->page()->progress()->createUniqueIdentifier();
> +	      
platformStrategies()->loaderStrategy()->loadResourceSynchronously(networkingCon
text(), identifier, newRequest, storedCredentials, error, response, data);

Really, we want to call the strategy with a 0 identifier?

> Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:103
> -    HostRecord* oldHost = m_identifiers.get(identifier);
> +    RefPtr<HostRecord> oldHost = loader->hostRecord();

Why can't oldHost be a plain pointer any more?