Skip to content
This repository was archived by the owner on Nov 1, 2022. It is now read-only.

Conversation

@gabrielluong
Copy link
Member

…ecify the preferred image size


Fixes #7138. This is very similar to BrowserIcons which uses an equivalent IconRequest.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@codecov
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #7448 into master will decrease coverage by 0.16%.
The diff coverage is 80.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7448      +/-   ##
============================================
- Coverage     77.76%   77.59%   -0.17%     
- Complexity     5045     5133      +88     
============================================
  Files           647      690      +43     
  Lines         24170    24954     +784     
  Branches       3538     3682     +144     
============================================
+ Hits          18795    19364     +569     
- Misses         3903     4070     +167     
- Partials       1472     1520      +48     
Impacted Files Coverage Δ Complexity Δ
.../mozilla/components/support/images/ImageRequest.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...la/components/support/images/loader/ImageLoader.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...nts/browser/thumbnails/utils/ThumbnailDiskCache.kt 76.66% <50.00%> (ø) 8.00 <2.00> (ø)
...zilla/components/browser/tabstray/TabViewHolder.kt 93.75% <100.00%> (ø) 1.00 <0.00> (ø)
...ponents/browser/thumbnails/ThumbnailsMiddleware.kt 80.00% <100.00%> (ø) 2.00 <0.00> (ø)
...omponents/browser/thumbnails/ThumbnailsUseCases.kt 93.33% <100.00%> (ø) 1.00 <0.00> (ø)
...nents/browser/thumbnails/loader/ThumbnailLoader.kt 55.55% <100.00%> (ø) 5.00 <0.00> (ø)
...nts/browser/thumbnails/storage/ThumbnailStorage.kt 100.00% <100.00%> (ø) 6.00 <1.00> (ø)
...feature/customtabs/store/CustomTabsServiceStore.kt 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...onents/feature/customtabs/verify/OriginVerifier.kt 60.00% <0.00%> (-3.16%) 5.00% <0.00%> (ø%)
... and 65 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d40dbe4...74c1e91. Read the comment docs.

@gabrielluong gabrielluong force-pushed the 7138 branch 2 times, most recently from 77a1e31 to 61951a6 Compare June 22, 2020 19:56
Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

This looks good!

It doesn't solve another problem we have: we store images to disk at their full res and not based on the desired quality (in this case 102dp).

That is to say, we're storing these large/high-res images to disk, but we only use them for a small thumbnail.

We can file a new issue for it: Add max size/res to images stored in ThumbnailStorage

@jonalmeida
Copy link
Contributor

Don't forget to put up a PR for Fenix to address the breaking API changes.

@gabrielluong
Copy link
Member Author

bors r=jonalmeida

bors bot pushed a commit that referenced this pull request Jun 25, 2020
7448: Issue #7138: Add ImageRequest to ImageLoader to allow consumers to sp… r=jonalmeida a=gabrielluong

…ecify the preferred image size




Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
@bors
Copy link

bors bot commented Jun 25, 2020

Build failed:

@gabrielluong
Copy link
Member Author

bors retry

bors bot pushed a commit that referenced this pull request Jun 25, 2020
7448: Issue #7138: Add ImageRequest to ImageLoader to allow consumers to sp… r=jonalmeida a=gabrielluong

…ecify the preferred image size




Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
@bors
Copy link

bors bot commented Jun 25, 2020

Build failed:

@gabrielluong
Copy link
Member Author

Last try seems to fail on a failed checkout of AC in the build for build-tooling-glean-gradle. Don't see anything wrong here with the given PR. Gonna retry.

@gabrielluong
Copy link
Member Author

bors retry

bors bot pushed a commit that referenced this pull request Jun 25, 2020
7448: Issue #7138: Add ImageRequest to ImageLoader to allow consumers to sp… r=jonalmeida a=gabrielluong

…ecify the preferred image size




Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
@bors
Copy link

bors bot commented Jun 25, 2020

Build failed:

…onsumers to specify the preferred image size
@gabrielluong
Copy link
Member Author

bors retry

@bors
Copy link

bors bot commented Jun 26, 2020

Build succeeded:

@bors bors bot merged commit 515eea3 into mozilla-mobile:master Jun 26, 2020
@gabrielluong gabrielluong deleted the 7138 branch June 26, 2020 01:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tweak the desired sizes of the thumbnail to make them less blurry

2 participants