903260 - [SMS][MMS] SettingsURL value is never used in sms/js/notification.js: Ringtones BROKEN.

Status: RESOLVED FIXED

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

Description

[Rick Waldron [:rwaldron]]

In this commit: https://github.com/mozilla-b2g/gaia/commit/90245e0e244063545b83f6b1f21279a04bb077d4#L6R11 The value isn't properly set to the localy cached settings object, later when the ringtone's Audio `src` is set, the value being set is `true`.


This commit flagrantly ignored the review that I provided on an earlier version and changes the approach used from the last commit I was shown here: https://github.com/KevinGrandon/gaia/commit/a07ac05622f3e0089449cda37ddc078f2fce59eb#apps/gallery/test/unit/settings_url_test.js

Comment 1

[Rick Waldron [:rwaldron]]

This is the original commit that I reviewed https://github.com/mozilla-b2g/gaia/pull/11431

Comment 2

[Rick Waldron [:rwaldron]]

This is the "outdated discussion" where I clarified the correct (yet ignored) approach: https://github.com/mozilla-b2g/gaia/pull/11431#discussion_r5667602

Comment 3

[Rick Waldron [:rwaldron]]

Attachment: Github Pull Request https://github.com/mozilla-b2g/gaia/pull/11447

Comment 4

[Kevin Grandon :kgrandon]

Thanks for taking care of this so quickly! The offending bug should've definitely been split up into various meta bugs and have had the appropriate reviewers for each app. This is something that Julien brought up in the last gaia weekly meeting. Will be sure to follow that methodology from here on out.

Comment 5

[Julien Wajsberg [:julienw]]

Or you could ask different reviewers for different parts of your patch too, would work too :) But most importantly, follow recommandations from reviewers ;)

Will review this shortly.

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 787947 [details] [review]
Github Pull Request https://github.com/mozilla-b2g/gaia/pull/11447

put some comments on github

I'd like to see how my comment in https://github.com/mozilla-b2g/gaia/commit/90245e0e244063545b83f6b1f21279a04bb077d4#commitcomment-3828736 will evolve, I sincerely hope we can implement this SettingsURL stuff correctly...

Comment 7

[Kevin Grandon :kgrandon]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Comment on attachment 787947 [details] [review]
> Github Pull Request https://github.com/mozilla-b2g/gaia/pull/11447
> 
> I'd like to see how my comment in
> https://github.com/mozilla-b2g/gaia/commit/
> 90245e0e244063545b83f6b1f21279a04bb077d4#commitcomment-3828736 will evolve,
> I sincerely hope we can implement this SettingsURL stuff correctly...

Hi Julien, makes sense. I'm going to copy your comment to the bug for the pr (bug 806374), and request info on it - I didn't write the patch. 

Also - I am not opposed to backing this out if you feel strongly about it.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 787947 [details] [review]
Github Pull Request https://github.com/mozilla-b2g/gaia/pull/11447

r=me with the nits in the test

Comment 9

[Rick Waldron [:rwaldron]]

master https://github.com/mozilla-b2g/gaia/commit/83af4fcb0f28018a42db640bddd93953a675d45a

v1-train https://github.com/mozilla-b2g/gaia/commit/63c8779713feab8dd6172496f4d5d3fe785c9927

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 5628b2ca37bea107d5da7f03959f70def2d55676
v1.1.0hd: 63c8779713feab8dd6172496f4d5d3fe785c9927

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/117ffe65bc76
https://hg.mozilla.org/mozilla-central/rev/71544e10532f

Comment 12

[Julien Wajsberg [:julienw]]

so, it shouldn't have been pushed to v1-train, as bug 806374 is not on v1-train.

Reverting.

Comment 13

[Julien Wajsberg [:julienw]]

v1-train: reverted in f7d6c5fe66dc64ec86c9737b959e1ceb3cf4f707

Tim, I suppose the 1.1.0hd branch will get updated at the next merge ?

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: f7d6c5fe66dc64ec86c9737b959e1ceb3cf4f707

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Merged.