Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2404463002:
Avoid <picture> preloading when viewport not initialized. (Closed)
|
Created:
4 years, 2 months ago by Yoav Weiss Modified:
4 years, 2 months ago Reviewers:
Charlie Harrison CC:
chromium-reviews, blink-reviews-html_chromium.org, loading-reviews+parser_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid <picture> preloading when viewport not initialized.
On rare occasions, it seems that viewport dimensions are not properly
initialized before being passed to the HTMLPreloadScanner, which
results in potential preloading of the wrong <picture> based resource.
That issue will no longer be relevant once ParseHTMLOnMainThread
lands and MediaValuesCached related logic is removed from the
preloadScanner, but in the mean time, it's causing test flakiness.
This CL changes the preload scanner's behavior, so that it would avoid
preloading of picture based resources when the viewport dimensions
are unknown.
BUG=652513
Committed: https://crrev.com/1ea50761e75c1752ec0bf619106bfa63f9372dae
Cr-Commit-Position: refs/heads/master@{#424373}
Patch Set 1 #Patch Set 2 : Modify TODO #
Total comments: 1
Patch Set 3 : style #Messages
Total messages: 28 (20 generated)
The CQ bit was checked by yoav@yoav.ws 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: 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...)
Description was changed from ========== Avoid <picture> preloading when viewport not initialized. BUG=652513 ========== to ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 ==========
yoav@yoav.ws changed reviewers: + csharrison@chromium.org
Hey Charlie :) Can you take a look?
Will this really be fixed by the experiment landing or will an easy fix just be possible at that point. Do the failing tests succeed with the experimental flag on (--enable-blink-features=ParseHTMLOnMainThread)?
Description was changed from ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 ========== to ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands and MediaValuesCached related logic is removed from the preloadScanner, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 ==========
On 2016/10/07 18:15:06, Charlie Harrison wrote: > Will this really be fixed by the experiment landing or will an easy fix just be > possible at that point. Do the failing tests succeed with the experimental flag > on (--enable-blink-features=ParseHTMLOnMainThread)? You're right that the MediaValuesCached logic removal is also necessary to fix this. I modified the description accordingly.
The CQ bit was checked by yoav@yoav.ws 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.
LGTM % nit and wrapping your description to 80 cols. https://codereview.chromium.org/2404463002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right): https://codereview.chromium.org/2404463002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:811: // cases, at least in tests. nit: Fix comment reflow by removing line breaks.
LGTM % nit and wrapping your description to 80 cols. https://codereview.chromium.org/2404463002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right): https://codereview.chromium.org/2404463002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:811: // cases, at least in tests. nit: Fix comment reflow by removing line breaks.
The CQ bit was checked by yoav@yoav.ws 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...
Description was changed from ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands and MediaValuesCached related logic is removed from the preloadScanner, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 ========== to ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands and MediaValuesCached related logic is removed from the preloadScanner, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 ==========
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 yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2404463002/#ps40001 (title: "style")
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 ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands and MediaValuesCached related logic is removed from the preloadScanner, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 ========== to ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands and MediaValuesCached related logic is removed from the preloadScanner, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands and MediaValuesCached related logic is removed from the preloadScanner, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 ========== to ========== Avoid <picture> preloading when viewport not initialized. On rare occasions, it seems that viewport dimensions are not properly initialized before being passed to the HTMLPreloadScanner, which results in potential preloading of the wrong <picture> based resource. That issue will no longer be relevant once ParseHTMLOnMainThread lands and MediaValuesCached related logic is removed from the preloadScanner, but in the mean time, it's causing test flakiness. This CL changes the preload scanner's behavior, so that it would avoid preloading of picture based resources when the viewport dimensions are unknown. BUG=652513 Committed: https://crrev.com/1ea50761e75c1752ec0bf619106bfa63f9372dae Cr-Commit-Position: refs/heads/master@{#424373} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1ea50761e75c1752ec0bf619106bfa63f9372dae Cr-Commit-Position: refs/heads/master@{#424373} |
