Closed
Bug 1052100
Opened 10 years ago
Closed 10 years ago
Auto-update doesn't respect mobile hotspot setting
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: tgkokk, Assigned: tgkokk)
References
Details
Attachments
(1 file, 1 obsolete file)
2.32 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release) Build ID: 20140807084340 Steps to reproduce: 1. Designate a Wi-Fi as a mobile hotspot (Settings -> Data Usage -> Mobile hotspots (overflow button)). 2. Connect to that Wi-Fi. 2. Set auto-update to "Only over Wi-Fi". 3. Wait for an update (side note: you need to have a non-Play Store version, since Play Store manages updates on apps downloaded through it). Actual results: I get the notification that there is an update for Nightly and the update starts downloading automatically. Expected results: Nightly should behave as if I were on mobile data (i.e. show a notification that there is an update and offer the choice to download it).
Assignee | ||
Updated•10 years ago
|
OS: Mac OS X → Android
Hardware: x86 → ARM
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8471583 -
Flags: review?(snorp)
Updated•10 years ago
|
Assignee: nobody → t.kokkoris
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8471643 -
Flags: review?(snorp)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8471643 [details] [diff] [review] Make auto-updater respect mobile hotspot setting OK, this is it. I haven't been able to test whether it works, because the updater requires a URL (which is hard-coded in UpdateServiceHelper.java). However, I don't see why it might not, since the change is pretty insignificant. I'll make sure to test it when it lands.
Assignee | ||
Updated•10 years ago
|
Attachment #8471583 -
Attachment is obsolete: true
Attachment #8471583 -
Flags: review?(snorp)
Comment 4•10 years ago
|
||
Comment on attachment 8471643 [details] [diff] [review] Make auto-updater respect mobile hotspot setting Review of attachment 8471643 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok, but we don't want to break things for folks using ethernet (all three of them) ::: mobile/android/base/updater/UpdateService.java @@ +220,4 @@ > */ > boolean shouldStartDownload = hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) || > autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_ENABLED || > + (autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_WIFI && !mConnectivityManager.isActiveNetworkMetered()); I think you need to keep the TYPE_ETHERNET bit in there
Attachment #8471643 -
Flags: review?(snorp) → review-
Assignee | ||
Comment 5•10 years ago
|
||
Nope, they're fine. isActiveNetworkMetered() boils down to this: http://androidxref.com/4.4.4_r1/xref/frameworks/base/services/java/com/android/server/net/NetworkPolicyManagerService.java#1588 which checks whether the network is a mobile one or if there's a policy that has metered = true (by default, no policies exist). Now, the screen for changing a WiFi network to metered basically creates a new policy with metered = true. (http://androidxref.com/4.4.4_r1/xref/packages/apps/Settings/src/com/android/settings/net/DataUsageMeteredSettings.java#140 which calls http://androidxref.com/4.4.4_r1/xref/packages/apps/Settings/src/com/android/settings/net/NetworkPolicyEditor.java#208 which creates a new policy). (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4) > Comment on attachment 8471643 [details] [diff] [review] > Make auto-updater respect mobile hotspot setting > > Review of attachment 8471643 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks ok, but we don't want to break things for folks using ethernet (all > three of them) > > ::: mobile/android/base/updater/UpdateService.java > @@ +220,4 @@ > > */ > > boolean shouldStartDownload = hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) || > > autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_ENABLED || > > + (autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_WIFI && !mConnectivityManager.isActiveNetworkMetered()); > > I think you need to keep the TYPE_ETHERNET bit in there
Assignee | ||
Updated•10 years ago
|
Attachment #8471643 -
Flags: review- → review?(snorp)
Comment 6•10 years ago
|
||
(In reply to Theodore Kokkoris [:tgkokk] from comment #5) Indeed, I misread that line and thought we were checking that the connection type is WIFI. But we're just checking the autodownload policy.
Comment 7•10 years ago
|
||
Comment on attachment 8471643 [details] [diff] [review] Make auto-updater respect mobile hotspot setting Review of attachment 8471643 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok, but we don't want to break things for folks using ethernet (all three of them) ::: mobile/android/base/updater/UpdateService.java @@ +220,4 @@ > */ > boolean shouldStartDownload = hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) || > autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_ENABLED || > + (autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_WIFI && !mConnectivityManager.isActiveNetworkMetered()); I think you need to keep the TYPE_ETHERNET bit in there
Attachment #8471643 -
Flags: review?(snorp) → review+
Comment 8•10 years ago
|
||
Not sure why it reposted my comments there...
Assignee | ||
Comment 9•10 years ago
|
||
No, actually, you're right. We're checking both if the user is on WiFi and if they are not on a mobile network. We should only check if they aren't on a mobile network. Sorry, I'm going to fix that right away.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Theodore Kokkoris [:tgkokk] from comment #9) > No, actually, you're right. We're checking both if the user is on WiFi and > if they are not on a mobile network. We should only check if they aren't on > a mobile network. Sorry, I'm going to fix that right away. Sorry everyone for spamming, the patch is actually right.
Comment 11•10 years ago
|
||
(In reply to Theodore Kokkoris [:tgkokk] from comment #10) > (In reply to Theodore Kokkoris [:tgkokk] from comment #9) > > No, actually, you're right. We're checking both if the user is on WiFi and > > if they are not on a mobile network. We should only check if they aren't on > > a mobile network. Sorry, I'm going to fix that right away. > > Sorry everyone for spamming, the patch is actually right. Yeah, you basically changed the meaning from "Autodownload on WiFi (and ethernet)" to "Autodownload on unmetered connection". I'm alright with it.
Updated•10 years ago
|
Status: NEW → ASSIGNED
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3d9d71e1de77
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment 14•10 years ago
|
||
This doesn't work on Gingerbread (isActiveNetworkMetered API 16).
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•