Closed Bug 1445923 Opened 2 years ago Closed 1 year ago

WebAudio: Remove b2g dead code

Categories

(Core :: Web Audio, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox61 --- wontfix
firefox68 --- fixed

People

(Reporter: Sylvestre, Assigned: kalraabhishek98, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

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.
I can work on this
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.
cool assign it to me
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
 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
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
(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)
Ignore what I just said.. no Audio is playing! Sorry.
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.
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)

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

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

Assignee: joseph → nobody
Type: enhancement → task

I didn't know who to tag as reviewer.

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

Flags: needinfo?(sledru)

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

Flags: needinfo?(sledru)

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

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

Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b234fb94513
Removed references of systemAppUrl along with meaningless code. r=gsvelto
Status: NEW → RESOLVED
Closed: 1 year 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.