Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
100563004:
Redirect HTML resource bytes directly to parser thread (Blink side CL) (Closed)
|
Created:
7 years ago by oystein (OOO til 10th of July) Modified:
6 years, 8 months ago CC:
blink-reviews, sof, eae+blinkwatch, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@parserthread_decodermove Visibility:
Public. |
DescriptionRedirect HTML resource bytes directly to parser thread (Blink side)
Chrome side: https://codereview.chromium.org/109283006
R=eseidel@chromium.org,abarth@chromium.org
BUG=277886
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170476
Patch Set 1 : #Patch Set 2 : Missing include #
Total comments: 25
Patch Set 3 : Cache parser thread pointer in HTMLDocumentParser #
Total comments: 14
Patch Set 4 : #
Total comments: 11
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 15
Patch Set 8 : Review fixes #Patch Set 9 : Leak fix #
Total comments: 3
Patch Set 10 : #Patch Set 11 : Rebase #Patch Set 12 : Conflict fix #Messages
Total messages: 38 (0 generated)
Hi again guys, first pass of this :). There's two linked CLs, one for the Blink side and one for the Chrome side (that seemed to be the way to do this?).
The general approach looks fine. Most of my comments below are about naming. We'll probably need to iterate on the CL a few more time before we get it fully polished. One general note: it seems like you're plumbing the didAddParserResourceMessageFilter notification through a number of layers in Blink that don't care about this information at all. I wonder if the WebThreadedResourceProvider should have two kinds of clients: one that exists on the main thread and one that exists on the background thread. We could then notify the main thread client directly that we switched to the background thread... https://codereview.chromium.org/100563004/diff/60001/Source/core/dom/Document... File Source/core/dom/DocumentParser.h (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/dom/Document... Source/core/dom/DocumentParser.h:58: virtual void parserResourceMessageFilterAdded() { } Blink doesn't know about "message filters". You're going to need to express this in a more generic way. Maybe "startParsingOnBackgroundThread" ? https://codereview.chromium.org/100563004/diff/60001/Source/core/fetch/Resour... File Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/fetch/Resour... Source/core/fetch/ResourceLoader.cpp:187: return OwnPtr<blink::WebParserResourceBridge>().release(); return nullptr https://codereview.chromium.org/100563004/diff/60001/Source/core/fetch/Resour... File Source/core/fetch/ResourceLoader.h (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/fetch/Resour... Source/core/fetch/ResourceLoader.h:66: PassOwnPtr<blink::WebParserResourceBridge> constructParserResourceBridge(); constructFoo -> createFoo https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... File Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.cpp:135: // attached WebParserResourceBridge) That sounds very fragile. How about we just make a copy of the data that this object can own instead of having this complex lifetime coupling. https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.cpp:210: m_parserThreadIsStandalone = true; I'm not sure what this state variable means. It looks like a switch between receiving data from the main thread and from receiving data directly from the network stack. https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.cpp:216: updateDocument(m_decoder->decode(entry->first, entry->second)); Do we need to worry about one of these updateDocument calls stopping parsing? https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.cpp:221: std::swap(emptyQueue, m_queuedData); These idioms will be different once you switch to using WTF collections. https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... File Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.h:39: #include <vector> wtf/Vector.h https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.h:72: virtual void OnReceivedData(const char* data, size_t length); didReceiveData https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.h:114: typedef std::vector< std::pair<const char*, size_t> > DataQueue; Please use a named struct instead of std::pair https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... File Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:93: startBackgroundParser(); Does this integrate correctly with m_isPinnedToMainThread? https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:680: m_parserThread = resourceBridge->getParserThread(); getParserThread -> parserThread https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... File Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.h:107: static void destructResourceBridge(PassOwnPtr<blink::WebParserResourceBridge>); destruct -> destroy https://codereview.chromium.org/100563004/diff/60001/Source/core/loader/Docum... File Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/loader/Docum... Source/core/loader/DocumentLoader.cpp:894: return OwnPtr<blink::WebParserResourceBridge>().release(); return nullptr; https://codereview.chromium.org/100563004/diff/60001/public/platform/WebParse... File public/platform/WebParserResourceBridge.h (right): https://codereview.chromium.org/100563004/diff/60001/public/platform/WebParse... public/platform/WebParserResourceBridge.h:38: class WebParserResourceBridge { Is this specific to parsers? It seems like a generic mechanism for receiving data callbacks on a background thread. WebThreadedResourceProvider ? https://codereview.chromium.org/100563004/diff/60001/public/platform/WebParse... public/platform/WebParserResourceBridge.h:40: class Peer { I'd make this a top-level class instead of a nested class. Perhaps WebThreadedResourceClient? https://codereview.chromium.org/100563004/diff/60001/public/platform/WebParse... public/platform/WebParserResourceBridge.h:42: virtual void OnReceivedData(const char* data, size_t length) = 0; didReceivedData https://codereview.chromium.org/100563004/diff/60001/public/platform/WebParse... public/platform/WebParserResourceBridge.h:45: virtual void setPeer(Peer*) = 0; setClient https://codereview.chromium.org/100563004/diff/60001/public/platform/WebParse... public/platform/WebParserResourceBridge.h:46: virtual WebThread* getParserThread() = 0; If this interface isn't specific to parsing, perhaps "resourceThread" ? https://codereview.chromium.org/100563004/diff/60001/public/platform/WebURLLo... File public/platform/WebURLLoader.h (right): https://codereview.chromium.org/100563004/diff/60001/public/platform/WebURLLo... public/platform/WebURLLoader.h:76: virtual WebParserResourceBridge* constructParserResourceBridge() { return 0; } At this layer, I'm not sure it makes sense to have parser-specific concepts. However, a generic concept of "I would like to receive the response to this request on another thread" makes sense. We don't need to have a fully general implementation in the first iteration, but we should design the API with that future in mind. https://codereview.chromium.org/100563004/diff/60001/public/platform/WebURLLo... File public/platform/WebURLLoaderClient.h (right): https://codereview.chromium.org/100563004/diff/60001/public/platform/WebURLLo... public/platform/WebURLLoaderClient.h:73: virtual void didAddParserResourceMessageFilter() { } Again, this API is too coupled to the implementation details. Blink has no knowledge of things like resource message filters and the WebURLLoader shouldn't have any knowledge of things like parsers. didSwitchToBackgroundThread ?
Thanks abarth, great comments. Second draft is up! https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... File Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.cpp:135: // attached WebParserResourceBridge) On 2013/12/18 18:28:49, abarth wrote: > That sounds very fragile. How about we just make a copy of the data that this > object can own instead of having this complex lifetime coupling. The lifetime coupling is still needed to be able to safely make the calls between the Chrome and Blink parts of this, so making a copy here wouldn't simplify any code elsewhere as far as I can tell. https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.cpp:210: m_parserThreadIsStandalone = true; On 2013/12/18 18:28:49, abarth wrote: > I'm not sure what this state variable means. It looks like a switch between > receiving data from the main thread and from receiving data directly from the > network stack. Renamed to m_receivingDataOnlyFromResourceProvider. A bit verbose though, but best I could come up with. https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/BackgroundHTMLParser.cpp:216: updateDocument(m_decoder->decode(entry->first, entry->second)); On 2013/12/18 18:28:49, abarth wrote: > Do we need to worry about one of these updateDocument calls stopping parsing? Shouldn't matter. As soon as a parsed chunk is processed on the main thread and signals for the parsing to stop, it'll invalidate the weakptrs that the BackgroundHTMLParser holds, so any further updates will be ignored. https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... File Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/100563004/diff/60001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:93: startBackgroundParser(); On 2013/12/18 18:28:49, abarth wrote: > Does this integrate correctly with m_isPinnedToMainThread? As far as I can tell so far, parsers which aren't pinned when constructed but are pinned later never have "useThreading" set in HTMLParserOptions. If there's any cases like that I haven't seen yet I'd love to try them out though.
I'd like to get the crashes with the first CL nailed down before investing a lot of time reviewing this CL.
On 2014/01/14 04:05:51, abarth wrote: > I'd like to get the crashes with the first CL nailed down before investing a lot > of time reviewing this CL. The crash is confirmed fixed now; the previous CL (and the associated crash fix) are both present in 34.0.1802.2 (yesterday's canary) with no incidences of the crash that I can see.
On 2014/01/24 18:36:25, Oystein wrote: > The crash is confirmed fixed now; the previous CL (and the associated crash fix) > are both present in 34.0.1802.2 (yesterday's canary) with no incidences of the > crash that I can see. Is the comment out of date? https://code.google.com/p/chromium/issues/detail?id=329603#c10
On 2014/01/24 18:43:13, abarth wrote: > On 2014/01/24 18:36:25, Oystein wrote: > > The crash is confirmed fixed now; the previous CL (and the associated crash > fix) > > are both present in 34.0.1802.2 (yesterday's canary) with no incidences of the > > crash that I can see. > > Is the comment out of date? > > https://code.google.com/p/chromium/issues/detail?id=329603#c10 Yes; there was a canary which landed for Mac which had a webkit revision from in between the revert of the revert landed, and the fixed CL. Just discussed this with Daniel.
On 2014/01/24 18:44:40, Oystein wrote: > On 2014/01/24 18:43:13, abarth wrote: > > On 2014/01/24 18:36:25, Oystein wrote: > > > The crash is confirmed fixed now; the previous CL (and the associated crash > > fix) > > > are both present in 34.0.1802.2 (yesterday's canary) with no incidences of > the > > > crash that I can see. > > > > Is the comment out of date? > > > > https://code.google.com/p/chromium/issues/detail?id=329603#c10 > > Yes; there was a canary which landed for Mac which had a webkit revision from in > between the revert of the revert landed, and the fixed CL. Just discussed this > with Daniel. Oh, that's great!
Removing the WIP label; I'm starting to feel pretty OK about this now. The one thing I have left to do before this can land (pending review rework, of course) is to move the invoking of site_isolation_policy.cc to the I/O thread (if possible). Right now it's only active on the main thread, so bytes going straight to the parser thread currently bypasses it. With the current code this doesn't matter as it's just generating UMA metrics (and since we do eventually pass these bytes to the main thread too those still get collected), but once the class is used for actual resource blocking this'll fail (and there's browser tests for this that currently fail).
On 2014/01/28 01:22:16, Oystein wrote: > Removing the WIP label; I'm starting to feel pretty OK about this now. > > The one thing I have left to do before this can land (pending review rework, of > course) is to move the invoking of site_isolation_policy.cc to the I/O thread > (if possible). Right now it's only active on the main thread, so bytes going > straight to the parser thread currently bypasses it. With the current code this > doesn't matter as it's just generating UMA metrics (and since we do eventually > pass these bytes to the main thread too those still get collected), but once the > class is used for actual resource blocking this'll fail (and there's browser > tests for this that currently fail). (The site_isolation_policy.cc change is on the Chrome-side though, and will be a separate warm-up CL).
ping!
I landed a change to HTMLParserThread since this was made which will confict with this change. :(
On 2014/02/03 19:16:21, eseidel wrote: > I landed a change to HTMLParserThread since this was made which will confict > with this change. :( Yeah I saw that, I can add the same kind of explicit teardown of the parser thread on the Chrome side in the companion CL to this one.
My main concern with this CL is that you haven't encapsulated the implementation details from the Chromium side. Instead, this code now has detailed knowledge of concepts like a resource filter and the I/O thread, which are Chromium-specific concepts. It might be worth spending some time thinking about how to structure the interaction between Blink and Chromium such that it makes sense without having a detailed understanding of both sides of the interaction. https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... File Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.cpp:113: // as the filter it installs on the I/O thread can only be removed Blink doesn't know anything about I/O threads. https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.cpp:135: memcpy(buffer->data(), data, length); buffer.append(data, length) https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.cpp:222: updateDocument(m_decoder->decode((*entry)->data(), (*entry)->size())); SharedBuffer has a better way of doing this sort of thing. https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... File Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.h:72: virtual void didReceivedData(const char* data, size_t length); OVERRIDE https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.h:81: void resourceFilterAdded(); Blink doesn't know anything about resource filters. Can we think of a name for this concept that makes sense at this layer? https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.h:115: Vector<OwnPtr<Vector<char> > > m_queuedData; SharedBuffer? https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... File Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.h:217: static blink::WebThread* s_parserThread; Why did you move this static out of its dedicated class?
Thanks abarth! New version up :). https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... File Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.cpp:113: // as the filter it installs on the I/O thread can only be removed On 2014/02/03 21:17:36, abarth wrote: > Blink doesn't know anything about I/O threads. Removed that part of the comment; it was just a clarification, not relevant to Blink. https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.cpp:135: memcpy(buffer->data(), data, length); On 2014/02/03 21:17:36, abarth wrote: > buffer.append(data, length) Done. https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.cpp:222: updateDocument(m_decoder->decode((*entry)->data(), (*entry)->size())); On 2014/02/03 21:17:36, abarth wrote: > SharedBuffer has a better way of doing this sort of thing. Done. https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... File Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.h:72: virtual void didReceivedData(const char* data, size_t length); On 2014/02/03 21:17:36, abarth wrote: > OVERRIDE Done. https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.h:81: void resourceFilterAdded(); On 2014/02/03 21:17:36, abarth wrote: > Blink doesn't know anything about resource filters. Can we think of a name for > this concept that makes sense at this layer? Renamed to "didSwitchedToBackgroundClient" (the name of the call which is propagated from the HTMLDocumentParser) https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/BackgroundHTMLParser.h:115: Vector<OwnPtr<Vector<char> > > m_queuedData; On 2014/02/03 21:17:36, abarth wrote: > SharedBuffer? Done. https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... File Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/100563004/diff/250001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.h:217: static blink::WebThread* s_parserThread; On 2014/02/03 21:17:36, abarth wrote: > Why did you move this static out of its dedicated class? The parser thread itself now has to be managed by Chrome and not Blink (to be able to schedule Chrome jobs on the thread, like installing the resource filter etc), so keeping it in a singleton on the Blink side doesn't make sense anymore. The static in HTMLDocumentParser is now essentially just a cached pointer. The use of the thread itself is also an implementation detail of HTMLDocumentParser and shouldn't be visible outside of it IMO. I've moved the explicit shutdown eseidel added to the Chrome side in the companion CL to this.
Apologies for the radical suggestions. I'd be happy to sit down to discuss in more detail. https://codereview.chromium.org/100563004/diff/390001/Source/core/html/parser... File Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/100563004/diff/390001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.cpp:85: virtual void didSwitchedToBackgroundClient() OVERRIDE FINAL { nit: opening paren should be on the next line per blink style. https://codereview.chromium.org/100563004/diff/390001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.cpp:87: m_parser.get()->didSwitchedToBackgroundClient(); nit: "didSwitched..." should be "didSwitch..." https://codereview.chromium.org/100563004/diff/390001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.cpp:351: // Here to let the resource bridge be destructed on the main thread. nit: I think this is a reference to ResourceLoaderBridge which is not something that is known to Blink. Comments like this may add confusion since it refers to something outside of the Blink codebase and that something may also disappear in future revisions of Chromium. https://codereview.chromium.org/100563004/diff/390001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.cpp:700: s_parserThread = resourceProvider->resourceProviderThread(); nit: perhaps it would be better to have Blink create the parser thread via Platform::createThread, and then pass that thread to createThreadedResourceProvider? That is more consistent with the notion that this is viewed as "the parser thread". https://codereview.chromium.org/100563004/diff/390001/public/platform/WebThre... File public/platform/WebThreadedResourceProvider.h (right): https://codereview.chromium.org/100563004/diff/390001/public/platform/WebThre... public/platform/WebThreadedResourceProvider.h:38: class WebThreadedResourceBackgroundClient { nit: The rule for the Blink public API is one file per type. https://codereview.chromium.org/100563004/diff/390001/public/platform/WebThre... public/platform/WebThreadedResourceProvider.h:39: public: this interface should have a virtual destructor too? https://codereview.chromium.org/100563004/diff/390001/public/platform/WebThre... public/platform/WebThreadedResourceProvider.h:45: virtual void didSwitchedToBackgroundClient() = 0; This function could also be part of WebURLLoaderClient. In other words, maybe it would be better to recast this CL as a mechanism to allow the WebURLLoader user to attach a data sink that should be fed on a user specified WebThread. that suggests that communication on the main thread can remain on WebURLLoaderClient, and then all you really need to spec is the equivalent of WebThreadedResourceBackgroundClient. I suspect you don't really even need a WebThreadedResourceProvider interface. We might even consider a generic interface like this: class WebDataReceiver { public: virtual void acceptData(const char* data, int dataLength) = 0; }; (^^^ note: the above uses |int| instead of |size_t| for consistency with WebURLLoaderClient::didReceiveData, but I could see the case for switching all of these byte count parameters to size_t. That might be better done in a different CL.) Then, you could probably just go with a method on WebURLLoader to set a WebDataReceiver. I'd argue that you should prescribe that a WebDataReceiver may only be set prior to the first call to didReceiveData. https://codereview.chromium.org/100563004/diff/390001/public/platform/WebThre... public/platform/WebThreadedResourceProvider.h:50: class WebThreadedResourceProvider { I think it would be helpful to provide some documentation about what a foreground and background client is. It is quite mysterious if you are only reading this header file. https://codereview.chromium.org/100563004/diff/390001/public/platform/WebThre... public/platform/WebThreadedResourceProvider.h:54: virtual WebThread* resourceProviderThread() = 0; I have a feeling that the resourceProviderThread is something that doesn't vary much, yet it is a virtual function, which means it can vary. Maybe it should be pushed to Blink instead of being queried by Blink? Or, maybe Blink could allocate it using Platform::createThread? https://codereview.chromium.org/100563004/diff/390001/public/platform/WebURLL... File public/platform/WebURLLoader.h (right): https://codereview.chromium.org/100563004/diff/390001/public/platform/WebURLL... public/platform/WebURLLoader.h:76: virtual WebThreadedResourceProvider* createThreadedResourceProvider() { return 0; } hmm, the resource being provided to the background thread is really the 'data'. It is about suppressing WebURLLoaderClient::didReceiveData in favor of WebThreadedResourceBackgroundClient::didReceiveData. On that note, I wonder if we shouldn't use the term "data" in place of "resource"? it would be helpful to document the API better. it is not clear when one is supposed to call createThreadedResourceProvider relative to other methods on WebURLLoader.
Thanks Darin! https://codereview.chromium.org/100563004/diff/390001/public/platform/WebThre... File public/platform/WebThreadedResourceProvider.h (right): https://codereview.chromium.org/100563004/diff/390001/public/platform/WebThre... public/platform/WebThreadedResourceProvider.h:45: virtual void didSwitchedToBackgroundClient() = 0; On 2014/02/16 07:12:27, darin wrote: > This function could also be part of WebURLLoaderClient. In other words, maybe it > would be better to recast this CL as a mechanism to allow the WebURLLoader user > to attach a data sink that should be fed on a user specified WebThread. that > suggests that communication on the main thread can remain on WebURLLoaderClient, > and then all you really need to spec is the equivalent of > WebThreadedResourceBackgroundClient. I suspect you don't really even need a > WebThreadedResourceProvider interface. > > We might even consider a generic interface like this: > > class WebDataReceiver { > public: > virtual void acceptData(const char* data, int dataLength) = 0; > }; > > (^^^ note: the above uses |int| instead of |size_t| for consistency with > WebURLLoaderClient::didReceiveData, but I could see the case for switching all > of these byte count parameters to size_t. That might be better done in a > different CL.) > Folding this into WebURLLoaderClient to make it more generic makes sense to me; I'm taking a stab at this now. > Then, you could probably just go with a method on WebURLLoader to set a > WebDataReceiver. I'd argue that you should prescribe that a WebDataReceiver may > only be set prior to the first call to didReceiveData. I think the last part there is something we should work towards, but it may be a stretch to do it as part of this CL as so much of the setup we do of the parsing and document writing is done on-demand when the first data packet arrives, rather than as a result of the HTTP response (as far as I can tell). It does mean we have to do a bit of a song'n'dance to make sure we process the data in the right order on the background thread (like this CL does in BackgroundHTMLParser.cpp), but maybe I can move that around so that the logic there is contained somewhere centrally and doesn't need to be re-done for each other resource type which would reuse this system. Sounds reasonable?
On Thu, Feb 20, 2014 at 10:43 AM, <oysteine@chromium.org> wrote: > Thanks Darin! > > > > https://codereview.chromium.org/100563004/diff/390001/public/platform/ > WebThreadedResourceProvider.h > File public/platform/WebThreadedResourceProvider.h (right): > > https://codereview.chromium.org/100563004/diff/390001/public/platform/ > WebThreadedResourceProvider.h#newcode45 > public/platform/WebThreadedResourceProvider.h:45: virtual void > didSwitchedToBackgroundClient() = 0; > On 2014/02/16 07:12:27, darin wrote: > >> This function could also be part of WebURLLoaderClient. In other >> > words, maybe it > >> would be better to recast this CL as a mechanism to allow the >> > WebURLLoader user > >> to attach a data sink that should be fed on a user specified >> > WebThread. that > >> suggests that communication on the main thread can remain on >> > WebURLLoaderClient, > >> and then all you really need to spec is the equivalent of >> WebThreadedResourceBackgroundClient. I suspect you don't really even >> > need a > >> WebThreadedResourceProvider interface. >> > > We might even consider a generic interface like this: >> > > class WebDataReceiver { >> public: >> virtual void acceptData(const char* data, int dataLength) = 0; >> }; >> > > (^^^ note: the above uses |int| instead of |size_t| for consistency >> > with > >> WebURLLoaderClient::didReceiveData, but I could see the case for >> > switching all > >> of these byte count parameters to size_t. That might be better done in >> > a > >> different CL.) >> > > > Folding this into WebURLLoaderClient to make it more generic makes sense > to me; I'm taking a stab at this now. > > > Then, you could probably just go with a method on WebURLLoader to set >> > a > >> WebDataReceiver. I'd argue that you should prescribe that a >> > WebDataReceiver may > >> only be set prior to the first call to didReceiveData. >> > > I think the last part there is something we should work towards, but it > may be a stretch to do it as part of this CL as so much of the setup we > do of the parsing and document writing is done on-demand when the first > data packet arrives, rather than as a result of the HTTP response (as > far as I can tell). It does mean we have to do a bit of a song'n'dance > to make sure we process the data in the right order on the background > thread (like this CL does in BackgroundHTMLParser.cpp), but maybe I can > move that around so that the logic there is contained somewhere > centrally and doesn't need to be re-done for each other resource type > which would reuse this system. Sounds reasonable? > > OK, I see. I hadn't considered that complication. I suppose it should be sufficient to say that the WebDataReceiver may be set on the WebURLLoader on the main thread, and from that point forward, didReceiveData will not be called on WebURLLoaderClient. You could also say that the WebDataReceiver once set cannot be unset or changed. -Darin > https://codereview.chromium.org/100563004/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
New version up, ptal. The Blink side is really simplified now, and almost all of the logic is contained in threadeddataprovider.cc on the Chrome side of things. Darin, one question: Blink will now push the thread to use (the parser thread) up to Chrome as suggested, but I couldn't find any ways for Chrome to post jobs to the WebThread. The current workaround is a bit of an iffy static_cast from WebThread to WebThreadImpl in threadeddataprovider.cc; I'm not sure how kosher that is :/.
On 2014/03/04 21:05:07, Oystein wrote: > The current workaround is a bit of an iffy static_cast from > WebThread to WebThreadImpl in threadeddataprovider.cc; I'm not sure how kosher > that is :/. It's fine for the provider of the WebThread to know about the dynamic type of the implementation. In this case, the WebThread is provided to blink through the blink::Platform interface. The implementation of this is content/ on the Chromium side. It's fine for other parts of content/ on the Chromium side to know which implementation of WebThread is being provided to blink. We downcast in other parts of the Blink bindings.
On 2014/03/04 21:07:50, jamesr wrote: > On 2014/03/04 21:05:07, Oystein wrote: > > The current workaround is a bit of an iffy static_cast from > > WebThread to WebThreadImpl in threadeddataprovider.cc; I'm not sure how kosher > > that is :/. > > It's fine for the provider of the WebThread to know about the dynamic type of > the implementation. In this case, the WebThread is provided to blink through > the blink::Platform interface. The implementation of this is content/ on the > Chromium side. It's fine for other parts of content/ on the Chromium side to > know which implementation of WebThread is being provided to blink. We downcast > in other parts of the Blink bindings. Gotcha, I'll rest easy then :)
darin/abarth: ping :)
This version is much better. Thank you! I've left some minor comments below. https://codereview.chromium.org/100563004/diff/1010001/Source/core/fetch/Reso... File Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/fetch/Reso... Source/core/fetch/ResourceLoader.cpp:185: if (m_loader) Should this be an error if m_loader isn't truthy? What does the caller expect to happen in that case? https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... File Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... Source/core/html/parser/BackgroundHTMLParser.h:70: void appendBytes(PassOwnPtr<Vector<char> >); We might want to rename this function so that it's clear what the difference is between acceptData and appendBytes. On the surface, they seem similar. https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... File Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... Source/core/html/parser/HTMLDocumentParser.cpp:78: ParserDataReceiver(WeakPtr<BackgroundHTMLParser> backgroundParser) Please make one-argument constructors explicit. https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... Source/core/html/parser/HTMLDocumentParser.cpp:696: document()->loader()->attachThreadedDataReceiver(new ParserDataReceiver(m_backgroundParser)); adoptPtr(new ParserDataReceiver(... https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... File Source/core/html/parser/HTMLParserThread.h (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... Source/core/html/parser/HTMLParserThread.h:49: blink::WebThread& ensureThread(); I'd rename this function to something like platformThread() https://codereview.chromium.org/100563004/diff/1010001/Source/core/loader/Doc... File Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/loader/Doc... Source/core/loader/DocumentLoader.cpp:785: if (mainResourceLoader()) Should it be an error to call this function without a mainResourceLoader()? What does the caller expect to happen in that case? https://codereview.chromium.org/100563004/diff/1010001/Source/core/loader/Doc... File Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/loader/Doc... Source/core/loader/DocumentLoader.h:129: void attachThreadedDataReceiver(blink::WebThreadedDataReceiver*); PassOwnPtr<blink::WebThreadedDataReceiver> https://codereview.chromium.org/100563004/diff/1010001/public/platform/WebThr... File public/platform/WebThreadedDataReceiver.h (right): https://codereview.chromium.org/100563004/diff/1010001/public/platform/WebThr... public/platform/WebThreadedDataReceiver.h:29: */ You can use the new-style copyright header for new files: http://dev.chromium.org/blink/coding-style#TOC-License
Thanks abarth! New version up with the below comments. https://codereview.chromium.org/100563004/diff/1010001/Source/core/fetch/Reso... File Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/fetch/Reso... Source/core/fetch/ResourceLoader.cpp:185: if (m_loader) On 2014/03/10 21:52:20, abarth wrote: > Should this be an error if m_loader isn't truthy? What does the caller expect > to happen in that case? Since the use of a threaded data receiver is an optional thing, I think it's OK if the object just gets deleted (leaked before, but should now be OK). Alternatively we could add a canUseThreadedDataReceiver() function and pass through the whole stack, if we want to avoid the redundant object creation in these cases? https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... File Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... Source/core/html/parser/BackgroundHTMLParser.h:70: void appendBytes(PassOwnPtr<Vector<char> >); On 2014/03/10 21:52:20, abarth wrote: > We might want to rename this function so that it's clear what the difference is > between acceptData and appendBytes. On the surface, they seem similar. Done. https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... File Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... Source/core/html/parser/HTMLDocumentParser.cpp:78: ParserDataReceiver(WeakPtr<BackgroundHTMLParser> backgroundParser) On 2014/03/10 21:52:20, abarth wrote: > Please make one-argument constructors explicit. Done. https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... Source/core/html/parser/HTMLDocumentParser.cpp:696: document()->loader()->attachThreadedDataReceiver(new ParserDataReceiver(m_backgroundParser)); On 2014/03/10 21:52:20, abarth wrote: > adoptPtr(new ParserDataReceiver(... Done. https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... File Source/core/html/parser/HTMLParserThread.h (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/html/parse... Source/core/html/parser/HTMLParserThread.h:49: blink::WebThread& ensureThread(); On 2014/03/10 21:52:20, abarth wrote: > I'd rename this function to something like platformThread() Done. https://codereview.chromium.org/100563004/diff/1010001/Source/core/loader/Doc... File Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/100563004/diff/1010001/Source/core/loader/Doc... Source/core/loader/DocumentLoader.h:129: void attachThreadedDataReceiver(blink::WebThreadedDataReceiver*); On 2014/03/10 21:52:20, abarth wrote: > PassOwnPtr<blink::WebThreadedDataReceiver> Done. https://codereview.chromium.org/100563004/diff/1010001/public/platform/WebThr... File public/platform/WebThreadedDataReceiver.h (right): https://codereview.chromium.org/100563004/diff/1010001/public/platform/WebThr... public/platform/WebThreadedDataReceiver.h:29: */ On 2014/03/10 21:52:20, abarth wrote: > You can use the new-style copyright header for new files: > > http://dev.chromium.org/blink/coding-style#TOC-License Done.
LGTM!
On 2014/03/18 00:27:24, abarth wrote: > LGTM! Fantastic, thanks :). One last-minute issue fixed: I added a bool return to WebURLLoader::attachThreadedDataReceiver to signal whether ownership of the parameter was actually assumed or not (i.e. attachment successful); if false the ResourceLoader.cpp will take care of deleting the data receiver itself. Assuming that's OK, should I wait until the Chrome-side part of the CL is approved as well before committing?
ok
https://codereview.chromium.org/100563004/diff/1050001/public/platform/WebURL... File public/platform/WebURLLoader.h (right): https://codereview.chromium.org/100563004/diff/1050001/public/platform/WebURL... public/platform/WebURLLoader.h:75: // on its thread, rather than the WebURLLoaderClient. If successful, ownership you should probably indicate what thread the WebThreadedDataReceiver will be deleted on. https://codereview.chromium.org/100563004/diff/1050001/public/platform/WebURL... public/platform/WebURLLoader.h:75: // on its thread, rather than the WebURLLoaderClient. If successful, ownership "on its thread" -> might be nice to spell out that you mean the WebThread returned from WebThreadedDataReceiver::backgroundThread. Also, nit: why didn't you just pass the WebThread as a parameter to attachThreadedDataReceiver? The WebThread returned from WebThreadedDataReceiver probably needs to be stable, so it doesn't make much sense for it to be a dynamic property (i.e., virtual function).
https://codereview.chromium.org/100563004/diff/1050001/public/platform/WebURL... File public/platform/WebURLLoader.h (right): https://codereview.chromium.org/100563004/diff/1050001/public/platform/WebURL... public/platform/WebURLLoader.h:75: // on its thread, rather than the WebURLLoaderClient. If successful, ownership On 2014/03/19 03:51:07, darin wrote: > "on its thread" -> might be nice to spell out that you mean the WebThread > returned from WebThreadedDataReceiver::backgroundThread. Done. > Also, nit: why didn't you just pass the WebThread as a parameter to > attachThreadedDataReceiver? The WebThread returned from WebThreadedDataReceiver > probably needs to be stable, so it doesn't make much sense for it to be a > dynamic property (i.e., virtual function). The WebThread being a property of the WebThreadedDataReceiver made sense so it could be used for validation on the receiving side too (i.e. the assert in ParserDataReceiver::acceptData), and it seemed nicer to only define it in one location (i.e. backgroundThread()). If it would make the API clearer I could move that up to ParserDataReceiver itself though, and also pass it along as an explicit parameter to attachThreadedDataReceiver().
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/100563004/1070001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for public/platform/WebURLLoader.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file public/platform/WebURLLoader.h
Hunk #2 FAILED at 70.
1 out of 2 hunks FAILED -- saving rejects to file
public/platform/WebURLLoader.h.rej
Patch: public/platform/WebURLLoader.h
Index: public/platform/WebURLLoader.h
diff --git a/public/platform/WebURLLoader.h b/public/platform/WebURLLoader.h
index
e183a32104018f69dca07a9600f50054b9e64edd..d8fba1b281d01166928492a6c992aedf3bb2d395
100644
--- a/public/platform/WebURLLoader.h
+++ b/public/platform/WebURLLoader.h
@@ -37,6 +37,7 @@
namespace blink {
class WebData;
+class WebThreadedDataReceiver;
class WebURLLoaderClient;
class WebURLResponse;
struct WebURLError;
@@ -69,6 +70,13 @@ public:
// its previous value. For example, a preload request starts with low
// priority, but may increase when the resource is needed for rendering.
virtual void didChangePriority(WebURLRequest::Priority newPriority) { }
+
+ // Try to attach the given data receiver which from this point will receive
data
+ // on its thread (provided by WebThreadedDataReceiver::backgroundThread(),
+ // rather than the WebURLLoaderClient. If successful, ownership
+ // of the data receiver is assumed by the WebURLLoader and the receiver
should
+ // be deleted on the main thread when no longer needed.
+ virtual bool attachThreadedDataReceiver(WebThreadedDataReceiver*) { return
false; }
};
} // namespace blink
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/100563004/1090001
The CQ bit was checked by oysteine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/100563004/1110001
Message was sent while issue was closed.
Change committed as 170476 |
