694672 - [birch] Support doorhanger/ContentPermissionPrompts

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Add doorhangers

Add support for doorhanger/ContentPermissionPrompts for geolocation, indexedb, popups, notifications, etc.

The GUI is very basic, functionality works.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Mockup GUI

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Testing URL that asks for multiple notifications at once:
http://sjeng.org/mozilla/geo.html

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 567196 [details] [diff] [review]
Patch 1. Add doorhangers

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

I'd prefer to have Mark to review the JS part of this patch. I'd like to see those nits and issues handled before pushing.

::: embedding/android/DoorHanger.java
@@ +56,5 @@
> +        final DoorHangerPopup dhp = new DoorHangerPopup(mCtx);
> +        dhp.mPopUp.setOnDismissListener(new PopupWindow.OnDismissListener() {
> +                @Override
> +                public void onDismiss(){
> +                    mPopups.remove(mPopups.lastIndexOf(dhp));

Don't you need to re-position all remaining popups once one popup is dismissed? Otherwise there will be empty space between the remaining popups, no?

::: embedding/android/DoorHangerPopup.java
@@ +39,5 @@
> +
> +import android.content.Context;
> +import android.widget.*;
> +import android.view.*;
> +import android.widget.LinearLayout.*;

Replace those wildcard imports with specific imports.

@@ +42,5 @@
> +import android.view.*;
> +import android.widget.LinearLayout.*;
> +
> +public class DoorHangerPopup {
> +    private Context mCtx;

mContext for clarity?

@@ +45,5 @@
> +public class DoorHangerPopup {
> +    private Context mCtx;
> +    private LinearLayout mLayout;
> +    private LinearLayout mChoicesLayout;
> +    private TextView mTv;

mTextView would be easier to understand and it's not too long.

@@ +47,5 @@
> +    private LinearLayout mLayout;
> +    private LinearLayout mChoicesLayout;
> +    private TextView mTv;
> +    private Button mButton;
> +    private LayoutParams mLayoutPar;

mLayoutParams? Not too long and easier to understand.

@@ +48,5 @@
> +    private LinearLayout mChoicesLayout;
> +    private TextView mTv;
> +    private Button mButton;
> +    private LayoutParams mLayoutPar;
> +    public PopupWindow mPopUp;

mPopup would be more consistent with DoorHangerPopup.

@@ +50,5 @@
> +    private Button mButton;
> +    private LayoutParams mLayoutPar;
> +    public PopupWindow mPopUp;
> +
> +    public DoorHangerPopup(Context ctx) {

I'd expect DoorHangerPopup to actually be a Popup. Any reason why not?

@@ +64,5 @@
> +        mLayout.setOrientation(LinearLayout.VERTICAL);
> +        mChoicesLayout.setOrientation(LinearLayout.HORIZONTAL);
> +        mLayout.addView(mTv, mLayoutPar);
> +        mLayout.addView(mChoicesLayout, mLayoutPar);
> +        mPopUp.setContentView(mLayout);

I think you UI layout code should be defined in XML layout files. It will make it easier to change and style the UI. Have a look at how BrowserToolbar is written as an example.

@@ +67,5 @@
> +        mLayout.addView(mChoicesLayout, mLayoutPar);
> +        mPopUp.setContentView(mLayout);
> +    }
> +
> +    public void addButton(String aTxt, int aCb) {

aButtonText and aCallback for clarity?

@@ +68,5 @@
> +        mPopUp.setContentView(mLayout);
> +    }
> +
> +    public void addButton(String aTxt, int aCb) {
> +        final String sCb = Integer.toString(aCb);

Empty line here for readability?

@@ +76,5 @@
> +                public void onClick(View v) {
> +                    GeckoEvent e = new GeckoEvent("doorhanger-reply", sCb);
> +                    GeckoAppShell.sendEventToGecko(e);
> +                    mPopUp.dismiss();
> +                }});

This code is not indented like other parts of the code base.

@@ +85,5 @@
> +        mTv.setText(aTxt);
> +    }
> +
> +    public void showAtHeight(int y) {
> +        Display display = ((WindowManager)mCtx.getSystemService(Context.WINDOW_SERVICE)).getDefaultDisplay();

Space between the (WindowManager) casting and the variable.

@@ +87,5 @@
> +
> +    public void showAtHeight(int y) {
> +        Display display = ((WindowManager)mCtx.getSystemService(Context.WINDOW_SERVICE)).getDefaultDisplay();
> +        int width = display.getWidth();
> +        int height = display.getHeight();

Empty line here for readability?

::: embedding/android/GeckoAppShell.java
@@ +1661,5 @@
>                  Log.i("GeckoShell", "progress - " + current + "/" + total);
>              } else if (type.equals("onCameraCapture")) {
>                  //GeckoApp.mAppContext.doCameraCapture(geckoObject.getString("path"));
>                  GeckoApp.mAppContext.doCameraCapture();
> +            }  else if (type.equals("doorhanger")) {

This code is pretty long to be inside an 'if'. I think this should be moved to handleDoorHanger method in GeckoApp maybe? Just like other events handled here.

@@ +1681,5 @@
> +                                    Log.i("GeckoShell", "Label: " + label
> +                                          + " CallbackId: " + callBackId);
> +                                    dhp.addButton(label, callBackId);
> +                                } catch (Exception e) {
> +                                    Log.i("GeckoShell", "JSON throws " + e);

It's usually not a good idea to handle generic Exception like this. What kind of exceptions might be thrown here? Should they be handled separately?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch v2. Add doorhangers

Incorporated review comments. Added tabs support.

No UX changes for now.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 568796 [details] [diff] [review]
Patch v2. Add doorhangers


>diff --git a/embedding/android/DoorHanger.java b/embedding/android/DoorHanger.java

>+    public void redraw(int tabId) {

"redraw" is a bit too much graphics related. How about "updateForTab"

>+            if (dhp.mTabId == tabId) {
>+                dhp.setOnDismissListener(new PopupWindow.OnDismissListener() {
>+                        @Override
>+                        public void onDismiss() {
>+                            mPopups.remove(mPopups.lastIndexOf(dhp));
>+                        }
>+                    });

Did you mean to indent 8 spaces here? We normally use 4 spaces

>+                dhp.showAtHeight(10 + yoff);
>+                yoff += dhp.getHeight() + 10;

nit: "yoff" is a bit abbreviated. Why not "yOffset" ?

>diff --git a/embedding/android/DoorHangerPopup.java b/embedding/android/DoorHangerPopup.java

>+    public void addButton(String aText, int aCallback) {
>+
>+        final String sCallback = Integer.toString(aCallback);

Remove the blank line

>+        mButton.setOnClickListener(new Button.OnClickListener() {
>+                public void onClick(View v) {
>+                    GeckoEvent e = new GeckoEvent("doorhanger-reply", sCallback);
>+                    GeckoAppShell.sendEventToGecko(e);
>+                    dismiss();
>+                }});

Again with 8 spaces instead of 4 spaces

>+    public void showAtHeight(int y) {

>+        update(0, height-110-y, width, 100);

nit: height - 110 - y
and should the 110 and 100 be named constants?

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java

>+    public static void handleDoorHanger(JSONObject geckoObject) throws JSONException {
>+        getMainHandler().post(new Runnable() {

Would getHandler() work too? It's not the main UI thread

>+                    int activeTab = Tabs.getInstance().getSelectedTabId();
>+                    GeckoApp.mAppContext.mDoorHanger.redraw(activeTab);

Add a blank line and a comment here to let us know that you are making sure the doorhanger is only shown if it's associated with the selected tab

>+            } else if (type.equals("doorhanger")) {
>+                handleDoorHanger(geckoObject);

I am trying to move to more consistent message naming. Can we use "Doorhanger:Add" and "Doorhanger:Reply"

>diff --git a/embedding/android/resources/layout/doorhangerpopup.xml b/embedding/android/resources/layout/doorhangerpopup.xml

>+  <TextView
>+      android:id="@+id/doorhanger_title"
>+      android:layout_width="wrap_content"
>+      android:layout_height="wrap_content"
>+      />
>+  <LinearLayout
>+      android:id="@+id/doorhanger_choices"
>+      android:orientation="horizontal"
>+      android:layout_width="wrap_content"
>+      android:layout_height="wrap_content"
>+      />

nit: I think the group decision was to put "android:id=..." on the same line as the tag name, and align all other attributes with the first attribute

>+</LinearLayout>
>\ No newline at end of file

Add a newline at the end of file

>diff --git a/mobile/components/ContentPermissionPrompt.js b/mobile/components/ContentPermissionPrompt.js

>+  _callBackStack : [],

"_callbacks"

>   prompt: function(request) {

>+    let tabID = this.getTabForRequest(request);
>+    dump("Notifying tabId: " + tabID);

remove the dumps if you are happy with the patch

>+    dump("Notifying: " + message);

same

>+    let DoorhangerEventListener = {
>+      _ContentPermissionObj : {},

_contentPermission: this,

should work. no need to pass in thru init()

>+      observe: function(aSubject, aTopic, aData) {

aData is an integer in a string, right?

>+        if (aTopic == "doorhanger-reply") {
>+          dump("DoorHanger-reply topic: " + aTopic + " data: " + aData);

remove dump if this works OK

>+          let data = JSON.parse(aData);
>+          let cbId = parseInt(data);

JSON.parse on aData seems wrong. Why not just parseInt(aData)  ?

>+    let JavaMessage = {
>+          "gecko": {
>+              "type"           : "doorhanger",
>+              "message"        : message,
>+              "severity"       : "PRIORITY_WARNING_MEDIUM",
>+              "buttons"        : buttons,
>+              "tabID"          : tabID
>+          }};

      let json = {
        gecko: {
          type: "doorhanger",
          message: message,
          severity: "PRIORITY_WARNING_MEDIUM",
          buttons: buttons,
          tabID: tabID
        }
      };

In JS we just use a JS literal for JSON and 2 spaces for indents

r+, but add a new patch with changes before checking in

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch v3. Add doorhangers

Carrying forward review.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

http://hg.mozilla.org/projects/birch/rev/3a14d7985160

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Ahem, this is still missing popup blocking, at least.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #9)
> Ahem, this is still missing popup blocking, at least.

File a new bug. This bug is good for covering the basic DoorHanger mechanism

Comment 11

[Aaron Train [:aaronmt]]

bug 697110 filed for popup blocking

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111026 Firefox/10.0a1 Fennec/10.0a1