Use "Child" instead of "Content" to denote GeckoView child process modules

RESOLVED FIXED in Firefox 65

Status

P3
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: jchen, Assigned: fluffyemily)

Tracking

unspecified
mozilla65
All
Android

Firefox Tracking Flags

(geckoview62 wontfix, geckoview63 wontfix, geckoview64 wontfix, firefox-esr60 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Right now we have GeckoViewContent.js (frame script for child process) and GeckoViewContent.jsm (module for content listener in parent process). I think it'd be less confusing if we rename GeckoViewContent.js to GeckoViewChild.js (and rename GeckoViewContentModule.jsm to GeckoViewChildModule.jsm as well).
Comment 0 isn't totally correct. GeckoViewContent.js is the frame script for GeckoViewContent.jsm in the child process. Similarly, GeckoViewScrollContent.js is the frame script for GeckoViewScroll.jsm in the child process.

So I think the correct changes should be,
* Rename GeckoViewContent.js to GeckoViewContentChild.js
* Rename GeckoViewScrollContent.js to GeckoViewScrollChild.js
* Rename GeckoViewContentModule.jsm to GeckoViewChildModule.jsm

These changes will avoid the confusion of using the term "Content" to denote both "child process" and "content listener".
Summary: Rename GeckoViewContent.js to GeckoViewChild.js → Use "Child" instead of "Content" to denote GeckoView child process modules
All of that sounds good except:

> * Rename GeckoViewContentModule.jsm to GeckoViewChildModule.jsm

That one is named to be consistent with the "content listener" functionality, as you said, so it seems like it should stay as-is. This module may not even interact with a child (content) process.
Hi. I'd like to be assigned to solve this bug. :)

Do I need to change anything inside any file?
Flags: needinfo?(nchen)
Thanks! Yes you have to change the corresponding names inside the renamed files. For example, after renaming GeckoViewScrollContent.js to GeckoViewScrollChild.js, you need to change `class GeckoViewScrollContent` [1] to `class GeckoViewScrollChild` inside that file.

[1] https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/mobile/android/chrome/geckoview/GeckoViewScrollContent.js#17
Flags: needinfo?(nchen)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #2)
> All of that sounds good except:
> 
> > * Rename GeckoViewContentModule.jsm to GeckoViewChildModule.jsm
> 
> That one is named to be consistent with the "content listener"
> functionality, as you said, so it seems like it should stay as-is. This
> module may not even interact with a child (content) process.

GeckoViewContentModule.jsm is actually used by child process frame scripts, so should be GeckoViewChildModule.jsm. It's confusing :) (hence the need for this bug)
Posted patch Patch_Bug1395217 (obsolete) — Splinter Review
Please review and assign this bug to me.

Thanks
Assignee: nobody → 1991manish.kumar
Attachment #8964186 - Flags: review?(nchen)
Comment on attachment 8964186 [details] [diff] [review]
Patch_Bug1395217

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

Looking good Manish! Just a few more changes to make:

1) Rename "GeckoViewNavigationContent.js" to "GeckoViewNavigationChild.js". This file was added recently.

2) Rename "GeckoViewContentSettings.js" to "GeckoViewSettingsChild.js". This file was also added recently.

3) In all the files that you changed, change "GeckoViewContentModule" to "GeckoViewChildModule" as well.

For example, several files have this line
>  ChromeUtils.import("resource://gre/modules/GeckoViewContentModule.jsm");
which should be changed to
>  ChromeUtils.import("resource://gre/modules/GeckoViewChildModule.jsm");

and this line
>  class XXX extends GeckoViewContentModule
which should be changed to
>  class XXX extends GeckoViewChildModule

4) Search for references to the old file names and change them to the new names.

https://searchfox.org is a good way to search for references. For example, if you search for "GeckoViewScrollContent.js", you can see it is referenced in "mobile/android/chrome/geckoview/jar.mn" and "mobile/android/modules/geckoview/GeckoViewScroll.jsm", so those files should be changed to use "GeckoViewScrollChild.js".
Attachment #8964186 - Flags: review?(nchen) → feedback+
Posted patch Patch_Bug1395217 (obsolete) — Splinter Review
Please Review.
Thanks
Attachment #8964186 - Attachment is obsolete: true
Attachment #8964432 - Flags: review?(nchen)
Comment on attachment 8964432 [details] [diff] [review]
Patch_Bug1395217

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

Thanks! Almost there! Just a few changes to make.

1) Make sure you change "GeckoViewScrollContent.js" to "GeckoViewScrollChild.js" in mobile/android/modules/geckoview/GeckoViewScroll.jsm

1) Make sure you change "GeckoViewContent.js" to "GeckoViewContentChild.js" in mobile/android/modules/geckoview/GeckoViewScroll.jsm

::: mobile/android/chrome/geckoview/jar.mn
@@ +9,3 @@
>    content/geckoview.xul
>    content/geckoview.js
>    content/GeckoViewContent.js

Change this line to "content/GeckoViewContentChild.js"

@@ +9,4 @@
>    content/geckoview.xul
>    content/geckoview.js
>    content/GeckoViewContent.js
> +  content/GeckoViewSettingsChild.js

Move this line below the last line (content/GeckoViewScrollContent.js), so they're in alphabetical order.

@@ +14,1 @@
>    content/GeckoViewScrollContent.js

Change this line to "content/GeckoViewScrollChild.js"

::: mobile/android/modules/geckoview/moz.build
@@ +7,4 @@
>  EXTRA_JS_MODULES += [
>      'AndroidLog.jsm',
>      'GeckoViewContent.jsm',
> +    'GeckoViewChildModule.jsm',

Move this line above the previous line ('GeckoViewContent.jsm',) so they're in alphabetical order.
Attachment #8964432 - Flags: review?(nchen) → feedback+
(In reply to Jim Chen [:jchen] [:darchons] from comment #9)

> 1) Make sure you change "GeckoViewContent.js" to "GeckoViewContentChild.js"
> in mobile/android/modules/geckoview/GeckoViewScroll.jsm

Sorry, that should be make sure you change "GeckoViewContent.js" to "GeckoViewContentChild.js" in mobile/android/modules/geckoview/GeckoViewContent.jsm
How can I test this after changes on my system?
Flags: needinfo?(nchen)
You should make sure it builds, and then you can run our automated tests [1] on your machine. Another option is, if you upload a patch, we can help you push a try run that will run the tests on Mozilla's infrastructure.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Geckoview-Junit_Tests
Flags: needinfo?(nchen)
Assignee: 1991manish.kumar → nobody
(Assignee)

Updated

6 months ago
Assignee: nobody → etoop
Status: NEW → ASSIGNED
(Assignee)

Comment 13

6 months ago
:jchen this is the apk that is causing me build issues mentioned in https://mozilla.slack.com/archives/C8Y4T8UQH/p1538494590000100
Attachment #9013726 - Flags: feedback+
(Assignee)

Comment 14

6 months ago
Posted patch Bug1395217_Etoop.patch (obsolete) — Splinter Review
My first m-c patch so please be gentle :)
Attachment #8964432 - Attachment is obsolete: true
Attachment #9013726 - Attachment is obsolete: true
Attachment #9014091 - Flags: review+
Comment on attachment 9014091 [details] [diff] [review]
Bug1395217_Etoop.patch

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

r?jchen
Attachment #9014091 - Flags: review+ → review?(nchen)
Thanks, Emily! To request a code review, just set the patch's review status flag to "review?" ("r?"). The code reviewer will set the patch's review status flag to "review+" ("r+") to approve a patch.

We might want to uplift this change to GV 63 Beta to make uplifts easier. If someone fixes a bug in 64's GeckoViewChildModule.jsm, uplifting the fix to 63's old filename GeckoViewContentModule.jsm will require some manual patch rebasing.
status-firefox62: --- → wontfix
status-firefox63: --- → affected
status-firefox64: --- → affected
status-firefox-esr60: --- → wontfix
status-geckoview62: --- → wontfix
(Assignee)

Comment 17

6 months ago
(In reply to Chris Peterson [:cpeterson] from comment #16)
> Thanks, Emily! To request a code review, just set the patch's review status
> flag to "review?" ("r?"). The code reviewer will set the patch's review
> status flag to "review+" ("r+") to approve a patch.

Oops, sorry. It's been quite a while since I last used bugzilla!
Comment on attachment 9014091 [details] [diff] [review]
Bug1395217_Etoop.patch

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

Thanks Emily! Looks good with only a couple changes needed.

::: mobile/android/chrome/geckoview/GeckoViewProgressContent.js
@@ -9,5 @@
>  XPCOMUtils.defineLazyModuleGetters(this, {
>    Services: "resource://gre/modules/Services.jsm",
>  });
>  
> -class GeckoViewProgressContent extends GeckoViewContentModule {

`GeckoViewProgressContent` to `GeckoViewProgressChild` (and rename the file)

::: mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js
@@ +12,4 @@
>  
>  // Dispatches GeckoView:ShowSelectionAction and GeckoView:HideSelectionAction to
>  // the GeckoSession on accessible caret changes.
> +class GeckoViewSelectionActionContent extends GeckoViewChildModule {

`GeckoViewSelectionActionContent` to GeckoViewSelectionActionChild` (and rename the file too)

::: mobile/android/chrome/geckoview/jar.mn
@@ +14,2 @@
>    content/GeckoViewProgressContent.js
>    content/GeckoViewPromptContent.js

Rename "GeckoViewPromptContent.js" to "GeckoViewPromptChild.js"
Attachment #9014091 - Flags: review?(nchen) → feedback+
(Assignee)

Comment 19

6 months ago
Posted patch Bug1395217_2.patch (obsolete) — Splinter Review
Addressed Review Comments
Attachment #9015210 - Flags: review?(nchen)
(Assignee)

Updated

6 months ago
Attachment #9014091 - Attachment is obsolete: true
Comment on attachment 9015210 [details] [diff] [review]
Bug1395217_2.patch

r+ assuming tests pass. Thanks Emily!
Attachment #9015210 - Flags: review?(nchen) → review+
Hi Emily, just a reminder to land this. It's in danger of bit-rotting :)
Flags: needinfo?(etoop)
(Assignee)

Comment 22

5 months ago
(In reply to Jim Chen [:jchen] [:darchons] from comment #21)
> Hi Emily, just a reminder to land this. It's in danger of bit-rotting :)

Hi, I've never landed anything in m-c before. What's the best way to do that?
Flags: needinfo?(etoop) → needinfo?(nchen)
(In reply to Emily Toop (:fluffyemily) from comment #22)
> Hi, I've never landed anything in m-c before. What's the best way to do that?

1. The easiest option is to add the "checkin-needed" Bugzilla keyword to the bug. I'll do that now. :)

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_6_Getting_code_into_the_tree

2. The second easiest option is to use Phabricator code review tool (instead of attaching raw patch files) and its auto-lander bot, Lando:

https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#quick-start

3. The third option is push your patch to the mozilla-inbound Mercurial server.

https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html

* For options 2 and 3, you will need Mozilla commit access level 3 to push code to Mercurial:

https://www.mozilla.org/en-US/about/governance/policies/commit/
Flags: needinfo?(nchen)
Keywords: checkin-needed
Emily: Hi, we received the following errors when trying to apply your patch, so please provide an updated one.

applying Bug1395217_2.patch
10:53 AM patching file mobile/android/chrome/geckoview/GeckoViewProgressChild.js
10:53 AM Hunk #1 FAILED at 9
10:53 AM 1 out of 2 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/GeckoViewProgressChild.js.rej
10:53 AM patching file mobile/android/chrome/geckoview/GeckoViewSelectionActionChild.js
10:53 AM Hunk #1 FAILED at 11
10:53 AM 1 out of 2 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/GeckoViewSelectionActionChild.js.rej
10:53 AM patching file mobile/android/chrome/geckoview/jar.mn
10:53 AM Hunk #1 FAILED at 10
10:53 AM 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/jar.mn.rej
10:53 AM unable to find 'mobile/android/chrome/geckoview/GeckoViewProgressContent.js' for patching
10:53 AM (use '--prefix' to apply patch relative to the current directory)
10:53 AM 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/GeckoViewProgressContent.js.rej
10:53 AM unable to find 'mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js' for patching
10:53 AM (use '--prefix' to apply patch relative to the current directory)
10:53 AM 2 out of 2 hunks FAILED -- saving rejects to file mobile/android/chrome/geckoview/GeckoViewSelectionActionContent.js.rej
10:53 AM patching file mobile/android/modules/geckoview/GeckoViewChildModule.jsm
10:53 AM Hunk #1 FAILED at 3
10:53 AM 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/modules/geckoview/GeckoViewChildModule.jsm.rej
10:53 AM patch failed, unable to continue (try -v)
10:53 AM patch failed, rejects left in working directory
10:53 AM errors during apply, please fix and qrefresh Bug1395217_2.patch

Clearing the checkin-needed. Thanks!
Flags: needinfo?(etoop)
Keywords: checkin-needed
(Assignee)

Comment 25

5 months ago
Posted patch Bug1395217_Rebased.patch (obsolete) — Splinter Review
Flags: needinfo?(etoop)
(Assignee)

Updated

5 months ago
Keywords: checkin-needed
Please provide a patch which successfully applies with |hg import|.
Flags: needinfo?(etoop)
Keywords: checkin-needed
(Assignee)

Comment 27

5 months ago
I really hope this works. It's been a real headache figuring out how to rebase on top of mercurial. This applied successfully locally.
Attachment #9015210 - Attachment is obsolete: true
Attachment #9018201 - Attachment is obsolete: true
Flags: needinfo?(etoop)
(Assignee)

Updated

5 months ago
Keywords: checkin-needed
Encountered this while applying the patch:

applying bug-1395217_rebased.patch
patching file mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
Hunk #1 FAILED at 562
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1395217_rebased.patch

Removing checkin-needed tag.
Flags: needinfo?(etoop)
Keywords: checkin-needed
(Assignee)

Comment 29

5 months ago
Renamed:

* `GeckoViewContent.js` -> `GeckoViewContentChild.js`
* `GeckoViewNavigationContent.js` -> `GeckViewNavigationChild.js`
* `GeckoViewProgressContent.js` -> `GeckoViewProgressChild.js`
* `GeckoViewPromptContent.js` -> `GeckoViewPromptChild.js`
* `GeckoViewScrollContent.js` -> `GeckoViewScrollChild.js`
* `GeckoViewSelectionActionContent.js` -> `GeckoViewSelectionActionChild.js`
* `GeckoViewSettingsContent.js` -> `GeckoViewSettingsChild.js`
* `GeckoViewContentModule.jsm` -> `GeckoViewChildModule.jsm`

Comment 30

5 months ago
Pushed by etoop@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7331c7194e20
Rename Use "Child" instead of "Content" to denote GeckoView child process modules r=snorp

Comment 31

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7331c7194e20
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
64=wontfix because we don't need to uplift this fix for Focus 8.0's GV 64 beta test
status-firefox64: affected → wontfix
status-firefox63: affected → wontfix
status-geckoview63: --- → wontfix
(Assignee)

Updated

5 months ago
Flags: needinfo?(etoop)
status-geckoview64=wontfix
status-geckoview64: --- → wontfix

Updated

3 months ago
Product: Firefox for Android → GeckoView
Keywords: good-first-bug
Target Milestone: Firefox 65 → mozilla65
You need to log in before you can comment on or make changes to this bug.