Bug 1389598 (BonkGonk)

Remove remaining gonk references

RESOLVED FIXED in Firefox 57

Status

enhancement
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

(Blocks 2 bugs)

Trunk
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Assignee

Description

2 years ago
It's no longer supported in our build config as of bug 1382930. We can now remove remaining references.
Assignee

Comment 1

2 years ago
Looks like ~130 refs left [1], mostly textual. Jesup, what's the proper way to remove the webrtc bits [2]? Do we need to upstream something?

[1] http://searchfox.org/mozilla-central/search?q=gonk&case=false&regexp=false&path=
[2] http://searchfox.org/mozilla-central/search?q=gonk&case=false&regexp=false&path=%5Emedia%2Fwebrtc
Flags: needinfo?(rjesup)
Anything in media/webrtc/signaling can be just removed
In trunk, we'll have to submit a patch upstream against the current tip of webrtc.org code
Flags: needinfo?(rjesup)
Assignee

Updated

2 years ago
Depends on: 1389609
Assignee

Comment 3

2 years ago
This is a mega-patch, I can split out some of the trickier bits. It excludes
webrtc/trunk which I split out to a blocking bug but ti sounds like maybe we
can just do those here.

MozReview-Commit-ID: ErZsvaQsMeI
Assignee

Updated

2 years ago
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee

Comment 4

2 years ago
This is the webrtc/trunk portion.

MozReview-Commit-ID: 6xFka72gnDw
Assignee

Comment 5

2 years ago
Attachment #8896516 - Flags: review?(jmuizelaar)
Assignee

Comment 6

2 years ago
Attachment #8896517 - Flags: review?(rjesup)
Assignee

Comment 7

2 years ago
Attachment #8896518 - Flags: review?(bkelly)
Assignee

Comment 8

2 years ago
Attachment #8896519 - Flags: review?(nfroyd)
Assignee

Updated

2 years ago
Attachment #8896456 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8896459 - Attachment is obsolete: true
Attachment #8896517 - Flags: review?(rjesup) → review+
Comment on attachment 8896516 [details] [diff] [review]
Part 1: Remove gonk references from gfx/

Review of attachment 8896516 [details] [diff] [review]:
-----------------------------------------------------------------

Please drop the changed to the .md files. I don't think the clarity of them is improved by dropping references to Gonk
Attachment #8896516 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8896519 [details] [diff] [review]
Part 4: Remove remaining gonk refs

Review of attachment 8896519 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

::: old-configure.in
@@ -4024,5 @@
>  fi
>  AC_SUBST(MOZ_DISABLE_STARTUPCACHE)
>  
>  dnl ========================================================
> -dnl = Enable Pico Speech Synthesis (Gonk usually)

If this is "Gonk usually", do we enable it anywhere else?  Could we remove it if so?

::: toolkit/mozapps/installer/packager.mk
@@ -28,5 @@
>  endif
>  
> -ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
> -ELF_HACK_FLAGS = --fill
> -endif

File a followup bug to remove the corresponding option from elfhack?
Attachment #8896519 - Flags: review?(nfroyd) → review+
Comment on attachment 8896518 [details] [diff] [review]
Part 3: Remove gonk references from dom/

Review of attachment 8896518 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.  In particular, please have a media peer review the OMX media changes.

::: dom/media/platforms/omx/OmxPlatformLayer.cpp
@@ +281,5 @@
>      return OMX_VIDEO_CodingUnused;
>    }
>  }
>  
> +// For platforms without OMX IL support.

I think you should get a media peer to review this change.  Its unclear to me if this makes sense or not.  We have a "for non-OMX platforms" in a file only about OMX.  I mean, does this file even get used any more?

::: dom/media/platforms/omx/OmxPromiseLayer.h
@@ -135,5 @@
>  
>      NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BufferData)
>  
>      // In most cases, the ID of this buffer is the pointer address of mBuffer.
> -    // However, in platform like gonk, it is another value.

Instead of removing this entire line, maybe change it to something like:

"However, on some platforms it may be another value."

We don't want to change the documentation of the API that the returned value may or may not be a pointer value.

::: dom/media/webrtc/MediaEngineCameraVideoSource.h
@@ +108,5 @@
>    nsTArray<RefPtr<SourceMediaStream>> mSources; // When this goes empty, we shut down HW
>    nsTArray<PrincipalHandle> mPrincipalHandles; // Directly mapped to mSources.
>    RefPtr<layers::Image> mImage;
>    RefPtr<layers::ImageContainer> mImageContainer;
> +  int mWidth, mHeight;

So if this is now unprotected, shouldn't it be outside of the comment block about things protected by mMonitor?
Attachment #8896518 - Flags: review?(bkelly) → review+
Assignee

Comment 13

2 years ago
Comment on attachment 8896518 [details] [diff] [review]
Part 3: Remove gonk references from dom/

Jeff can you take a look at the media changes in the dom patch? bkelly had some concerns about omx in particular.
Attachment #8896518 - Flags: review?(jmuizelaar)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #13)
> Comment on attachment 8896518 [details] [diff] [review]
> Part 3: Remove gonk references from dom/
> 
> Jeff can you take a look at the media changes in the dom patch? bkelly had
> some concerns about omx in particular.

You should get a media peer to look instead of me.
Assignee

Updated

2 years ago
Blocks: 1390251
Assignee

Comment 15

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #13)
> > Comment on attachment 8896518 [details] [diff] [review]
> > Part 3: Remove gonk references from dom/
> > 
> > Jeff can you take a look at the media changes in the dom patch? bkelly had
> > some concerns about omx in particular.
> 
> You should get a media peer to look instead of me.

Oh sorry, I meant to tag jesup! I'll redirect.
Assignee

Comment 16

2 years ago
Comment on attachment 8896518 [details] [diff] [review]
Part 3: Remove gonk references from dom/

Review of attachment 8896518 [details] [diff] [review]:
-----------------------------------------------------------------

Randell, can you take a look at the media parts of this patch or redirect to someone who can? Ben had some concerns about the OMX changes in particular.
Attachment #8896518 - Flags: review?(jmuizelaar) → review?(rjesup)
Assignee

Comment 17

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> Comment on attachment 8896516 [details] [diff] [review]
> Part 1: Remove gonk references from gfx/
> 
> Review of attachment 8896516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please drop the changed to the .md files. I don't think the clarity of them
> is improved by dropping references to Gonk

Updated locally.
Assignee

Comment 18

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #11)
> Comment on attachment 8896519 [details] [diff] [review]
> Part 4: Remove remaining gonk refs
> 
> Review of attachment 8896519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me
> 
> ::: old-configure.in
> @@ -4024,5 @@
> >  fi
> >  AC_SUBST(MOZ_DISABLE_STARTUPCACHE)
> >  
> >  dnl ========================================================
> > -dnl = Enable Pico Speech Synthesis (Gonk usually)
> 
> If this is "Gonk usually", do we enable it anywhere else?  Could we remove
> it if so?

Emailed dev-platform [1], I don't know the state of it.

> ::: toolkit/mozapps/installer/packager.mk
> @@ -28,5 @@
> >  endif
> >  
> > -ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
> > -ELF_HACK_FLAGS = --fill
> > -endif
> 
> File a followup bug to remove the corresponding option from elfhack?

Filed bug 1390251.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/qASWiozASTs/8Rt-h6oiAgAJ
Assignee

Comment 19

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #12)
> Comment on attachment 8896518 [details] [diff] [review]
> Part 3: Remove gonk references from dom/
> 
> Review of attachment 8896518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.  In particular, please have a media peer
> review the OMX media changes.
> 
> ::: dom/media/platforms/omx/OmxPlatformLayer.cpp
> @@ +281,5 @@
> >      return OMX_VIDEO_CodingUnused;
> >    }
> >  }
> >  
> > +// For platforms without OMX IL support.
> 
> I think you should get a media peer to review this change.  Its unclear to
> me if this makes sense or not.  We have a "for non-OMX platforms" in a file
> only about OMX.  I mean, does this file even get used any more?

I've tagged a media peer. I'm not sure about the state of OMX, my guess is we can just make it a final class and get rid of the virtual functions. That's probably more of a follow-up.

> 
> ::: dom/media/platforms/omx/OmxPromiseLayer.h
> @@ -135,5 @@
> >  
> >      NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BufferData)
> >  
> >      // In most cases, the ID of this buffer is the pointer address of mBuffer.
> > -    // However, in platform like gonk, it is another value.
> 
> Instead of removing this entire line, maybe change it to something like:
> 
> "However, on some platforms it may be another value."
> 
> We don't want to change the documentation of the API that the returned value
> may or may not be a pointer value.

Updated locally, but if we make this final we can just get rid of the comment.

> ::: dom/media/webrtc/MediaEngineCameraVideoSource.h
> @@ +108,5 @@
> >    nsTArray<RefPtr<SourceMediaStream>> mSources; // When this goes empty, we shut down HW
> >    nsTArray<PrincipalHandle> mPrincipalHandles; // Directly mapped to mSources.
> >    RefPtr<layers::Image> mImage;
> >    RefPtr<layers::ImageContainer> mImageContainer;
> > +  int mWidth, mHeight;
> 
> So if this is now unprotected, shouldn't it be outside of the comment block
> about things protected by mMonitor?

Moved out of the comment block.
Assignee

Comment 20

2 years ago
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #18)
> Emailed dev-platform [1], I don't know the state of it.

Eitan pointed me to bug 1331694, so it's on it's way out.
Comment on attachment 8896518 [details] [diff] [review]
Part 3: Remove gonk references from dom/

Review of attachment 8896518 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webrtc/MediaEngineCameraVideoSource.h
@@ +108,5 @@
>    nsTArray<RefPtr<SourceMediaStream>> mSources; // When this goes empty, we shut down HW
>    nsTArray<PrincipalHandle> mPrincipalHandles; // Directly mapped to mSources.
>    RefPtr<layers::Image> mImage;
>    RefPtr<layers::ImageContainer> mImageContainer;
> +  int mWidth, mHeight;

> So if this is now unprotected, shouldn't it be outside of the comment block about things protected by mMonitor?

Yes, move it outside the protection block.

::: dom/xhr/tests/mochitest.ini
@@ +103,5 @@
>  skip-if = toolkit == 'android'
>  [test_xhr_send.html]
>  [test_xhr_send_readystate.html]
>  [test_XHR_system.html]
> +skip-if = (buildapp == 'b2g') # b2g-debug(12 total, 2 failing - .mozSystem == true - got false, expected true + ) b2g-desktop(12 total, 2 failing - .mozSystem == true - got false, expected true + )

Does it still make sense to have buildapp == 'b2g' skip?
Attachment #8896518 - Flags: review?(rjesup) → review+
Assignee

Comment 22

2 years ago
(In reply to Randell Jesup [:jesup] from comment #21)
> ::: dom/xhr/tests/mochitest.ini
> @@ +103,5 @@
> >  skip-if = toolkit == 'android'
> >  [test_xhr_send.html]
> >  [test_xhr_send_readystate.html]
> >  [test_XHR_system.html]
> > +skip-if = (buildapp == 'b2g') # b2g-debug(12 total, 2 failing - .mozSystem == true - got false, expected true + ) b2g-desktop(12 total, 2 failing - .mozSystem == true - got false, expected true + )
> 
> Does it still make sense to have buildapp == 'b2g' skip?

Not really, but we can take care of that in a different bug.

Updated

7 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.