Closed Bug 976951 Opened 7 years ago Closed 7 years ago

Customize network indicator by SIM displayed in quick settings

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: macajc, Assigned: macajc)

References

Details

(Whiteboard: [systemsfe])

Attachments

(3 files, 1 obsolete file)

The data network Icon displayed in quick settings will be customized per operator (based on mcc/mnc). Please refer to Bug 923449.
Target Milestone: --- → 1.4 S2 (28feb)
Attached file Proposed patch v1 (obsolete) —
Attachment #8382062 - Flags: review?(kaze)
The solution for this should behave similar to bug 970929. For this reason, the proposed patch follows the same philosophy. Fabien, you reviewed the last one and know how it works, that's the main reason I've solicited your review, if you think somebody else should review this, please let me know. Thanks!!
Kaze, are you reviewing this bug? :-)
Flags: needinfo?(kaze)
I am. I’m a bit puzzled because I find it very hacky. In order to replace an icon, this patch:
 • gets a stylesheet from a hard-coded URL;
 • checks all style rules matching a selector class (with a regexp…);
 • modifies the background-image in these style rules to use the replacement icon.

This is not as generic / solid as the related patches I’ve reviewed so far. This looks too breakable, especially without unit tests.
Comment on attachment 8382062 [details] [review]
Proposed patch v1

Carmen, have you considered replacing all these `data-*.png' icons by a CSS sprite? That would allow to leave the stylesheet unchanged, and the replacement sprite image could be attached directly to the `#quick-settings-data' element. That would seem much more solid to me.

I understand that it would be a bit less flexible if we want to replace only one data icon, but on the other hand it will ensure a proper visual consistency.

Please let me know if this approach is suitable for your needs. Thanks!
Attachment #8382062 - Flags: review?(kaze) → review-
Flags: needinfo?(kaze)
(In reply to Fabien Cazenave [:kaze] from comment #5)
> Comment on attachment 8382062 [details] [review]
> Proposed patch v1
> 
> Carmen, have you considered replacing all these `data-*.png' icons by a CSS
> sprite? That would allow to leave the stylesheet unchanged, and the
> replacement sprite image could be attached directly to the
> `#quick-settings-data' element. That would seem much more solid to me.
> 
> I understand that it would be a bit less flexible if we want to replace only
> one data icon, but on the other hand it will ensure a proper visual
> consistency.
> 
> Please let me know if this approach is suitable for your needs. Thanks!

Fabien, thanks very much for the suggestion. When making this change, one of my targets was minimize the memory impact of giving support to a set of operators on a single build. 
Ideally, the memory footprint of system should not be affected by the number of operators that share the same build (which can be a big number in theory at least). 
Changing the images to sprites would mean adding a sprite for each of the operators that want to have a different icon (all of them on the worst case). 
That would mean increasing the memory size of the images used for that and thus it would impact on the memory footprint of the system app

That's why I prefer my current solution.
There's another problem about using CSS sprites, and it's that it'll make the build process harder for OEMS (since they'll be the ones that will have to probably build the CSS sprites). 
In any case, if you prefer that solution let me know and I'll change it.
Flags: needinfo?(kaze)
I've updated the patch to add the unit test
To sum up the discussion we had on IRC: I’m okay with Carmen’s patch if we get unit tests that break when the CSS we depend on is changed — that’s not the case at the moment.

If it turns out it can’t be properly tested, we should go for the CSS sprite approach.

Regarding the memory footprint, I’ve just counted the weight of all `data-[type]-[on|off].png' icons in system/style/quicksettings/images. That’s 3.8kB for all icons (4.8kB in @1.5x, 5.5kB in @2x): I think we can afford it.

I’m not afraid either of making the process harder for OEMs, there are plenty of (even online) tools to make CSS sprites nowadays: if you can make an icon, you can make a sprite.
Flags: needinfo?(kaze)
Last word about the memory footprint of the CSS sprite… I just had a quick talk with Fabrice on IRC. He says we should not worry about the additional memory footprint of the System zip, as long as we don’t load all sprites in the DOM; and he confirms that if we change the background image of the `#quick-settings-data' element, the default background will be properly unloaded by Gecko.
Just to clarify the goal of this bug, some operators want to change some technology names for commercial reasons, the most common case we saw in some operators was displaying 4G for LTE. They do not want to change all possible values, just one or two and taking default values for remaining ones. So, we should make customization as easier as possible for them, giving them the option to only change some values instead of all of them at the same time.
That’s what my proposal would allow, yes.
Attached file Proposed patch v2
Attachment #8383695 - Flags: review?(kaze)
Attachment #8382062 - Attachment is obsolete: true
This version is done following what we talked on IRC. Sorry about the delay (most of the time was spent shuffling pixels ;)). This solution doesn't need unit tests because the changes are mostly done on the CSS file.
Comment on attachment 8383695 [details] [review]
Proposed patch v2

As you were worried by the fact Operators could make mistakes when providing this CSS sprite, I’d recommend ordering it in a more natural way — specifically, the “off” state between the two “edge” icons is not natural.

The rest looks fine. Glad to see the resulting PNG is less than 2kB, that should be an acceptable memory footprint. :-)
Attachment #8383695 - Flags: review?(kaze) → review-
(In reply to Fabien Cazenave [:kaze] from comment #15)
> Comment on attachment 8383695 [details] [review]
> Proposed patch v2
> 
> As you were worried by the fact Operators could make mistakes when providing
> this CSS sprite, I’d recommend ordering it in a more natural way —
> specifically, the “off” state between the two “edge” icons is not natural.
Are we really arguing about just the orders of the icons on a sprite that will be generated automatically from a set of icons? (and thus the operator or the OEM) won't really see the internal order ever. 

There should be documentation saying how the icon has to bee generated, though, but that's part of the build process and that's not this bug, I think. 


> 
> The rest looks fine. Glad to see the resulting PNG is less than 2kB, that
> should be an acceptable memory footprint. :-)
(In reply to Fabien Cazenave [:kaze] from comment #15)
> Comment on attachment 8383695 [details] [review]
> Proposed patch v2
> 
> As you were worried by the fact Operators could make mistakes when providing
> this CSS sprite, I’d recommend ordering it in a more natural way —
> specifically, the “off” state between the two “edge” icons is not natural.
> 
> The rest looks fine. Glad to see the resulting PNG is less than 2kB, that
> should be an acceptable memory footprint. :-)

The off icon is on the middle 'cause when no type is applied it shows the central point of the png.
With this sprite we don't need to do more changes on js or css
I’m always worried to land sub-optimal patches under pressure but I don’t want to block either.

I’m rushing to propose a slightly better patch within the next hour; if I can’t, I’ll just r+ your patch and trust Rik to make it better in a follow-up.
Depends on: 978161
Attached file link to pull request
Here’s a quick patch to address the comments I made earlier. Vivien, feel free to ignore this patch and r+ Carmen’s patch if you think it was OK. Thanks!
Attachment #8383815 - Flags: review?(21)
Suboptimal? Suboptimal?! I don't know if I should be angered or amused :P. Anyway, whatever the order of the icons is, it'll be a randomly chosen order. One that makes sense to you but not necessarily to me or anybody else. If you're happier with this order, I don't really care one way or the other
Can't we simply add 'LTE' in the sprite and let the operator opt-in this via the operator variant database ?

This way when someone change the SIM into his phone, the preference of the current operator will be be honored instead of having to stick to the choice of the default operator ?
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #21)
> Can't we simply add 'LTE' in the sprite and let the operator opt-in this via
> the operator variant database ?
> 
> This way when someone change the SIM into his phone, the preference of the
> current operator will be be honored instead of having to stick to the choice
> of the default operator ?

The idea of all of the single variant bugs in general, and that includes this one, is that the operators/OEMs can share a single build for a set of operators. That way they'll save on building, transport, storage... costs because they have a single product that's valid for several operators. That said, the builds should behave as close as non single variant builds. That is, once the initial operator has been set (usually the one that's actually sold you the phone) that's it, and from that moment onwards that phone is from *that* particular operator.

And that includes erasing all the resources for all the other operators the moment one has coalesced. Think of of as a Schrödinger phone :) It's on a indeterminate state until a operator has been chosen, but from that moment it has a known and fixed state. On other words, the initial operator icon set is the one you get even if you change your SIM to another operator (you also get the first operator set of apps, contact numbers,... everything).
(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #22)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #21)
> > Can't we simply add 'LTE' in the sprite and let the operator opt-in this via
> > the operator variant database ?
> > 
> > This way when someone change the SIM into his phone, the preference of the
> > current operator will be be honored instead of having to stick to the choice
> > of the default operator ?
> 
> The idea of all of the single variant bugs in general, and that includes
> this one, is that the operators/OEMs can share a single build for a set of
> operators. That way they'll save on building, transport, storage... costs
> because they have a single product that's valid for several operators. That
> said, the builds should behave as close as non single variant builds. That
> is, once the initial operator has been set (usually the one that's actually
> sold you the phone) that's it, and from that moment onwards that phone is
> from *that* particular operator.
> 
> And that includes erasing all the resources for all the other operators the
> moment one has coalesced. Think of of as a Schrödinger phone :) It's on a
> indeterminate state until a operator has been chosen, but from that moment
> it has a known and fixed state. On other words, the initial operator icon
> set is the one you get even if you change your SIM to another operator (you
> also get the first operator set of apps, contact numbers,... everything).

I still don't think that the right way to do it, but as I said on IRC I'm not going to block on that as this can be address in a followup.
(In reply to Carmen Jimenez Cabezas from comment #20)
> Suboptimal? Suboptimal?! I don't know if I should be angered or amused :P.

The patch I propose is not optimal either. I’ve had two nits about your PR and you didn’t want to address them. No big deal, but we have a review process to follow so I just propose another patch and I even propose the new reviewer to land yours, whatever fits.

Sorry if that sounded harsh to you. The point was to keep your code (you’re even credited for the JS part in my PR) and not bother you with my review nits.
Comment on attachment 8383695 [details] [review]
Proposed patch v2

r+ with Kaze's changes on top of it and the comments I made fixed.
Attachment #8383695 - Flags: review- → review+
Comment on attachment 8383815 [details] [review]
link to pull request

Let's merge this one in the other patch as that's basically just an image change.
Attachment #8383815 - Flags: review?(21)
Merged on master: https://github.com/mozilla-b2g/gaia/commit/61391cd5b77e5085dd1362eee7014c56fefbbdfb

Thanks for the changes, and sorry again for the confusion. I owe you a couple beers. :-)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I had to revert this for breaking gaia-unit test in https://github.com/mozilla-b2g/gaia/commit/49fb28df5c36a59c28d1e8a9c60fc7a189f82491

https://tbpl.mozilla.org/php/getParsedLog.php?id=35453512&tree=B2g-Inbound

gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/quick_settings_test.js | quick settings > "before each" hook | SettingsHelper is not defined
ReferenceError: SettingsHelper is not defined
Return code: 10
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
looking
Strange, Travis was green. Investigating.
Comment on attachment 8383815 [details] [review]
link to pull request

This line was missing: https://github.com/mozilla-b2g/gaia/pull/16743/files#diff-50

Vivien, can you have a look, please? That’s Carmen’s patch with the missing `requireApp('system/shared/js/settings_helper.js')` statement.
Attachment #8383815 - Flags: review?(21)
Comment on attachment 8383815 [details] [review]
link to pull request

We need to use a mock for settings helper, not the real file.
Attachment #8383815 - Flags: review?(21) → review-
My take on it :) 
The reason TBPL is complaining is because we run each test individually on TBPL. So another test may have loaded settings_helper and we don't see that on Travis.

When running only this test, we also had a "Mozactivity global leak". So I replaced the ad-hoc inits of the mocks with MocksHelper.
And I also took the opportunity to replace requireApp with require because we're gonna deprecate it (bug 978103).
Attachment #8384118 - Flags: review?(21)
There's another, more complete, mock for SettingsHelper that I included as part of bug 923444. With all the dance of PRs yesterday the line that included that mock was lost, I'm sorry. 
Anyway, Rik change should work also because the way the test is done it doesn't really matter what value the preference has.
Then again, hadn't I removed the try/catch, a missing mock wouldn't have made the test fails. That's a unforeseen complication ;)
Agreed. But getting bitten by tests should be considered as an unforeseen bonus. :-)

Thanks for the update. Sorry I didn’t notice your helper in system/test/unit — I should have used it. My bad.
Comment on attachment 8384118 [details]
https://github.com/mozilla-b2g/gaia/pull/16761/files

My turn! :-)

https://travis-ci.org/mozilla-b2g/gaia/jobs/19859594
> 1) [system] init_logo_handler_test.js > "before each" hook:
> TypeError: window.SettingsHelper.mSetup is not a function

I’ve just updated my PR to include your helpful fixes on quick_settings_tests.js, but using Carmen’s mock for SettingsHelper. Tests pass locally, waiting for Travis…
Attachment #8384118 - Flags: review-
Comment on attachment 8383815 [details] [review]
link to pull request

Travis is OK with the System app (though not green, because of Calendar right now). 

Vivien, sorry to bother you again.
Attachment #8383815 - Flags: review- → review?(21)
Merged on master:
https://github.com/mozilla-b2g/gaia/commit/135e95722dfff83340cdf0ee686f7d8d61ed7bef
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Sorry I didn't see your mock Carmen, I took the branch from the PR and it hasn't landed when that branch was created. My bad.
You need to log in before you can comment on or make changes to this bug.