Skip to content
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.

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

Add favicon #13054

Merged
merged 1 commit into from
Aug 24, 2015
Merged

Add favicon #13054

merged 1 commit into from
Aug 24, 2015

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Aug 22, 2015

Looks like this. Let the bikeshedding begin :)

favicon

void handleFavicon(HttpRequest request, HttpChannel channel) {
if (request.method() == RestRequest.Method.GET) {
try {
try (InputStream stream = getClass().getResourceAsStream("/config/favicon.ico")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we keep this in memory for performance reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dunno, it only impacts users using the browser. i figure that isnt perf critical, the server doesn't get hammered with those. maybe i'm wrong. file is 1KB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't come up with any more seriousness :)

@s1monw
Copy link
Contributor

s1monw commented Aug 22, 2015

LGTM I raised one little concern 🎱

@s1monw s1monw self-assigned this Aug 22, 2015
@tlrx
Copy link
Member

tlrx commented Aug 24, 2015

Nice... Looks like you were bored with unicast/multicast things :)

@jpountz
Copy link
Contributor

jpountz commented Aug 24, 2015

LGTM

s1monw added a commit that referenced this pull request Aug 24, 2015
@s1monw s1monw merged commit 707c6ca into elastic:master Aug 24, 2015
@s1monw s1monw deleted the favicon branch August 24, 2015 12:48
s1monw added a commit that referenced this pull request Aug 24, 2015
@clintongormley clintongormley added >enhancement :Core/Infra/REST API REST infrastructure and utilities and removed >non-issue labels Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants