Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(275)

Issue 871013007: Use enum BinaryType for WebSocket.binaryType, instead of DOMString (Closed)

Created:
5 years, 10 months ago by Paritosh Kumar
Modified:
5 years, 8 months ago
CC:
blink-reviews, yhirano+watch_chromium.org, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

This addresses a FIXME in WebSocket.idl Also removing logging for wrong value as now we are logging in binding code: https://codereview.chromium.org/955413002/. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193201

Patch Set 1 #

Patch Set 2 : #

Total comments: 2

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 8

Patch Set 6 : #

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -14 lines) Patch
M LayoutTests/http/tests/websocket/binary-type-expected.txt View 1 2 3 4 5 6 1 chunk +3 lines, -3 lines 0 comments Download
M Source/modules/websockets/DOMWebSocket.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/websockets/DOMWebSocketTest.cpp View 1 2 3 4 5 1 chunk +13 lines, -8 lines 0 comments Download
M Source/modules/websockets/WebSocket.idl View 1 2 2 chunks +1 line, -2 lines 0 comments Download

Messages

Total messages: 72 (18 generated)
Paritosh Kumar
Please have a look.
5 years, 10 months ago (2015-01-28 13:00:55 UTC) #2
Habib Virji
On 2015/01/28 13:00:55, Paritosh Kumar wrote: > Please have a look. Since DOMWebSocket.h defines returns ...
5 years, 10 months ago (2015-01-28 13:11:50 UTC) #3
Paritosh Kumar
On 2015/01/28 13:11:50, Habib Virji wrote: > On 2015/01/28 13:00:55, Paritosh Kumar wrote: > > ...
5 years, 10 months ago (2015-01-28 13:37:11 UTC) #4
Habib Virji
On 2015/01/28 13:37:11, Paritosh Kumar wrote: > On 2015/01/28 13:11:50, Habib Virji wrote: > > ...
5 years, 10 months ago (2015-01-28 14:06:27 UTC) #5
Paritosh Kumar
Thanks Habib Adding nbarth and yutak for more clarification. @nbarth, yutak: PTAL.
5 years, 10 months ago (2015-01-29 06:41:05 UTC) #7
Yuta Kitamura
I'm not aware of the context behind this FIXME; tyoshino may know something.
5 years, 10 months ago (2015-01-29 09:17:04 UTC) #9
tyoshino (SeeGerritForStatus)
On 2015/01/29 09:17:04, Yuta Kitamura wrote: > I'm not aware of the context behind this ...
5 years, 10 months ago (2015-01-29 12:18:12 UTC) #10
Nils Barth (inactive)
- haraken@: What's our feeling on logging invalid enumeration values? (Current status is silently ignore ...
5 years, 10 months ago (2015-02-02 23:02:28 UTC) #12
haraken
Thanks Nils! > - haraken@: What's our feeling on logging invalid enumeration values? > (Current ...
5 years, 10 months ago (2015-02-02 23:36:54 UTC) #13
Paritosh Kumar
Thanks to all Apologize for delay in update. @Nils: >I believe you should also update: ...
5 years, 10 months ago (2015-02-11 18:29:25 UTC) #14
Nils Barth (inactive)
Thanks for update Paritosh! I think CL is fine, so long as you move the ...
5 years, 10 months ago (2015-02-11 18:58:34 UTC) #15
Nils Barth (inactive)
https://codereview.chromium.org/871013007/diff/20001/Source/bindings/templates/attributes.cpp File Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/871013007/diff/20001/Source/bindings/templates/attributes.cpp#newcode266 Source/bindings/templates/attributes.cpp:266: currentExecutionContext(info.GetIsolate())->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, "The provided value '"+string+"' is not a ...
5 years, 10 months ago (2015-02-11 18:58:45 UTC) #16
jsbell
Awesome to see this. I'm tackling another FIXME in WebSocket.idl over in: https://codereview.chromium.org/954933003/ These hopefully ...
5 years, 10 months ago (2015-02-25 18:56:45 UTC) #18
Paritosh Kumar
Many Apologize for too much delay in update. I uploaded a new patch for logging ...
5 years, 10 months ago (2015-02-26 11:32:27 UTC) #19
Paritosh Kumar
As now we are logging in binding code for invalid enum type, as https://codereview.chromium.org/955413002/ does ...
5 years, 8 months ago (2015-03-30 08:02:32 UTC) #20
tyoshino (SeeGerritForStatus)
lgtm Thanks!
5 years, 8 months ago (2015-03-30 08:12:02 UTC) #21
tyoshino (SeeGerritForStatus)
Please update the description to explain what you did than saying "some FIXME is addressed" ...
5 years, 8 months ago (2015-03-30 08:13:01 UTC) #22
Paritosh Kumar
On 2015/03/30 08:13:01, tyoshino wrote: > Please update the description to explain what you did ...
5 years, 8 months ago (2015-03-30 08:33:12 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871013007/40001
5 years, 8 months ago (2015-03-30 08:46:14 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/54827)
5 years, 8 months ago (2015-03-30 09:29:29 UTC) #27
Nils Barth (inactive)
On 2015/03/30 08:33:12, Paritosh Kumar wrote: > Thanks. Updated the description. In future, could you ...
5 years, 8 months ago (2015-03-30 12:36:03 UTC) #28
Paritosh Kumar
Thanks Nils > In future, could you also make sure the *Title* is descriptive? Sure. ...
5 years, 8 months ago (2015-03-30 13:10:10 UTC) #29
Nils Barth (inactive)
On 2015/03/30 13:10:10, Paritosh Kumar wrote: > As we are using ASSERT_NOT_REACHED() in DOMWebSocket::binaryType() > ...
5 years, 8 months ago (2015-03-30 14:12:27 UTC) #30
Nils Barth (inactive)
On 2015/03/30 13:10:10, Paritosh Kumar wrote: > Actually, This test doesn't hit the ASSERT in ...
5 years, 8 months ago (2015-03-30 14:13:19 UTC) #31
Paritosh Kumar
Thanks Nils > Thus, could you replace the EXPECT_EQ with EXPECT_FATAL_FAILURE, as: > EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("hoge"), ""); ...
5 years, 8 months ago (2015-03-31 06:51:57 UTC) #32
Nils Barth (inactive)
On 2015/03/31 06:51:57, Paritosh Kumar wrote: > > Thus, could you replace the EXPECT_EQ with ...
5 years, 8 months ago (2015-03-31 13:54:26 UTC) #33
Paritosh Kumar
Thanks Nils > Could you try using EXPECT_DEATH instead? > https://code.google.com/p/googletest/wiki/AdvancedGuide > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests As ASSERT_* ...
5 years, 8 months ago (2015-04-01 07:06:01 UTC) #34
Nils Barth (inactive)
On 2015/04/01 07:06:01, Paritosh Kumar wrote: > Thanks Nils > > > Could you try ...
5 years, 8 months ago (2015-04-01 14:23:11 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871013007/60001
5 years, 8 months ago (2015-04-01 14:25:57 UTC) #38
Paritosh Kumar
On 2015/04/01 14:23:11, Nils Barth (inactive) wrote: > On 2015/04/01 07:06:01, Paritosh Kumar wrote: > ...
5 years, 8 months ago (2015-04-01 14:26:11 UTC) #39
Paritosh Kumar
On 2015/04/01 14:26:11, Paritosh Kumar wrote: > On 2015/04/01 14:23:11, Nils Barth (inactive) wrote: > ...
5 years, 8 months ago (2015-04-01 15:02:33 UTC) #41
Nils Barth (inactive)
On 2015/04/01 15:02:33, Paritosh Kumar wrote: > On 2015/04/01 14:26:11, Paritosh Kumar wrote: > > ...
5 years, 8 months ago (2015-04-01 15:34:50 UTC) #42
Paritosh Kumar
On 2015/04/01 15:34:50, Nils Barth (inactive) wrote: > On 2015/04/01 15:02:33, Paritosh Kumar wrote: > ...
5 years, 8 months ago (2015-04-02 13:09:40 UTC) #43
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871013007/80001
5 years, 8 months ago (2015-04-02 13:21:01 UTC) #46
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871013007/80001
5 years, 8 months ago (2015-04-02 13:30:30 UTC) #47
Nils Barth (inactive)
Thanks, still LGTM (with nits). https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp File Source/modules/websockets/DOMWebSocketTest.cpp (right): https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode668 Source/modules/websockets/DOMWebSocketTest.cpp:668: BTW, could you remove ...
5 years, 8 months ago (2015-04-02 14:56:36 UTC) #49
Paritosh Kumar
Thanks. As we can see, the result of cq-dry-run is failing test http/tests/websocket/binary-type.html I didn't ...
5 years, 8 months ago (2015-04-02 15:41:32 UTC) #50
Nils Barth (inactive)
https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp File Source/modules/websockets/DOMWebSocketTest.cpp (right): https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode668 Source/modules/websockets/DOMWebSocketTest.cpp:668: On 2015/04/02 15:41:32, Paritosh Kumar wrote: > On 2015/04/02 ...
5 years, 8 months ago (2015-04-02 15:54:56 UTC) #51
Paritosh Kumar
Thanks Nils Addressed your comments. Please have a look. https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp File Source/modules/websockets/DOMWebSocketTest.cpp (right): https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode668 Source/modules/websockets/DOMWebSocketTest.cpp:668: ...
5 years, 8 months ago (2015-04-02 16:14:04 UTC) #52
Nils Barth (inactive)
On 2015/04/02 15:41:32, Paritosh Kumar wrote: > Thanks. > As we can see, the result ...
5 years, 8 months ago (2015-04-02 16:20:39 UTC) #53
Paritosh Kumar
On 2015/04/02 16:20:39, Nils Barth (inactive) wrote: > On 2015/04/02 15:41:32, Paritosh Kumar wrote: > ...
5 years, 8 months ago (2015-04-02 17:04:18 UTC) #54
Nils Barth (inactive)
On 2015/04/02 17:04:18, Paritosh Kumar wrote: > On 2015/04/02 16:20:39, Nils Barth (inactive) wrote: > ...
5 years, 8 months ago (2015-04-02 17:32:09 UTC) #55
jsbell
I believe this is now failing due to https://codereview.chromium.org/1047993002 Paritosh: if you sync to the ...
5 years, 8 months ago (2015-04-02 17:38:36 UTC) #57
jsbell
FWIW, I suggest that Paritosh updated the -expected.txt file in this CL to match the ...
5 years, 8 months ago (2015-04-02 17:46:09 UTC) #58
Nils Barth (inactive)
Thanks for details Joshua! Paritosh, per suggestion, could you sync, update, and land this CL, ...
5 years, 8 months ago (2015-04-02 17:58:56 UTC) #59
bashi
On 2015/04/02 17:38:36, jsbell wrote: > I believe this is now failing due to https://codereview.chromium.org/1047993002 ...
5 years, 8 months ago (2015-04-03 01:35:10 UTC) #60
Nils Barth (inactive)
On 2015/04/02 17:38:36, jsbell wrote: > > bashi@ - can we get the nice errors ...
5 years, 8 months ago (2015-04-03 14:41:46 UTC) #61
Paritosh Kumar
On 2015/04/03 14:41:46, Nils Barth (inactive) wrote: > On 2015/04/02 17:38:36, jsbell wrote: > > ...
5 years, 8 months ago (2015-04-03 16:45:22 UTC) #62
Paritosh Kumar
On 2015/04/03 16:45:22, Paritosh Kumar wrote: > On 2015/04/03 14:41:46, Nils Barth (inactive) wrote: > ...
5 years, 8 months ago (2015-04-04 11:27:47 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871013007/120001
5 years, 8 months ago (2015-04-04 11:28:19 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/871013007/120001
5 years, 8 months ago (2015-04-06 17:12:06 UTC) #69
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193201
5 years, 8 months ago (2015-04-06 18:25:45 UTC) #70
jsbell
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1053013006/ by jsbell@chromium.org. ...
5 years, 8 months ago (2015-04-06 19:10:36 UTC) #71
Nils Barth (inactive)
5 years, 8 months ago (2015-04-06 19:16:00 UTC) #72
Message was sent while issue was closed.
Thanks Joshua!
Looks like we can't use death tests.
Paritosh, could you remove the death test and resubmit?
Thanks!

Powered by Google App Engine
This is Rietveld 408576698