Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(661)

Unified Diff: third_party/WebKit/Source/core/loader/resource/ImageResource.cpp

Issue 2650113002: Phase II Step 2: Remove setIsPlaceholder() in updateImage() (Closed)
Patch Set: Rebase. Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
diff --git a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp b/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
index fb554447b44ecd9020267d7dc38acf96e78a8f87..9029a454746656aba5192e4d459f54a8f1333219 100644
--- a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
+++ b/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
@@ -23,6 +23,8 @@
#include "core/loader/resource/ImageResource.h"
+#include <stdint.h>
+#include <v8.h>
#include <memory>
#include "core/loader/resource/ImageResourceContent.h"
@@ -36,6 +38,7 @@
#include "platform/loader/fetch/ResourceFetcher.h"
#include "platform/loader/fetch/ResourceLoader.h"
#include "platform/loader/fetch/ResourceLoadingLog.h"
+#include "platform/network/HTTPParsers.h"
#include "platform/weborigin/SecurityViolationReportingPolicy.h"
#include "public/platform/Platform.h"
#include "v8/include/v8.h"
@@ -79,7 +82,9 @@ class ImageResource::ImageResourceInfoImpl final
return m_resource->response();
}
ResourceStatus getStatus() const override { return m_resource->getStatus(); }
- bool isPlaceholder() const override { return m_resource->isPlaceholder(); }
+ bool shouldShowPlaceholder() const override {
+ return m_resource->shouldShowPlaceholder();
+ }
bool isCacheValidator() const override {
return m_resource->isCacheValidator();
}
@@ -101,9 +106,6 @@ class ImageResource::ImageResourceInfoImpl final
return m_resource->resourceError();
}
- void setIsPlaceholder(bool isPlaceholder) override {
- m_resource->m_isPlaceholder = isPlaceholder;
- }
void setDecodedSize(size_t size) override {
m_resource->setDecodedSize(size);
}
@@ -173,7 +175,7 @@ ImageResource* ImageResource::fetch(FetchRequest& request,
fetcher->requestResource(request, ImageResourceFactory(request)));
if (resource &&
request.placeholderImageRequestType() != FetchRequest::AllowPlaceholder &&
- resource->m_isPlaceholder) {
+ resource->shouldShowPlaceholder()) {
// If the image is a placeholder, but this fetch doesn't allow a
// placeholder, then load the original image. Note that the cache is not
// bypassed here - it should be fine to use a cached copy if possible.
@@ -197,7 +199,9 @@ ImageResource::ImageResource(const ResourceRequest& resourceRequest,
m_devicePixelRatioHeaderValue(1.0),
m_hasDevicePixelRatioHeaderValue(false),
m_isSchedulingReload(false),
- m_isPlaceholder(isPlaceholder),
+ m_placeholderOption(
+ isPlaceholder ? PlaceholderOption::ShowAndReloadPlaceholderAlways
+ : PlaceholderOption::DoNotReloadPlaceholder),
m_flushTimer(this, &ImageResource::flushImageIfNeeded) {
DCHECK(getContent());
RESOURCE_LOADING_DVLOG(1) << "new ImageResource(ResourceRequest) " << this;
@@ -320,10 +324,6 @@ void ImageResource::flushImageIfNeeded(TimerBase*) {
}
}
-bool ImageResource::willPaintBrokenImage() const {
- return errorOccurred();
-}
-
void ImageResource::decodeError(bool allDataReceived) {
size_t size = encodedSize();
@@ -380,6 +380,21 @@ void ImageResource::error(const ResourceError& error) {
true);
}
+// Determines if |response| likely contains the entire resource for the purposes
+// of determining whether or not to show a placeholder, e.g. if the server
+// responded with a full 200 response or if the full image is smaller than the
+// requested range.
+static bool isEntireResource(const ResourceResponse& response) {
+ if (response.httpStatusCode() != 206)
+ return true;
+
+ int64_t firstBytePosition = -1, lastBytePosition = -1, instanceLength = -1;
+ return parseContentRangeHeaderFor206(
+ response.httpHeaderField("Content-Range"), &firstBytePosition,
+ &lastBytePosition, &instanceLength) &&
+ firstBytePosition == 0 && lastBytePosition + 1 == instanceLength;
+}
+
void ImageResource::responseReceived(
const ResourceResponse& response,
std::unique_ptr<WebDataConsumerHandle> handle) {
@@ -402,6 +417,47 @@ void ImageResource::responseReceived(
m_hasDevicePixelRatioHeaderValue = false;
}
}
+
+ if (m_placeholderOption ==
+ PlaceholderOption::ShowAndReloadPlaceholderAlways &&
+ isEntireResource(this->response())) {
+ if (this->response().httpStatusCode() < 400 ||
+ this->response().httpStatusCode() >= 600) {
+ // Don't treat a complete and broken image as a placeholder if the
+ // response code is something other than a 4xx or 5xx error.
+ // This is done to prevent reissuing the request in cases like
+ // "204 No Content" responses to tracking requests triggered by <img>
+ // tags, and <img> tags used to preload non-image resources.
+ m_placeholderOption = PlaceholderOption::DoNotReloadPlaceholder;
+ } else {
+ m_placeholderOption = PlaceholderOption::ReloadPlaceholderOnDecodeError;
+ }
+ }
+}
+
+bool ImageResource::shouldShowPlaceholder() const {
+ switch (m_placeholderOption) {
+ case PlaceholderOption::ShowAndReloadPlaceholderAlways:
+ return true;
+ case PlaceholderOption::ReloadPlaceholderOnDecodeError:
+ case PlaceholderOption::DoNotReloadPlaceholder:
+ return false;
+ }
+ NOTREACHED();
+ return false;
+}
+
+bool ImageResource::shouldReloadBrokenPlaceholder() const {
+ switch (m_placeholderOption) {
+ case PlaceholderOption::ShowAndReloadPlaceholderAlways:
+ return errorOccurred();
+ case PlaceholderOption::ReloadPlaceholderOnDecodeError:
+ return getStatus() == ResourceStatus::DecodeError;
+ case PlaceholderOption::DoNotReloadPlaceholder:
+ return false;
+ }
+ NOTREACHED();
+ return false;
}
static bool isLoFiImage(const ImageResource& resource) {
@@ -421,7 +477,8 @@ void ImageResource::reloadIfLoFiOrPlaceholderImage(
if (policy == kReloadIfNeeded && !shouldReloadBrokenPlaceholder())
return;
- if (!m_isPlaceholder && !isLoFiImage(*this))
+ if (m_placeholderOption == PlaceholderOption::DoNotReloadPlaceholder &&
+ !isLoFiImage(*this))
return;
// Prevent clients and observers from being notified of completion while the
@@ -436,10 +493,9 @@ void ImageResource::reloadIfLoFiOrPlaceholderImage(
setPreviewsStateNoTransform();
- if (m_isPlaceholder) {
- m_isPlaceholder = false;
+ if (m_placeholderOption != PlaceholderOption::DoNotReloadPlaceholder)
clearRangeRequestHeader();
- }
+ m_placeholderOption = PlaceholderOption::DoNotReloadPlaceholder;
if (isLoading()) {
loader()->cancel();

Powered by Google App Engine
This is Rietveld 408576698