WebAudio: Remove b2g dead code

RESOLVED FIXED in Firefox 68

Status

()

task
P3
normal
Rank:
25
RESOLVED FIXED
Last year
11 days ago

People

(Reporter: sylvestre, Assigned: kalraabhishek98, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox68 fixed)

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(2 attachments)

https://dxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelAgent.cpp#100

We should remove this line and the other impacted.

I can mentor this bug.

Comment 1

Last year
I can work on this

Comment 2

Last year
we need to remove this right?

 nsresult rv =
    mozilla::Preferences::GetCString("b2g.system_startup_url", systemAppUrl);
  if (NS_FAILED(rv)) {
    return NS_OK;
  }
I think you should also remove the rest related to systemAppUrl.

Comment 4

Last year
cool assign it to me
Comment hidden (mozreview-request)

Comment 6

Last year
hey let me know if any changes are required
Yeah, please try to compile fx with your change. I don't think it is working.
Assignee: nobody → videetssinghai

Comment 8

Last year
 2:42.68 Elapsed: 65.47s; From /home/firefox-dev/mozilla-central/objdir-frontend/dist/bin/browser: Kept 2991 existing; Added/updated 76; Removed 0 files and 0 directories.
 2:44.17 Elapsed: 0.10s; From ../../dist/idl: Kept 908 existing; Added/updated 0; Removed 0 files and 0 directories.
 2:46.05   adding: install.rdf (deflated 53%)
 2:46.11   adding: plugins/libnptest.so (deflated 66%)
 2:46.18   adding: plugins/libnpsecondtest.so (deflated 66%)
 2:46.25   adding: plugins/libnpthirdtest.so (deflated 66%)
 2:46.33   adding: plugins/libnpswftest.so (deflated 66%)
 2:50.90 Packaging specialpowers@mozilla.org.xpi...
 2:51.07 Packaging quitter@mozilla.org.xpi...
 2:51.28 Packaging mozscreenshots@mozilla.org.xpi...
 2:51.42 0 compiler warnings present.
 2:51.45 Overall system resources - Wall time: 162s; CPU: 60%; Read bytes: 430964736; Write bytes: 46182400; Read time: 487276; Write time: 28648
 2:51.56 Your build was successful!
To view resource usage of the build, run |mach resource-usage|.
To take your build for a test drive, run: |mach run|
For more information on what to do now, see https://developer.mozilla.org/docs/Developer_Guide/So_You_Just_Built_Firefox



Here it shows fine. Can you send your output?
Rank: 25
Priority: -- → P3
I don't think you actually built it. Just by reading at the code, I can tell you that it won't build.

Can you find why? :)
so I need to keep the systemAppUrl reference because it is used in other places?
No, this is something else. Comment #8 isn't possible with the change you submitted.
Let me know if you don't find what is wrong.
The code to be removed - 

nsAutoCString systemAppUrl;
  nsresult rv =
    mozilla::Preferences::GetCString("b2g.system_startup_url", systemAppUrl);
  if (NS_FAILED(rv)) {
    return NS_OK;
  }
.
.
.
.
.
.
if (spec.Equals(systemAppUrl)) {
      return NS_OK;
    }

Is this correct?
good, you found why I was saying it cannot work :) Bravo!

I am not an expert in that code but I think you should investigate in which context we reach line 113.
If this is when b2g.system_startup_url doesn't exit, you can probably remove more code.
Hey, I will need some time for it, till that can I push this code?

Also, What does getCString() do?
(In reply to Videet Singhai from comment #14)
> Hey, I will need some time for it, till that can I push this code?
Take your time, there is no rush.
 
> Also, What does getCString() do?
See:
https://dxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.cpp?q=%2Bfunction%3A%22mozilla%3A%3APreferences%3A%3AGetCString%28const+char+%2A%2C+nsACString+%26%2C+mozilla%3A%3APrefValueKind%29%22&redirect_type=single#4052
Reporter

Updated

Last year
Blocks: nukeb2g
(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> good, you found why I was saying it cannot work :) Bravo!
> 
> I am not an expert in that code but I think you should investigate in which
> context we reach line 113.
> If this is when b2g.system_startup_url doesn't exit, you can probably remove
> more code.

hey I am free now and ready to work more on this bug.

Can you tell me how would I know which code is to be removed?
You should investigate what the variable is used for and what the functions are doing with it.
You should also make sure that the code still builds with the changes.
:Videet 
still you are working on this?

if not, Can I fix this?
Flags: needinfo?(videetssinghai)
Sure
Flags: needinfo?(videetssinghai)
Resetting for lack of progress.
Someone else can take it!
Assignee: videetssinghai → nobody

Comment 21

Last year
(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> Resetting for lack of progress.
> Someone else can take it!

Hi there! I have removed the following code mentioned above, as well as lines 143-146 (although I'm unsure about removing these lines.) 

https://dxr.mozilla.org/mozilla-central/rev/99c19a66c3a2fbf8108d4b8a161cded31e948409/dom/audiochannel/AudioChannelAgent.cpp#109-114,124-126,143-146 the lines highlighted have been removed.

The code seems to build fine with these changes.

From looking around, it seems like this is the only place `FindCorrectWindow` is called.
Flags: needinfo?(sledru)

Comment 22

Last year
Ignore what I just said.. no Audio is playing! Sorry.

Comment 23

Last year
Re-adding lines 143-146 seemed to fix the audio not playing. I'll have to look around more for stuff to remove (I'm new to this!).

It doesn't help that my builds are taking 20 minutes each time.

Comment 24

Last year
It builds and audio works if I leave in line 143. (still leaving out 144-146).
Redirecting to Paul as I am on pto
Flags: needinfo?(sledru) → needinfo?(padenot)
Assignee: nobody → joseph
Flags: needinfo?(padenot)
Assignee

Comment 26

3 months ago

Can I work on this? I'm new to open-source.

Reporter

Comment 27

3 months ago

Yeah, please submit a patch and we will assign you the bug.

Assignee: joseph → nobody
Reporter

Updated

3 months ago
Type: enhancement → task
Assignee

Comment 29

3 months ago

I didn't know who to tag as reviewer.

Review Needed: https://phabricator.services.mozilla.com/D26503

Assignee

Updated

3 months ago
Flags: needinfo?(sledru)
Reporter

Comment 30

3 months ago

You should have a look at the history of the file to find the right person

Flags: needinfo?(sledru)
Assignee

Comment 31

3 months ago

I tagged smaug. Correct me if I'm wrong.

Assignee

Comment 32

3 months ago

"This revision is now accepted and ready to land."

Comment 33

3 months ago
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b234fb94513
Removed references of systemAppUrl along with meaningless code. r=gsvelto

Comment 34

3 months ago
bugherder
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → kalraabhishek98
You need to log in before you can comment on or make changes to this bug.