Last Comment Bug 694672 - [birch] Support doorhanger/ContentPermissionPrompts
: [birch] Support doorhanger/ContentPermissionPrompts
Status: VERIFIED FIXED
: feature
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: Gian-Carlo Pascutto [:gcp]
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 695183 (view as bug list)
Depends on: 697086 697087 697088
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-14 15:14 PDT by Gian-Carlo Pascutto [:gcp]
Modified: 2016-07-29 14:19 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Patch 1. Add doorhangers (20.38 KB, patch)
2011-10-14 15:14 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
Mockup GUI (75.09 KB, image/jpeg)
2011-10-17 14:31 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details
Patch v2. Add doorhangers (26.04 KB, patch)
2011-10-21 15:34 PDT, Gian-Carlo Pascutto [:gcp]
mark.finkle: review+
Details | Diff | Splinter Review
Patch v3. Add doorhangers (28.17 KB, patch)
2011-10-25 04:45 PDT, Gian-Carlo Pascutto [:gcp]
gpascutto: review+
Details | Diff | Splinter Review

Description Gian-Carlo Pascutto [:gcp] 2011-10-14 15:14:43 PDT
Created attachment 567196 [details] [diff] [review]
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] 2011-10-17 14:31:43 PDT
Created attachment 567580 [details]
Mockup GUI
Comment 2 Gian-Carlo Pascutto [:gcp] 2011-10-17 14:33:14 PDT
Testing URL that asks for multiple notifications at once:
http://sjeng.org/mozilla/geo.html
Comment 3 [:fabrice] Fabrice Desré 2011-10-17 15:00:53 PDT
*** Bug 695183 has been marked as a duplicate of this bug. ***
Comment 4 Lucas Rocha (:lucasr) 2011-10-18 15:19:05 PDT
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] 2011-10-21 15:34:25 PDT
Created attachment 568796 [details] [diff] [review]
Patch v2. Add doorhangers

Incorporated review comments. Added tabs support.

No UX changes for now.
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-24 12:13:59 PDT
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] 2011-10-25 04:45:05 PDT
Created attachment 569329 [details] [diff] [review]
Patch v3. Add doorhangers

Carrying forward review.
Comment 8 Gian-Carlo Pascutto [:gcp] 2011-10-25 05:27:24 PDT
http://hg.mozilla.org/projects/birch/rev/3a14d7985160
Comment 9 Gian-Carlo Pascutto [:gcp] 2011-10-25 06:54:51 PDT
Ahem, this is still missing popup blocking, at least.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 07:51:36 PDT
(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] 2011-10-26 06:32:08 PDT
bug 697110 filed for popup blocking

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

Note You need to log in before you can comment on or make changes to this bug.