Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2114213002:
Fix loading placeholders for Lo-Fi (Closed)
|
Created:
4 years, 5 months ago by megjablon Modified:
4 years, 5 months ago CC:
blink-reviews, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, gavinp+loader_chromium.org, jam, Nate Chapin, loading-reviews+fetch_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, tyoshino+watch_chromium.org, Yoav Weiss Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix loading placeholders for Lo-Fi
WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove
Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest.
This allows us to change the state in WebKit where we cannot update extra
data and pass along on RequestInfo, rather than update extra data later.
Synchronously update the Lo-Fi state in RenderFrameImpl so new frame are
not created with the old state during the asynchronous frame message.
Also, clear the image resource, notify observers, and force a reload when
the original request had Lo-Fi on. If the image was already loaded, make
sure the image had a Lo-Fi response before reloading.
BUG=620203
Committed: https://crrev.com/7c7c3a25861b8e0a27ef7a9a0115f1ec9ea772b5
Cr-Commit-Position: refs/heads/master@{#405353}
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressing comment and fixing frame issue #Patch Set 3 : patch 2124453002 #Patch Set 4 : update test #
Total comments: 7
Patch Set 5 : whitespace fix and rebase #
Total comments: 3
Patch Set 6 : remove frame race fix #Patch Set 7 : git cl try #Patch Set 8 : rebase #
Messages
Total messages: 53 (26 generated)
Description was changed from ========== Fix loading images for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. Also, force reloading using an error in ImageResource::reloadIfLoFi. BUG=620203 ========== to ========== Fix loading images for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. Also, force reloading using an error in ImageResource::reloadIfLoFi. BUG=620203 ==========
megjablon@chromium.org changed reviewers: + yhirano@chromium.org
megjablon@chromium.org changed reviewers: + hiroshige@chromium.org - yhirano@chromium.org
megjablon@chromium.org changed reviewers: + creis@chromium.org
creis: content/* hiroshige: ImageResource.cpp
Description was changed from ========== Fix loading images for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. Also, force reloading using an error in ImageResource::reloadIfLoFi. BUG=620203 ========== to ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. Also, force reloading using an error in ImageResource::reloadIfLoFi. BUG=620203 ==========
Description was changed from ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. Also, force reloading using an error in ImageResource::reloadIfLoFi. BUG=620203 ========== to ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. This allows us to change the state in WebKit where we cannot update extra data and pass along on RequestInfo, rather than update extra data later. Also, force reloading using an error in ImageResource::reloadIfLoFi. BUG=620203 ==========
Patchset #1 (id:1) has been deleted
Thanks for fixing! Could you add a test for this issue? I created https://codereview.chromium.org/2124453002/ for unit tests for Blink-side change (but probably it doesn't cover the content/-side change of this CL). https://codereview.chromium.org/2114213002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2114213002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.cpp:520: error(ResourceError::cancelledError(url())); Instead of calling error(), how about clear(); m_data.clear(); (i.e. not calling |updateImage(true)|), or clear(); m_data.clear(); notifyObservers(); (See Patch Set 1 of https://chromiumcodereview.appspot.com/1928823002/#ps1)? IIUC, the image is not update because, in updateImageAndClearBuffer(), updateImage(true) is called after clearImage(), and here |m_image| is non-null and points to the old image, and |m_image| is not re-created or updated after reloadIfLoFi(). If so, not calling |updateImage(true)| might resolve the problem. I'd like to avoid using error() here, because calling error() after finish() looks strange, and doing what is needed for reloading explicitly here would be clearer than depending on error() (that does many things).
creis@chromium.org changed reviewers: + clamy@chromium.org, mkwst@chromium.org
[+clamy, mkwst] Hmm, I'm not a great reviewer for these content/child changes, since I'm not sure what the implications are. Nasko's out, but maybe clamy@ would know from her work on PlzNavigate? Also adding mkwst@ in case he knows and wants to take a first look.
Patchset #2 (id:40001) has been deleted
Merged in hiroshige's test cl. Looking into how to write content tests for this. I don't see a good place to write them yet. Do the content reviewers have any recommendations? Also can I add the test in a follow up cl? Lo-Fi is currently broken in M52 without this so I need to get it merged ASAP. https://codereview.chromium.org/2114213002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2114213002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.cpp:520: error(ResourceError::cancelledError(url())); On 2016/07/04 12:02:29, hiroshige wrote: > Instead of calling error(), how about > clear(); > m_data.clear(); > (i.e. not calling |updateImage(true)|), or > clear(); > m_data.clear(); > notifyObservers(); > (See Patch Set 1 of https://chromiumcodereview.appspot.com/1928823002/#ps1)? > > IIUC, the image is not update because, in updateImageAndClearBuffer(), > updateImage(true) is called after clearImage(), and here |m_image| is non-null > and points to the old image, and |m_image| is not re-created or updated after > reloadIfLoFi(). > If so, not calling |updateImage(true)| might resolve the problem. > > I'd like to avoid using error() here, because calling error() after finish() > looks strange, and doing what is needed for reloading explicitly here would be > clearer than depending on error() (that does many things). Thanks! Was looking for another way that would load the images properly, but was confused why updateImageAndClearBuffer didn't work.
Description was changed from ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. This allows us to change the state in WebKit where we cannot update extra data and pass along on RequestInfo, rather than update extra data later. Also, force reloading using an error in ImageResource::reloadIfLoFi. BUG=620203 ========== to ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. This allows us to change the state in WebKit where we cannot update extra data and pass along on RequestInfo, rather than update extra data later. Synchronously update the Lo-Fi state in RenderFrameImpl so new frame are not created with the old state during the asynchronous frame message. Also, clear the image resource, notify observers, and force a reload when the original request had Lo-Fi on. If the image was already loaded, make sure the image had a Lo-Fi response before reloading. BUG=620203 ==========
Blink-side non-owner lgtm. https://codereview.chromium.org/2114213002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2114213002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ImageResource.cpp:514: if (m_resourceRequest.loFiState() != WebURLRequest::LoFiOn) nit: two whitespaces after |!=|.
FYI the changes to ImageResourceTest are not mergeable (at least simply), because it uses |jpegImage2| that is added in M-54.
Thanks! A few questions below. https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2564: SendToAllFrames(new FrameMsg_DisableLoFiImages(MSG_ROUTING_NONE)); I'm not sure this works with PlzNavigate enabled. Do you also want to disable LoFi for any ongoing navigation in the WebContents? In this case, with PlzNavigate enabled there's probably some extra-work to do. https://codereview.chromium.org/2114213002/diff/100001/content/child/request_... File content/child/request_extra_data.h (left): https://codereview.chromium.org/2114213002/diff/100001/content/child/request_... content/child/request_extra_data.h:101: LoFiState lofi_state() const { Just checking if I understand correctly: currently we have information about LoFi state both in ResourceRequest and RequestExtra data?
https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2564: SendToAllFrames(new FrameMsg_DisableLoFiImages(MSG_ROUTING_NONE)); On 2016/07/07 15:16:30, clamy wrote: > I'm not sure this works with PlzNavigate enabled. Do you also want to disable > LoFi for any ongoing navigation in the WebContents? In this case, with > PlzNavigate enabled there's probably some extra-work to do. Will didCommitProvisionalLoad be called later for the main frame with PlzNavigate? That's the only place we update the main frame's Lo-Fi state from an external value. Otherwise, every subframe inherits the parent frame's state and the only way we can change that is through the DisableLoFiImages call. I tried to test with --enable-browser-side-navigation, but origin/master has a crash with the flag: resource_prefetch_common.cc(176)] Check failed: is_valid() && rhs.is_valid(). https://codereview.chromium.org/2114213002/diff/100001/content/child/request_... File content/child/request_extra_data.h (left): https://codereview.chromium.org/2114213002/diff/100001/content/child/request_... content/child/request_extra_data.h:101: LoFiState lofi_state() const { On 2016/07/07 15:16:30, clamy wrote: > Just checking if I understand correctly: currently we have information about > LoFi state both in ResourceRequest and RequestExtra data? Yep. Which is why they got out of sync. https://codereview.chromium.org/2114213002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2114213002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ImageResource.cpp:514: if (m_resourceRequest.loFiState() != WebURLRequest::LoFiOn) On 2016/07/07 05:21:58, hiroshige wrote: > nit: two whitespaces after |!=|. Done.
https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2564: SendToAllFrames(new FrameMsg_DisableLoFiImages(MSG_ROUTING_NONE)); On 2016/07/07 19:31:52, megjablon wrote: > On 2016/07/07 15:16:30, clamy wrote: > > I'm not sure this works with PlzNavigate enabled. Do you also want to disable > > LoFi for any ongoing navigation in the WebContents? In this case, with > > PlzNavigate enabled there's probably some extra-work to do. > > Will didCommitProvisionalLoad be called later for the main frame with > PlzNavigate? That's the only place we update the main frame's Lo-Fi state from > an external value. Otherwise, every subframe inherits the parent frame's state > and the only way we can change that is through the DisableLoFiImages call. > > I tried to test with --enable-browser-side-navigation, but origin/master has a > crash with the flag: > > resource_prefetch_common.cc(176)] Check failed: is_valid() && rhs.is_valid(). Well if you have a navigation that's ongoing we can call didCommitProvisional for the main frame after we send these IPCs. That said, this would result in a document change, so previous LoFi images will go away (but the new document may be using lofi). What's the behavior supposed to be in that case? Normally there should be no issue with content_browsertests & content_unittests using PlzNavigate (except if using ServiceWorker). Chrome layer might have issues though.
On 2016/07/08 14:46:26, clamy wrote: > https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... > File content/browser/web_contents/web_contents_impl.cc (right): > > https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... > content/browser/web_contents/web_contents_impl.cc:2564: SendToAllFrames(new > FrameMsg_DisableLoFiImages(MSG_ROUTING_NONE)); > On 2016/07/07 19:31:52, megjablon wrote: > > On 2016/07/07 15:16:30, clamy wrote: > > > I'm not sure this works with PlzNavigate enabled. Do you also want to > disable > > > LoFi for any ongoing navigation in the WebContents? In this case, with > > > PlzNavigate enabled there's probably some extra-work to do. > > > > Will didCommitProvisionalLoad be called later for the main frame with > > PlzNavigate? That's the only place we update the main frame's Lo-Fi state from > > an external value. Otherwise, every subframe inherits the parent frame's state > > and the only way we can change that is through the DisableLoFiImages call. > > > > I tried to test with --enable-browser-side-navigation, but origin/master has a > > crash with the flag: > > > > resource_prefetch_common.cc(176)] Check failed: is_valid() && rhs.is_valid(). > > Well if you have a navigation that's ongoing we can call didCommitProvisional > for the main frame after we send these IPCs. That said, this would result in a > document change, so previous LoFi images will go away (but the new document may > be using lofi). What's the behavior supposed to be in that case? > > Normally there should be no issue with content_browsertests & content_unittests > using PlzNavigate (except if using ServiceWorker). Chrome layer might have > issues though. I believe the snackbar to load the Lo-Fi images shouldn't show until after didCommitProvisionalLoad because it's only shown once a Lo-Fi image has been loaded. Is there a case I'm missing where a Lo-Fi image could be fetched before didCommitProvisionalLoad on the main frame? If there's a document change then we expect Lo-Fi to be reenabled if the Network Quality Estimator says so. As long as Tab.java's didStartPageLoad is called to reset the boolean which stores if the Lo-Fi snackbar has been shown for the page we're fine. We would have an issue if the Lo-Fi snackbar doesn't show again.
On 2016/07/08 19:03:21, megjablon wrote: > On 2016/07/08 14:46:26, clamy wrote: > > > https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... > > File content/browser/web_contents/web_contents_impl.cc (right): > > > > > https://codereview.chromium.org/2114213002/diff/100001/content/browser/web_co... > > content/browser/web_contents/web_contents_impl.cc:2564: SendToAllFrames(new > > FrameMsg_DisableLoFiImages(MSG_ROUTING_NONE)); > > On 2016/07/07 19:31:52, megjablon wrote: > > > On 2016/07/07 15:16:30, clamy wrote: > > > > I'm not sure this works with PlzNavigate enabled. Do you also want to > > disable > > > > LoFi for any ongoing navigation in the WebContents? In this case, with > > > > PlzNavigate enabled there's probably some extra-work to do. > > > > > > Will didCommitProvisionalLoad be called later for the main frame with > > > PlzNavigate? That's the only place we update the main frame's Lo-Fi state > from > > > an external value. Otherwise, every subframe inherits the parent frame's > state > > > and the only way we can change that is through the DisableLoFiImages call. > > > > > > I tried to test with --enable-browser-side-navigation, but origin/master has > a > > > crash with the flag: > > > > > > resource_prefetch_common.cc(176)] Check failed: is_valid() && > rhs.is_valid(). > > > > Well if you have a navigation that's ongoing we can call didCommitProvisional > > for the main frame after we send these IPCs. That said, this would result in a > > document change, so previous LoFi images will go away (but the new document > may > > be using lofi). What's the behavior supposed to be in that case? > > > > Normally there should be no issue with content_browsertests & > content_unittests > > using PlzNavigate (except if using ServiceWorker). Chrome layer might have > > issues though. > > I believe the snackbar to load the Lo-Fi images shouldn't show until after > didCommitProvisionalLoad because it's only shown once a Lo-Fi image has been > loaded. Is there a case I'm missing where a Lo-Fi image could be fetched before > didCommitProvisionalLoad on the main frame? > > If there's a document change then we expect Lo-Fi to be reenabled if the Network > Quality Estimator says so. As long as Tab.java's didStartPageLoad is called to > reset the boolean which stores if the Lo-Fi snackbar has been shown for the page > we're fine. We would have an issue if the Lo-Fi snackbar doesn't show again. I see. There should be no problem with PlzNavigate then. Lgtm.
megjablon@chromium.org changed reviewers: + nasko@chromium.org
nasko: frame_messages.h esprehn: WebKit
megjablon@chromium.org changed reviewers: + esprehn@chromium.org
Not LGTM for now, as sync messages from browser to renderer are disallowed in Chromium. https://codereview.chromium.org/2114213002/diff/120001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2114213002/diff/120001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2562: // Lo-Fi images must first be disabled synchronously so new frames are not You cannot call into renderers synchronously from the browser process. The ordering of IPCs will basically guarantee that you won't reload a frame before it is set to disabled state. For new frames, it is a race condition that I don't know if you can avoid. https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... content/common/frame_messages.h:640: IPC_SYNC_MESSAGE_ROUTED0_0(FrameMsg_DisableLoFiImages) Why is this a sync message? Sync messages should never be sent from the browser to the renderer, as it allows any malicious renderer to hang the entire browser.
https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... content/common/frame_messages.h:640: IPC_SYNC_MESSAGE_ROUTED0_0(FrameMsg_DisableLoFiImages) On 2016/07/12 19:03:57, nasko wrote: > Why is this a sync message? Sync messages should never be sent from the browser > to the renderer, as it allows any malicious renderer to hang the entire browser. It also makes deadlock possible, because the renderer can be sending sync IPCs to the browser at the same time.
Patchset #6 (id:140001) has been deleted
On 2016/07/12 19:17:06, Charlie Reis wrote: > https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... > File content/common/frame_messages.h (right): > > https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... > content/common/frame_messages.h:640: > IPC_SYNC_MESSAGE_ROUTED0_0(FrameMsg_DisableLoFiImages) > On 2016/07/12 19:03:57, nasko wrote: > > Why is this a sync message? Sync messages should never be sent from the > browser > > to the renderer, as it allows any malicious renderer to hang the entire > browser. > > It also makes deadlock possible, because the renderer can be sending sync IPCs > to the browser at the same time. Removed the race condition fix. Will address in a follow up cl because the rest of this addresses the more pressing issue of no lo-fi images reloading. esprehn@ can you PTAL? Thanks.
On 2016/07/12 21:12:06, megjablon wrote: > On 2016/07/12 19:17:06, Charlie Reis wrote: > > > https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... > > File content/common/frame_messages.h (right): > > > > > https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... > > content/common/frame_messages.h:640: > > IPC_SYNC_MESSAGE_ROUTED0_0(FrameMsg_DisableLoFiImages) > > On 2016/07/12 19:03:57, nasko wrote: > > > Why is this a sync message? Sync messages should never be sent from the > > browser > > > to the renderer, as it allows any malicious renderer to hang the entire > > browser. > > > > It also makes deadlock possible, because the renderer can be sending sync IPCs > > to the browser at the same time. > > Removed the race condition fix. Will address in a follow up cl because the rest > of this addresses the more pressing issue of no lo-fi images reloading. esprehn@ > can you PTAL? Thanks. You could keep the SendToAllFrames call and IPC, just don't make it synchronous. However, if this code is good enough and requires merging back to other milestones, it might be good to keep it simpler.
On 2016/07/12 23:26:09, nasko wrote: > On 2016/07/12 21:12:06, megjablon wrote: > > On 2016/07/12 19:17:06, Charlie Reis wrote: > > > > > > https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... > > > File content/common/frame_messages.h (right): > > > > > > > > > https://codereview.chromium.org/2114213002/diff/120001/content/common/frame_m... > > > content/common/frame_messages.h:640: > > > IPC_SYNC_MESSAGE_ROUTED0_0(FrameMsg_DisableLoFiImages) > > > On 2016/07/12 19:03:57, nasko wrote: > > > > Why is this a sync message? Sync messages should never be sent from the > > > browser > > > > to the renderer, as it allows any malicious renderer to hang the entire > > > browser. > > > > > > It also makes deadlock possible, because the renderer can be sending sync > IPCs > > > to the browser at the same time. > > > > Removed the race condition fix. Will address in a follow up cl because the > rest > > of this addresses the more pressing issue of no lo-fi images reloading. > esprehn@ > > can you PTAL? Thanks. > > You could keep the SendToAllFrames call and IPC, just don't make it synchronous. > However, if this code is good enough and requires merging back to other > milestones, it might be good to keep it simpler. The original SendToAllFrames call is still there, just not modified. The only reason for adding the new one was to move part of the code out of the async ipc into sync, but now we can just keep it as is in the one OnReloadLoFiImages IPC. This code is good enough to fix the regression. The race condition was something that was always there that I was trying to fix since I saw it while debugging, but it doesn't affect the lo-fi experience as much. I agree lets keep the fix simple, and I can dig into the other issue later.
esprehn@chromium.org changed reviewers: + dcheng@chromium.org, japhet@chromium.org
dcheng@ and japhet@ does this look okay to you? megjablon is trying to make M52 with this. :)
On 2016/07/13 at 00:35:21, esprehn wrote: > dcheng@ and japhet@ does this look okay to you? megjablon is trying to make M52 with this. :) btw nasko@ are you happy with the patch now?
the changes lgtm. i am a bit curious how we decide whether something goes in 'extra data' or 'request info' though, it seems somewhat arbitrary =)
Now that there are no more IPC changes, LGTM based on clamy@'s review.
Patchset #7 (id:180001) has been deleted
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2114213002/#ps160001 (title: "remove frame race fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, dcheng@chromium.org, nasko@chromium.org, clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2114213002/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. This allows us to change the state in WebKit where we cannot update extra data and pass along on RequestInfo, rather than update extra data later. Synchronously update the Lo-Fi state in RenderFrameImpl so new frame are not created with the old state during the asynchronous frame message. Also, clear the image resource, notify observers, and force a reload when the original request had Lo-Fi on. If the image was already loaded, make sure the image had a Lo-Fi response before reloading. BUG=620203 ========== to ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. This allows us to change the state in WebKit where we cannot update extra data and pass along on RequestInfo, rather than update extra data later. Synchronously update the Lo-Fi state in RenderFrameImpl so new frame are not created with the old state during the asynchronous frame message. Also, clear the image resource, notify observers, and force a reload when the original request had Lo-Fi on. If the image was already loaded, make sure the image had a Lo-Fi response before reloading. BUG=620203 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. This allows us to change the state in WebKit where we cannot update extra data and pass along on RequestInfo, rather than update extra data later. Synchronously update the Lo-Fi state in RenderFrameImpl so new frame are not created with the old state during the asynchronous frame message. Also, clear the image resource, notify observers, and force a reload when the original request had Lo-Fi on. If the image was already loaded, make sure the image had a Lo-Fi response before reloading. BUG=620203 ========== to ========== Fix loading placeholders for Lo-Fi WebURLRequest and RequestExtraData Lo-Fi state became out of sync. Remove Lo-Fi state from RequestExtraData and keep updated on the WebUrlRequest. This allows us to change the state in WebKit where we cannot update extra data and pass along on RequestInfo, rather than update extra data later. Synchronously update the Lo-Fi state in RenderFrameImpl so new frame are not created with the old state during the asynchronous frame message. Also, clear the image resource, notify observers, and force a reload when the original request had Lo-Fi on. If the image was already loaded, make sure the image had a Lo-Fi response before reloading. BUG=620203 Committed: https://crrev.com/7c7c3a25861b8e0a27ef7a9a0115f1ec9ea772b5 Cr-Commit-Position: refs/heads/master@{#405353} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7c7c3a25861b8e0a27ef7a9a0115f1ec9ea772b5 Cr-Commit-Position: refs/heads/master@{#405353} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
