1445923 - WebAudio: Remove b2g dead code

Status: RESOLVED FIXED

(Core :: Web Audio, task, P3)

Description

[Sylvestre Ledru [:Sylvestre]]

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

[Videet Singhai]

I can work on this

Comment 2

[Videet Singhai]

we need to remove this right?

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

Comment 3

[Sylvestre Ledru [:Sylvestre]]

I think you should also remove the rest related to systemAppUrl.

Comment 4

[Videet Singhai]

cool assign it to me

Comment 5

[Videet Singhai]

Attachment: Bug 1445923: removed b2g dead code from WebAudio

Review commit: https://reviewboard.mozilla.org/r/228088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228088/

Comment 6

[Videet Singhai]

hey let me know if any changes are required

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Yeah, please try to compile fx with your change. I don't think it is working.

Comment 8

[Videet Singhai]

 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 [e-mail]...
 2:51.07 Packaging [e-mail]...
 2:51.28 Packaging [e-mail]...
 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?

Comment 9

[Sylvestre Ledru [:Sylvestre]]

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? :)

Comment 10

[Videet Singhai]

so I need to keep the systemAppUrl reference because it is used in other places?

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 12

[Videet Singhai]

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?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 14

[Videet Singhai]

Hey, I will need some time for it, till that can I push this code?

Also, What does getCString() do?

Comment 15

[Sylvestre Ledru [:Sylvestre]]

(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

Comment 16

[Videet Singhai]

(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?

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 18

[Manish [:manishkk]]

:Videet 
still you are working on this?

if not, Can I fix this?

Comment 19

[Videet Singhai]

Sure

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Resetting for lack of progress.
Someone else can take it!

Comment 21

[Joseph]

(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.

Comment 22

[Joseph]

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

Comment 23

[Joseph]

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

[Joseph]

It builds and audio works if I leave in line 143. (still leaving out 144-146).

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Redirecting to Paul as I am on pto

Comment 26

[Arlak]

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

Comment 27

[Sylvestre Ledru [:Sylvestre]]

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

Comment 28

[Arlak]

Attachment: Bug 1445923 - Removed references of systemAppUrl along with meaningless code.

Comment 29

[Arlak]

I didn't know who to tag as reviewer.

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

Comment 30

[Sylvestre Ledru [:Sylvestre]]

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

Comment 31

[Arlak]

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

Comment 32

[Arlak]

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b234fb94513
Removed references of systemAppUrl along with meaningless code. r=gsvelto

Comment 34

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5b234fb94513