Skip to content

Commit

Permalink
WebSocketHandshakeClient::OnConnectionEstablished signature change.
Browse files Browse the repository at this point in the history 
Chromium changes:

https://chromium.googlesource.com/chromium/src/+/26e73cfd90affbf1bdf3efbe80bf2903b0e866ae

commit 26e73cfd90affbf1bdf3efbe80bf2903b0e866ae
Author: Yutaka Hirano <[email protected]>
Date:   Tue Aug 6 02:38:13 2019 +0000

    Stop giving WebSocketClient to WebSocket creation function

    Instead, give it as an argument  of
    mojom.WebSocketHandshakeClient.OnConnectionEstablished.

    This is a follow up CL for
    https://chromium-review.googlesource.com/c/chromium/src/+/1728917.
    Passing the mojo::InterfaceRequest<WebSocketClient> at
    OnConnectionEstablished is less error prone because

     1) It is unable to bind the implementation before that, and
     2) It is now clear that we should detect connection errors on
        |handshake_client| until the connection is established, and
        |client| after that.

    Bug: 989406, 967524

https://chromium.googlesource.com/chromium/src/+/1007403a797957b8af46e962cdfc1f6f671954a4

commit 1007403a797957b8af46e962cdfc1f6f671954a4
Author: Yoichi Osato <[email protected]>
Date:   Thu Aug 8 13:11:57 2019 +0000

    [WebSocket] Use Datapipe to transfer DataFrame buffer instead of ReadOnlyBuffer

    This CL introduces mojo DataPipe instead of ReadOnlyBuffer.
    With DataPipe, we can reduce memory copy and optimize quota control.

    Unfortunately, this CL makes performance slower:
    ToT:   147 MB/s
    Patch: 128 MB/s
    That's because this CL replaces minimize part and we will do
    the optimization later on.
    If the optimization doesn't go over the original score before
    M78 (will be branch cut on Sep 5th), we will revert this change.

    Bug: 865001

https://chromium.googlesource.com/chromium/src/+/fcaa2a2c441e28a5cb95cc90946712efb0ce085f

commit fcaa2a2c441e28a5cb95cc90946712efb0ce085f
Author: Yoichi Osato <[email protected]>
Date:   Wed Aug 28 08:22:36 2019 +0000

    [WebSocket] Remove manual quota control for receiving.

    Because we're using mojo data pipe, which has own quota control,
    to transfer received data, we don't have manual one, or
    mojom::WebSocket.AddReceiveFlowControlQuota().
    This CL does a sort of work removing the function.

    mojom::WebSocket.AddReceiveFlowControlQuota(quota) did 5 tasks:
    browser side(mainly on WebSocketChannel)
     1. Trigger the first ReadFrame if renderer gets not throttled.
    For #1, this patch moves the trigger to mojo WebSocket.StartReceiving.

     2. Trigger next ReadFrame if there is enough quota.
     3. Send pending dataframes based on added quota.
    This patch moves task #2 and #3 to websocket.cc and datapipe itself.

     4. Dropchannel if all pending frames are received by renderer when closed.
    Because we can care #4 w/o renderer ping back, this patch move
    RespondToClosingHandshake()to WebSocketChannel::ReadFrames().

    renderer side(mainly on WebSocketChannelImpl)
     5. Ping browser that backpressure is turned off.
    For task #5, this patch changes not to call the mojo but throttle
    datapipe reading with WebSocketHandleImpl::ConsumePendingDataFrames().

    receive-arraybuffer-1MBx100.htmll?iteration=100 measurement on local build:
    ToT:            144 MB/s (stdev: 4.56 MB/s)
    Patch:          208 MB/s (stdev: 6.15 MB/s)
      (+44% to ToT, +41% to ReadOnlyBuffer)
    ReadOnlyBuffer: 147 MB/s

https://chromium.googlesource.com/chromium/src/+/a461132e64f53f45997aaa4222e05cdf5d10ea56

commit a461132e64f53f45997aaa4222e05cdf5d10ea56
Author: Julie Jeongeun Kim <[email protected]>
Date:   Fri Aug 30 04:13:36 2019 +0000

    Reland "Convert WebSocket to new Mojo types"

    This is a reland of 06a7200b370f38f39c97a4810d0931ab5596014e

    It updates WebSocketChannelImplTest::EstablishConnection
    with new Mojo types without InterfacePtr and MakeRequest.

    [email protected]

    Original change's description:
    > Convert WebSocket to new Mojo types
    >
    > This CL converts WebSocketPtr, to new Mojo types and
    > updates OnConnectionEstablished from websocket.mojom.
    >
    > Bug: 955171, 978694
    > Change-Id: I82416b209e2380241b64ebd3d829dd8bde5247c7
    > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1773016
    > Commit-Queue: Julie Kim <[email protected]>
    > Reviewed-by: John Abd-El-Malek <[email protected]>
    > Reviewed-by: Sam McNally <[email protected]>
    > Reviewed-by: Yutaka Hirano <[email protected]>
    > Reviewed-by: Ken Rockot <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#691574}

    Bug: 955171, 978694
  • Loading branch information
@mkarolin
mkarolin committed Sep 25, 2019
1 parent 4f29f48 commit e81c749
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 9 deletions.
9 changes: 5 additions & 4 deletions browser/net/brave_proxying_web_socket.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,15 +204,16 @@ void BraveProxyingWebSocket::ContinueToHeadersReceived() {
}

void BraveProxyingWebSocket::OnConnectionEstablished(
network::mojom::WebSocketPtr websocket,
mojo::PendingRemote<network::mojom::WebSocket> websocket,
mojo::PendingReceiver<network::mojom::WebSocketClient> client_receiver,
const std::string& selected_protocol,
const std::string& extensions,
uint64_t receive_quota_threshold) {
mojo::ScopedDataPipeConsumerHandle readable) {
DCHECK(forwarding_handshake_client_);
DCHECK(!is_done_);
forwarding_handshake_client_->OnConnectionEstablished(
std::move(websocket), selected_protocol, extensions,
receive_quota_threshold);
std::move(websocket), std::move(client_receiver), selected_protocol,
extensions, std::move(readable));

OnError(net::ERR_FAILED);
}
Expand Down
12 changes: 7 additions & 5 deletions browser/net/brave_proxying_web_socket.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
#include "brave/browser/net/resource_context_data.h"
#include "brave/browser/net/url_context.h"
#include "content/public/browser/content_browser_client.h"
#include "mojo/public/cpp/bindings/binding.h"
#include "mojo/public/cpp/bindings/binding_set.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "services/network/public/cpp/resource_request.h"
#include "services/network/public/cpp/resource_response.h"
#include "services/network/public/mojom/network_context.mojom.h"
Expand Down Expand Up @@ -73,10 +73,12 @@ class BraveProxyingWebSocket : public network::mojom::WebSocketHandshakeClient,
network::mojom::WebSocketHandshakeRequestPtr request) override;
void OnResponseReceived(
network::mojom::WebSocketHandshakeResponsePtr response) override;
void OnConnectionEstablished(network::mojom::WebSocketPtr websocket,
const std::string& selected_protocol,
const std::string& extensions,
uint64_t receive_quota_threshold) override;
void OnConnectionEstablished(
mojo::PendingRemote<network::mojom::WebSocket> websocket,
mojo::PendingReceiver<network::mojom::WebSocketClient> client_receiver,
const std::string& selected_protocol,
const std::string& extensions,
mojo::ScopedDataPipeConsumerHandle readable) override;

// network::mojom::AuthenticationHandler method:
void OnAuthRequired(const net::AuthChallengeInfo& auth_info,
Expand Down

0 comments on commit e81c749

Please sign in to comment.