Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2455833002:
Pass task_runner_ to server and send 404 when incorrect url (Closed)
|
Created:
4 years, 1 month ago by Sarmad Hashmi Modified:
4 years, 1 month ago CC:
chromium-reviews, kalyank, sadrul, pfeldman, devtools-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPass task_runner_ to server and send 404 when incorrect url
A few small changes for the ui_devtools server:
- Don't create a new thread and just pass a task_runner_.
- Send 404 when invalid url is requested from the server.
- Fix for weird error where HttpServer calls OnClose with a connection
id that never even connected in the first place. Just do nothing if
the connection id doesn't exist in our list of connections connected to
a UI client.
BUG=648701
Committed: https://crrev.com/6011c1bd16ebebe75c97cdfed9849b8d14197a6e
Cr-Commit-Position: refs/heads/master@{#428713}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Make a default create method (without task_runner) #
Total comments: 2
Patch Set 3 : sadruls comments #
Total comments: 4
Patch Set 4 : sadruls comments #
Total comments: 2
Patch Set 5 : sadruls comments #
Messages
Total messages: 30 (17 generated)
mhashmi@chromium.org changed reviewers: + sadrul@chromium.org
Just some minor changes to the ui devtools server.
The ui_devtools changes look reasonable. But are you sure the ash change is OK? Because I expect the httpserver should really run in a separate thread? https://codereview.chromium.org/2455833002/diff/1/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2455833002/diff/1/ash/common/wm_shell.cc#newc... ash/common/wm_shell.cc:94: base::ThreadTaskRunnerHandle::Get()); Are sure the current thread (i.e. the UI thread) is the right thread to run the server on? https://codereview.chromium.org/2455833002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2455833002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_server.cc:153: return; Hm, any idea why this happens? https://codereview.chromium.org/2455833002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2455833002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_server.h:23: UiDevToolsServer(scoped_refptr<base::SingleThreadTaskRunner> task_runner); explicit
https://codereview.chromium.org/2455833002/diff/1/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2455833002/diff/1/ash/common/wm_shell.cc#newc... ash/common/wm_shell.cc:94: base::ThreadTaskRunnerHandle::Get()); On 2016/10/28 02:32:27, sadrul wrote: > Are sure the current thread (i.e. the UI thread) is the right thread to run the > server on? Changed. https://codereview.chromium.org/2455833002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2455833002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_server.cc:153: return; On 2016/10/28 02:32:27, sadrul wrote: > Hm, any idea why this happens? So, I realized we need this either way because if you the HttpServer calls OnClose after an OnHttpRequest, the connection_id being closed wouldn't have inspected any client in the first place so we wouldn't do anything with it. https://codereview.chromium.org/2455833002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2455833002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_server.h:23: UiDevToolsServer(scoped_refptr<base::SingleThreadTaskRunner> task_runner); On 2016/10/28 02:32:27, sadrul wrote: > explicit Done.
https://codereview.chromium.org/2455833002/diff/20001/components/ui_devtools/... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2455833002/diff/20001/components/ui_devtools/... components/ui_devtools/devtools_server.h:28: scoped_refptr<base::SingleThreadTaskRunner> task_runner); I would remove the default version (i.e. without param). If the param is null, then a new thread is created.
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2455833002/diff/20001/components/ui_devtools/... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2455833002/diff/20001/components/ui_devtools/... components/ui_devtools/devtools_server.h:28: scoped_refptr<base::SingleThreadTaskRunner> task_runner); On 2016/10/29 05:39:51, sadrul wrote: > I would remove the default version (i.e. without param). If the param is null, > then a new thread is created. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2455833002/diff/40001/components/ui_devtools/... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2455833002/diff/40001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:41: DCHECK(task_runner_); You can remove the default ctor, and optionally create |thread_| and set |task_runner_| if task_runner is null. https://codereview.chromium.org/2455833002/diff/40001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:84: std::unique_ptr<net::ServerSocket> socket( Early return if |server_| is already set. (or if this isn't expected to be called more than once, DCHECK(!server_))
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2455833002/diff/40001/components/ui_devtools/... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2455833002/diff/40001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:41: DCHECK(task_runner_); On 2016/10/29 17:14:59, sadrul wrote: > You can remove the default ctor, and optionally create |thread_| and set > |task_runner_| if task_runner is null. Done. https://codereview.chromium.org/2455833002/diff/40001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:84: std::unique_ptr<net::ServerSocket> socket( On 2016/10/29 17:14:59, sadrul wrote: > Early return if |server_| is already set. (or if this isn't expected to be > called more than once, DCHECK(!server_)) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2455833002/diff/60001/components/ui_devtools/... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2455833002/diff/60001/components/ui_devtools/... components/ui_devtools/devtools_server.h:33: UiDevToolsServer(); Remove this?
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mhashmi@chromium.org changed reviewers: + sky@chromium.org
+sky@ for owners review https://codereview.chromium.org/2455833002/diff/60001/components/ui_devtools/... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2455833002/diff/60001/components/ui_devtools/... components/ui_devtools/devtools_server.h:33: UiDevToolsServer(); On 2016/10/30 18:03:35, sadrul wrote: > Remove this? Done.
ash LGTM
The CQ bit was checked by mhashmi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2455833002/#ps80001 (title: "sadruls comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Pass task_runner_ to server and send 404 when incorrect url A few small changes for the ui_devtools server: - Don't create a new thread and just pass a task_runner_. - Send 404 when invalid url is requested from the server. - Fix for weird error where HttpServer calls OnClose with a connection id that never even connected in the first place. Just do nothing if the connection id doesn't exist in our list of connections connected to a UI client. BUG=648701 ========== to ========== Pass task_runner_ to server and send 404 when incorrect url A few small changes for the ui_devtools server: - Don't create a new thread and just pass a task_runner_. - Send 404 when invalid url is requested from the server. - Fix for weird error where HttpServer calls OnClose with a connection id that never even connected in the first place. Just do nothing if the connection id doesn't exist in our list of connections connected to a UI client. BUG=648701 Committed: https://crrev.com/6011c1bd16ebebe75c97cdfed9849b8d14197a6e Cr-Commit-Position: refs/heads/master@{#428713} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6011c1bd16ebebe75c97cdfed9849b8d14197a6e Cr-Commit-Position: refs/heads/master@{#428713} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
