Closed Bug 1489933 Opened 3 years ago Closed 3 years ago

Make GeckoDisplay and PanZoomController constructors protected


(GeckoView :: General, enhancement, P5)



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

Tracking Status
geckoview62 --- wontfix
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed


(Reporter: paul, Assigned: paul, Mentored)



(2 files, 1 obsolete file)

I'm working on a Servo implementation of the GeckoView and I'd like to extend the GeckoDisplay class, but can't as its constructor is private.
Probably enough to make the constructor protected?
Mentor: nchen
Keywords: good-first-bug
I can fix this. Changing the constructor declaration to protected should fix it.
Assignee: nobody → jai2mad
Thanks for your work Jai! Could you create a patch file and upload it to Bugzilla? (You can follow some examples here, You should also flag the patch for review (by changing the review flag to "?" in the attachment details page, and you can use my email as the reviewer).

Flags: needinfo?(jai2mad)
Summary: Make GeckoDisplay constructor public → Make GeckoDisplay constructor protected
Jai, do you have time to make a patch?
(In reply to Paul Rouget [:paul] from comment #6)
> Jai, do you have time to make a patch?

Hey sorry Paul, I tried hard, but couldn't understand how to make the patch for the fix.
Flags: needinfo?(jai2mad)
Summary: Make GeckoDisplay constructor protected → Make GeckoDisplay and PanZoomController constructors protected
Attached patch v1Splinter Review
Assignee: jai2mad → paul
Attachment #9007978 - Attachment is obsolete: true
Attachment #9012813 - Flags: review?(snorp)
This patch is similar to bug 1492771. So far, these are the only changes we need to start supporting Servo on top of GeckoView.
Comment on attachment 9012813 [details] [diff] [review]

Review of attachment 9012813 [details] [diff] [review]:

I really don't think this is useful at all for GeckoView consumers, since neither object can be constructed by the app right now, but I guess it doesn't hurt much either. Looks good with nits.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/
@@ +148,5 @@
>          return handleMouseEvent(event.getActionMasked(), event.getEventTime(),
>                                  event.getMetaState(), x, y, event.getButtonState());
>      }
> +    /* package */ protected PanZoomController(final LayerSession session) {

Remove /* package */, same as the other case.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/
@@ +16,5 @@
>   */
>  public class GeckoDisplay {
>      private final GeckoSession session;
> +    /* package */ protected GeckoDisplay(final GeckoSession session) {

Remove /* package */ since you're making it protected. We put /* package */ as a convention because the only way to get package visibility is to not have any modifier at all.
Attachment #9012813 - Flags: review?(snorp) → review+
Attached patch v2Splinter Review
Thank you.

I addressed your comments.

Ready to land.
Attachment #9013239 - Flags: review?(snorp)
Attachment #9013239 - Flags: review?(snorp) → review+
Pushed by
Make GeckoDisplay and PanZoomController constructors protected r=snorp
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Product: Firefox for Android → GeckoView
Keywords: good-first-bug
Target Milestone: Firefox 64 → mozilla64
You need to log in before you can comment on or make changes to this bug.