Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LazyLoad: add new test for changing base URL #20683

Merged
merged 1 commit into from
Dec 15, 2019
Merged

LazyLoad: add new test for changing base URL #20683

merged 1 commit into from
Dec 15, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Dec 9, 2019 •
edited
Loading

Add new tests for changing base URL, the test
original-base-url-applied-tentative.html changed the document URL, which
only implicitly changed the document's base URL. These tests explicitly
change the document's base URL via the <base> element.

Bug: 984983
Change-Id: I5ab855aaecc8bd12502eb32a3b1d26ae5ced31a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1954508
Reviewed-by: Dominic Farolino <dom@chromium.org>
Commit-Queue: Rob Buis <rbuis@igalia.com>
Cr-Commit-Position: refs/heads/master@{#724964}

wpt-pr-bot
wpt-pr-bot approved these changes Dec 9, 2019
View reviewed changes
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@rwlbuis rwlbuis requested a review from annevk December 11, 2019 07:49
@rwlbuis
Copy link
Contributor

rwlbuis commented Dec 11, 2019

@annevk do you think the tests are correct? Currently failing in Chrome and we are not sure if Chrome is wrong or the tests :)

annevk
annevk reviewed Dec 11, 2019
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick skim this basically looks correct to me, I do have some suggestions/questions though.

loading/lazyload/original-base-url-applied-2-tentative.html Outdated Show resolved Hide resolved
loading/lazyload/original-base-url-applied-2-tentative.html Outdated Show resolved Hide resolved
loading/lazyload/original-base-url-applied-2-tentative.html Show resolved Hide resolved
loading/lazyload/original-base-url-applied-2-tentative.html Outdated
});

async_test(function(t) {
below_viewport_img.promise.then(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're dealing with a promise, using promise_test and async functions might be nicer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very familiar with them, would prefer to stick to current approach.

annevk
annevk approved these changes Dec 12, 2019
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I recommend finding some time to learn them though as both promise_test (guarantees sequential execution compared to async_test, and looks nice if you're dealing with promises anyway) and async functions (look nice) are pretty great.

@rwlbuis
Copy link
Contributor

rwlbuis commented Dec 12, 2019

Sure, I recommend finding some time to learn them though as both promise_test (guarantees sequential execution compared to async_test, and looks nice if you're dealing with promises anyway) and async functions (look nice) are pretty great.

Ok will look into it :) Thanks for the review.

domfarolino
domfarolino requested changes Dec 14, 2019
View reviewed changes
Copy link
Member

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, while finishing this review I realized I should have probably written this up in https://crrev.com/c/1954508. Can you address these over there?

loading/lazyload/original-base-url-applied-2-tentative.html Outdated Show resolved Hide resolved
loading/lazyload/original-base-url-applied-2-tentative.html Outdated Show resolved Hide resolved
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1954508 branch 3 times, most recently from 6c5ef0b to 506af24 Compare December 15, 2019 06:41
@rwlbuis @chromium-wpt-export-bot
LazyLoad: add new test for changing base URL
96828fe 
Add new tests for changing base URL, the test
original-base-url-applied-tentative.html changed the document URL, which
only implicitly changed the document's base URL. These tests explicitly
change the document's base URL via the <base> element.

Bug: 984983
Change-Id: I5ab855aaecc8bd12502eb32a3b1d26ae5ced31a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1954508
Reviewed-by: Dominic Farolino <dom@chromium.org>
Commit-Queue: Rob Buis <rbuis@igalia.com>
Cr-Commit-Position: refs/heads/master@{#724964}
domfarolino
domfarolino approved these changes Dec 15, 2019
View reviewed changes
Copy link
Member

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving since I've approved on crrev and the CL has landed

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit fce0667 into master Dec 15, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1954508 branch December 15, 2019 15:31
@domfarolino domfarolino mentioned this pull request Feb 12, 2020
Merged
annevk added a commit to whatwg/html that referenced this pull request Feb 12, 2020
@bengreenstein @domfarolino @annevk
Add <img loading=lazy> "lazy loading images"
0283e9e 
Follow-up: #5236.

Tests: 
* web-platform-tests/wpt#14914
* web-platform-tests/wpt#17985
* web-platform-tests/wpt#18087
* web-platform-tests/wpt#20634
* web-platform-tests/wpt#20747
* web-platform-tests/wpt#20683
* web-platform-tests/wpt#20774
* web-platform-tests/wpt#20680
* Some as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1542784

Closes #2806. Closes #5250.

Co-authored-by: Dominic Farolino <domfarolino@gmail.com>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwlbuis rwlbuis rwlbuis left review comments

@wpt-pr-bot wpt-pr-bot wpt-pr-bot approved these changes

@domfarolino domfarolino domfarolino approved these changes

@annevk annevk annevk approved these changes

Assignees
No one assigned
Labels
chromium-export loading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chromium-wpt-export-bot @rwlbuis @annevk @domfarolino @wpt-pr-bot