Closed
Bug 1272270
Opened 8 years ago
Closed 8 years ago
Improve system app detection in the AudioChannel code to fix existing tests
Categories
(Firefox OS Graveyard :: AudioChannel, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: gsvelto, Assigned: gsvelto)
References
Details
Attachments
(3 files)
18.20 KB,
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
6.20 KB,
text/plain
|
Details | |
1.59 KB,
patch
|
gsvelto
:
feedback+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1254282 +++ Bug 1254282 removed the dependency on the deprecated apps API from the AudioChannel code but cut a little too deep. Identifying the system app correctly is required for correct operation. Similarly opening all channels to non-chrome code has also proven problematic and not very useful since nearly all gaia code that requires a specific audio channel is being moved into chrome anyway.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8751689 [details] [diff] [review] [PATCH] Fix detection of the system app when dealing with audio channels and fix the associated tests This patch identifies the system app window using the 'b2g.system_startup_url' preference which I also use in the tests to get the proper behavior. Also I've removed the ability for non-chrome apps to access default channels; we originally decided for it because we thought some apps that use non-default channels would stay out of chrome code but it turns out they'll become chrome too so it's pointless.
Attachment #8751689 -
Flags: review?(fabrice)
Comment 3•8 years ago
|
||
I had to add a check against uri being null while running the mochitests as chrome. Diff is trivial:
> diff --git a/dom/audiochannel/AudioChannelAgent.cpp b/dom/audiochannel/AudioChannelAgent.cpp
> index 2a94787..2919777 100644
> --- a/dom/audiochannel/AudioChannelAgent.cpp
> +++ b/dom/audiochannel/AudioChannelAgent.cpp
> @@ -121,9 +121,12 @@ AudioChannelAgent::FindCorrectWindow(nsPIDOMWindowInner* aWindow)
>
> nsCOMPtr<nsIURI> uri;
> principal->GetURI(getter_AddRefs(uri));
> + if (!uri) {
> + return NS_OK;
> + }
> +
> nsAutoCString spec;
> uri->GetSpec(spec);
> -
> if (spec.Equals(systemAppUrl)) {
> return NS_OK;
> }
I also had to fix something on the test itself (this was the reason why activestatechanged was not kicking in)
Comment 4•8 years ago
|
||
I'm also hitting a similar segfault there
Comment 5•8 years ago
|
||
Comment on attachment 8751689 [details] [diff] [review] [PATCH] Fix detection of the system app when dealing with audio channels and fix the associated tests Review of attachment 8751689 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense to me!
Attachment #8751689 -
Flags: review?(fabrice) → review+
Comment 6•8 years ago
|
||
Attachment #8751809 -
Flags: feedback?(gsvelto)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8751809 [details] [diff] [review] Hacking AudioChannel to bypass segfault I've adjusted my patch to detect this scenario.
Attachment #8751809 -
Flags: feedback?(gsvelto) → feedback+
Assignee | ||
Comment 8•8 years ago
|
||
Pushed to pine: https://hg.mozilla.org/projects/pine/rev/7f85d26e8aad The patch includes a slight change to make it forward compatible with Alexandre's patch-set in bug 1266035.
Assignee | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•