:Margaret Leibovic Assignee
	Description • 11 years ago

E.g. an add-on that adds a Pocket panel to about:home needs to authenticate a user to show that user's saved Pocket items.

	:Margaret Leibovic Assignee
	Comment 1 • 11 years ago

Providing a new UI and APIs to let a service authenticate users may be too ambitious for a v1 lists feature.

Before jumping into adding new APIs here, I think we should experiment with creating an add-on that authenticates a user by opening a new tab to let the user authenticate on the web, then uses its chrome privileges to get whatever cookie it needs to send an authenticated request to fetch new list data. I have no experience dealing with authentication cookies but I *think* this should work.

Working on this bug would involve finding (or creating) some service to use to test, and digging into gecko JS to figure out what we can do to get at cookies.

cc'ing some people who may be interested in exploring this :)

	:Margaret Leibovic Assignee
	Comment 2 • 10 years ago

(In reply to :Margaret Leibovic from comment #1)

> Before jumping into adding new APIs here, I think we should experiment with
> creating an add-on that authenticates a user by opening a new tab to let the
> user authenticate on the web, then uses its chrome privileges to get
> whatever cookie it needs to send an authenticated request to fetch new list
> data. I have no experience dealing with authentication cookies but I *think*
> this should work.

Ta-da! We've already achieved this!

So, let's be more ambitious and try to provide some APIs to let add-ons authenticate from the panel.

needinfo to ibarlow to think about authentication UI. I'm thinking we should just have a view with some text and a button. The add-ons can specify what strings appear in these, but clicking the button will launch their authentication flow (implemented as some JS callback function).

I started brainstorming an implementation approach here: https://mobile.etherpad.mozilla.org/hub-auth

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 3 • 10 years ago

(In reply to :Margaret Leibovic from comment #2)

> So, let's be more ambitious and try to provide some APIs to let add-ons
> authenticate from the panel.

I gotta tell you, I am a little worried about yak-shaving and falling down rabbit hoes with this approach. Do we have any existing examples in any native applications of a "universal" authentication UI? We have all seen UI's that take a username and password, then pass those to some application-specific API. But that seems like some bygone era. These days there are various forms of crypto and tokens and keys and ways to link apps to accounts. It's all very complicated, well to an old guy like me.

I'd rather not spend any chunk of time creating a better mouse trap, if "opening a webpage in a new tab" works now.

Let's first figure out what we really need from the authentication system. How can we improve the flow and UX we currently use.

	:Margaret Leibovic Assignee
	Comment 4 • 10 years ago

Maybe I wasn't clear in my previous comment, I'm just envisioning us providing a button that will launch a user into that new tab, not creating a native UI for the entire experience.

Basically, we need to deal with the fact that if the user somehow fails to authenticate with our current demo add-ons, they're kinda screwed unless the add-on provides some weird alternate UI, like a menu item, to start the authentication flow.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 5 • 10 years ago

(In reply to :Margaret Leibovic from comment #4)
> Maybe I wasn't clear in my previous comment, I'm just envisioning us
> providing a button that will launch a user into that new tab, not creating a
> native UI for the entire experience.

\o/

> Basically, we need to deal with the fact that if the user somehow fails to
> authenticate with our current demo add-ons, they're kinda screwed unless the
> add-on provides some weird alternate UI, like a menu item, to start the
> authentication flow.

OK. This is easier for me to follow, and can be turned into a use-case/ux-design without much fuss.

	Richard Newman [:rnewman]
	Comment 6 • 10 years ago

Android provides some fairly abstract prior art which you might want to consider.

The triumverate of Account, AbstractAccountAuthenticator, and caller.

Callers can ask the Authenticator to give them some kind of "token" for a provided account. The Authenticator can create an Intent pointing to an activity to collect user input if necessary. The caller and the Authenticator need to agree on a token type, and of course on the Account type.

The existence of an Account implies a credentialed user of that type, able to authenticate requests.

We could do something similar here: "hey, Firefox! please ask the user to pick a 'Flickr' account, and give me a 'flickr' auth token". Of course, the app would need to know what to do with it, and you hit all the same problems that we hit in Sync with multiple apps sharing account types.

But the principle is fairly sound.


http://developer.android.com/reference/android/accounts/AbstractAccountAuthenticator.html#getAuthToken%28android.accounts.AccountAuthenticatorResponse,%20android.accounts.Account,%20java.lang.String,%20android.os.Bundle%29

	Ian Barlow (:ibarlow)
	Comment 7 • 10 years ago

(In reply to Mark Finkle (:mfinkle) from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > Maybe I wasn't clear in my previous comment, I'm just envisioning us
> > providing a button that will launch a user into that new tab, not creating a
> > native UI for the entire experience.
> 
> \o/
> 
> > Basically, we need to deal with the fact that if the user somehow fails to
> > authenticate with our current demo add-ons, they're kinda screwed unless the
> > add-on provides some weird alternate UI, like a menu item, to start the
> > authentication flow.
> 
> OK. This is easier for me to follow, and can be turned into a
> use-case/ux-design without much fuss.


So, this is overall user flow as I see it, for what happens after a user selects a panel to add: http://cl.ly/image/110c010T2g19

And here is a higher fidelity mockup of what that "You still need to sign in" page might look like on about:home. I'm thinking that add-ons could customize the icon they use, the descriptive text and the button text. http://cl.ly/image/282D0R39321u (note that the authenticated-but-empty screen is for bug 965622)

	:Margaret Leibovic Assignee
	Comment 8 • 10 years ago

Pretty straightforward change to store a new property on a panel in JS and keep track of a new flag on PanelConfig.

	:Margaret Leibovic Assignee
	Comment 9 • 10 years ago

I haven't gotten very far with this yet, but I wanted to get your feedback on the layout approach.

I imagine we'll hide this auth UI and show the regular layout once the user is authenticated. We'll also need to implement the PanelAuthCache (or whatever we call it) to know whether or not to show this UI.

	Lucas Rocha (:lucasr)
	Comment 10 • 10 years ago

Comment on attachment 8379543 [details] [diff] [review]
(Part 1) Add authHandler option to Home.panels.register

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

Nice.

::: mobile/android/modules/Home.jsm
@@ +178,5 @@
>        id: panel.id,
>        title: panel.title,
>        layout: panel.layout,
> +      views: panel.views,
> +      authRequired: !!panel.authHandler

I have a slight preference for requiresAuth but I don't feel strongly about it.

	Lucas Rocha (:lucasr)
	Comment 11 • 10 years ago

Comment on attachment 8379548 [details] [diff] [review]
WIP - (Part 2) Add a view for authentication UI to DynamicPanel

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

It's probably a good idea to implement the AuthLayout logic in a separate custom view that takes an 'HomeConfig.AuthConfig' (the thing that is send from gecko that has the button text, message and image_url) in its constructor. This way you avoid bloating DynamicPanel with auth layout logic. You need any stub view in the layout as you'd be creating an AuthLayout instance and adding to DynamicPanel on demand.

::: mobile/android/base/home/DynamicPanel.java
@@ +119,5 @@
> +        final ViewGroup view = (ViewGroup) inflater.inflate(R.layout.home_dynamic_panel, container, false);
> +
> +        Log.i(LOGTAG, "*** " + mPanelConfig.getTitle() + " - auth required: " + mPanelConfig.isAuthRequired());
> +
> +        if (mPanelConfig.isAuthRequired()) {

I'd factor out the code to show the auth UI into a separate method (setAuthVisible or something) because you'll probably need to update that later when the auth state for the panel changes.

@@ +123,5 @@
> +        if (mPanelConfig.isAuthRequired()) {
> +            // TODO: Check to see if the user is authenticated before showing the auth UI,
> +            // set up text and button, add button click listener.
> +            final ViewStub authViewStub = (ViewStub) view.findViewById(R.id.home_auth_view_stub);
> +            authViewStub.inflate();

Store the the auth UI in a class member so that you're able to make the setAuthVisibility() call idempotent. Something like:

public void setAuthVisible(boolean visible) {
    if (visible && mAuthLayout == null) {
        final ViewStub authViewStub = (ViewStub) view.findViewById(R.id.home_auth_view_stub);
        mAuthLayout = authViewStub.inflate();
    }

    mAuthLayout.setVisibility(visible ? View.VISIBLE : View.GONE);
    mLayout.setVisibility(visible ? View.GONE : View.VISIBLE);
}

It's probably a good idea to rename mLayout to mPanelLayout for clarity.

@@ +128,5 @@
> +
> +            mLayout.setVisibility(View.GONE);
> +        }
> +
> +        view.addView(mLayout);

You better use addView(mLayout, 0) here to ensure the auth is always stacked on top of the layout.

	:Margaret Leibovic Assignee
	Comment 12 • 10 years ago

This first patch is a bit of refactoring, based on top of what we did in bug 974139. As I was working to add new auth-related things onto the panel, I felt like having the Panel "constructor" do the validation work felt more appropriate.

	:Margaret Leibovic Assignee
	Comment 13 • 10 years ago

This is an updated version of the previously reviewed patch, updating where we set "requiresAuth" now that we got rid of _panelToJSON.

Right now this discards options.authHandler (other than setting the requiresAuth flag), but I deal with that in future patches.

	:Margaret Leibovic Assignee
	Comment 14 • 10 years ago

This patch stores the auth UI bits in their own type.

	:Margaret Leibovic Assignee
	Comment 16 • 10 years ago

This is the meat of the UI. The view still isn't polished, but it gets the functionality in place.

The authHandler API is a bit awkward because we don't want to pass JS functions over to Java with the panel, so I decided to just store those in an _authHandlers object in Home.jsm.

This patch doesn't deal with the actual authentication state

	:Margaret Leibovic Assignee
	Comment 17 • 10 years ago

This is just the start of the PanelAuthCache. I still need to figure out where it's going to live, and when exactly we're going to flip the authentication state.

I'm contemplating adding a Home.panels.setAuthenticated API in Home.jsm, so the add-on can indicate that its authenticated after it does whatever it needs to do. Then this API could send a message to Java to update the cache, or it could directly set a shared pref itself.

We also have to decide when we'll call the DynamicPanel's setAuthVisible method to hide the auth UI.

I'm going to take a break from working on this today, since I feel this is a good point to stop and discuss.

	Lucas Rocha (:lucasr)
	Comment 18 • 10 years ago

Comment on attachment 8381764 [details] [diff] [review]
(Part 0) Move panel validation into Panel constructor

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

Nice.

::: mobile/android/modules/Home.jsm
@@ +199,4 @@
>        throw "Home.panels: Can't create a home panel without an id and title!";
>      }
>  
> +    this.layout = options.layout;

nit: I think the code would look a bit cleaner if you assigned everything on top and then did the checks later. Something like:

this.id = id;
this.title = options.title;
this.layout = options.layout;
this.views = options.views;

....

	Lucas Rocha (:lucasr)
	Comment 19 • 10 years ago

Comment on attachment 8381766 [details] [diff] [review]
(Part 1) Add authHandler option to Home.panels.register

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

Wondering why Panel doesn't hold a reference to the auth handler. Giving f+ for now.

::: mobile/android/base/home/HomeConfig.java
@@ +291,5 @@
>              }
>          }
>  
> +        public boolean isAuthRequired() {
> +            return mFlags.contains(Flags.REQUIRES_AUTH);

I was wondering: once we start defining things like authText, authButtonText and authImage, this method will become something more like:

   return (mAuthConfig != null)

No?

::: mobile/android/modules/Home.jsm
@@ +242,5 @@
> +        throw "Home.panels: authHandler must include valid isAuthenticated function: panel.id = " + this.id;
> +      }
> +      if (!("authenticate" in options.authHandler) || typeof options.authHandler.authenticate !== "function") {
> +        throw "Home.panels: authHandler must include valid authenticate function: panel.id = " + this.id;
> +      }

Won't you hold a reference to the auth handler somewhere?

	Lucas Rocha (:lucasr)
	Comment 20 • 10 years ago

Comment on attachment 8381767 [details] [diff] [review]
(Part 2) Create AuthConfig type to store auth UI options

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

Giving f+ to get some validation on authConfig in JS.

::: mobile/android/modules/Home.jsm
@@ +248,5 @@
>        this.requiresAuth = true;
> +
> +      this.authConfig = {
> +        messageText: options.authHandler.messageText,
> +        buttonText: options.authHandler.buttonText,

We probably want to require at least the text and button text here? Image can be optional I guess.

	Lucas Rocha (:lucasr)
	Comment 21 • 10 years ago

Comment on attachment 8381768 [details] [diff] [review]
(Part 3) Rename mLayout -> mPanelLayout in DyanmicPanel

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

Great.

	Lucas Rocha (:lucasr)
	Comment 22 • 10 years ago

Comment on attachment 8381770 [details] [diff] [review]
(Part 4) Add a view for authentication UI to DynamicPanel

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

Looks great. Besides the small tweaks, sort out the auth handler life cycle on the JS side and use Picasso.

::: mobile/android/base/home/AuthLayout.java
@@ +21,5 @@
> +import android.widget.TextView;
> +
> +import android.util.Log;
> +
> +class AuthLayout extends LinearLayout {

PanelAuthLayout for clarity and consistency?

@@ +23,5 @@
> +import android.util.Log;
> +
> +class AuthLayout extends LinearLayout {
> +
> +    public AuthLayout(Context context, final AuthConfig authConfig, final String panelId) {

I'd just pass a PanelConfig instead and throw if getAuthConfig() returns null.

nit: not a fan of these 'finals' in the arguments. They add noise and don't mean much. Your call.

@@ +27,5 @@
> +    public AuthLayout(Context context, final AuthConfig authConfig, final String panelId) {
> +        super(context);
> +
> +        setOrientation(LinearLayout.VERTICAL);
> +        LayoutInflater.from(context).inflate(R.layout.home_auth, this);

panel_auth_layout to match the class name?

@@ +41,5 @@
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomePanels:Authenticate", panelId));
> +            }
> +        });
> +
> +        final ImageView imageView = (ImageView) findViewById(R.id.image);

You should use Picasso here:

Picasso.with(getContext())
       .load(authConfig.getImageUrl())
       .into(imageView);

::: mobile/android/base/home/DynamicPanel.java
@@ +113,5 @@
>          }
>  
>          Log.d(LOGTAG, "Created layout of type: " + mPanelConfig.getLayoutType());
>  
> +        final FrameLayout view = new FrameLayout(getActivity());

Maybe just store it in a class member to avoid calling getView() later? mParentContainer? mView? mParentView?

::: mobile/android/modules/Home.jsm
@@ +259,5 @@
> +
> +      // Store authentication callback functions separately, so that they are not
> +      // passed to Java as part of the Panel JSON object.
> +      _authHandlers[this.id] = {
> +        isAuthenticated: options.authHandler.authenticate,

isAuthenticated?

@@ +293,5 @@
>        requestId: requestId
>      });
>    };
>  
> +  handlePanelsAuthenticate = function(id) {

With this patch as is, the auth handler will only get registered if we create a Panel instance in response to an install, update, or HomePanels:Get operation. This means that if the panel only registers, the auth handler will be undefined. You'll have to create a Panel instance here to ensure it gets registered. Maybe auth handler should have a separate kind of constructor that uses the same options callback from _registeredPanels?

Maybe throw a more meaningful exception if _authHandler doesn't contain the id?

	Lucas Rocha (:lucasr)
	Comment 23 • 10 years ago

Comment on attachment 8381775 [details] [diff] [review]
(WIP - Part 5) Create a PanelAuthCache to keep track of whether or not panel is authenciated

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

I assume we'll need a listener interface in PanelAuthCache to track changes in the auth state in DynamicPanel.

::: mobile/android/base/home/PanelAuthCache.java
@@ +30,5 @@
> +    }
> +
> +    public boolean isAuthenticated(String panelId) {
> +        final SharedPreferences prefs = getSharedPreferences();
> +        return prefs.getBoolean(getPanelAuthKey(panelId), false);

Just wondering: maybe we should store more contextual information in the cache so that we can validate/invalidate it later? I'm thinking about things like timestamp, username, etc. Maybe this is something the add-ons can define themselves? I'm mostly considering the cases where add-ons need to track when it got authenticated and maybe some more extra bits. If we do that, you'd only be checking for the existence of a PREFS_PANEL_AUTH_PREFIX+PANEL_ID key. If there is one, it's authenticated.

On the other hand, add-ons developers can persist that themselves however they like outside the auth cache anyway. Maybe we should we keep auth cache simple.

@@ +33,5 @@
> +        final SharedPreferences prefs = getSharedPreferences();
> +        return prefs.getBoolean(getPanelAuthKey(panelId), false);
> +    }
> +
> +    public void setAuthenticated(String panelId, boolean authenticated) {

I think the Java side of PanelAuthCache should not have any write access to the cache. The auth cache should only be changed from the JS side.

	:Margaret Leibovic Assignee
	Comment 24 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #22)

> @@ +41,5 @@
> > +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomePanels:Authenticate", panelId));
> > +            }
> > +        });
> > +
> > +        final ImageView imageView = (ImageView) findViewById(R.id.image);
> 
> You should use Picasso here:
> 
> Picasso.with(getContext())
>        .load(authConfig.getImageUrl())
>        .into(imageView);

Does Picasso handle loading local images? I feel like we should encourage add-ons to use local images for static UI like this, since it's unlikely to be changing between add-on updates.

	:Margaret Leibovic Assignee
	Comment 25 • 10 years ago

Updated to address review comments.

	:Margaret Leibovic Assignee
	Comment 26 • 10 years ago

I combined parts 1 and 2 into one patch, and instead of keeping track of a separate flag, it just uses the presence of an AuthConfig to determine whether or not auth is required.

Also, in future patches, I found that I didn't need the isAuthenticated function, so I left that out. We can always add that back in if we find there's a need for it, but I found it made more sense to just let the add-on set whether or not it's authenticated.

	:Margaret Leibovic Assignee
	Comment 27 • 10 years ago

Addressed review comments. This uses Picasso as suggested, but I think we should discuss what kinds of patterns we want to encourage add-ons to use here, and consider switching back to BitmapUtils if we want the add-ons to just use local images.

Instead of keeping track of the authenticate callbacks in a separate object, I decided that it made more sense to just get them directly from the panel options when we need them.

	:Margaret Leibovic Assignee
	Comment 28 • 10 years ago

Oops, posted wrong version of the patch.

	:Margaret Leibovic Assignee
	Comment 29 • 10 years ago

This is still a bit of a WIP, but it actually does something useful now :)

One of my main concerns is that we shouldn't be reading prefs on the main thread, so we'll need to add some logic to handle that.

Testing with my Pocket add-on, I actually found this works nicely. The add-on panel options code looks like this:

  function optionsCallback() {
    return {
      id: PANEL_ID,
      title: "Pocket",
      layout: Home.panels.Layout.FRAME,
      views: [{
        type: Home.panels.View.LIST,
        dataset: DATASET_ID
      }],
      authHandler: {
        authenticate: function authenticate() {
          Pocket.authenticate(function() {
            Home.panels.setAuthenticated(PANEL_ID, true);
            updateData(openPocketPanel);
          });
        },
        messageText: "Please log in to Pocket",
        buttonText: "Log in",
        imageUrl: "https://getpocket.com/i/v4/pocket_logo@1x.png"
      }
    };
  }

And while the panel was open, I ran |Home.panels.setAuthenticated(PANEL_ID, false)| from the remote console, and watched the UI flip to the auth layout, which was nice.

	Lucas Rocha (:lucasr)
	Comment 30 • 10 years ago

(In reply to :Margaret Leibovic from comment #24)
> (In reply to Lucas Rocha (:lucasr) from comment #22)
> 
> > @@ +41,5 @@
> > > +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomePanels:Authenticate", panelId));
> > > +            }
> > > +        });
> > > +
> > > +        final ImageView imageView = (ImageView) findViewById(R.id.image);
> > 
> > You should use Picasso here:
> > 
> > Picasso.with(getContext())
> >        .load(authConfig.getImageUrl())
> >        .into(imageView);
> 
> Does Picasso handle loading local images? I feel like we should encourage
> add-ons to use local images for static UI like this, since it's unlikely to
> be changing between add-on updates.

AFAIK, yes, Picasso does support local image loading. Should be simple to test this with an add-on.

	Lucas Rocha (:lucasr)
	Comment 31 • 10 years ago

(In reply to :Margaret Leibovic from comment #27)
> Created attachment 8385040 [details] [diff] [review]
> (Part 4 v2) Add a view for authentication UI to DynamicPanel
> 
> Addressed review comments. This uses Picasso as suggested, but I think we
> should discuss what kinds of patterns we want to encourage add-ons to use
> here, and consider switching back to BitmapUtils if we want the add-ons to
> just use local images.

Yeah, it's probably a good idea to advice add-on devs to use local images here but I think we should still keep the support for remote images. Picasso does support loading local images, see:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty/com/squareup/picasso/Picasso.java#161

	Lucas Rocha (:lucasr)
	Comment 32 • 10 years ago

Comment on attachment 8385038 [details] [diff] [review]
(Parts 1-2) Create AuthConfig and add authHandler option to Home.panels.register API

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

Looks great, just needs to validate AuthConfig/AuthHandler consistently between Java and JS.

::: mobile/android/base/home/HomeConfig.java
@@ +726,5 @@
> +
> +        public AuthConfig(JSONObject json) throws JSONException, IllegalArgumentException {
> +            mMessageText = json.optString(JSON_KEY_MESSAGE_TEXT);
> +            mButtonText = json.optString(JSON_KEY_BUTTON_TEXT);
> +            mImageUrl = json.optString(JSON_KEY_IMAGE_URL);

optString returns an empty string if the key doesn't exist. Is that intended? Maybe this should be null for clarity i.e. use optString(JSON_KEY_..., null) instead.

I think you should still add a validate() method that logs a warning if none of the values is defined, just to let the add-on developer know that he/she is defining an unnecessary AuthConfig.

::: mobile/android/modules/Home.jsm
@@ +242,5 @@
> +      if (!options.authHandler.authenticate || typeof options.authHandler.authenticate !== "function") {
> +        throw "Home.panels: Invalid authHandler authenticate function: panel.id = " + this.id;
> +      }
> +      if (!options.authHandler.messageText) {
> +        throw "Home.panels: Invalid authHandler messageText: panel.id = " + this.id;

If you're validating those here, you should do the same in Java for consistency. We could probably allow add-on devs to just use a default message/button/image by leaving those undefined here.

	Lucas Rocha (:lucasr)
	Comment 33 • 10 years ago

Comment on attachment 8385050 [details] [diff] [review]
(Part 4 v2) Add a view for authentication UI to DynamicPanel

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

Looks great. Just giving f+ for now to make sure we discuss the default image vs. hide image aspect.

::: mobile/android/base/home/DynamicPanel.java
@@ +51,5 @@
>  
>      // Dataset ID to be used by the loader
>      private static final String DATASET_REQUEST = "dataset_request";
>  
> +    // The main view for this fragment. This holds the PanelLayout and PanelAuthLayout.

nit: This holds -> Contains

@@ +132,5 @@
>      public void onDestroyView() {
>          super.onDestroyView();
>          mPanelLayout = null;
> +        mPanelAuthLayout = null;
> +        mView = null;

nit: move the mView null assignment to be the first one.

@@ +161,5 @@
>      @Override
>      protected void load() {
>          Log.d(LOGTAG, "Loading layout");
> +
> +        if (mPanelConfig.getAuthConfig() != null) {

I would kinda prefer having a higher-level method here like requiresAuth() to make this easier to follow.

::: mobile/android/base/home/PanelAuthLayout.java
@@ +28,5 @@
> +        super(context);
> +
> +        final AuthConfig authConfig = panelConfig.getAuthConfig();
> +        if (authConfig == null) {
> +            throw new IllegalStateException("Can't create PanelAuthLayout without valid AuthConfig");

without a valid...

@@ +37,5 @@
> +
> +        final TextView messageView = (TextView) findViewById(R.id.message);
> +        messageView.setText(authConfig.getMessageText());
> +
> +        final String panelId = panelConfig.getId();

nit: reorder this to make it easier to follow:

final Button buttonView = (Button) findViewById(R.id.button);
buttonView.setText(authConfig.getButtonText());

final String panelId = panelConfig.getId();
buttonView.setOnClickListener(new View.OnClickListener() {
...
}

@@ +53,5 @@
> +            final ImageView imageView = (ImageView) findViewById(R.id.image);
> +            imageView.setVisibility(View.VISIBLE);
> +
> +            Picasso.with(getContext())
> +                   .load(imageUrl)

I'd rather have a default image here so that the UI looks streamlined in all cases. Check with ibarlow.

::: mobile/android/base/resources/layout/panel_auth_layout.xml
@@ +5,5 @@
> +<merge xmlns:android="http://schemas.android.com/apk/res/android">
> +
> +    <ImageView android:id="@+id/image"
> +               android:layout_width="fill_parent"
> +               android:layout_height="wrap_content"

We should probably set a specific size for this image here to enforce a consistent layout. Ask ibarlow for specs.

@@ +8,5 @@
> +               android:layout_width="fill_parent"
> +               android:layout_height="wrap_content"
> +               android:scaleType="center"
> +               android:paddingBottom="10dp"
> +               android:visibility="gone"/>

I'd personally prefer to use a default image (instead of hiding it) if no image_url is defined in AuthConfig. Please check with ibarlow.

::: mobile/android/modules/Home.jsm
@@ +293,5 @@
> +      throw "Home.panels: Invalid authHandler for panel.id = " + id;
> +    }
> +    if (!options.authHandler.authenticate || typeof options.authHandler.authenticate !== "function") {
> +      throw "Home.panels: Invalid authHandler authenticate function: panel.id = " + this.id;
> +    }

Yeah, validating the authHandler here makes sense.

	Lucas Rocha (:lucasr)
	Comment 34 • 10 years ago

Comment on attachment 8385051 [details] [diff] [review]
(Part 5) Create a PanelAuthCache to keep track of whether or not panel is authenciated

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

::: mobile/android/base/home/DynamicPanel.java
@@ +170,5 @@
>      protected void load() {
>          Log.d(LOGTAG, "Loading layout");
>  
> +        // XXX: We should read the auth pref on the background thread.
> +        if (mPanelConfig.getAuthConfig() != null && !mPanelAuthCache.isAuthenticated(mPanelConfig.getId())) {

It's absolutely fine to do this in an UIAsyncTask. You'll probably have to lazy create the PanelLayout here instead of onCreateView() to avoid flashing it before showing the auth UI if the panel needs auth. In this case, you should probably also move the PanelAuthCache.setOnChangeListener() call to after mPanelLayout is created.  

(new UiAsyncTask<Void, Void, Boolean>(ThreadUtils.getBackgroundHandler()) {
    @Override
    public synchronized String doInBackground(Void... params) {
        return mPanelAuthCache.isAuthenticated(mPanelConfig.getId());
    }

    @Override
    public void onPostExecute(Boolean isAuthenticated) {
         if (isAuthenticated) {
             ...
         } else {
             ...
         }
    }
}).execute();

You should avoid running the UIAsyncTask altogether if the AuthConfig is null though.

@@ +184,5 @@
>              mPanelAuthLayout = new PanelAuthLayout(getActivity(), mPanelConfig);
>              mView.addView(mPanelAuthLayout, 0);
>          }
>  
> +        if (mPanelAuthLayout != null) {

This fix should probably be moved to the patch that introduces setAuthVisible().

@@ +309,5 @@
> +        @Override
> +        public void onChange(String panelId, boolean isAuthenticated) {
> +            if (!mPanelConfig.getId().equals(panelId)) {
> +                return;
> +            }

nit: add empty line here.

@@ +313,5 @@
> +            }
> +            if (isAuthenticated) {
> +                mPanelLayout.load();
> +            }
> +            setAuthVisible(!isAuthenticated);

I have a slight preference for calling mPanelLayout.load() after setAuthVisible().

::: mobile/android/base/home/PanelAuthCache.java
@@ +9,5 @@
> +import android.content.SharedPreferences;
> +import android.content.SharedPreferences.OnSharedPreferenceChangeListener;
> +import android.preference.PreferenceManager;
> +import android.util.Log;
> +

Class comment would be nice here. Explain how this class relates to the JS side of things and how it's expected to be used by DynamicPanel.

@@ +17,5 @@
> +    // Keep this in sync with the constant defined in Home.jsm
> +    private static final String PREFS_PANEL_AUTH_PREFIX = "home_panels_auth_";
> +
> +    private final Context mContext;
> +    private PrefsListener mPrefsListener;

mSharedPrefsListener for clarity.

@@ +29,5 @@
> +        mContext = context;
> +    }
> +
> +    private SharedPreferences getSharedPreferences() {
> +        return PreferenceManager.getDefaultSharedPreferences(mContext);

FYI: this will have to be moved to the PROFILE scope if I land my patch for bug 940575 before you land this.

@@ +57,5 @@
> +            prefs.registerOnSharedPreferenceChangeListener(mPrefsListener);
> +        }
> +    }
> +
> +    private class PrefsListener implements OnSharedPreferenceChangeListener {

Rename to SharedPrefsListener for clarity.

::: mobile/android/modules/Home.jsm
@@ +372,5 @@
> +    setAuthenticated: function(id, isAuthenticated) {
> +      _assertPanelExists(id);
> +
> +      let authKey = PREFS_PANEL_AUTH_PREFIX + id;
> +      let sharedPrefs = new SharedPreferences();

We'll have to use a PROFILE-scoped branch for this SharedPreferences() instance here.

	:Margaret Leibovic Assignee
	Comment 35 • 10 years ago

ibarlow, your assistance is requested! For a bit of context, in this patch I decided to just show no image if the add-on didn't specify one, but I see lucasr's point about providing a default image for consistency. Do you have an opinion about these point below?

(In reply to Lucas Rocha (:lucasr) from comment #33)

> @@ +53,5 @@
> > +            final ImageView imageView = (ImageView) findViewById(R.id.image);
> > +            imageView.setVisibility(View.VISIBLE);
> > +
> > +            Picasso.with(getContext())
> > +                   .load(imageUrl)
> 
> I'd rather have a default image here so that the UI looks streamlined in all
> cases. Check with ibarlow.

> ::: mobile/android/base/resources/layout/panel_auth_layout.xml
> @@ +5,5 @@
> > +<merge xmlns:android="http://schemas.android.com/apk/res/android">
> > +
> > +    <ImageView android:id="@+id/image"
> > +               android:layout_width="fill_parent"
> > +               android:layout_height="wrap_content"
> 
> We should probably set a specific size for this image here to enforce a
> consistent layout. Ask ibarlow for specs.

> @@ +8,5 @@
> > +               android:layout_width="fill_parent"
> > +               android:layout_height="wrap_content"
> > +               android:scaleType="center"
> > +               android:paddingBottom="10dp"
> > +               android:visibility="gone"/>
> 
> I'd personally prefer to use a default image (instead of hiding it) if no
> image_url is defined in AuthConfig. Please check with ibarlow.

	:Margaret Leibovic Assignee
	Comment 36 • 10 years ago

I updated the layout to be a bit closer to what we actually want, and to use the Firefox logo as a default image. I think we should move polishing this off into another bug, but just uploading this for reference.

	:Margaret Leibovic Assignee
	Comment 37 • 10 years ago

Addressed review comments. I think we should leave the message/button text required, since it feels odd to have a generic message/button text that could lead to authenticating anywhere.

	:Margaret Leibovic Assignee
	Comment 38 • 10 years ago

Addressed review comments and included default image as suggested. I also tweaked the layout to look a bit closer to the mock-ups, but I think we can push the final polish details into a follow-up bug.

	:Margaret Leibovic Assignee
	Comment 39 • 10 years ago

Update to use UiAsyncTask. I moved mPanelLayout creation into a helper method that creates the view if necessary before loading it.

	:Margaret Leibovic Assignee
	Comment 40 • 10 years ago

Comment on attachment 8385707 [details] [diff] [review]
(Part 5 v2) Create a PanelAuthCache to keep track of whether or not panel is authenciated

I'm running into an NPE on orientation change, so we need to do more to handle the fact that mPanelLayout might be null. Downgrading to feedback request.

	Lucas Rocha (:lucasr)
	Comment 41 • 10 years ago

Comment on attachment 8385665 [details] [diff] [review]
(Parts 1-2 v2) Create AuthConfig and add authHandler option to Home.panels.register API

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

Nice.

::: mobile/android/base/home/HomeConfig.java
@@ +755,5 @@
> +            validate();
> +        }
> +
> +        private void validate() {
> +            if (mMessageText == null) {

Better do a TextUtils.isEmpty() here as it checks for both null and empty strings.

@@ +759,5 @@
> +            if (mMessageText == null) {
> +                throw new IllegalArgumentException("Can't create AuthConfig with null message text");
> +            }
> +
> +            if (mButtonText == null) {

Same here.

	Lucas Rocha (:lucasr)
	Comment 42 • 10 years ago

Comment on attachment 8385666 [details] [diff] [review]
(Part 4 v3) Add a view for authentication UI to DynamicPanel

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

Looks good with the suggested fixes.

::: mobile/android/base/home/DynamicPanel.java
@@ +173,5 @@
> +    /**
> +     * @return true if this panel requires authentication.
> +     */
> +    private boolean requiresAuth() {
> +        return mPanelConfig.getAuthConfig() != null;

nit: enclose expression with ()'s

::: mobile/android/base/home/PanelAuthLayout.java
@@ +53,5 @@
> +        final String imageUrl = authConfig.getImageUrl();
> +
> +        if (TextUtils.isEmpty(imageUrl)) {
> +            // Use a default image if an image URL isn't specified.
> +            imageView.setImageResource(R.drawable.icon_home_empty_firefox);

No need to handle the placeholder manually. We should just lean on Picasso's placeholder support here. Simply change the Picasso calls below to be:

 Picasso.with(getContext())
        .load(imageUrl)
        .placeholder(R.drawable.icon_home_empty_firefox)
        .into(imageView);

And Picasso will (in theory) to the right thing for us.

::: mobile/android/base/resources/drawable/panel_auth_button.xml
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<selector xmlns:android="http://schemas.android.com/apk/res/android">

I'm kinda surprised with didn't have such drawable before.

@@ +12,5 @@
> +                  android:top="-2dp">
> +
> +                <shape android:shape="rectangle" >
> +                    <stroke android:width="2dp"
> +                            android:color="#FFE0E4E7" />

Make sure you're not duplicating some of our pre-existing colors from colors.xml.

::: mobile/android/base/resources/layout/panel_auth_layout.xml
@@ +6,5 @@
> +
> +    <!-- Empty spacer view -->
> +    <View android:layout_width="fill_parent"
> +          android:layout_height="0dip"
> +          android:layout_weight="1"/>

Given that you're adding an extra view just to align things here, it's probably simpler/more robust to just enclose the image/message views in layout with gravity center and weight=1?

@@ +10,5 @@
> +          android:layout_weight="1"/>
> +
> +    <ImageView android:id="@+id/image"
> +               android:layout_width="fill_parent"
> +               android:layout_height="wrap_content"

Please set a specific size here to enforce a max width/height on the image. We should probably specify the expected image size for this UI in the API docs.

	Lucas Rocha (:lucasr)
	Comment 43 • 10 years ago

Comment on attachment 8385707 [details] [diff] [review]
(Part 5 v2) Create a PanelAuthCache to keep track of whether or not panel is authenciated

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

I'm starting to think we should re-purpose setAuthVisible() to something more general. Having setAuthVisible() setting mPanelLayout's visibility and then having a separate loadPanelLayout() method feels a bit fragile. In practice, whenever we make mPanelLayout visible, we should also load it. Here's what I suggest:

1. Re-purpose setAuthVisible() into something like setUIMode(enum UIMode), where UIMode can be PANEL or AUTH. I've used 'UIMode' in the toolbar code too. I'm open to other suggestions though.
2. In setUIMode(), lazy load both auth and panel layouts in there (analogous to what you're already doing with the auth layout but also applied to panel layout)
3. In setUIMode(), call panel layout load() whenever setUIMode() is called with PANEL.

::: mobile/android/base/home/DynamicPanel.java
@@ +165,5 @@
> +                    return mPanelAuthCache.isAuthenticated(mPanelConfig.getId());
> +                }
> +
> +                @Override
> +                public void onPostExecute(Boolean isAuthenticated) {

This would become something like:

setUIMode(isAuthenticated ? UIMode.PANEL : UIMode.AUTH);

@@ +182,5 @@
>      /**
> +     * Lazily creates panel layout to prevent it from flashing before
> +     * authentication UI is shown.
> +     */
> +    private void loadPanelLayout() {

I'd still keep this method as createPanelLayout() and call it in setUIMode() accordingly.

@@ +348,5 @@
> +            setAuthVisible(!isAuthenticated);
> +
> +            if (isAuthenticated) {
> +                loadPanelLayout();
> +            }

The setAuthVisible() and loadPanelLayout() calls above would become simply:

setUIMode(isAthenticated ? UIMode.PANEL : UIMode.AUTH);

::: mobile/android/base/home/PanelAuthCache.java
@@ +18,5 @@
> + * {@code DynamicPanel} uses this cache to determine whether or not to
> + * show authentication UI for dynamic panels, including listening for
> + * changes in authentication state.
> + */
> +class PanelAuthCache {

I should be pretty simpler to add browser instrumentation tests for this class btw. The infra is still in flux so, for now, file a follow-up bug so that we get back to this later.

	:Margaret Leibovic Assignee
	Comment 44 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #42)

> ::: mobile/android/base/resources/drawable/panel_auth_button.xml
> @@ +2,5 @@
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +
> > +<selector xmlns:android="http://schemas.android.com/apk/res/android">
> 
> I'm kinda surprised with didn't have such drawable before.

I based this off the home_banner drawable, but the darker line is inverted in that drawable. Perhaps it would be better to just find a way to re-use that one?

> @@ +12,5 @@
> > +                  android:top="-2dp">
> > +
> > +                <shape android:shape="rectangle" >
> > +                    <stroke android:width="2dp"
> > +                            android:color="#FFE0E4E7" />
> 
> Make sure you're not duplicating some of our pre-existing colors from
> colors.xml.

These colors aren't in colors.xml, but they are in home_banner.xml. I can file a follow-up to consolidate these colors in colors.xml, and look into sharing a background drawable for both this button and the home banner.

> ::: mobile/android/base/resources/layout/panel_auth_layout.xml
> @@ +6,5 @@
> > +
> > +    <!-- Empty spacer view -->
> > +    <View android:layout_width="fill_parent"
> > +          android:layout_height="0dip"
> > +          android:layout_weight="1"/>
> 
> Given that you're adding an extra view just to align things here, it's
> probably simpler/more robust to just enclose the image/message views in
> layout with gravity center and weight=1?

I copied this from home_empty_panel.xml, so we may want a follow-up to fix that view as well.

	:Margaret Leibovic Assignee
	Comment 45 • 10 years ago

I liked your suggestion for a UIMode enum, this feels cleaner.

Sorry I've been so slow iterating on these patches, it will be nice to finally land them!

	Lucas Rocha (:lucasr)
	Comment 46 • 10 years ago

Comment on attachment 8389521 [details] [diff] [review]
(Part 5 v3) Create a PanelAuthCache to keep track of whether or not panel is authenciated

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

Looks awesome. Just want to understand why that null check is necessary before giving r+.

::: mobile/android/base/home/DynamicPanel.java
@@ +344,5 @@
>          public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) {
>              final DatasetRequest request = getRequestFromLoader(loader);
>  
>              Log.d(LOGTAG, "Finished loader for request: " + request);
> +            if (mPanelLayout != null) {

How could mPanelLayout be null here is we only call load() after creating it? Rotation? Maybe we should load the auth state in a loader too (instead of UIAsyncTask) so that the async operation doesn't get cancelled on rotation?

	:Margaret Leibovic Assignee
	Comment 47 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #46)

> ::: mobile/android/base/home/DynamicPanel.java
> @@ +344,5 @@
> >          public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) {
> >              final DatasetRequest request = getRequestFromLoader(loader);
> >  
> >              Log.d(LOGTAG, "Finished loader for request: " + request);
> > +            if (mPanelLayout != null) {
> 
> How could mPanelLayout be null here is we only call load() after creating
> it? Rotation? Maybe we should load the auth state in a loader too (instead
> of UIAsyncTask) so that the async operation doesn't get cancelled on
> rotation?

I think I was just being overly cautious, I'm not seeing a crash when I remove this check, so I don't think it's necessary.

However, I did notice a problem where the auth layout disappears on rotation, so I'll need to investigate that...

	:Margaret Leibovic Assignee
	Comment 48 • 10 years ago

(In reply to :Margaret Leibovic from comment #47)
> (In reply to Lucas Rocha (:lucasr) from comment #46)
> 
> > ::: mobile/android/base/home/DynamicPanel.java
> > @@ +344,5 @@
> > >          public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) {
> > >              final DatasetRequest request = getRequestFromLoader(loader);
> > >  
> > >              Log.d(LOGTAG, "Finished loader for request: " + request);
> > > +            if (mPanelLayout != null) {
> > 
> > How could mPanelLayout be null here is we only call load() after creating
> > it? Rotation? Maybe we should load the auth state in a loader too (instead
> > of UIAsyncTask) so that the async operation doesn't get cancelled on
> > rotation?
> 
> I think I was just being overly cautious, I'm not seeing a crash when I
> remove this check, so I don't think it's necessary.

Scratch that, it is crashing if the panel layout is been created and displayed, and then the device is rotated.

	:Margaret Leibovic Assignee
	Comment 49 • 10 years ago

I just made a mess trying to come up with a loader solution for this, but basically the main issue is that we don't call load() again after the fragment gets recreated on rotation, and since we throw the old views away, the new views never get recreated. So, I'm not actually sure that moving the auth state check into a loader will fix our problem here. I experimented with calling load() in onActivityCreated instead of loadIfVisible(), and that fixes the problem (if I also keep that null check), but that doesn't seem like a great solution.

We should discuss this more to figure out what kind of approach we should take here.

	:Margaret Leibovic Assignee
	Comment 50 • 10 years ago

No longer a P1, since we don't need this for 30.

	:Margaret Leibovic Assignee
	Comment 51 • 10 years ago

I want to try to land this sooner rather than later, so that we can use it for user testing (also so that my patches won't keep bitrotting!).

Lucas, can you help give me feedback on the best way to approach this problem? As I mentioned in comment 49, the issue here is that the views are getting destroyed, but they aren't necessarily getting recreated because DynamicPanel.load does not get called again.

A partial solution could be to continue to create the views in onCreateView, although we'd need to move where we check the authentication state as well, since we need a way to know whether or not to show the authentication UI when the views are recreated.

If we were to use a loader instead of the UiAsyncTask, what would that look like? Would that solve this problem?

	Lucas Rocha (:lucasr)
	Comment 58 • 10 years ago

Comment on attachment 8396578 [details] [diff] [review]
Reset isLoaded state in HomeFragment on configuration change (r=margaret)

This patch fixes the problem with load() not being called on device rotation. This is serious issue, we should definitely uplift. I can file a separate bug if you want.

	Lucas Rocha (:lucasr)
	Comment 59 • 10 years ago

Comment on attachment 8396583 [details] [diff] [review]
Only load if fragment is also loaded

This is the only change I've made in your "part 5" patch.

	Lucas Rocha (:lucasr)
	Comment 60 • 10 years ago

I had to rebase all your patches locally in order to track down this issue. I'm posting the rebased patches here in case you find them useful. I'm made a bit more than rebasing in part 1-2 because the code has changed quite a bit since you wrote it.

	:Margaret Leibovic Assignee
	Comment 61 • 10 years ago

Comment on attachment 8396578 [details] [diff] [review]
Reset isLoaded state in HomeFragment on configuration change (r=margaret)

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

Good catch. I think we should put this in a separate bug to make it easier to track uplifting.

I'll apply all your re-based patches to test things out myself before getting ready to land all the other patches here.

	:Margaret Leibovic Assignee
	Comment 62 • 10 years ago

Comment on attachment 8396578 [details] [diff] [review]
Reset isLoaded state in HomeFragment on configuration change (r=margaret)

Moved this over to bug 987963.

	:Margaret Leibovic Assignee
	Comment 63 • 10 years ago

Comment on attachment 8381768 [details] [diff] [review]
(Part 3) Rename mLayout -> mPanelLayout in DyanmicPanel

Marking the bitrotted versions as obsolete, to make the attachments in here less confusing.

	:Margaret Leibovic Assignee
	Comment 64 • 10 years ago

It looks like this is the only patch missing an actual r+.

I folded in your additional diff, and I found that this was working well with a panel that required auth, but the panel data was failing to show up with panels that *don't* require auth.

To fix this, I reordered the logic in loadIfVisible to set mIsLoaded = true before calling load(). However, could you elaborate about why we need this isLoaded check? Is this just to account for the possibility of ending up in setUIMode if the UiAsyncTask onPostExecute runs after the panel is unloaded? If that is the case, maybe we should put something in there to catch this case instead of inside setUIMode.

	Lucas Rocha (:lucasr)
	Comment 65 • 10 years ago

Comment on attachment 8396703 [details] [diff] [review]
Create a PanelAuthCache to keep track of whether or not panel is authenciated (r=lucasr)

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

(In reply to :Margaret Leibovic from comment #64)
> Created attachment 8396703 [details] [diff] [review]
> Create a PanelAuthCache to keep track of whether or not panel is
> authenciated (r=lucasr)
> 
> It looks like this is the only patch missing an actual r+.
> 
> I folded in your additional diff, and I found that this was working well
> with a panel that required auth, but the panel data was failing to show up
> with panels that *don't* require auth.

Nice catch. This is happening because the setUIMode is called asynchronously. 

> To fix this, I reordered the logic in loadIfVisible to set mIsLoaded = true
> before calling load(). However, could you elaborate about why we need this
> isLoaded check?

This is to avoid loading data if the auth cache state changes while the panel is not loaded. Looking back at this code, the check should be different.

> Is this just to account for the possibility of ending up in
> setUIMode if the UiAsyncTask onPostExecute runs after the panel is unloaded?

Not really, but we probably protect against this case too.

Here's what I suggest:

- Get rid of my isLoaded() getter.
- Factor out the early check in loadIfVisible() into a canLoad() method.

protected void canLoad() {
    return (mCanLoadHint && isVisible() && getUserVisibleHint());
}

- Check with canLoad() instead of isLoaded() in setUIMode() to decide whether we can call mLayout.load() or not.
- Hold a reference to the UIAsyncTask triggered in load() and cancel it if onDestroyView is called while it's running. Clear the reference in onPostExecute.

	Lucas Rocha (:lucasr)
	Comment 67 • 10 years ago

Comment on attachment 8397199 [details] [diff] [review]
Bug 988355 - Properly handle device rotation in DynamicPanel (r=margaret)

Oopsie, wrong bug.

	:Margaret Leibovic Assignee
	Comment 68 • 10 years ago

Updated to address feedback.

I tested this out with panels with and without authentication, and I wasn't able to find any problems.

	Lucas Rocha (:lucasr)
	Comment 69 • 10 years ago

Comment on attachment 8397264 [details] [diff] [review]
Create a PanelAuthCache to keep track of whether or not panel is authenciated (r=lucasr)

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

Awesome.

::: mobile/android/base/home/HomeFragment.java
@@ +228,2 @@
>      protected void loadIfVisible() {
> +        if (!canLoad()) {

While you're at it, maybe merge the two if's?

if (!canLoad() || mIsLoaded) {
    return;
}

load();
mIsLoaded = true;

	:Margaret Leibovic Assignee
	Comment 70 • 10 years ago

(In reply to Lucas Rocha (:lucasr) from comment #69)
> Comment on attachment 8397264 [details] [diff] [review]
> Create a PanelAuthCache to keep track of whether or not panel is
> authenciated (r=lucasr)
> 
> Review of attachment 8397264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome.

\o/

> ::: mobile/android/base/home/HomeFragment.java
> @@ +228,2 @@
> >      protected void loadIfVisible() {
> > +        if (!canLoad()) {
> 
> While you're at it, maybe merge the two if's?
> 
> if (!canLoad() || mIsLoaded) {
>     return;
> }
> 
> load();
> mIsLoaded = true;

Good call.

	:Margaret Leibovic Assignee
	Comment 71 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/e12099be1b4e
https://hg.mozilla.org/integration/fx-team/rev/17edc0786565
https://hg.mozilla.org/integration/fx-team/rev/57002a8b0151
https://hg.mozilla.org/integration/fx-team/rev/189ae6f118ce
https://hg.mozilla.org/integration/fx-team/rev/fde355461074

	:Margaret Leibovic Assignee
	Comment 72 • 10 years ago

Also updated our docs in the wiki page:
https://wiki.mozilla.org/Mobile/Projects/Third-party_service_integration_MVP#JS_APIs

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 73 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/e12099be1b4e
https://hg.mozilla.org/mozilla-central/rev/17edc0786565
https://hg.mozilla.org/mozilla-central/rev/57002a8b0151
https://hg.mozilla.org/mozilla-central/rev/189ae6f118ce
https://hg.mozilla.org/mozilla-central/rev/fde355461074

	:Margaret Leibovic Assignee
	Comment 74 • 10 years ago

Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.