-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Add chrome_android data #1356
Add chrome_android data #1356
Conversation
I've made a few notes in the linked tickets. As far as the missing dates, I'm not sure I can prioritize getting information on release that old. I don't believe Google supports them either. |
here is the missing chrome version info. The issue is that a most of the failures are around chrome's android version (e.g. chrome 4.4) rather than an actual chrome version. those could be aliased against the linked data, but I dont know the mappings |
The new info looks good to me. |
@Elchi3 any chance this will be merged? :) |
It will be at some point. This adds additional validation to the whole repo and there are many open pull requests that would need rebasing against stronger validation. I'm working on getting the pull request backlog cleared first. Thanks for your patience. |
929af0f
to
83e9b43
Compare
That data looks like Chrome desktop releases, note that this PR is about Chrome Android. I think the releases I have here are correct. There just wasn't a Chrome Android sometimes and it only started with version 18, as stated in https://chromereleases.googleblog.com/2012/06/chrome-for-android-out-of-beta.html I will update invalid versions in our data. |
I've added a commit that updates all invalid versions to something that I think make sense:
Not sure who can review this, but it would be nice to get merged now that the backlog is smaller. |
6dd5ba1
to
304ce9a
Compare
The important part to review here is the first commit that adds chrome android release data: Validation then checks against the entire compat data (version_added / version_removed) and in the second commit I've corrected invalid data that was found. I don't think these changes need special reviewing, because the tests would catch mistakes there. The PR is rebased against the current master. |
@wbamberg would you review this for me? |
By the way, I suggest NOT listing dates for unreleased versions of Chrome. We generally keep to our six-week schedule, but delays are not unheard of. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked chrome_android.json, that the links to the release notes worked and identified a Chrome Android release with the same number and a matching date. Looks good except for a couple of minor discrepancies.
I agree with Joe on not having dates in the future for releases.
I skimmed the other diffs and they look sensible.
Great work @Elchi3 .
browsers/chrome_android.json
Outdated
"status": "retired" | ||
}, | ||
"26": { | ||
"release_date": "2013-03-03", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> "2013-04-03"
browsers/chrome_android.json
Outdated
}, | ||
"48": { | ||
"release_date": "2016-01-26", | ||
"release_notes": "https://chromereleases.googleblog.com/2016/01/chrome-for-android-update.html", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no blog post here - it seems there wasn't a blog post for this release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find one either. I'm removing this.
browsers/chrome_android.json
Outdated
"status": "current" | ||
}, | ||
"69": { | ||
"release_date": "2018-09-04", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Joe, having release dates in the future seems dodgy :).
304ce9a
to
cf40e07
Compare
Thank you @wbamberg! 🎉 I've addressed the review comments and rebased against latest master to ensure no new data is invalid according to the now added version validation here. Let's merge this 🙂 |
The build will fail here until we've fixed the chrome_android versions in the data. I'm opening it to discuss the correct chrome_android versions.
Sources are: https://en.wikipedia.org/wiki/Google_Chrome_version_history and the posts on https://chromereleases.googleblog.com
Note that there has been Chrome for Android 18, then 25, and then 26-66.
Once we merge this, the current open PRs and future PRs shouldn't commit invalid Chrome for Android versions anymore as the build will fail otherwise.
This fixes #1353 and another item on #591.
CC'ing @jpmedley