Web Inspector: 404 Image Load does not appear as a failure in Web Inspector
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 May 2017 20:36:27 +0000 (20:36 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 May 2017 20:36:27 +0000 (20:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=171587
<rdar://problem/13222846>

Patch by Joseph Pecoraro <pecoraro@apple.com> on 2017-05-03
Reviewed by Matt Baker.

Source/WebCore:

* inspector/InspectorPageAgent.h:
* inspector/InspectorPageAgent.cpp:
(WebCore::InspectorPageAgent::cachedResourceContent):
(WebCore::prepareCachedResourceBuffer): Deleted.
Inline the function to make this less confusing.

(WebCore::InspectorPageAgent::buildObjectForFrameTree):
Treat a DecodeError as a failure.

Source/WebInspectorUI:

* UserInterface/Models/Resource.js:
(WebInspector.Resource.prototype.createObjectURL):
This may return null if the data is not a Blob. This can happen if we
loaded non-base64Encoded text content for an Image. Such as a 404 response.

(WebInspector.Resource.prototype.hadLoadingError):
Consistent way to check for any kind of loading error.

(WebInspector.Resource.prototype.getImageSize):
Handle failure to create an object URL.

* UserInterface/Views/FontResourceContentView.js:
(WebInspector.FontResourceContentView.prototype.contentAvailable):
* UserInterface/Views/ImageResourceContentView.js:
(WebInspector.ImageResourceContentView.prototype.contentAvailable):
Handle failure to create an object URL.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@216138 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/inspector/InspectorPageAgent.cpp
Source/WebCore/inspector/InspectorPageAgent.h
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Models/Resource.js
Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js
Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js
Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js
Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js
Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js

index f66232f..6f87600 100644 (file)
@@ -1,3 +1,20 @@
+2017-05-03  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: 404 Image Load does not appear as a failure in Web Inspector
+        https://bugs.webkit.org/show_bug.cgi?id=171587
+        <rdar://problem/13222846>
+
+        Reviewed by Matt Baker.
+
+        * inspector/InspectorPageAgent.h:
+        * inspector/InspectorPageAgent.cpp:
+        (WebCore::InspectorPageAgent::cachedResourceContent):
+        (WebCore::prepareCachedResourceBuffer): Deleted.
+        Inline the function to make this less confusing.
+
+        (WebCore::InspectorPageAgent::buildObjectForFrameTree):
+        Treat a DecodeError as a failure.
+
 2017-05-03  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         Async image decoding should be disabled for snapshots, printing and preview
index 6aa6a08..5bca31e 100644 (file)
@@ -97,21 +97,6 @@ static bool decodeBuffer(const char* buffer, unsigned size, const String& textEn
     return false;
 }
 
-static bool prepareCachedResourceBuffer(CachedResource* cachedResource, bool* hasZeroSize)
-{
-    *hasZeroSize = false;
-    if (!cachedResource)
-        return false;
-
-    // Zero-sized resources don't have data at all -- so fake the empty buffer, instead of indicating error by returning 0.
-    if (!cachedResource->encodedSize()) {
-        *hasZeroSize = true;
-        return true;
-    }
-
-    return true;
-}
-
 static bool hasTextContent(CachedResource* cachedResource)
 {
     // FIXME: <https://webkit.org/b/165495> Web Inspector: XHR / Fetch for non-text content should not show garbled text
@@ -127,18 +112,16 @@ static bool hasTextContent(CachedResource* cachedResource)
 
 bool InspectorPageAgent::cachedResourceContent(CachedResource* cachedResource, String* result, bool* base64Encoded)
 {
-    // FIXME: result should be a String& and base64Encoded should be a bool&.
-    bool hasZeroSize;
-    bool prepared = prepareCachedResourceBuffer(cachedResource, &hasZeroSize);
-    if (!prepared)
+    if (!cachedResource)
         return false;
 
+    if (!cachedResource->encodedSize()) {
+        *result = String();
+        return true;
+    }
+
     *base64Encoded = !hasTextContent(cachedResource);
     if (*base64Encoded) {
-        if (hasZeroSize) {
-            *result = { };
-            return true;
-        }
         if (auto* buffer = cachedResource->resourceBuffer()) {
             *result = base64Encode(buffer->data(), buffer->size());
             return true;
@@ -146,11 +129,6 @@ bool InspectorPageAgent::cachedResourceContent(CachedResource* cachedResource, S
         return false;
     }
 
-    if (hasZeroSize) {
-        *result = emptyString();
-        return true;
-    }
-
     if (cachedResource) {
         switch (cachedResource->type()) {
         case CachedResource::CSSStyleSheet:
@@ -904,7 +882,7 @@ Ref<Inspector::Protocol::Page::FrameResourceTree> InspectorPageAgent::buildObjec
             .release();
         if (cachedResource->wasCanceled())
             resourceObject->setCanceled(true);
-        else if (cachedResource->status() == CachedResource::LoadError)
+        else if (cachedResource->status() == CachedResource::LoadError || cachedResource->status() == CachedResource::DecodeError)
             resourceObject->setFailed(true);
         String sourceMappingURL = InspectorPageAgent::sourceMapURLForResource(cachedResource);
         if (!sourceMappingURL.isEmpty())
index 2883181..939e70c 100644 (file)
@@ -74,7 +74,7 @@ public:
         XHRResource,
         FetchResource,
         WebSocketResource,
-        OtherResource
+        OtherResource,
     };
 
     static bool cachedResourceContent(CachedResource*, String* result, bool* base64Encoded);
index 123edf7..52fc2c1 100644 (file)
@@ -1,3 +1,40 @@
+2017-05-03  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: 404 Image Load does not appear as a failure in Web Inspector
+        https://bugs.webkit.org/show_bug.cgi?id=171587
+        <rdar://problem/13222846>
+
+        Reviewed by Matt Baker.
+
+        * UserInterface/Models/Resource.js:
+        (WebInspector.Resource.prototype.createObjectURL):
+        This may return null if the data is not a Blob. This can happen if we
+        loaded non-base64Encoded text content for an Image. Such as a 404 response.
+
+        (WebInspector.Resource.prototype.hadLoadingError):
+        Consistent way to check for any kind of loading error.
+
+        (WebInspector.Resource.prototype.getImageSize):
+        Handle failure to create an object URL.
+
+        * UserInterface/Views/FontResourceContentView.js:
+        (WebInspector.FontResourceContentView.prototype.contentAvailable):
+        * UserInterface/Views/ImageResourceContentView.js:
+        (WebInspector.ImageResourceContentView.prototype.contentAvailable):
+        Handle failure to create an object URL.
+:
+        * UserInterface/Views/ResourceContentView.js:
+        (WebInspector.ResourceContentView):
+        (WebInspector.ResourceContentView.prototype.showGenericErrorMessage):
+        (WebInspector.ResourceContentView.prototype._protocolError): Deleted.
+        Provide a way for subclasses to show a generic resource loading error.
+
+        * UserInterface/Views/ResourceTimelineDataGridNode.js:
+        (WebInspector.ResourceTimelineDataGridNode.prototype.createCellContent):
+        * UserInterface/Views/ResourceTreeElement.js:
+        (WebInspector.ResourceTreeElement.prototype._updateStatus):
+        Use the consistent helper for denoting loading errors.
+
 2017-05-02  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         [GTK] Web Inspector: Remove GTK+ icon FontVariantSmallCaps.svg
index 22aa88b..46e3222 100644 (file)
@@ -347,13 +347,14 @@ WebInspector.Resource = class Resource extends WebInspector.SourceCode
     {
         // If content is not available, fallback to using original URL.
         // The client may try to revoke it, but nothing will happen.
-        if (!this.content)
+        let content = this.content;
+        if (!content)
             return this._url;
 
-        var content = this.content;
-        console.assert(content instanceof Blob, content);
+        if (content instanceof Blob)
+            return URL.createObjectURL(content);
 
-        return URL.createObjectURL(content);
+        return null;
     }
 
     isMainResource()
@@ -831,6 +832,11 @@ WebInspector.Resource = class Resource extends WebInspector.SourceCode
         this.markAsCached();
     }
 
+    hadLoadingError()
+    {
+        return this._failed || this._canceled || this._statusCode >= 400;
+    }
+
     getImageSize(callback)
     {
         // Throw an error in the case this resource is not an image.
@@ -870,8 +876,10 @@ WebInspector.Resource = class Resource extends WebInspector.SourceCode
         image.addEventListener("load", imageDidLoad.bind(this), false);
 
         // Set the image source using an object URL once we've obtained its data.
-        this.requestContent().then(function(content) {
+        this.requestContent().then((content) => {
             objectURL = image.src = content.sourceCode.createObjectURL();
+            if (!objectURL)
+                requestContentFailure.call(this);
         }, requestContentFailure.bind(this));
     }
 
index 310c22d..77405f8 100644 (file)
@@ -55,7 +55,11 @@ WebInspector.FontResourceContentView = class FontResourceContentView extends Web
 
     contentAvailable(content, base64Encoded)
     {
-        this.element.removeChildren();
+        this._fontObjectURL = this.resource.createObjectURL();
+        if (!this._fontObjectURL) {
+            this.showGenericErrorMessage();
+            return;
+        }
 
         const uniqueFontName = "WebInspectorFontPreview" + (++WebInspector.FontResourceContentView._uniqueFontIdentifier);
 
@@ -68,7 +72,7 @@ WebInspector.FontResourceContentView = class FontResourceContentView extends Web
         if (this._styleElement && this._styleElement.parentNode)
             this._styleElement.parentNode.removeChild(this._styleElement);
 
-        this._fontObjectURL = this.resource.createObjectURL();
+        this.element.removeChildren();
 
         this._styleElement = document.createElement("style");
         this._styleElement.textContent = "@font-face { font-family: \"" + uniqueFontName + "\"; src: url(" + this._fontObjectURL + ")" + format + "; }";
index 54fb13a..d86f6b1 100644 (file)
@@ -41,9 +41,14 @@ WebInspector.ImageResourceContentView = class ImageResourceContentView extends W
 
     contentAvailable(content, base64Encoded)
     {
+        let objectURL = this.resource.createObjectURL();
+        if (!objectURL) {
+            this.showGenericErrorMessage();
+            return;
+        }
+
         this.element.removeChildren();
 
-        var objectURL = this.resource.createObjectURL();
         this._imageElement = document.createElement("img");
         this._imageElement.addEventListener("load", function() { URL.revokeObjectURL(objectURL); });
         this._imageElement.src = objectURL;
index f413675..5d96576 100644 (file)
@@ -44,7 +44,7 @@ WebInspector.ResourceContentView = class ResourceContentView extends WebInspecto
         this.element.addEventListener("click", this._mouseWasClicked.bind(this), false);
 
         // Request content last so the spinner will always be removed in case the content is immediately available.
-        resource.requestContent().then(this._contentAvailable.bind(this)).catch(this._protocolError.bind(this));
+        resource.requestContent().then(this._contentAvailable.bind(this)).catch(this.showGenericErrorMessage.bind(this));
 
         if (!this.managesOwnIssues) {
             WebInspector.issueManager.addEventListener(WebInspector.IssueManager.Event.IssueWasAdded, this._issueWasAdded, this);
@@ -77,6 +77,11 @@ WebInspector.ResourceContentView = class ResourceContentView extends WebInspecto
         // Implemented by subclasses.
     }
 
+    showGenericErrorMessage()
+    {
+        this._contentError(WebInspector.UIString("An error occurred trying to load the resource."));
+    }
+
     addIssue(issue)
     {
         // This generically shows only the last issue, subclasses can override for better handling.
@@ -118,11 +123,6 @@ WebInspector.ResourceContentView = class ResourceContentView extends WebInspecto
         this.dispatchEventToListeners(WebInspector.ResourceContentView.Event.ContentError);
     }
 
-    _protocolError(error)
-    {
-        this._contentError(WebInspector.UIString("An error occurred trying to load the resource."));
-    }
-
     _hasContent()
     {
         return !this.element.querySelector(".indeterminate-progress-spinner");
index 6cefd92..e1db19f 100644 (file)
@@ -96,7 +96,7 @@ WebInspector.ResourceTimelineDataGridNode = class ResourceTimelineDataGridNode e
     {
         let resource = this._resource;
 
-        if (resource.failed || resource.canceled || resource.statusCode >= 400)
+        if (resource.hadLoadingError())
             cell.classList.add("error");
 
         let value = this.data[columnIdentifier];
index 54e1742..4f334de 100644 (file)
@@ -170,7 +170,7 @@ WebInspector.ResourceTreeElement = class ResourceTreeElement extends WebInspecto
 
     _updateStatus()
     {
-        if (this._resource.failed)
+        if (this._resource.hadLoadingError())
             this.addClassName(WebInspector.ResourceTreeElement.FailedStyleClassName);
         else
             this.removeClassName(WebInspector.ResourceTreeElement.FailedStyleClassName);
@@ -179,7 +179,7 @@ WebInspector.ResourceTreeElement = class ResourceTreeElement extends WebInspecto
             // Remove the spinner.
             this.status = "";
         } else {
-            var spinner = new WebInspector.IndeterminateProgressSpinner;
+            let spinner = new WebInspector.IndeterminateProgressSpinner;
             this.status = spinner.element;
         }
     }