Verify old suggestedsites distribution format looks good with new AS top sites icons and write code to prevent regressions

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks 1 bug)

unspecified
Firefox 57
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [mobileAS])

Attachments

(9 attachments, 2 obsolete attachments)

150.51 KB, image/png
Details
136.55 KB, image/png
Details
144.35 KB, image/png
Details
144.25 KB, image/png
Details
142.36 KB, image/png
Details
144.82 KB, image/png
Details
144.08 KB, image/png
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
We updated the format for top sites icons in bug 1388379 so now we need to get our distributions to update as well.

(In reply to Mike Kaply [:mkaply] from bug 1388379 comment #18)
> For reference, here's what we tell partners:
> 
> https://wiki.mozilla.org/Mobile/Distribution_Files#Suggested_sites
> 
> We probably need a way to maintain the old assets if at all possible (since
> not everyone will be on 57 immediately)

Follow-up to bug 1388379 hence blocking and adding to the current iteration.
Mike, what do you mean by

> "We probably need a way to maintain the old assets if at all possible (since not
> everyone will be on 57 immediately)"?  

For context, I don't know how distributions get their data (do they download it? And that's why they need to be able to provide both? Or is it just shipped together?; Note: Richard answered this on IRC: "some distributions ship effectively as part of the OS; some are downloaded OTA; some are repackaged with Firefox.").
Flags: needinfo?(mozilla)
The real problem is that distributions copy their files into the profile and they are used from that location.

So any existing users will always be using the old versions of files (even in activity stream).

So we at least want to see how the old stuff looks in activity stream.
Flags: needinfo?(mozilla)
We need to ensure distributions look good in AS for:
- the new format
- the old format: distributions in the wild never update (see comment 2)

TODO for this issue:
- Get distributions working in local dev builds: I need this to debug/verify changes but I've been struggling to get this to work so I've been working with mkaply to do this
- Fix any issues with distributions icons in both the new and old formats (note: in mkaply's screenshots, it looks like the icons of the old format have stopped working and a generated icon is used instead)
- Figure out what to do with bgcolor in region.properties & update the code accordingly: it's not used by the new format (the images fill the space) so it feels like we should remove it from the spec to avoid confusion but it's used by the old format so needs to continue to work. bgcolor could also be useful if we ever decide not to fill the top sites space with the whole icon (but we could extract this from the image)
- Update tests to ensure no one breaks dual support for the new and old formats
- Update the distribution specs
- Notify partners of the updated spec and request changes
- Coordinate release of new distributions with 57 release

Resources:
- What we tell our partners: https://wiki.mozilla.org/Mobile/Distribution_Files#Suggested_sites
- Install distributions locally: https://bugzilla.mozilla.org/show_bug.cgi?id=1391413#c4
- mkaply's screenshots of current state of old and new top sites & distributions: https://bug1391413.bmoattachments.org/attachment.cgi?id=8902316

---

NI mkaply:

1) To be explicit, I'm waiting on you to help me get distributions working in local dev builds (I'll work on another bug in the mean time).

2) How would I update the distribution specs? Do I just update https://wiki.mozilla.org/Mobile/Distribution_Files#Suggested_sites directly? If so, how do I ensure distributions created up to FF 56 don't accidentally use the new format (if that's even a problem)?

3) How do we notify partners of the updated spec and ask them to update?

4) How do we ensure the updated distributions are only released at the same time as 57?
Flags: needinfo?(mozilla)
Summary: Follow-up: update AS default top sites specs for distributions; ask distributions to update → Follow-up: update AS default top sites code and specs for distributions; ask distributions to update
> - Get distributions working in local dev builds: I need this to debug/verify
> changes but I've been struggling to get this to work so I've been working
> with mkaply to do this

Completed, with Mike's help – thanks!

> - Fix any issues with distributions icons in both the new and old formats
> (note: in mkaply's screenshots, it looks like the icons of the old format
> have stopped working and a generated icon is used instead)

I took a screenshot of what the top sites looks like on the initial run (placeholder icons) and then another screenshot of what top sites looks like after I restart Firefox (icons appear). This is unexpected...

Mike, is this a new issue?

That being said, the mailru distribution icons look fine. I haven't updated the specs yet so they must be using the old format. Before the other remaining todos in this bug, I'd like to:
1) Find the actual icon files for mailru to verify my thoughts
2) See a list of all distributions that I can test against to make sure they look okay
3) Make sure the code supporting the old distributions is not easy to break and well commented to ensure it doesn't break
Attachment #8904654 - Attachment description: Screenshots: top sites on first run → Screenshots: top sites on first run (placeholder icons)
(In reply to Michael Comella (:mcomella) from comment #5)
> I took a screenshot of what the top sites looks like on the initial run
> (placeholder icons) and then another screenshot of what top sites looks like
> after I restart Firefox (icons appear). This is unexpected...
> 
> Mike, is this a new issue?

Spoke on IRC: this is a new issue – I filed bug 1396941.

> 1) Find the actual icon files for mailru to verify my thoughts

It looks like the icons for mailru already contain background colors and margins, which is the new format (with exception to the imgae size) and which is why they look good. :)

> 2) See a list of all distributions that I can test against to make sure they
> look okay

mkaply has given me access. He said there are only a small number of distributions that use suggestedsites so maybe it'd be better to test all of them and, if they already look good, don't worry too much about a generalized solution that will work for all icons.
Comment on attachment 8904654 [details]
Screenshots: top sites on first run (placeholder icons)

No longer relevant: see bug 1396941.
Attachment #8904654 - Attachment is obsolete: true
Attachment #8904658 - Attachment is obsolete: true
I have verified that all distributions with suggestedsites have good looking icons with the new code – I will post screenshots. That being said, some distributions will have problems with the titles once bug 1391413 lands (note: this bug landing or not will affect what specs we can give distributions) – we may need to add workarounds so we should do that work in bug 1391413. I'll add my comment there about which existing titles work well and which do not.

The next step is to make sure the existing code to make these old format icons work is not fragile and is well commented. Then, from comment 3:

- Figure out what to do with bgcolor in region.properties & update the code accordingly: it's not used by the new format (the images fill the space) so it feels like we should remove it from the spec to avoid confusion but it's used by the old format so needs to continue to work. bgcolor could also be useful if we ever decide not to fill the top sites space with the whole icon (but we could extract this from the image)
- Update tests to ensure no one breaks dual support for the new and old formats
- Update the distribution specs
- Notify partners of the updated spec and request changes
- Coordinate release of new distributions with 57 release

Once I have more info from Mike about the process for some of these ^ steps, it might make more sense to break these out into separate bugs.
Summary: Follow-up: update AS default top sites code and specs for distributions; ask distributions to update → Follow-up: update AS default top sites icons and specs for distributions; ask distributions to update
> 1) To be explicit, I'm waiting on you to help me get distributions working in local dev builds (I'll work on another bug in the mean time).

Done

> 2) How would I update the distribution specs? Do I just update https://wiki.mozilla.org/Mobile/Distribution_Files#Suggested_sites directly? If so, how do I ensure distributions created up to FF 56 don't accidentally use the new format (if that's even a problem)?

Yes. I'm responsible for the actual code around these distributions, so I can make necessary updated.

> 3) How do we notify partners of the updated spec and ask them to update?

I'll take the lead on doing that. It's only a couple partners.

> 4) How do we ensure the updated distributions are only released at the same time as 57?

I'll make sure to not upload the new distributions until 57 is released. It won't be perfect, but it's the best we can do.
Flags: needinfo?(mozilla)
Attachment #8904685 - Attachment description: Screenshot: mailru → Screenshot old format: mailru
Attachment #8904686 - Attachment description: Screenshot: 1and1 → Screenshot old format: 1and1
Attachment #8904687 - Attachment description: Screenshot: gmx → Screenshot old format: gmx
Attachment #8904688 - Attachment description: Screenshot: mailcom → Screenshot old format: mailcom
Attachment #8904689 - Attachment description: Screenshot: webde → Screenshot old format: webde
Alias: as-android-distributions
Alias: as-android-distributions
(In reply to Michael Comella (:mcomella) from comment #8)
> The next step is to make sure the existing code to make these old format
> icons work is not fragile and is well commented. Then, from comment 3:
> 
> - Figure out what to do with bgcolor in region.properties & update the code
> accordingly: it's not used by the new format (the images fill the space) so
> it feels like we should remove it from the spec to avoid confusion but it's
> used by the old format so needs to continue to work. bgcolor could also be
> useful if we ever decide not to fill the top sites space with the whole icon
> (but we could extract this from the image)

We should do these two now, in the current bug.

> - Update tests to ensure no one breaks dual support for the new and old
> formats

This can happen in the current bug, or split it out, but note that it doesn't need to be done in the 57 timeframe.

> - Update the distribution specs

I filed the followup bug 1396986.

> - Notify partners of the updated spec and request changes

I filed bug 1396994.

> - Coordinate release of new distributions with 57 release

I filed bug 1396995.
No longer blocks: 1396986, 1388379
Depends on: 1388379
Summary: Follow-up: update AS default top sites icons and specs for distributions; ask distributions to update → Verify old suggestedsites distribution format looks good with new AS top sites icons and write code to prevent regressions
Note: I decided to keep the format the same (with exception to icon size) so regression tests should no longer be necessary. Commits (with comments) to follow.
Comment hidden (mozreview-request)
These `bgcolor`s are the background colors of the new icons added in bug
1388379. I was originally intending to remove the bgcolor attribute in this
bug, which is why I didn't change the colors in the previous bug.

I decided not to remove the bgcolor attribute to avoid churn: we'd have to
maintain multiple permitted schemas for the region.properties (one where
bgcolor is required and one where it is not), update the documentation to
clarify the changes before and after Firefox 57, communicate the new format to
several partners, etc. It seemed easier to continue to require bgcolor and, if
there is transparency in the icon, we use bgcolor, otherwise we don't. This is
already the way distributions work: some icons have transparency, some don't!

In practice, the updated bgcolors on the suggested sites doesn't make a
difference: there is no transparency in the new icons so these bgcolors are
never shown.

Review commit: https://reviewboard.mozilla.org/r/177086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177086/

Comment 21

2 years ago
mozreview-review
Comment on attachment 8905286 [details]
Bug 1394641: AS FaviconView shares styles; add distribution comments.

https://reviewboard.mozilla.org/r/177084/#review182256
Attachment #8905286 - Flags: review?(s.kaspari) → review+

Comment 22

2 years ago
mozreview-review
Comment on attachment 8905287 [details]
Bug 1394641: Update suggestedsites bgcolor for new icons.

https://reviewboard.mozilla.org/r/177086/#review182258
Attachment #8905287 - Flags: review?(s.kaspari) → review+

Comment 23

2 years ago
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/07a89d9a16a7
AS FaviconView shares styles; add distribution comments. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/57f74d890cfe
Update suggestedsites bgcolor for new icons. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/07a89d9a16a7
https://hg.mozilla.org/mozilla-central/rev/57f74d890cfe
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.