Remove MOZ_WIDGET_GONK completely

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

54 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(12 attachments, 2 obsolete attachments)

19.15 KB, patch
erahm
: review+
njn
: checkin+
Details | Diff | Splinter Review
19.87 KB, patch
glandium
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.48 KB, patch
glandium
: review+
njn
: checkin+
Details | Diff | Splinter Review
10.29 KB, patch
jld
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.02 KB, patch
glandium
: review+
njn
: checkin+
Details | Diff | Splinter Review
9.38 KB, patch
kats
: review+
njn
: checkin+
Details | Diff | Splinter Review
7.90 KB, patch
jld
: review+
njn
: checkin+
Details | Diff | Splinter Review
19.28 KB, patch
mccr8
: review+
njn
: checkin+
Details | Diff | Splinter Review
24.61 KB, patch
gerald
: review+
njn
: checkin+
Details | Diff | Splinter Review
26.75 KB, patch
mccr8
: review+
jdm
: review+
njn
: checkin+
Details | Diff | Splinter Review
7.21 KB, patch
snorp
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.90 KB, patch
gsvelto
: review+
njn
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
Gonk widget support is dead.

- bug 1355752 comment 25 is where the decision was made to stop supporting it.

- Bug 1357323 removed support for building the Gonk widget code, as well as dom/system/gonk/, hal/gonk/, and some other Gonk-specific code.

- Bug 1373478 removed widget/gonk/.

This bug is about removing all remaining traces of MOZ_WIDGET_GONK.
(Assignee)

Comment 1

2 years ago
As well as the straightforward things, this lets us remove ReadSysFile and
WriteSysFile, which in turn lets us remove TestFileUtils.cpp.
Attachment #8887765 - Flags: review?(erahm)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Attachment #8887768 - Flags: review?(mh+mozilla)
(Assignee)

Comment 3

2 years ago
Attachment #8887770 - Flags: review?(mh+mozilla)
(Assignee)

Comment 5

2 years ago
Attachment #8887786 - Flags: review?(mh+mozilla)
(Assignee)

Comment 6

2 years ago
Attachment #8887789 - Flags: review?(gsvelto)
(Assignee)

Updated

2 years ago
Attachment #8887789 - Attachment is obsolete: true
Attachment #8887789 - Flags: review?(gsvelto)
(Assignee)

Comment 8

2 years ago
Attachment #8887795 - Flags: review?(jld)
Attachment #8887768 - Flags: review?(mh+mozilla) → review+
Attachment #8887770 - Flags: review?(mh+mozilla) → review+
Attachment #8887786 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8887765 [details] [diff] [review]
Remove MOZ_WIDGET_GONK from xpcom/

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

Nice cleanup, thanks!
Attachment #8887765 - Flags: review?(erahm) → review+
Comment on attachment 8887781 [details] [diff] [review]
Remove MOZ_WIDGET_GONK from security/

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

r=me.  A couple comments, but no need to change anything.

::: security/manager/pki/nsNSSDialogHelper.cpp
@@ -22,5 @@
> -#ifdef MOZ_WIDGET_GONK
> -  // On b2g devices, we need to proxy the dialog creation & management
> -  // to Gaia.
> -  return NS_ERROR_NOT_IMPLEMENTED;
> -#endif

I'm not officially a peer for this, but it seems reasonable.

::: security/sandbox/linux/broker/SandboxBroker.cpp
@@ -459,5 @@
> -  static const long nr_setreuid = __NR_setreuid;
> -  static const long nr_setregid = __NR_setregid;
> -#endif
> -  if (syscall(nr_setregid, getgid(), AID_APP + mChildPid) != 0 ||
> -      syscall(nr_setreuid, getuid(), AID_APP + mChildPid) != 0) {

Thanks for reminding me about this.  I filed bug 1382246 for some related cleanup.

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
@@ +39,5 @@
>  SandboxBrokerPolicyFactory::SandboxBrokerPolicyFactory()
>  {
>    // Policy entries that are the same in every process go here, and
>    // are cached over the lifetime of the factory.
> +#if defined(MOZ_CONTENT_SANDBOX)

FYI, this part is likely to have merge conflicts with bug 1308400.
Attachment #8887781 - Flags: review?(jld) → review+
Comment on attachment 8887795 [details] [diff] [review]
Remove MOZ_WIDGET_GONK from ipc/

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

r=me.  Again, there are some additional removals that can happen, but those can happen in bug 1316153; this patch is fine as-is.

::: ipc/chromium/src/base/process_util_linux.cc
@@ +26,3 @@
>  /*
> + * We fall back to an arbitrary UID. This is generally the UID for user
> + * `nobody', albeit it is not always the case.

As far as I know we can rip out *all* the stuff about uids/gids here; B2G was the only platform where it could be used.  (I think the “platforms that are not gonk based” comment was a reference to porting B2G to a non-Android-based Linux, like what bug 731498 proposed.)

Bug 1316153 already covers this, so I'll comment over there to explicitly mention it.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +76,5 @@
>  #endif
>  
> +// We currently don't drop privileges on any platform, because we have to worry
> +// about plugins and extensions breaking.
> +static const bool kLowRightsSubprocesses = false;

This can be constant-propagated, but that cleanup is also part of bug 1316153.
Attachment #8887795 - Flags: review?(jld) → review+
(Assignee)

Updated

2 years ago
Attachment #8887768 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8887770 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8887786 - Flags: checkin+
(Assignee)

Comment 14

2 years ago
As well as the obvious #ifdefs, this allows DOMHwMediaStream to be
removed, and also the "phone-state-changed" observer.
Attachment #8888149 - Flags: review?(gsquelart)
(Assignee)

Comment 15

2 years ago
As well as the obvious #ifdef stuff, the patch removes
TCPSocket::SetAppIdAndBrowser(), which means
{TCPSocketParent,TCPServerSocketParent}::{GetAppId,GetInIsolatedMozBrowser}()
can also be removed.
Attachment #8888163 - Flags: review?(continuation)
(Assignee)

Updated

2 years ago
Attachment #8888171 - Attachment is obsolete: true
Attachment #8888171 - Flags: review?(snorp)
Comment on attachment 8888119 [details] [diff] [review]
Remove MOZ_WIDGET_GONK from dom/{base,ipc,plugins}

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

I'm not a peer of any of this code, but the patch is pretty trivial.
Attachment #8888119 - Flags: review?(continuation) → review+
(Assignee)

Comment 20

2 years ago
More patches to land.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Duplicate of this bug: 1382694
No longer depends on: 1382694
Attachment #8887791 - Flags: review?(gsvelto) → review+
Comment on attachment 8888163 [details] [diff] [review]
Remove MOZ_WIDGET_GONK from several dom/ subdirectories

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

I didn't review the dom/network changes, as I don't know anything about that code and it was more complex than deleting some #ifdef blocks. Maybe jdm could review it.

::: dom/html/nsIFormControl.h
@@ +279,1 @@
>           // On Android/B2G, date/time input appears as a normal text box.

Drop the reference to B2G in the comment while you are here, maybe?
Attachment #8888163 - Flags: review?(josh)
Attachment #8888163 - Flags: review?(continuation)
Attachment #8888163 - Flags: review+
Attachment #8888163 - Flags: review?(josh) → review+
(Assignee)

Comment 23

2 years ago
Attachment #8888585 - Flags: review?(gsvelto)
(Assignee)

Updated

2 years ago
Attachment #8887781 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8887791 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8887795 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8888119 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8888163 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8887765 - Flags: checkin+
Comment on attachment 8888585 [details] [diff] [review]
Remove MOZ_WIDGET_GONK from hal/

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

LGTM but AFAIK most of the power-related methods were only used in MozPowerManager which I doubt we're using on any other platform. From the looks of it it's still living somewhere within the Navigator object:

https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/dom/base/Navigator.cpp#1428

We should probably file a bug to remove it.
Attachment #8888585 - Flags: review?(gsvelto) → review+
(Assignee)

Comment 26

2 years ago
> We should probably file a bug to remove it.

I filed bug 1382955 and made an attempt at a patch.
Attachment #8888172 - Flags: review?(snorp) → review+
Attachment #8888149 - Flags: review?(gsquelart) → review+
(Assignee)

Updated

2 years ago
Attachment #8888149 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8888172 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8888585 - Flags: checkin+
(Assignee)

Updated

2 years ago
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.