Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1156663005:
scheduleArchiveLoad should return false if archived resource is not available. (Closed)
|
Created:
5 years, 7 months ago by pals Modified:
5 years, 6 months ago CC:
blink-reviews, gavinp+loader_chromium.org, Nate Chapin, tyoshino+watch_chromium.org, Yoav Weiss Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionscheduleArchiveLoad should return false if archived resource is not available.
scheduleArchiveLoad should return false if archived resource is not available in the archive.
BUG=492041
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196665
Patch Set 1 #Patch Set 2 : Adding test #Patch Set 3 : Uploading .mht file as binary file #Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 37 (21 generated)
sanjoy.pal@samsung.com changed reviewers: + morrita@chromium.org
PTAL.
On 2015/05/26 06:05:33, pals wrote: > PTAL. In desktop chrome, the save page feature does not use .mhtml format. So, m_archiveResourceCollection is not created and scheduleArchiveLoad return false. So, the PayPal logo is loaded from cache/network. When a page is saved in archived .mhtml format, m_archiveResourceCollection becomes true, but even though the resource is not available in archive, scheduleArchiveLoad return true which makes ResourceFetcher:requestResource() think that resource is available in archive. So, it does not try load from cache/network and the logo is not shown. Please review the CL.
sanjoy.pal@samsung.com changed reviewers: + mkwst@chromium.org
Please review.
Does this produce a web-visible change in behavior? If so, we need a test.
On 2015/06/01 05:45:21, Mike West wrote: > Does this produce a web-visible change in behavior? If so, we need a test. When a page is saved in archived .mhtml format, m_archiveResourceCollection becomes true [1], but even though the resource is not available in archive, scheduleArchiveLoad return true [2] which makes ResourceFetcher:requestResource() think that resource is available in archive. So, it does not try load from cache/network [3] and the logo is not shown when the saved page is displayed. I am not sure how to simulate this case in a layout test as the resource needs to be in cache otherwise it has to fetched over network. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [3] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/06/01 06:18:24, pals wrote: > On 2015/06/01 05:45:21, Mike West wrote: > > Does this produce a web-visible change in behavior? If so, we need a test. > > When a page is saved in archived .mhtml format, m_archiveResourceCollection > becomes true [1], but even though the resource is not available in archive, > scheduleArchiveLoad return true [2] which makes > ResourceFetcher:requestResource() > think that resource is available in archive. So, it does not try load from > cache/network [3] and the logo is not shown when the saved page is displayed. > I am not sure how to simulate this case in a layout test as the resource > needs to be in cache otherwise it has to fetched over network. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I shall try to write a test.
On 2015/06/01 at 06:28:40, sanjoy.pal wrote: > On 2015/06/01 06:18:24, pals wrote: > > On 2015/06/01 05:45:21, Mike West wrote: > > > Does this produce a web-visible change in behavior? If so, we need a test. > > > > When a page is saved in archived .mhtml format, m_archiveResourceCollection > > becomes true [1], but even though the resource is not available in archive, > > scheduleArchiveLoad return true [2] which makes > > ResourceFetcher:requestResource() > > think that resource is available in archive. So, it does not try load from > > cache/network [3] and the logo is not shown when the saved page is displayed. > > I am not sure how to simulate this case in a layout test as the resource > > needs to be in cache otherwise it has to fetched over network. > > > > [1] > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > [2] > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > [3] > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > I shall try to write a test. It doesn't have to be a layout test, if the conditions are difficult to reproduce. Writing a unit test would be a perfectly reasonable approach.
I have added a layout test. PTAL.
Thanks for adding the test, LGTM.
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156663005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
sanjoy.pal@samsung.com changed reviewers: + samahto@cisco.com
@samahto I think this is failing due to same problem descibed here https://codereview.chromium.org/1027623003. I am not find out the way to upload .mht file as binary file. I tried modifying .gitattributes to include "*.mht -diff". could you help me in that?
Patchset #5 (id:180001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:200001) has been deleted
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
Patchset #4 (id:240001) has been deleted
The CQ bit was checked by sanjoy.pal@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1156663005/#ps320001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156663005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/65614)
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156663005/320001
Message was sent while issue was closed.
Committed patchset #5 (id:320001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196665 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
