Bug 170827 - Enough with these favicon.ico errors
Summary: Enough with these favicon.ico errors
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-13 14:40 PDT by Dean Jackson
Modified: 2017-05-12 12:54 PDT (History)
12 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (18.70 KB, patch)
2017-04-13 20:55 PDT, Joseph Pecoraro
achristensen: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (63.79 MB, application/zip)
2017-04-14 18:35 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2017-04-13 14:40:16 PDT
I'm sick of seeing these errors in the console:
Failed to load resource: the server responded with a status of 404 (Not Found)   https://something/favicon.ico

Especially when the content of the page didn't request it, but the browser did.

At best this could be a warning, maybe with better text? "The browser was unable to find a favicon for this site."
Comment 1 Radar WebKit Bug Importer 2017-04-13 18:57:02 PDT
<rdar://problem/31620816>
Comment 2 Joseph Pecoraro 2017-04-13 19:07:08 PDT
Steps to Reproduce:
1. $ run-safari --no-saved-state
2. Open and inspect about:blank
3. Drag and drop a local simple.html onto Safari
  => Error in console about favicon.ico 404
Comment 3 Joseph Pecoraro 2017-04-13 20:55:17 PDT
Created attachment 307086 [details]
[PATCH] Proposed Fix

I still want to get rid of the file:///favicon.ico but my brain continues to think "maybe this could be legit".

Maybe we should just prevent against this single file URL path, since its so unlikely to be real?
Comment 4 Build Bot 2017-04-13 20:56:56 PDT
Attachment 307086 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:155:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Sam Weinig 2017-04-13 21:40:10 PDT
Given that you are inspecting what the browser is doing, and it is making a request to your server, it seems like removing it altogether wouldn't be ideal (in fact, there are other requests the browser is making to your server, for other icons, that it would be nice if the inspector showed in some way).
Comment 6 Sam Weinig 2017-04-13 21:42:10 PDT
(In reply to Sam Weinig from comment #5)
> Given that you are inspecting what the browser is doing, and it is making a
> request to your server, it seems like removing it altogether wouldn't be
> ideal (in fact, there are other requests the browser is making to your
> server, for other icons, that it would be nice if the inspector showed in
> some way).

Perhaps just indicating a "soft failure", would be more appropriate?
Comment 7 Joseph Pecoraro 2017-04-14 14:07:59 PDT
(In reply to Sam Weinig from comment #6)
> Perhaps just indicating a "soft failure", would be more appropriate?

This patch converted the console message from an error to a warnings.

The "setHiddenFromInspector(true)" part of the patch hides the resource from the Network/Resources tab. I think its weird that it shows up there when it is not a resource requested by the page, I can add it back.

I see the Web Inspector as page centric, not browser centric. You can have two separate pages being inspected, and they don't show each others traffic. That said, I would see the benefit of showing some actions the browser takes on behalf of the page. In this case if the page explicitly set a favicon path it would make sense to show details about the browser's request for the icon.
Comment 8 Build Bot 2017-04-14 18:35:28 PDT
Comment on attachment 307086 [details]
[PATCH] Proposed Fix

Attachment 307086 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3536683

New failing tests:
webrtc/multi-video.html
Comment 9 Build Bot 2017-04-14 18:35:31 PDT
Created attachment 307178 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 10 Alex Christensen 2017-04-17 14:18:14 PDT
Comment on attachment 307086 [details]
[PATCH] Proposed Fix

This patch feels bad.  We're passing a loader around that is mostly nullptr except one place.  The layering feels wrong but I'm not sure how to describe what would be better.  This is a very specific case we want to ignore, but we seem to be ignoring errors from ALL icon loads.  Also, there are no tests.