Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2195113002:
DevTools: improve UX for hosted mode + fix bug (Closed)
|
Created:
4 years, 4 months ago by chenwilliam Modified:
4 years, 4 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: improve UX for hosted mode + fix bug
This CL includes several quick improvements for hosted mode:
- Properly serves InspectorBackendCommands.js. It turns out the metadata from
the file is necessary for certain operations (e.g. $0 in the console).
- Can now customize remote debugging port in addition to server port
(e.g. REMOTE_DEBUGGING_PORT=9333 PORT=1234 npm run server)
- Provides helpful info if you go to server root path: (e.g. http://localhost:8090/).
As Paul pointed out, users may go here to check if the server is working.
“Please go to http://localhost:9222#http://localhost:8090/front_end/inspector.html?experiments=true”
(Note: it accounts for custom ports passed in)
- Add chrome favicon (same favicon as https://developer.chrome.com/devtools)
I'll update the docs with the RDP info once this CL has landed.
BUG=629914
Committed: https://crrev.com/c453b7819ecc95fad800c61c31c8c2db31839725
Cr-Commit-Position: refs/heads/master@{#409878}
Patch Set 1 #Patch Set 2 : better favicon.ico implementation #Patch Set 3 : nit: formatting #
Total comments: 7
Patch Set 4 : Address CL feedback #
Total comments: 5
Patch Set 5 : minor fixes / formating #
Messages
Total messages: 19 (6 generated)
Description was changed from ========== DevTools: improve UX for hosted mode + fix bug - add chrome favicon to inspector.html BUG=629914 ========== to ========== DevTools: improve UX for hosted mode + fix bug This CL includes several quick improvements for hosted mode: - Properly serves InspectorBackendCommands.js. It turns out the metadata from the file is necessary for certain operations (e.g. $0 in the console). - Can now customize remote debugging port in addition to server port (e.g. REMOTE_DEBUGGING_PORT=9333 PORT=1234 npm run server) - Provides helpful info if you go to server root path: (e.g. http://localhost:8090/). As Paul pointed out, users may go here to check if the server is working. “Please go to http://localhost:9222#http://localhost:8090/front_end/inspector.html?experime... (Note: it accounts for custom ports passed in) - Add chrome favicon (same favicon as https://developer.chrome.com/devtools) I'll update the docs with the RDP info once this CL has landed. BUG=629914 ==========
chenwilliam@chromium.org changed reviewers: + lushnikov@chromium.org, paulirish@chromium.org
https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js (right): https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:42: if (filePath === "/favicon.ico") isn't there a favicon in the cloud already? Let's not add the resource and fetch it instead https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:75: "/front_end/sdk/protocol/js_protocol.json": getWebKitURL.bind(null, "platform/v8_inspector/js_protocol.json"), let's come up with some descriptive name instead of getWebkitURL and getFrontendURL, like "googlesourceURL" and "cloudURL"
https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js (right): https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:78: "/front_end/InspectorBackendCommands.js": getFrontendURL.bind(null, "InspectorBackendCommands.js") one more question: why do we need to serve protocols altogether, if you serve InspectorBackendCommands?
https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js (right): https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:42: if (filePath === "/favicon.ico") On 2016/08/02 at 17:17:15, lushnikov wrote: > isn't there a favicon in the cloud already? Let's not add the resource and fetch it instead https://chrome-devtools-frontend.appspot.com/favicon.ico though it might be good to update that to be https://github.com/ChromeDevTools/devtools-logo
The good news is we don't need to serve the protocol json files for hosted mode anymore. Talking with dgozman, we can remove InspectorBackendHostedMode.js. I would like to do it in a later CL because there's a couple of tests that will need to be updated or removed (LayoutTests/inspector/inspector-backend-commands-generation.html and LayoutTests/inspector/inspector-backend-commands.html). https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js (right): https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:42: if (filePath === "/favicon.ico") On 2016/08/03 18:26:14, paulirish wrote: > On 2016/08/02 at 17:17:15, lushnikov wrote: > > isn't there a favicon in the cloud already? Let's not add the resource and > fetch it instead > > https://chrome-devtools-frontend.appspot.com/favicon.ico > > though it might be good to update that to be > https://github.com/ChromeDevTools/devtools-logo Done. Discussed with Paul and we agreed the Chrome logo is a better favicon (Chrome DevTools logo doesn't look distinctive at a small size). https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:75: "/front_end/sdk/protocol/js_protocol.json": getWebKitURL.bind(null, "platform/v8_inspector/js_protocol.json"), On 2016/08/02 17:17:15, lushnikov wrote: > let's come up with some descriptive name instead of > getWebkitURL and getFrontendURL, like "googlesourceURL" and "cloudURL" Done. https://codereview.chromium.org/2195113002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:78: "/front_end/InspectorBackendCommands.js": getFrontendURL.bind(null, "InspectorBackendCommands.js") On 2016/08/02 17:26:26, lushnikov wrote: > one more question: why do we need to serve protocols altogether, if you serve > InspectorBackendCommands? After discussing with dgozman, we don't need to serve the protocols for hosted mode now that we can serve InspectorBackendCommands.js. https://codereview.chromium.org/2195113002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js (right): https://codereview.chromium.org/2195113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:137: var body = new Stream(); Using a stream to properly fetch images (e.g. favicon).
lgtm https://codereview.chromium.org/2195113002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js (right): https://codereview.chromium.org/2195113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:14: var entryLink = `http://localhost:${remoteDebuggingPort}#http://localhost:${serverPort}/front_end/inspector.html?experiments=true`; let's put this where it's used, no need to make it global https://codereview.chromium.org/2195113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:98: var proxyFileURL= proxyFilePathToURL[filePath](commitHash); nit: space's missing
https://codereview.chromium.org/2195113002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js (right): https://codereview.chromium.org/2195113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:14: var entryLink = `http://localhost:${remoteDebuggingPort}#http://localhost:${serverPort}/front_end/inspector.html?experiments=true`; On 2016/08/04 01:17:27, lushnikov wrote: > let's put this where it's used, no need to make it global Done. https://codereview.chromium.org/2195113002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/scripts/hosted_mode/server.js:98: var proxyFileURL= proxyFilePathToURL[filePath](commitHash); On 2016/08/04 01:17:27, lushnikov wrote: > nit: space's missing Done.
The CQ bit was checked by chenwilliam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2195113002/#ps80001 (title: "minor fixes / formating")
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.
Description was changed from ========== DevTools: improve UX for hosted mode + fix bug This CL includes several quick improvements for hosted mode: - Properly serves InspectorBackendCommands.js. It turns out the metadata from the file is necessary for certain operations (e.g. $0 in the console). - Can now customize remote debugging port in addition to server port (e.g. REMOTE_DEBUGGING_PORT=9333 PORT=1234 npm run server) - Provides helpful info if you go to server root path: (e.g. http://localhost:8090/). As Paul pointed out, users may go here to check if the server is working. “Please go to http://localhost:9222#http://localhost:8090/front_end/inspector.html?experime... (Note: it accounts for custom ports passed in) - Add chrome favicon (same favicon as https://developer.chrome.com/devtools) I'll update the docs with the RDP info once this CL has landed. BUG=629914 ========== to ========== DevTools: improve UX for hosted mode + fix bug This CL includes several quick improvements for hosted mode: - Properly serves InspectorBackendCommands.js. It turns out the metadata from the file is necessary for certain operations (e.g. $0 in the console). - Can now customize remote debugging port in addition to server port (e.g. REMOTE_DEBUGGING_PORT=9333 PORT=1234 npm run server) - Provides helpful info if you go to server root path: (e.g. http://localhost:8090/). As Paul pointed out, users may go here to check if the server is working. “Please go to http://localhost:9222#http://localhost:8090/front_end/inspector.html?experime... (Note: it accounts for custom ports passed in) - Add chrome favicon (same favicon as https://developer.chrome.com/devtools) I'll update the docs with the RDP info once this CL has landed. BUG=629914 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: improve UX for hosted mode + fix bug This CL includes several quick improvements for hosted mode: - Properly serves InspectorBackendCommands.js. It turns out the metadata from the file is necessary for certain operations (e.g. $0 in the console). - Can now customize remote debugging port in addition to server port (e.g. REMOTE_DEBUGGING_PORT=9333 PORT=1234 npm run server) - Provides helpful info if you go to server root path: (e.g. http://localhost:8090/). As Paul pointed out, users may go here to check if the server is working. “Please go to http://localhost:9222#http://localhost:8090/front_end/inspector.html?experime... (Note: it accounts for custom ports passed in) - Add chrome favicon (same favicon as https://developer.chrome.com/devtools) I'll update the docs with the RDP info once this CL has landed. BUG=629914 ========== to ========== DevTools: improve UX for hosted mode + fix bug This CL includes several quick improvements for hosted mode: - Properly serves InspectorBackendCommands.js. It turns out the metadata from the file is necessary for certain operations (e.g. $0 in the console). - Can now customize remote debugging port in addition to server port (e.g. REMOTE_DEBUGGING_PORT=9333 PORT=1234 npm run server) - Provides helpful info if you go to server root path: (e.g. http://localhost:8090/). As Paul pointed out, users may go here to check if the server is working. “Please go to http://localhost:9222#http://localhost:8090/front_end/inspector.html?experime... (Note: it accounts for custom ports passed in) - Add chrome favicon (same favicon as https://developer.chrome.com/devtools) I'll update the docs with the RDP info once this CL has landed. BUG=629914 Committed: https://crrev.com/c453b7819ecc95fad800c61c31c8c2db31839725 Cr-Commit-Position: refs/heads/master@{#409878} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c453b7819ecc95fad800c61c31c8c2db31839725 Cr-Commit-Position: refs/heads/master@{#409878}
Message was sent while issue was closed.
On 2016/08/04 19:59:50, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/c453b7819ecc95fad800c61c31c8c2db31839725 > Cr-Commit-Position: refs/heads/master@{#409878} Updated the docs with the RDP info as suggestions: bit.ly/devtools-contribution-guide
Message was sent while issue was closed.
This is Chromium code, but you are using a Chrome favicon... Is this intentional?
Message was sent while issue was closed.
On 2016/08/05 at 19:21:59, phistuck wrote: > This is Chromium code, but you are using a Chrome favicon... Is this intentional? We're using an icon for clear distinction of tabs. We chose to reuse the icon that's on the appspot (which is used for remote debugging hosted frontends). Your concern is noted. We can discuss with folks about the icon hosted there. If you want to help make a favicon from https://github.com/chromedevtools/devtools-logo that looks decent, we can try to get it placed there. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
