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

RESOLVED FIXED in Firefox 65

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: jchen, Assigned: fluffyemily)

Tracking

unspecified
mozilla65
All
Android
Dependency tree / graph

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

Comment 11

11 months ago
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)

Updated

10 months ago
Assignee: 1991manish.kumar → nobody
Assignee

Updated

9 months ago
Assignee: nobody → etoop
Status: NEW → ASSIGNED
Assignee

Comment 13

9 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

9 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.
Assignee

Comment 17

9 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

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

Updated

8 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

8 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

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

Updated

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

Comment 27

8 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

8 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

8 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

8 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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7331c7194e20
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
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
Assignee

Updated

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

Updated

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