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

Fix a bug preventing map.loaded() from returning true with raster tiles. #3302

Merged
merged 1 commit into from
Oct 24, 2016
Merged

Fix a bug preventing map.loaded() from returning true with raster tiles. #3302

merged 1 commit into from
Oct 24, 2016

Conversation

vicapow
Copy link
Contributor

@vicapow vicapow commented Oct 6, 2016

Launch Checklist

  • Uses a transparent png when there's no content from raster tiles.
  • Removes unused return img; because getArrayBuffer does nothing with the returned value from it's callback.
  • properly sets the tile state when the request is aborted or errors.

@vicapow vicapow mentioned this pull request Oct 6, 2016
Closed
vicapow
vicapow commented Oct 6, 2016
View reviewed changes
js/util/ajax.js
if (imgData.byteLength) {
img.src = (window.URL || window.webkitURL).createObjectURL(blob);
} else {
img.src = transparentPngUrl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't do this, the tile becomes black and webGL warnings are logged.

lucaswoj
lucaswoj suggested changes Oct 6, 2016
View reviewed changes
Copy link
Contributor

@lucaswoj lucaswoj left a comment

Choose a reason for hiding this comment

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

Thank you so much @vicapow! 🙇

Do you have any ideas on testing these changes? It is unfortunate that getImage isn't tested in our unit tests and our integration tests (mapbox-gl-test-suite) don't provide an easy way to simulate a 204 response.

I'm interested in working with you to fix one of those two blockers 😄

js/source/raster_tile_source.js
if (tile.aborted) {
this.state = 'unloaded';
return callback(null);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this fixing another bug? If so, thank you! Out of curiosity: what were the symptoms of this bug? I'm guessing something along the lines of "map.loaded() doesn't return true if there are any tiles were aborted."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, technically, it's a seperate bug that manifested itself in the same way (preventing loaded() from being true.)

I'm guessing something along the lines of "map.loaded() doesn't return true if there are any tiles were aborted."

Yes, exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm interested in working with you to fix one of those two blockers

Happy to help with some guidance on how best to write tests for this.

js/util/ajax.js
@@ -59,7 +61,11 @@ exports.getImage = function(url, callback) {
(window.URL || window.webkitURL).revokeObjectURL(img.src);
};
var blob = new window.Blob([new Uint8Array(imgData)], { type: 'image/png' });
img.src = (window.URL || window.webkitURL).createObjectURL(blob);
if (imgData.byteLength) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you decide to branch on imgData.byteLength rather than "status is 204 No Content"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not that familiar with the spec but I figured it was possible a response could include an empty PNG (with byteLength > 0) and still return a 204.

Copy link
Contributor

@lucaswoj lucaswoj Oct 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

According to the w3c spec, a 204 response may not include any body

10.2.5 204 No Content

...

The 204 response MUST NOT include a message-body, and thus is always terminated by the first empty line after the header fields.

My intuition is that we should keep the behaviour you have implemented in response to a 204 header and fail loudly if any other type of response contains no data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know that about 204. With that said, is an empty response from a 200 still valid? My intuition is yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

10.2.1 200 OK

The request has succeeded. The information returned with the response is dependent on the method used in the request, for example:

GET an entity corresponding to the requested resource is sent in the response;
...

My interpretation is that a 200 response must include a body. Per the spec, a trivial transparent PNG would be acceptable but the lack of a body would be unacceptable. In practice, a 200 response with no body is more likely a bug than intended behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucaswoj I updated the PR. is this better?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM! Now we need to figure out a way to test this code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucaswoj any ideas how best to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is unfortunate that we don't have any unit tests for this module yet 😬. I suggest that we create a test/js/util/ajax.test.js file and add at least a test for this case. Sinon's window. useFakeXMLHttpRequest should make it relatively easy to simulate this case.

vicapow
vicapow commented Oct 17, 2016
View reviewed changes
js/util/ajax.js
@@ -34,6 +34,9 @@ exports.getArrayBuffer = function(url, callback) {
callback(e);
};
xhr.onload = function() {
if (xhr.response.byteLength === 0 && xhr.status === 200) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

specifically, this bit

@lucaswoj lucaswoj mentioned this pull request Oct 20, 2016
Merged
@lucaswoj
Copy link
Contributor

This is ready to merge once there is a unit test per https://github.com/mapbox/mapbox-gl-js/pull/3302/files#r83751352 😄

@mourner
Copy link
Member

mourner commented Oct 20, 2016

And also needs another rebase after #3408.

@vicapow
Copy link
Contributor Author

vicapow commented Oct 23, 2016

@lucaswoj okay! I've rebased from master and added tests for ajax.getArrayBuffer()

lucaswoj
lucaswoj approved these changes Oct 24, 2016
View reviewed changes
Copy link
Contributor

@lucaswoj lucaswoj left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you @vicapow!

@lucaswoj lucaswoj merged commit f183b41 into mapbox:master Oct 24, 2016
@jfirebaugh jfirebaugh mentioned this pull request May 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucaswoj lucaswoj lucaswoj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vicapow @lucaswoj @mourner