Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14071008:
WebSocket object should fire error event when WebSocket server can't be connected. (Closed)
|
Created:
7 years, 8 months ago by Li Yin Modified:
7 years, 8 months ago CC:
blink-reviews, jamesr, abarth_chromum.org Base URL:
https://chromium.googlesource.com/chromium/blink@master Visibility:
Public. |
DescriptionWebSocket object should fire error event when WebSocket server can't be connected.
The patch is migrated from https://bugs.webkit.org/show_bug.cgi?id=87336
BUG=128057
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148893
Patch Set 1 #
Total comments: 23
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Rebased because WebSocketChannel.cpp is renamed to MainThreadWebSocketChannel.cpp #Patch Set 4 : Add WebSocketStreamError.cpp #Patch Set 5 : Change copyright information in new file #Patch Set 6 : Add new layouttests into TestExceptions because it depends on chromium side patch #Messages
Total messages: 50 (0 generated)
From https://bugs.webkit.org/show_bug.cgi?id=87336#c37 Yuta thought it was okay in general, but it still lacks an official r+. Could you please have a look? Thanks in advance.
On 2013/04/15 04:09:28, tkent wrote: I'll take a look at this too.
I'll take time for this tomorrow, sorry. In addition to code review, we need to notify blink API review team of this since it changes WebSocket's behavior. As it's already on the spec for long time and didn't get strong objection on the WebKit bugzilla, I think we can go through it smoothly. Please take a look at this. I'll help you prepare for it. https://sites.google.com/a/chromium.org/dev/blink?pli=1#launch-process https://codereview.chromium.org/14071008/diff/1/LayoutTests/http/tests/websoc... File LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html (right): https://codereview.chromium.org/14071008/diff/1/LayoutTests/http/tests/websoc... LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html:4: <script src="../../../../js-test-resources/js-test-pre.js"></script> please remove ../../../..
> Please take a look at this. I'll help you prepare for it. > https://sites.google.com/a/chromium.org/dev/blink?pli=1#launch-process The link could not be loaded correctly from my machine :( > <script src="../../../../js-test-resources/js-test-pre.js"></script> > please remove ../../../.. I will fix it. Thanks.
On 2013/04/15 14:16:43, tyoshino wrote: > In addition to code review, we need to notify blink API review team of this > since it changes WebSocket's behavior. > > As it's already on the spec for long time and didn't get strong objection on the > WebKit bugzilla, I think we can go through it smoothly. This is a trivial change [1], and I'm not sure if we need API review for it. IMO we don't need. Anyway, over-communication is good, and please ask a question about trivial change process in blink-dev. [1] http://www.chromium.org/blink#trivial-changes
On 2013/04/16 12:29:29, tkent wrote: > This is a trivial change [1], and I'm not sure if we need API review for it. IMO > we don't need. > Anyway, over-communication is good, and please ask a question about trivial > change process in blink-dev. > > [1] http://www.chromium.org/blink#trivial-changes From [1], Trivial platform changes do not need to meet the requirements above (API Review). They should be small fixes that have low risk of disrupting web developers. I think this patch should belong to this case. And some days ago, I committed a patch for MediaStream, the behavior was similar with this patch, and it didn't need API review. Please see https://codereview.chromium.org/13776002/
On 2013/04/15 14:16:43, tyoshino wrote: > LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html:4: > <script src="../../../../js-test-resources/js-test-pre.js"></script> > please remove ../../../.. I found all the tests in WebSocket used this style. But it seems there is not real directory js-test-resources existed, it was so strange. Is DRT doing some tricks? And the tests can't work in chromium browser correctly.
On 2013/04/17 07:39:01, Li Yin wrote: > On 2013/04/15 14:16:43, tyoshino wrote: > > > LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html:4: > > <script src="../../../../js-test-resources/js-test-pre.js"></script> > > please remove ../../../.. > > I found all the tests in WebSocket used this style. But it seems there is not > real directory js-test-resources existed, it was so strange. Is DRT doing some > tricks? And the tests can't work in chromium browser correctly. See this https://code.google.com/p/chromium/codesearch#search/&q=js-test-resources%20-... We have an alias.
On 2013/04/17 00:41:46, Li Yin wrote: > On 2013/04/16 12:29:29, tkent wrote: > > This is a trivial change [1], and I'm not sure if we need API review for it. > IMO > > we don't need. > > Anyway, over-communication is good, and please ask a question about trivial > > change process in blink-dev. > > > > [1] http://www.chromium.org/blink#trivial-changes > > From [1], Trivial platform changes do not need to meet the requirements above > (API Review). They should be small fixes that have low risk of disrupting web > developers. I think this patch should belong to this case. > And some days ago, I committed a patch for MediaStream, the behavior was similar > with this patch, and it didn't need API review. > Please see https://codereview.chromium.org/13776002/ Yes. I discussed this with Kent. It's ok to go without any process. Please just assert that this change just improves conformance to the standard in the CL description. Thanks
On 2013/04/17 08:32:23, tyoshino wrote: > On 2013/04/17 07:39:01, Li Yin wrote: > > On 2013/04/15 14:16:43, tyoshino wrote: > > > > > > LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html:4: > > > <script src="../../../../js-test-resources/js-test-pre.js"></script> > > > please remove ../../../.. > > > > I found all the tests in WebSocket used this style. But it seems there is not > > real directory js-test-resources existed, it was so strange. Is DRT doing some > > tricks? And the tests can't work in chromium browser correctly. > > See this > https://code.google.com/p/chromium/codesearch#search/&q=js-test-resources%252... > We have an alias. Thanks for your clarification. But I am still confused why we need do this. It seems this alias make things more difficult. The tests could not run directly in chromium browser.
On 2013/04/17 12:40:50, Li Yin wrote: > On 2013/04/17 08:32:23, tyoshino wrote: > > On 2013/04/17 07:39:01, Li Yin wrote: > > > On 2013/04/15 14:16:43, tyoshino wrote: > > > > > > > > > > LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html:4: > > > > <script src="../../../../js-test-resources/js-test-pre.js"></script> > > > > please remove ../../../.. > > > > > > I found all the tests in WebSocket used this style. But it seems there is > not > > > real directory js-test-resources existed, it was so strange. Is DRT doing > some > > > tricks? And the tests can't work in chromium browser correctly. > > > > See this > > > https://code.google.com/p/chromium/codesearch#search/&q=js-test-resources%252... > > We have an alias. > > Thanks for your clarification. But I am still confused why we need do this. It > seems this alias make things more difficult. The tests could not run directly in > chromium browser. Ah, ok. It's convenient if they're relative. We can launch ws server and use them directly for quick check. You can skip removing them.
https://codereview.chromium.org/14071008/diff/1/Source/WebCore/platform/netwo... File Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp (right): https://codereview.chromium.org/14071008/diff/1/Source/WebCore/platform/netwo... Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp:37: #include "NotImplemented.h" please insert SocketStreamError.h here. It look includes in this file are organized into .h for which this file contains corresponding implementation and the others (components they depend). https://codereview.chromium.org/14071008/diff/1/Source/WebCore/platform/netwo... Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp:159: m_handle->m_client->didFailSocketStream(m_handle, *(PassRefPtr<SocketStreamError>(err))); i'm not 100% sure if we can choose not to do the same as didClose (save m_handle, clear it and then call the did.* method). Could you please do that for now maybe with FIXME: just use m_handle? https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.cpp (left): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:334: InspectorInstrumentation::didReceiveWebSocketFrameError(m_document, m_identifier, message); removed as intended? https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:335: m_document->addConsoleMessage(NetworkMessageSource, ErrorMessageLevel, message); ditto https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.cpp (right): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:212: m_failed = true; Let's check m_client here. I know that it's currently called by the registered client and it'll never be NULL, but I think it's better not to connect these classes tightly using the knowledge. https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:217: m_handle->disconnect(); // Will call didClose(). Could you please s/didClose/didCloseSocketStream/ using this opportunity? https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:342: LOG(Network, "Error Message: %s, FailURL: %s", message.utf8().data(), failingURL.utf8().data()); Please - add "WebSocketChannel %p" boilerplate - quote %s ('%s') https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:344: if (m_client && !m_closing && !m_failed) { Sorry if i'm misunderstanding, but !m_closing is not necessary here? https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.h (right): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.h:195: bool m_failed; how about naming this m_didFailOfClientAlreadyRun?
On 2013/04/17 13:50:01, tyoshino wrote: > Ah, ok. It's convenient if they're relative. We can launch ws server and use > them directly for quick check. > > You can skip removing them. I was skipping looking into LayoutTests files in detail since one you take in the change I suggested for the other CL, these results need to be rebased, but I just went through them and understood that they're no-websocket-server case tests. Yes, especially when looking at these tests, removing ../../../.. makes no sense.
https://codereview.chromium.org/14071008/diff/1/LayoutTests/http/tests/websoc... File LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html (right): https://codereview.chromium.org/14071008/diff/1/LayoutTests/http/tests/websoc... LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html:1: <!DOCTYPE HTML> HTML -> html https://codereview.chromium.org/14071008/diff/1/LayoutTests/http/tests/websoc... LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html:20: var url = "ws://localhost:8888"; //here it should have no websocket server to listen. follow the Google's js style guide. "...; // Here ..." https://codereview.chromium.org/14071008/diff/1/LayoutTests/http/tests/websoc... LayoutTests/http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html:73: } nice test :) https://codereview.chromium.org/14071008/diff/1/LayoutTests/http/tests/websoc... File LayoutTests/http/tests/websocket/tests/hybi/workers/resources/connect-error-by-no-websocket-server.js (right): https://codereview.chromium.org/14071008/diff/1/LayoutTests/http/tests/websoc... LayoutTests/http/tests/websocket/tests/hybi/workers/resources/connect-error-by-no-websocket-server.js:32: var url = "ws://localhost:8888"; //here it should have no websocket server to listen. ; //here -> ; // Here
https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.cpp (left): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:335: m_document->addConsoleMessage(NetworkMessageSource, ErrorMessageLevel, message); On 2013/04/17 13:50:10, tyoshino wrote: > ditto Currently, the onerror event is still called very sensitively. And most of websocket tests will trigger this event. For example: When websocket server send closed frame, and then immediately abort the underlying tcp connection, but browser is still needed to send close frame. It will trigger onerror event. See close-code-and-reason.html What I want to say is that if we add console message, most of the websocket layout tests expectation will be modified. And maybe exposing these internal error information to users is not necessary. What is your thought? https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.cpp (right): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:217: m_handle->disconnect(); // Will call didClose(). On 2013/04/17 13:50:10, tyoshino wrote: > Could you please s/didClose/didCloseSocketStream/ using this opportunity? didClose will been invoked already after m_handle->disconnect(). Because m_handle->disconnect() will trigger underlying SocketStream close, and then the didClose callback will be invoked when underlying SocketStream was closed.
On 2013/04/18 02:50:05, Li Yin wrote: > https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... > File Source/modules/websockets/WebSocketChannel.cpp (left): > > https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... > Source/modules/websockets/WebSocketChannel.cpp:335: > m_document->addConsoleMessage(NetworkMessageSource, ErrorMessageLevel, message); > On 2013/04/17 13:50:10, tyoshino wrote: > > ditto > Currently, the onerror event is still called very sensitively. And most of > websocket tests will trigger this event. > For example: > When websocket server send closed frame, and then immediately abort the > underlying tcp connection, but browser is still needed to send close frame. It > will trigger onerror event. See close-code-and-reason.html > > What I want to say is that if we add console message, most of the websocket > layout tests expectation will be modified. > And maybe exposing these internal error information to users is not necessary. > > What is your thought? If add the console message, there are 65 sub websocket tests failed. All the expected text need the added message: CONSOLE MESSAGE: WebSocket network error: error code -3, net::ERR_ABORTED
https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.cpp (left): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:335: m_document->addConsoleMessage(NetworkMessageSource, ErrorMessageLevel, message); On 2013/04/18 02:50:05, Li Yin wrote: > On 2013/04/17 13:50:10, tyoshino wrote: > > ditto > Currently, the onerror event is still called very sensitively. And most of > websocket tests will trigger this event. > For example: > When websocket server send closed frame, and then immediately abort the > underlying tcp connection, but browser is still needed to send close frame. It > will trigger onerror event. See close-code-and-reason.html > > What I want to say is that if we add console message, most of the websocket > layout tests expectation will be modified. > And maybe exposing these internal error information to users is not necessary. > > What is your thought? OK. Please keep them removed. Let's postpone fixing that. https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.cpp (right): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:217: m_handle->disconnect(); // Will call didClose(). On 2013/04/18 02:50:05, Li Yin wrote: > On 2013/04/17 13:50:10, tyoshino wrote: > > Could you please s/didClose/didCloseSocketStream/ using this opportunity? > > didClose will been invoked already after m_handle->disconnect(). > Because m_handle->disconnect() will trigger underlying SocketStream close, and > then the didClose callback will be invoked when underlying SocketStream was > closed. Yes. But it's named didCloseSocketStream. Not didClose. Then, didCloseSocketStream calls m_client->didClose(). We're in WebSocketChannel. Not to confuse readers, we should clarify what this didClose() means. If it's m_client->didClose(), we should write so. We've renamed WebSocketChannel::didClose() to didCloseSocketStream() in this CL. I think didClose() in this comment meant WebSocketChannel::didClose(). But it wasn't updated. https://bugs.webkit.org/show_bug.cgi?id=65374 https://bugs.webkit.org/attachment.cgi?id=102482&action=review
On 2013/04/18 03:25:26, Li Yin wrote: > On 2013/04/18 02:50:05, Li Yin wrote: > > > https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... > > File Source/modules/websockets/WebSocketChannel.cpp (left): > > > > > https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... > > Source/modules/websockets/WebSocketChannel.cpp:335: > > m_document->addConsoleMessage(NetworkMessageSource, ErrorMessageLevel, > message); > > On 2013/04/17 13:50:10, tyoshino wrote: > > > ditto > > Currently, the onerror event is still called very sensitively. And most of > > websocket tests will trigger this event. > > For example: > > When websocket server send closed frame, and then immediately abort the > > underlying tcp connection, but browser is still needed to send close frame. It > > will trigger onerror event. See close-code-and-reason.html > > > > What I want to say is that if we add console message, most of the websocket > > layout tests expectation will be modified. > > And maybe exposing these internal error information to users is not necessary. > > > > What is your thought? > > If add the console message, there are 65 sub websocket tests failed. > All the expected text need the added message: > CONSOLE MESSAGE: WebSocket network error: error code -3, net::ERR_ABORTED OK. Thanks for the data. As I replied, please keep it.
On 2013/04/18 03:29:55, tyoshino wrote: > OK. Thanks for the data. As I replied, please keep it. I meant keep removed.
> > > > <script src="../../../../js-test-resources/js-test-pre.js"></script> > > > > please remove ../../../.. (snip) > Thanks for your clarification. But I am still confused why we need do this. It > seems this alias make things more difficult. The tests could not run directly in > chromium browser. Old tests used relative path for these files, but newly introduced tests use absolute path. So, I strongly +1 on tyoshino's suggestion. Running layout tests from file:// scheme is not supported basically. Please run a layout test though the server launched by new-run-webkit-httpd and new-run-webkit-websocket-server. See, also useragent-in-openinghandshake.html reviewed by ap@webkit.
I discussed with Takashi. You can leave it relative in this CL. Either way, we'll clean-up them later.
https://codereview.chromium.org/14071008/diff/1/Source/WebCore/platform/netwo... File Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp (right): https://codereview.chromium.org/14071008/diff/1/Source/WebCore/platform/netwo... Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp:159: m_handle->m_client->didFailSocketStream(m_handle, *(PassRefPtr<SocketStreamError>(err))); On 2013/04/17 13:50:10, tyoshino wrote: > i'm not 100% sure if we can choose not to do the same as didClose (save > m_handle, clear it and then call the did.* method). Could you please do that for > now maybe with FIXME: just use m_handle? didClose will be always invoked at the end. So clear m_handle should be done in the didClose function, but not in didFail. About "Could you please do that for now maybe with FIXME: just use m_handle?" Sorry, I can't catch your point. https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.cpp (right): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:217: m_handle->disconnect(); // Will call didClose(). Okay, thanks for your clarification. I will change the comments "Will call didClose" into "Will call didCloseSocketStream". https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:344: if (m_client && !m_closing && !m_failed) { On 2013/04/17 13:50:10, tyoshino wrote: > Sorry if i'm misunderstanding, but !m_closing is not necessary here? It is not necessary any more, because the issue is fixed on https://codereview.chromium.org/10668018 I will remove it.
https://codereview.chromium.org/14071008/diff/28002/LayoutTests/http/tests/we... File LayoutTests/http/tests/websocket/tests/hybi/nocache.html (left): https://codereview.chromium.org/14071008/diff/28002/LayoutTests/http/tests/we... LayoutTests/http/tests/websocket/tests/hybi/nocache.html:31: onerror event will be invoked, because WebSocket server aborts the underlying connection suddenly without waiting for receiving and sending closed frame. In order to let the test be passed, don't register onerror event handle.
https://codereview.chromium.org/14071008/diff/1/Source/WebCore/platform/netwo... File Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp (right): https://codereview.chromium.org/14071008/diff/1/Source/WebCore/platform/netwo... Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp:159: m_handle->m_client->didFailSocketStream(m_handle, *(PassRefPtr<SocketStreamError>(err))); On 2013/04/18 07:03:03, Li Yin wrote: > didClose will be always invoked at the end. So clear m_handle should be done in > the didClose function, but not in didFail. I was trying to remember why m_handle is set to 0 in didClose() before calling didCloseSocketStream(), but couldn't. Neither the review log nor change log have any discussion for this code. WebSocketChannel::didCloseSocketStream() resets WebSocketChannel::m_handle, and then, SocketStreamHandle loses reference counts, and SocketStreamHandleInternal owned by SocketStreamHandle also goes away. I believe L146-149 is related to this destruction chain, but not sure... Resetting SocketStreamHandleInternal::m_handle just prevents SocketStreamHandleInternal::did.*() methods to touch the SocketStreamHandle instance, but when SocketStreamHandle is dead, this class is also dead. So, touching this->m_handle is bad at all. We should rather prevent m_socket from touching this instance. WebSocketStreamHandle is also in the same boat... > About "Could you please do that for now maybe with FIXME: just use m_handle?" > Sorry, I can't catch your point. I meant to ask you to do the same as L146-149 here just to be safe. But I was wrong. Never mind. Now this method (didFail) is calling didFailSocketStream. Not didCloseSocketStream. didFailSocketStream doesn't reset WebSocketChannel::m_handle. SocketStreamHandleInternal::didClose must also get called later, so we shouldn't reset m_handle here. Again, please never mind this comment. Sorry. https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... File Source/modules/websockets/WebSocketChannel.cpp (right): https://codereview.chromium.org/14071008/diff/1/Source/modules/websockets/Web... Source/modules/websockets/WebSocketChannel.cpp:217: m_handle->disconnect(); // Will call didClose(). On 2013/04/18 07:03:03, Li Yin wrote: > Okay, thanks for your clarification. I will change the comments "Will call > didClose" into "Will call didCloseSocketStream". Yes. Thanks https://codereview.chromium.org/14071008/diff/28002/LayoutTests/http/tests/we... File LayoutTests/http/tests/websocket/tests/hybi/nocache.html (left): https://codereview.chromium.org/14071008/diff/28002/LayoutTests/http/tests/we... LayoutTests/http/tests/websocket/tests/hybi/nocache.html:31: On 2013/04/18 08:14:50, Li Yin wrote: > onerror event will be invoked, because WebSocket server aborts the underlying > connection suddenly without waiting for receiving and sending closed frame. > In order to let the test be passed, don't register onerror event handle. OK. We'll do readdition of onerror in bulk later.
On 2013/04/18 08:29:59, tyoshino wrote: > https://codereview.chromium.org/14071008/diff/1/Source/WebCore/platform/netwo... > Again, please never mind this comment. Sorry. It doesn't matter. And now do you think the patch is okay for landing?
On 2013/04/19 01:17:33, Li Yin wrote: > It doesn't matter. > And now do you think the patch is okay for landing? Yes. LGTM. Sorry but let me commit this after 10668018 as AUTHORS change is in that CL. Re: CLA, I've confirmed you're listed in our CLA signer list. If you want to commit this first, please include AUTHORS change in this CL and then I'll check commit flag.
Hi tkent, Could you please take a look at this CL again?
On 2013/04/17 08:34:34, tyoshino wrote: > On 2013/04/17 00:41:46, Li Yin wrote: > > On 2013/04/16 12:29:29, tkent wrote: > > > This is a trivial change [1], and I'm not sure if we need API review for it. > > IMO > > > we don't need. > > > Anyway, over-communication is good, and please ask a question about trivial > > > change process in blink-dev. > > > > > > [1] http://www.chromium.org/blink#trivial-changes > > > > From [1], Trivial platform changes do not need to meet the requirements above > > (API Review). They should be small fixes that have low risk of disrupting web > > developers. I think this patch should belong to this case. > > And some days ago, I committed a patch for MediaStream, the behavior was > similar > > with this patch, and it didn't need API review. > > Please see https://codereview.chromium.org/13776002/ > > Yes. I discussed this with Kent. It's ok to go without any process. Please just > assert that this change just improves conformance to the standard in the CL > description. Please make sure to do this, too. Thanks
On 2013/04/19 04:22:57, tyoshino wrote: > On 2013/04/19 01:17:33, Li Yin wrote: > > It doesn't matter. > > And now do you think the patch is okay for landing? > > Yes. LGTM. Sorry but let me commit this after 10668018 as AUTHORS change is in > that CL Please commit this patch before 10668018, because the patch from 10668018 is depended on this. WebSocketStreamError is needed to implement in Blink-side.
On 2013/04/19 04:24:32, tyoshino wrote: > > Yes. I discussed this with Kent. It's ok to go without any process. Please > just > > assert that this change just improves conformance to the standard in the CL > > description. > > Please make sure to do this, too. Thanks Yes, this patch fixed the issue when websocket server is not connected, chromium browser should trigger onerror event. And no behavior changes of WebSocket API. Thanks for your review.
On 2013/04/19 04:49:09, Li Yin wrote: > Please commit this patch before 10668018, because the patch from 10668018 is > depended on this. > WebSocketStreamError is needed to implement in Blink-side. Oh. OK. I forgot that Blink doesn't use AUTHORS file based copyright management. According to [1] and [2], the new files should have copy right notice. Could you please add one referring to 3-clause BSD license we're using in Chromium [3] but replacing "The Chromium Authors" with your affiliation (Intel Inc.) and // with /* */ or <!-- --> if necessary. [1] https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/blink-dev/... [2] http://www.webkit.org/coding/contributing.html [3] http://src.chromium.org/viewvc/chrome/trunk/src/LICENSE Thanks
On 2013/04/19 06:43:16, tyoshino wrote: > On 2013/04/19 04:49:09, Li Yin wrote: > > Please commit this patch before 10668018, because the patch from 10668018 is > > depended on this. > > WebSocketStreamError is needed to implement in Blink-side. > > Oh. OK. > > I forgot that Blink doesn't use AUTHORS file based copyright management. > > According to [1] and [2], the new files should have copy right notice. Could you > please add one referring to 3-clause BSD license we're using in Chromium [3] but > replacing "The Chromium Authors" with your affiliation (Intel Inc.) and // with > /* */ or <!-- --> if necessary. > > [1] > https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/blink-de... > [2] http://www.webkit.org/coding/contributing.html > [3] http://src.chromium.org/viewvc/chrome/trunk/src/LICENSE > > Thanks The copyright information has been changed based on newest License file. Thanks.
On 2013/04/19 07:25:25, Li Yin wrote: > The copyright information has been changed based on newest License file. Thanks. Thanks. Could you please add copyright info to .js and .html files you newly authored, too? I know that there was culture to omit it for those files in WebKit project, but I'm not fully sure if it can be applied to Blink too. Then, please wait for tkent's review.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/li.yin@intel.com/14071008/45001
Presubmit check for 14071008-45001 failed and returned exit status -2001. The presubmit check was hung. It took 360.0 seconds to execute and the time limit is 360.0 seconds. INFO:root:Found 15 file(s).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/li.yin@intel.com/14071008/45001
Presubmit check for 14071008-45001 failed and returned exit status -2001. The presubmit check was hung. It took 360.0 seconds to execute and the time limit is 360.0 seconds. INFO:root:Found 15 file(s).
On 2013/04/23 03:19:03, tkent wrote: > lgtm Thanks for your review. It seems commit-bot can't work now. Could you please help to commit it? Thanks in advance.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/li.yin@intel.com/14071008/45001
Presubmit check for 14071008-45001 failed and returned exit status -2001. The presubmit check was hung. It took 360.0 seconds to execute and the time limit is 360.0 seconds. INFO:root:Found 15 file(s).
Yes. will do
Let me try breaking this into WebSocketStreamError addition CL and CL for the rest. onerror will be invoked only when chromium side cl is also landed. Not to bother gardener (developer on duty for Blink rollout), I wanna land these layout tests with the change that brings this behavior change.
On 2013/04/23 07:31:00, tyoshino wrote: > Let me try breaking this into WebSocketStreamError addition CL and CL for the > rest. onerror will be invoked only when chromium side cl is also landed. Not to > bother gardener (developer on duty for Blink rollout), I wanna land these layout > tests with the change that brings this behavior change. Right. Let me add the changed layout tests into TestException, and will remove them till the chromium side patch is landed also. Thanks for your kind reminder.
On 2013/04/23 07:41:16, Li Yin wrote: > On 2013/04/23 07:31:00, tyoshino wrote: > > Let me try breaking this into WebSocketStreamError addition CL and CL for the > > rest. onerror will be invoked only when chromium side cl is also landed. Not > to > > bother gardener (developer on duty for Blink rollout), I wanna land these > layout > > tests with the change that brings this behavior change. > > Right. Let me add the changed layout tests into TestException, and will remove > them till the chromium side patch is landed also. > Thanks for your kind reminder. That's also fine. OK. Please do that.
On 2013/04/23 07:42:45, tyoshino wrote: > On 2013/04/23 07:41:16, Li Yin wrote: > > On 2013/04/23 07:31:00, tyoshino wrote: > > > Let me try breaking this into WebSocketStreamError addition CL and CL for > the > > > rest. onerror will be invoked only when chromium side cl is also landed. Not > > to > > > bother gardener (developer on duty for Blink rollout), I wanna land these > > layout > > > tests with the change that brings this behavior change. > > > > Right. Let me add the changed layout tests into TestException, and will remove > > them till the chromium side patch is landed also. > > Thanks for your kind reminder. > > That's also fine. OK. Please do that. Thanks. LGTM
Message was sent while issue was closed.
Committed patchset #6 manually as r148893 (presubmit successful). |
