Closed Bug 1489933 Opened Last year Closed 11 months ago

Make GeckoDisplay and PanZoomController constructors protected

Categories

(GeckoView :: General, enhancement, P5)

enhancement

Tracking

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

RESOLVED FIXED
mozilla64
Tracking Status
geckoview62 --- wontfix
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: paul, Assigned: paul, Mentored)

Details

Attachments

(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
Yes.
I can fix this. Changing the constructor declaration to protected should fix it.
Assignee: nobody → jai2mad
Status: NEW → ASSIGNED
Thanks for your work Jai! Could you create a patch file and upload it to Bugzilla? (You can follow some examples here, https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues#How_to_use_MQ_for_Mozilla_development). 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).

Thanks!
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]
v1

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/PanZoomController.java
@@ +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/GeckoDisplay.java
@@ +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 jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fcf985374f
Make GeckoDisplay and PanZoomController constructors protected r=snorp
https://hg.mozilla.org/mozilla-central/rev/66fcf985374f
Status: ASSIGNED → RESOLVED
Closed: 11 months 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.