Skip to content

inspector: update faviconUrl #29562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

inspector: update faviconUrl #29562

wants to merge 2 commits into from

Conversation

dokugo
Copy link
Contributor

@dokugo dokugo commented Sep 15, 2019 •
edited
Loading

Seeing no icon for the remote target at chrome://inspect/#devices when debugging via Chrome DevTools with --inspect switch. GET request for the icon is returning 404 status code because of incorrect request URL: https://nodejs.org/static/favicon.ico.
Changed faviconUrl at src/inspector_socket_server.cc to https://nodejs.org/static/images/favicons/favicon.ico. The reason for all this is icon files being moved in nodejs/nodejs.org@ce04045 commit.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Sep 15, 2019
@devnexen
Copy link
Contributor

Thanks for your contribution. Fine by me just little detail from lint
src/inspector_socket_server.cc:346: Lines should be <= 80 characters long [whitespace/line_length] [2]

@ZYSzys
Copy link
Member

ZYSzys commented Sep 15, 2019

Ping @dokugo , could you please fix the linter so that we can land it ?

@dokugo
Copy link
Contributor Author

dokugo commented Sep 15, 2019

Fixed the linter issue. I should have run make lint initially but it slipped off my mind.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25492/

@danbev danbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 4, 2019
danbev pushed a commit that referenced this pull request Oct 4, 2019
@dokugo @danbev
inspector: update faviconUrl
24d43ca 
PR-URL: #29562
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@danbev
Copy link
Contributor

danbev commented Oct 4, 2019

Landed in 24d43ca. 🎉

@danbev danbev closed this Oct 4, 2019
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
@dokugo @BridgeAR
inspector: update faviconUrl
b2cce13 
PR-URL: #29562
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
Merged
@dokugo dokugo deleted the update-favicon-url branch November 9, 2019 01:45
@dokugo dokugo restored the update-favicon-url branch November 9, 2019 01:45
@kissofire kissofire mentioned this pull request Feb 25, 2021
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Feb 25, 2021
Open
@ron-a20 ron-a20 mentioned this pull request Feb 26, 2021
Open
@sitedata sitedata mentioned this pull request Feb 26, 2021
Open
@meghasfdc meghasfdc mentioned this pull request Feb 26, 2021
Open
@137717unity 137717unity mentioned this pull request Feb 26, 2021
Open
@Rednickle Rednickle mentioned this pull request Feb 26, 2021
Open
This was referenced May 5, 2021
Open
Closed
Closed
Open
Open
@snyk-bot snyk-bot mentioned this pull request May 26, 2021
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Oct 30, 2022
Open
@137717unity 137717unity mentioned this pull request Oct 30, 2022
Open
@molllyn1 molllyn1 mentioned this pull request Oct 30, 2022
Open
@kissofire kissofire mentioned this pull request Oct 30, 2022
Open
@wedataintelligence wedataintelligence mentioned this pull request Oct 30, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Oct 31, 2022
Open
@ron-a20 ron-a20 mentioned this pull request Oct 31, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Oct 31, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Nov 10, 2022
Open
@kissofire kissofire mentioned this pull request Dec 6, 2023
Open
@137717unity 137717unity mentioned this pull request Dec 6, 2023
Open
@ron-a20 ron-a20 mentioned this pull request Dec 6, 2023
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Dec 6, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request Dec 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@ZYSzys ZYSzys ZYSzys approved these changes

+1 more reviewer

@devnexen devnexen devnexen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@dokugo @devnexen @ZYSzys @nodejs-github-bot @danbev @lpinca @cjihrig @gengjiawen @BridgeAR