1489933 - Make GeckoDisplay and PanZoomController constructors protected

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P5)

Description

[Paul Rouget [:paul]]

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.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Probably enough to make the constructor protected?

Comment 2

[Paul Rouget [:paul]]

Yes.

Comment 3

[Jai Kotia]

I can fix this. Changing the constructor declaration to protected should fix it.

Comment 4

[Jai Kotia]

Attachment: GeckoDisplay constructor access modifier changed to protected from private

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

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!

Comment 6

[Paul Rouget [:paul]]

Jai, do you have time to make a patch?

Comment 7

[Jai Kotia]

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

Comment 8

[Paul Rouget [:paul]]

Attachment: v1

Comment 9

[Paul Rouget [:paul]]

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 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 11

[Paul Rouget [:paul]]

Attachment: v2

Comment 12

[Paul Rouget [:paul]]

Thank you.

I addressed your comments.

Ready to land.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fcf985374f
Make GeckoDisplay and PanZoomController constructors protected r=snorp

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/66fcf985374f