Chromium Code Reviews
|
|
DescriptionAdd WebDataConsumerHandle::Reader interface.
As a robust API for WebDataConsumerHandle, we add Reader interface on it and
deprecate existing read / beginRead / endRead / registerClient /
unregisterClient methods. The new API is better because
- Reader is now bound to a thread.
- WebDataConsumerHandle is now thread-safe.
- A Handle subclass can extend Reader APIs.
The followup CLs will remove the deprecated functions.
BUG=480746
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197029
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #
Total comments: 15
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 2
Patch Set 11 : #Messages
Total messages: 24 (8 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org
Are you going to remove read() etc. from WebDataConsumerHandle in a 3-step way? If so, mention that in the CL description and comments (not just "Below are deprecated functions." but "will shortly removed"). https://codereview.chromium.org/1149563007/diff/60001/public/platform/WebData... File public/platform/WebDataConsumerHandle.h (right): https://codereview.chromium.org/1149563007/diff/60001/public/platform/WebData... public/platform/WebDataConsumerHandle.h:21: // A WebDataConsumerHandle which has an active reader is called "locked". nit: the word "active reader" might be confusing (i.e. there's no method like Reader::isActive(). how about just "reader"?) Mention that a Reader can outlive corresponding WebDataConsumerHandle, and can be used until Reader's destruction even WebDataConsumerHandle is already destructed. https://codereview.chromium.org/1149563007/diff/60001/public/platform/WebData... public/platform/WebDataConsumerHandle.h:74: // other readers. The returned reader is bound to the calling thread and What occurs when there is another reader when we call obtainReader()? - returns nullptr (I prefer this), - or assert failure. It might be convenient if obtainReader() returns nullptr if (and only if) there is another reader; e.g. CompositeDataConsumerHandler might call obtainReader() on supplied WebDataConsumerHandle, which might have a reader already. https://codereview.chromium.org/1149563007/diff/60001/public/platform/WebData... public/platform/WebDataConsumerHandle.h:77: PassOwnPtr<Reader> obtainReader(Client* client) { return adoptPtr(obtainReaderInternal(client)); } Can |client| be nullptr? (I assume yes)
I've not yet decided how to remove the deprecated functions. https://codereview.chromium.org/1149563007/diff/60001/public/platform/WebData... File public/platform/WebDataConsumerHandle.h (right): https://codereview.chromium.org/1149563007/diff/60001/public/platform/WebData... public/platform/WebDataConsumerHandle.h:21: // A WebDataConsumerHandle which has an active reader is called "locked". On 2015/06/04 10:06:14, hiroshige wrote: > nit: the word "active reader" might be confusing (i.e. there's no method like > Reader::isActive(). how about just "reader"?) > > Mention that a Reader can outlive corresponding WebDataConsumerHandle, and can > be used until Reader's destruction even WebDataConsumerHandle is already > destructed. Done. https://codereview.chromium.org/1149563007/diff/60001/public/platform/WebData... public/platform/WebDataConsumerHandle.h:74: // other readers. The returned reader is bound to the calling thread and On 2015/06/04 10:06:15, hiroshige wrote: > What occurs when there is another reader when we call obtainReader()? > - returns nullptr (I prefer this), > - or assert failure. > > It might be convenient if obtainReader() returns nullptr if (and only if) there > is another reader; e.g. CompositeDataConsumerHandler might call obtainReader() > on supplied WebDataConsumerHandle, which might have a reader already. It is simply not allowed. I will put an assertion, but no defined behavior is specified. https://codereview.chromium.org/1149563007/diff/60001/public/platform/WebData... public/platform/WebDataConsumerHandle.h:77: PassOwnPtr<Reader> obtainReader(Client* client) { return adoptPtr(obtainReaderInternal(client)); } On 2015/06/04 10:06:14, hiroshige wrote: > Can |client| be nullptr? (I assume yes) Done.
Patchset #4 (id:100001) has been deleted
I uploaded followup CLs [1] and [2]. 1: https://codereview.chromium.org/1164493008/ 2: https://codereview.chromium.org/1163653005/
So, we are ready to remove the deprecated APIs if we commit the follow-up CLs, right?
On 2015/06/08 04:15:23, hiroshige wrote: > So, we are ready to remove the deprecated APIs if we commit the follow-up CLs, > right? Yes. Updated the comment and the issue description.
lgtm. https://codereview.chromium.org/1149563007/diff/140001/public/platform/WebDat... File public/platform/WebDataConsumerHandle.h (right): https://codereview.chromium.org/1149563007/diff/140001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:93: // instaed. nit: s/instaed/instead/.
https://codereview.chromium.org/1149563007/diff/140001/public/platform/WebDat... File public/platform/WebDataConsumerHandle.h (right): https://codereview.chromium.org/1149563007/diff/140001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:93: // instaed. On 2015/06/08 05:25:28, hiroshige wrote: > nit: s/instaed/instead/. Done.
During implementing FetchBlobDataConsumerHandle, I found following points that I'd like to clarify in the spec of WebDataConsumerHandle. https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... File public/platform/WebDataConsumerHandle.h (right): https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:33: ShouldWait, What are differences of Busy and ShouldWait? What should we do for Busy? (wait for Client notification and try again?) https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:35: UnexpectedError, I think we should define a kind of state diagram of the handle, so that users of handles can deduce what will occur. For example (I'm not sure this is correct; let's discuss offline): State 0 (initial state): No data. read() returns ShouldWait. Goto State 1 if end-of-stream is reached. Goto State 2 if data is received. Goto State 3 or 4 if something wrong occurred. State 1 (Done): read() returns Done. State 2 (Data arrived): read() returns Ok. Goto State 0 if all arrived data is consumed. Remain State 2 otherwise. Remain State 2 if more data is received. Goto State 3 or 4 if something wrong occurred. State 3 (ResourceExhausted): read() returns ResourceExhausted. State 4 (UnexpectedError): read() returns UnexpectedError. https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:43: virtual void didGetReadable() = 0; As discussed offline briefly, we have to define when did this notification is fired more clearly (related to the comment about the state diagram above). Some specific questions: - When we do partial read (i.e. call endRead() with size < *available returned by beginRead()), the handle remains still 'readable' (because there are still remaining bytes). When didGetReadable() is called next? Immediately or when the next data arrives? - Can we assume read() returns Ok or Done when didGetReadable() is called? (I assume No, allowing redundant notifications would simplify implementation) https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:43: virtual void didGetReadable() = 0; Also, I'd like state reentrancy explicitly, e.g. "in didGetReadable() we can call read()/beginRead()/endRead() or destruct handle/reader; read()/beginRead()/endRead()/dtors of handle/read don't call didGeadReadable() synchronously, thus didGetReadable() is not called recursively." etc. (I'm not sure my assumption above is correct though) https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:58: // be stored in |*readSize|. This function cannot be called when a Is |*readSize| always valid? (i.e. even in the case of failure?) https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:60: // Returns Done when it reaches to the end of the data. Is |*readSize| always 0 when Done? https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:63: // Begins a two-phase read. On success, the function stores a buffer What is "success"? i.e. Result == Ok || Done? If not successful, are the values of |*buffer| and |*available| unspecified/undefined? https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:65: // Returns Done when it reaches to the end of the data. When Done, |*available| can be non-zero?
Patchset #8 (id:200001) has been deleted
https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... File public/platform/WebDataConsumerHandle.h (right): https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:33: ShouldWait, On 2015/06/09 14:26:24, hiroshige wrote: > What are differences of Busy and ShouldWait? > What should we do for Busy? (wait for Client notification and try again?) Busy is imported from mojo. Please use Ok, Done and ShouldWait. https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:43: virtual void didGetReadable() = 0; On 2015/06/09 14:26:24, hiroshige wrote: > As discussed offline briefly, we have to define when did this notification is > fired more clearly (related to the comment about the state diagram above). > > Some specific questions: > - When we do partial read (i.e. call endRead() with size < *available returned > by beginRead()), the handle remains still 'readable' (because there are still > remaining bytes). When didGetReadable() is called next? Immediately or when the > next data arrives? > - Can we assume read() returns Ok or Done when didGetReadable() is called? (I > assume No, allowing redundant notifications would simplify implementation) Done. https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:43: virtual void didGetReadable() = 0; On 2015/06/09 14:26:24, hiroshige wrote: > Also, I'd like state reentrancy explicitly, e.g. "in didGetReadable() we can > call read()/beginRead()/endRead() or destruct handle/reader; > read()/beginRead()/endRead()/dtors of handle/read don't call didGeadReadable() > synchronously, thus didGetReadable() is not called recursively." etc. (I'm not > sure my assumption above is correct though) Done. https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:58: // be stored in |*readSize|. This function cannot be called when a On 2015/06/09 14:26:24, hiroshige wrote: > Is |*readSize| always valid? (i.e. even in the case of failure?) I added L62 but I think it is a tautology and does not make any sense. https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:60: // Returns Done when it reaches to the end of the data. On 2015/06/09 14:26:24, hiroshige wrote: > Is |*readSize| always 0 when Done? Done. https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:63: // Begins a two-phase read. On success, the function stores a buffer On 2015/06/09 14:26:24, hiroshige wrote: > What is "success"? i.e. Result == Ok || Done? > If not successful, are the values of |*buffer| and |*available| > unspecified/undefined? Done. https://codereview.chromium.org/1149563007/diff/180001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:65: // Returns Done when it reaches to the end of the data. On 2015/06/09 14:26:24, hiroshige wrote: > When Done, |*available| can be non-zero? Done.
yhirano@chromium.org changed reviewers: + tkent@chromium.org
+tkent for OWNER review.
https://codereview.chromium.org/1149563007/diff/260001/public/platform/WebDat... File public/platform/WebDataConsumerHandle.h (right): https://codereview.chromium.org/1149563007/diff/260001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:13: #include <base/memory/scoped_ptr.h> We don't allow base/ dependency yet though there are some exceptions.
https://codereview.chromium.org/1149563007/diff/260001/public/platform/WebDat... File public/platform/WebDataConsumerHandle.h (right): https://codereview.chromium.org/1149563007/diff/260001/public/platform/WebDat... public/platform/WebDataConsumerHandle.h:13: #include <base/memory/scoped_ptr.h> On 2015/06/12 07:51:49, tkent wrote: > We don't allow base/ dependency yet though there are some exceptions. Done.
lgtm
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org Link to the patchset: https://codereview.chromium.org/1149563007/#ps280001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149563007/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197029 |
