-
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 Opera for Android #1712
Add Opera for Android #1712
Conversation
8e616f8
to
dfee534
Compare
9f170f2
to
1af6529
Compare
So, this is tricky, but I found this: https://ftp.opera.com/pub/opera/android/ I can confirm these versions:
So far I can't confirm:
|
Oh and I think in this case it doesn't make sense to link to Opera Desktop release notes as it is not the same. |
It looks like I missed responding to a request for review. I do not have access to any information beyond the sources the Forlian has already consulted. |
@ExE-Boss Would you like to update the Opera for Android data based on #1712 (comment)? I'm curious to see how much data will fail and how we could correct it. |
I plan to look into it |
Moving the discussion to this PR. What was discussed in #2909 is as follows:
|
"Looks like we have a little bit of an inconsistency on our hands in terms of the version numbers. Opera Android has different version numbers than Opera Desktop, that’s already been established in the discussion on #1712 up above." According to one of Chrome's Developer Advocates who worked for Opera until a few years ago: The wikipedia article is incorrect. Opera for Android followed the OPERA_VERSION = CHROMIUM_VERSION - 13 formula, just like Opera for Desktop. However, it looks like this recently changed. The current Opera for Android version is v47~, but its Chromium version is v66~. Opera did not tell us. |
My wild guess is that "OPERA_VERSION = CHROMIUM_VERSION - 13" worked until Opera Android 37. I think it would be great to find out if there is a new formula for the later versions. Edit: See above #1712 (comment) for what we currently know. |
Opera employees should know. |
@chrisdavidmills Do you know Opera employees that could help us here? |
@Elchi3 I don't know anyone from Opera any more; everyone I know left. @mathiasbynens was there much more recently than me; so was Andreas Bovens. |
In case a few more are helpful in the meantime...
|
Thanks @poshaughnessy! 👍 Especially the forum link was super helpful! I've updated my comment above (#1712 (comment)) with what we know now. |
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've only reviewed files in api/ that start with "A" and I found things that look wrong or maybe I don't understand the changes. Please fix or explain.
review?(@Elchi3): I’ve now addressed your review. Let’s get this merged before more stuff breaks. |
There are a lot of conflicts already and this will also conflict with #3658. If there's some vetting of the API/CSS/etc. data needed, can the browsers/opera.json and browsers/opera_android.json changes be merged first? Per #591 opera_android is the only browser missing in browsers/, and I noticed this today when trying to update data for it in #3731. |
@foolip You need to run a script that updates the data according to #1712 (comment) (without it, the build will fail, as right now, the data assumes that Opera Android version == Opera Desktop version, but the Opera Android version is at least 9 versions behind). |
7976180
to
21a0512
Compare
@ExE-Boss I'm not setting any version numbers though, just changing null to true. |
My script also affects the case when |
21a0512
to
26f6b78
Compare
So, do I need to do anything if I'm just changing null to true based on testing in the real browser, or is it just that there's still more work that could be done? |
It means that there will be merge conflicts (I’d recommend merging this first, as this addresses some |
We'll definitely be facing merge conflicts, so the sooner we can get this merged, the better! |
So, I've looked into this again and the mapping seems to be applied correctly now. Thanks for your work! The only thing that this doesn't seem to take into account is that when mobile chromiums differ from desktop chromiums. This is quite rare, but for example, look at https://developer.mozilla.org/en-US/docs/Web/CSS/@viewport/zoom. Here, Chrome for Android is much later than Chrome desktop (I don't know if the data is correct btw) but your mapping here takes the Chrome desktop number. I would assume that Opera Android is closer to Chrome for Android and thus the Opera Android number could have been based on the Chrome Android version. I believe that majority of the changes in this PR is correct and I think that the above scenario is relatively minor. So, I think it would make sense to go ahead and merge this if I don't hear other thoughts on this matter. It needs another rebase though. Sorry. |
Rule of thumb: if device hardware is involved, don't be surprised if desktop and mobile have different version numbers. In fact you can almost expect it. (The hardware interaction here comes from the fact that zooming on mobile has to be done by pinching the screen.) The difference is usually only a few versions. There seems to be a difference of 32 versions for this feature. If the difference is excessively large, ping me and I'll look into it. |
26f6b78
to
aa314c9
Compare
This data is based on information I found on the internet and might not be entirely accurate.
Thanks for the rebase! I've double checked again https://github.com/mdn/browser-compat-data/blob/aa314c9bce0d3dea20a08011a0f6feeb833fc52a/browsers/opera_android.json against #1712 (comment) In the comment I said that there weren't these releases: But you've added them to opera_android.json. Why? Did you find evidence that these releases existed? |
I think I forgot to remove them. |
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.
We need to remove the non-existing releases here.
Also, I think, linking to the desktop release notes is more confusing than helpful (this can be a follow-up, though).
review?(@Elchi3): I’ve removed the non‑existent Opera Android versions, now would probably be a good time to merge this before more merge conflicts pile up. |
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.
Okay, here we go, I'm merging this. 🎉
Thanks for your continued work on this for over a year, @ExE-Boss! 🥇 Very much appreciated 👍
This wasn't an easy one.
Part of #591
Note: This might not actually be fully accurate, as I don’t use Opera.
review?(@Elchi3, @jpmedley)