Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20752003:
Android: Bump up the fallback OS version number to 4.3.0 (Closed)
|
Created:
7 years, 5 months ago by joth Modified:
7 years, 4 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBump up the fallback OS version number to 4.3.0
JellyBean MR2 revenge of the Beans is out.
Update the fallback OS version number to something a bit more recent.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215220
Patch Set 1 #
Total comments: 4
Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc#newc... base/sys_info_android.cc:21: const int kDefaultAndroidMinorVersion = 3; Why isn't this stuff pulled from the OS? This seems to bake into Chrome the version number for the OS... but yet we could be running on any version of the OS. What am I missing?
https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc#newc... base/sys_info_android.cc:21: const int kDefaultAndroidMinorVersion = 3; On 2013/07/27 18:18:01, jar wrote: > Why isn't this stuff pulled from the OS? > > This seems to bake into Chrome the version number for the OS... but yet we could > be running on any version of the OS. > > What am I missing? The clue is in "kDefault..." (and, in the comment above it) - this is only used if the real OS version can't be sucked out of the OS at runtime. (See the method immediately below). This should only fail when the app is running on pre-production HW / OS that does not yet have an OS version assigned. We fallback to a sane known-released OS version. Letting this get too old (or report project code-names for that matter) means it falls foul of UA sniffing on sites that trigger legacy support when the OS is not known (or at least, can lead people to incorrectly suspect that maybe happening in weird bug diagnosis, which is was what happened to me yesterday)
https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc#newc... base/sys_info_android.cc:21: const int kDefaultAndroidMinorVersion = 3; On 2013/07/27 19:23:39, joth wrote: > On 2013/07/27 18:18:01, jar wrote: > > Why isn't this stuff pulled from the OS? > > > > This seems to bake into Chrome the version number for the OS... but yet we > could > > be running on any version of the OS. > > > > What am I missing? > > The clue is in "kDefault..." (and, in the comment above it) - this is only used > if the real OS version can't be sucked out of the OS at runtime. (See the method > immediately below). This should only fail when the app is running on > pre-production HW / OS that does not yet have an OS version assigned. We > fallback to a sane known-released OS version. > Letting this get too old (or report project code-names for that matter) means it > falls foul of UA sniffing on sites that trigger legacy support when the OS is > not known (or at least, can lead people to incorrectly suspect that maybe > happening in weird bug diagnosis, which is was what happened to me yesterday) > That is mostly a great answer... but... Is it really preferable to conflate results for a known OS with trash when the OS is not responding with its real version? I would think it would be desirable to have a very different (nonexistent??) OS to handle this case, rather than merge in with a real version. Can you explain why this is good?
On 28 July 2013 12:19, <jar@chromium.org> wrote: > > https://codereview.chromium.**org/20752003/diff/1/base/sys_** > info_android.cc<https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc> > File base/sys_info_android.cc (right): > > https://codereview.chromium.**org/20752003/diff/1/base/sys_** > info_android.cc#newcode21<https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc#newcode21> > base/sys_info_android.cc:21: const int kDefaultAndroidMinorVersion = 3; > On 2013/07/27 19:23:39, joth wrote: > >> On 2013/07/27 18:18:01, jar wrote: >> > Why isn't this stuff pulled from the OS? >> > >> > This seems to bake into Chrome the version number for the OS... but >> > yet we > >> could >> > be running on any version of the OS. >> > >> > What am I missing? >> > > The clue is in "kDefault..." (and, in the comment above it) - this is >> > only used > >> if the real OS version can't be sucked out of the OS at runtime. (See >> > the method > >> immediately below). This should only fail when the app is running on >> pre-production HW / OS that does not yet have an OS version assigned. >> > We > >> fallback to a sane known-released OS version. >> Letting this get too old (or report project code-names for that >> > matter) means it > >> falls foul of UA sniffing on sites that trigger legacy support when >> > the OS is > >> not known (or at least, can lead people to incorrectly suspect that >> > maybe > >> happening in weird bug diagnosis, which is was what happened to me >> > yesterday) > > > That is mostly a great answer... but... > > Is it really preferable to conflate results for a known OS with trash > when the OS is not responding with its real version? I would think it > would be desirable to have a very different (nonexistent??) OS to handle > this case, rather than merge in with a real version. > > Can you explain why this is good? > No I don't think I can offer any further explanation. Maybe Grace can? I think this was copied from what the Android browser has done since the beginning of time. See "PREVIOUS_VERSION" in https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...
I'm not sure how I got on the review list... but I've expressed my opinion below. You're welcome to do what you think will get you the most data in the most helpful format (land if you think it is a good thing... the other reviewer likes it ;-) ). https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc File base/sys_info_android.cc (right): https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc#newc... base/sys_info_android.cc:21: const int kDefaultAndroidMinorVersion = 3; On 2013/07/28 19:19:02, jar wrote: > On 2013/07/27 19:23:39, joth wrote: > > On 2013/07/27 18:18:01, jar wrote: > > > Why isn't this stuff pulled from the OS? > > > > > > This seems to bake into Chrome the version number for the OS... but yet we > > could > > > be running on any version of the OS. > > > > > > What am I missing? > > > > The clue is in "kDefault..." (and, in the comment above it) - this is only > used > > if the real OS version can't be sucked out of the OS at runtime. (See the > method > > immediately below). This should only fail when the app is running on > > pre-production HW / OS that does not yet have an OS version assigned. We > > fallback to a sane known-released OS version. > > Letting this get too old (or report project code-names for that matter) means > it > > falls foul of UA sniffing on sites that trigger legacy support when the OS is > > not known (or at least, can lead people to incorrectly suspect that maybe > > happening in weird bug diagnosis, which is was what happened to me yesterday) > > > > That is mostly a great answer... but... > > Is it really preferable to conflate results for a known OS with trash when the > OS is not responding with its real version? I would think it would be desirable > to have a very different (nonexistent??) OS to handle this case, rather than > merge in with a real version. > > Can you explain why this is good? IMO, sending up the old answer will seemingly allow us to spot when we are not getting data (not getting the real OS version).... unless we'll filter these out completely <sigh>.
A lot of sites are sniffing the OS version to decide what content they provide. nonexistent version is like not providing it. The user experience will be bad. On Sun, Jul 28, 2013 at 1:36 PM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 28 July 2013 12:19, <jar@chromium.org> wrote: > >> >> https://codereview.chromium.**org/20752003/diff/1/base/sys_** >> info_android.cc<https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc> >> File base/sys_info_android.cc (right): >> >> https://codereview.chromium.**org/20752003/diff/1/base/sys_** >> info_android.cc#newcode21<https://codereview.chromium.org/20752003/diff/1/base/sys_info_android.cc#newcode21> >> base/sys_info_android.cc:21: const int kDefaultAndroidMinorVersion = 3; >> On 2013/07/27 19:23:39, joth wrote: >> >>> On 2013/07/27 18:18:01, jar wrote: >>> > Why isn't this stuff pulled from the OS? >>> > >>> > This seems to bake into Chrome the version number for the OS... but >>> >> yet we >> >>> could >>> > be running on any version of the OS. >>> > >>> > What am I missing? >>> >> >> The clue is in "kDefault..." (and, in the comment above it) - this is >>> >> only used >> >>> if the real OS version can't be sucked out of the OS at runtime. (See >>> >> the method >> >>> immediately below). This should only fail when the app is running on >>> pre-production HW / OS that does not yet have an OS version assigned. >>> >> We >> >>> fallback to a sane known-released OS version. >>> Letting this get too old (or report project code-names for that >>> >> matter) means it >> >>> falls foul of UA sniffing on sites that trigger legacy support when >>> >> the OS is >> >>> not known (or at least, can lead people to incorrectly suspect that >>> >> maybe >> >>> happening in weird bug diagnosis, which is was what happened to me >>> >> yesterday) >> >> >> That is mostly a great answer... but... >> >> Is it really preferable to conflate results for a known OS with trash >> when the OS is not responding with its real version? I would think it >> would be desirable to have a very different (nonexistent??) OS to handle >> this case, rather than merge in with a real version. >> >> Can you explain why this is good? >> > > No I don't think I can offer any further explanation. > Maybe Grace can? I think this was copied from what the Android browser has > done since the beginning of time. See "PREVIOUS_VERSION" in > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > > -- thanks, Grace
lgtm
Opened a new bug to debate how we can improve this going forward: https://code.google.com/p/chromium/issues/detail?id=267152 jar is away and I'd like to move ahead with this fix for our immediate testing needs, so +willchan who reviewed the original CL
LGTM... based on rubber stamp approval... despite concerns
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joth@chromium.org/20752003/1
Message was sent while issue was closed.
Change committed as 215220 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
