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.

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 webview_android browser data #2690

Merged
merged 4 commits into from
Sep 14, 2018
Merged

Add webview_android browser data #2690

merged 4 commits into from
Sep 14, 2018

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Aug 29, 2018

The fixes the last item on #591: Add release data (allowed versions) for webview_android and validate our compat data with it.

Some notes here:

  • Android 4.4 is based on Chrome 30.
  • Android 4.4.3 is based on Chrome 33.
  • Android 5 had a webview based on Chrome 37 but also decoupled OS and browser, so the web engine can be updated separately. That means we can use Chrome Android version numbers from that point on.

@jpmedley can you have a look here? Does this allow-list for webview_android versions make sense to you?

@Elchi3 Elchi3 added the data:browsers Data about browsers (versions, release dates, etc). This data is used for validation. label Aug 29, 2018
@jpmedley
Copy link
Contributor

No because you've only listed the version of Chrome that Android shipped with. After the phone is activated, Chrome and webview auto-update with every Chrome release.

@Elchi3
Copy link
Member Author

Elchi3 commented Aug 30, 2018

No because you've only listed the version of Chrome that Android shipped with. After the phone is activated, Chrome and webview auto-update with every Chrome release.

Not sure I understand how this is contrary to what this PR does.

This PR adds valid WebView versions. WebView used to be part of the Android OS and so we're referring to the Android version for that period (caniuse does this as well btw). Some WebViews were already based on Chrome. I know of two here. Still we're going with the Android versions, as these are more commonly referred to when the WebView was not decoupled from Android OS. So, that leaves us with these:

  • 1
  • 1.1
  • 1.5
  • 1.6
  • 2.2
  • 2.3
  • 3
  • 4
  • 4.1
  • 4.2
  • 4.4 ( WebView based on Chrome 30)
  • 4.4.3 (WebView based on Chrome 33)

Then in Android 5, WebView was decoupled from Android OS and now auto updates with every Chrome release. So, instead of adding Android 5 as a valid version, we refer to the first version of Chrome that was available on Android 5 (this is 37). And now that we're decoupled from Android, we can start using Chrome versions until the current one. So, we end up with all of these:

  • 37
  • 38
  • 39
  • ...
  • 69

Please see the diff of this PR for all versions added.

@jpmedley
Copy link
Contributor

Sorry, I have a tendency to go on autopilot regarding the webview issue. It's the result of too many discussions with people who think webview is still tied to Android.

@Elchi3
Copy link
Member Author

Elchi3 commented Sep 4, 2018

There are also a few early android versions that already included a web browser. So, maybe we should add these as well. https://en.wikipedia.org/wiki/Android_version_history#Version_history_by_API_level

  • 1
  • 1.1
  • 1.5
  • 1.6

"1" would then be a valid first version which might make sense in a lot of cases. See #2717

Opinions?

@Elchi3 Elchi3 force-pushed the webview-android-data branch 2 times, most recently from 406ce57 to 0ea67d6 Compare September 6, 2018 15:11
@Elchi3
Copy link
Member Author

Elchi3 commented Sep 6, 2018

Updated to add early android versions. (also edited my comment above)

@Elchi3 Elchi3 force-pushed the webview-android-data branch from 0ea67d6 to a6b1cd5 Compare September 11, 2018 09:10
@Elchi3 Elchi3 requested a review from wbamberg September 11, 2018 09:13
@Elchi3
Copy link
Member Author

Elchi3 commented Sep 11, 2018

@wbamberg, this is similar to #1356. See above on how I propose to go with versions here (we're following more or less caniuse). I would appreciate a review, but let me know if I should find someone else. Thank you!

@Elchi3 Elchi3 force-pushed the webview-android-data branch from a6b1cd5 to c8fb937 Compare September 12, 2018 09:57
Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

The changes in browsers.json look fine, except one blog post seems to be missing again.

As for the changes in the data, from the diffs it looks like you've mostly done this:

  • whole numbers 4-28 inclusive -> true
  • 2.1 -> 2
  • 2.6 -> 3
  • 4.3, 29, 30 -> 4.4
  • 4.4.4, 31, 32, 33-> 4.4.3
  • 34, 35, 36 -> 37
  • 5.1 -> 38

But there are a few instances that don't follow this rule:

Without really understanding the reasoning behind the mappings, it's hard for me to tell if these are what you intended or not :).

},
"48": {
"release_date": "2016-01-26",
"release_notes": "https://chromereleases.googleblog.com/2016/01/chrome-for-android-update.html",
Copy link
Contributor

Choose a reason for hiding this comment

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

This link doesn't work - it looks like there wasn't a blog post for this release?

@Elchi3 Elchi3 force-pushed the webview-android-data branch from c8fb937 to 003dead Compare September 13, 2018 12:11
@Elchi3
Copy link
Member Author

Elchi3 commented Sep 13, 2018

Nice catches, @wbamberg ! 👍
The mapping you've identified is what I've tried to follow here.

Updated the instances that didn't follow these rules and removed the non-existent release notes.

@wbamberg
Copy link
Contributor

Your changes look good to me! There were some merge conflicts in Event.json, which I've tried to fix. But now there's also a linting error, where Event.stopImmediatePropagation has "version_added": "6" for webview_android .

@Elchi3 Elchi3 force-pushed the webview-android-data branch from c73ca86 to 02016f0 Compare September 14, 2018 10:23
@Elchi3
Copy link
Member Author

Elchi3 commented Sep 14, 2018

I've rebased again. I think we're ready to merge now.

Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks @Elchi3 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:browsers Data about browsers (versions, release dates, etc). This data is used for validation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants