943915 - Move SiteIdentityPopup to the toolbar package

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

It's meant to be owned by the toolbar, not BrowserApp.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Make DootHanger's API public (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Move DoorHanger to the widget package (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Small cleanup on the use of Resources in ArrowPopup (r=wesj)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Move SiteIdentityPopup to the toolbar package (r=wesj)

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Add ArrowPopup constructor without anchor view argument (r=wesj)

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Remove 'a' prefix from arguments in ArrowPopup/SiteIdentityPopup (r=wesj)

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Encapsulate SiteIdentityPopup into BrowserToolbar (r=wesj)

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Change SiteIdentityPopup's API to be package-scoped (r=wesj)

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Encapsulate button click listener in SiteIdentityPopup (r=wesj)

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8339351 [details] [diff] [review]
Make DootHanger's API public (r=wesj)

Preparatory work for bug 942862.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 8339351 [details] [diff] [review]
Make DootHanger's API public (r=wesj)

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

I hate the two DoorHanger files. These API's need cleaned up (in addition to moving things around). If you'd rather just move things here, and clean them up somewhere else, feel free to flip this to an r+. Otherwise, I'm r- just to make sure you see this.

::: mobile/android/base/DoorHanger.java
@@ +214,5 @@
>  
>          mChoicesLayout.addView(button, sButtonParams);
>      }
>  
> +    public void setOptions(final JSONObject options) {

I never like when these JSON methods are exposed. It seems to just tempt people to use them. "I'm in java and want to add a doorhanger, I'll just create a JSONObject and pass it in".

Maybe DoorHangerPopup.java is better to read these properties and feed them to the DoorHanger.

@@ +322,5 @@
>       *
>       * @param isShowing Whether or not this doorhanger is currently visible to the user.
>       *                 (e.g. the DoorHanger view might be VISIBLE, but its parent could be hidden)
>       */
> +    public boolean shouldRemove(boolean isShowing) {

Hmm.. This seems weird to expose to me. This method shouldn't be in here I think. It should probably be in DoorHangerPopup. Decreasing the persistence in it also seems... wrong. At best that should probably be decremented when a show() method was called.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8339354 [details] [diff] [review]
Move SiteIdentityPopup to the toolbar package (r=wesj)

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

Again, this is just API stuff that needs cleanup. Can you at least file some of it?

::: mobile/android/base/BrowserApp.java
@@ +26,5 @@
>  import org.mozilla.gecko.preferences.GeckoPreferences;
>  import org.mozilla.gecko.prompts.Prompt;
>  import org.mozilla.gecko.toolbar.AutocompleteHandler;
>  import org.mozilla.gecko.toolbar.BrowserToolbar;
> +import org.mozilla.gecko.toolbar.SiteIdentityPopup;

Its weird to see BrowserApp owning so much toolbar code. This makes that mistake obvious though, which is nice.

::: mobile/android/base/Tab.java
@@ +7,5 @@
>  
>  import org.mozilla.gecko.db.BrowserDB;
>  import org.mozilla.gecko.gfx.Layer;
>  import org.mozilla.gecko.home.HomePager;
> +import org.mozilla.gecko.toolbar.SiteIdentityPopup;

Hmm... This is only used to pull out SiteIdentityPopup.UNKNOWN. That should be in its own class, or something public.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ +6,2 @@
>  
> +import org.mozilla.gecko.BrowserApp;

Weird. This shouldn't be needed AFAICT. GeckoApp is in ArrowPopup (to get the width/height of geckoLayout, which at this point is actually always the screen height. i.e. this isn't needed at all. A Context would do.). In fact, I bet there's a bug there where we overflow the screen vertically now...

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 8339357 [details] [diff] [review]
Encapsulate SiteIdentityPopup into BrowserToolbar (r=wesj)

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

::: mobile/android/base/BrowserApp.java
@@ +197,5 @@
>              case SELECTED:
>                  if (Tabs.getInstance().isSelectedTab(tab)) {
>                      updateHomePagerForTab(tab);
>  
> +                    mBrowserToolbar.dismissSiteIdentityPopup();

BrowserToolbar has its own onTabChanged listener. Can we just move this there?

@@ +595,5 @@
>          if (dismissEditingMode()) {
>              return;
>          }
>  
> +        if (mBrowserToolbar.dismissSiteIdentityPopup()) {

I hate this back code too. Its an easy detail to overlook.

Maybe we can just add if (mBrowserToolbar.onBackPressed()) return; ?

@@ +1027,5 @@
>      public void refreshChrome() {
>          invalidateOptionsMenu();
>          updateSideBarState();
>          mTabsPanel.refresh();
> +        mBrowserToolbar.dismissSiteIdentityPopup();

I would rather this was general. mBrowserToolbar.refresh()?

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +473,5 @@
>              });
>          }
>      }
>  
> +    public boolean dismissSiteIdentityPopup() {

I'd personally rather the above than having BrowserApp know anything at all about the site identity popup.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8339358 [details] [diff] [review]
Change SiteIdentityPopup's API to be package-scoped (r=wesj)

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

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ +41,5 @@
> +    static final int LEVEL_UKNOWN = 0;
> +    static final int LEVEL_IDENTIFIED = 1;
> +    static final int LEVEL_VERIFIED = 2;
> +    static final int LEVEL_MIXED_CONTENT_BLOCKED = 3;
> +    static final int LEVEL_MIXED_CONTENT_LOADED = 4;

We should move these and the stuff above into a complex enum and then leave it public? Maybe we can put it in AppConstants? We'd have to be explicit about image levels I guess...

public enum SecurityLevel {
    UNKONWN  (0, "unknown"),
    VERIFIED (1, "verified"),
    ...

    private final String name;
    private final int level;
    SecurityLevel(int imageLevel, String name) { this.level = imageLevel; this.name = name; }
    public String getName() { return name; }
    public int getLevel() { return level; }
    public boolean matches(String name) { return this.name.equals(name); }
}

@@ +67,1 @@
>          if (IDENTIFIED.equals(mode)) {

Then this could be:

for (SecurityLevel level : SecurityLevel.values()) {
  if (level.matches(mode)) {
    return level;
  }
}

@@ +176,5 @@
>  
>      /*
>       * @param identityData A JSONObject that holds the current tab's identity data.
>       */
> +    void updateIdentity(JSONObject identityData) {

you could make the JSONObject final

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8339359 [details] [diff] [review]
Encapsulate button click listener in SiteIdentityPopup (r=wesj)

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

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ +56,5 @@
>      private TextView mVerifier;
>  
>      private DoorHanger mMixedContentNotification;
>  
> +    private OnButtonClickListener mButtonClickListener;

Make this final.

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #11)
> Comment on attachment 8339351 [details] [diff] [review]
> Make DootHanger's API public (r=wesj)
> 
> Review of attachment 8339351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I hate the two DoorHanger files. These API's need cleaned up (in addition to
> moving things around). If you'd rather just move things here, and clean them
> up somewhere else, feel free to flip this to an r+. Otherwise, I'm r- just
> to make sure you see this.

Yeah, I kinda hate the current DoorHanger stuff too. I've made a huge effort to not enter a rabbit hole here :-)

Filed bug 949200 to track this.

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #12)
> Comment on attachment 8339354 [details] [diff] [review]
> Move SiteIdentityPopup to the toolbar package (r=wesj)
> 
> Review of attachment 8339354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Again, this is just API stuff that needs cleanup. Can you at least file some
> of it?

Bug 949200 covers it, I guess?

> ::: mobile/android/base/BrowserApp.java
> @@ +26,5 @@
> >  import org.mozilla.gecko.preferences.GeckoPreferences;
> >  import org.mozilla.gecko.prompts.Prompt;
> >  import org.mozilla.gecko.toolbar.AutocompleteHandler;
> >  import org.mozilla.gecko.toolbar.BrowserToolbar;
> > +import org.mozilla.gecko.toolbar.SiteIdentityPopup;
> 
> Its weird to see BrowserApp owning so much toolbar code. This makes that
> mistake obvious though, which is nice.

Yeah, organic code and all :-)

> ::: mobile/android/base/Tab.java
> @@ +7,5 @@
> >  
> >  import org.mozilla.gecko.db.BrowserDB;
> >  import org.mozilla.gecko.gfx.Layer;
> >  import org.mozilla.gecko.home.HomePager;
> > +import org.mozilla.gecko.toolbar.SiteIdentityPopup;
> 
> Hmm... This is only used to pull out SiteIdentityPopup.UNKNOWN. That should
> be in its own class, or something public.

Yeah, this bit has been completely reworked in bug 945212. SiteIdentity data now has a proper class that Tab and SiteIdentityPopup use. 
 
> ::: mobile/android/base/toolbar/SiteIdentityPopup.java
> @@ +6,2 @@
> >  
> > +import org.mozilla.gecko.BrowserApp;
> 
> Weird. This shouldn't be needed AFAICT. GeckoApp is in ArrowPopup (to get
> the width/height of geckoLayout, which at this point is actually always the
> screen height. i.e. this isn't needed at all. A Context would do.). In fact,
> I bet there's a bug there where we overflow the screen vertically now...

True. I've filed bug 949204 to track this. Just want this code to land asap (before I leave on vacation).

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #13)
> Comment on attachment 8339357 [details] [diff] [review]
> Encapsulate SiteIdentityPopup into BrowserToolbar (r=wesj)
> 
> Review of attachment 8339357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +197,5 @@
> >              case SELECTED:
> >                  if (Tabs.getInstance().isSelectedTab(tab)) {
> >                      updateHomePagerForTab(tab);
> >  
> > +                    mBrowserToolbar.dismissSiteIdentityPopup();
> 
> BrowserToolbar has its own onTabChanged listener. Can we just move this
> there?

Good point, done.

> @@ +595,5 @@
> >          if (dismissEditingMode()) {
> >              return;
> >          }
> >  
> > +        if (mBrowserToolbar.dismissSiteIdentityPopup()) {
> 
> I hate this back code too. Its an easy detail to overlook.
> 
> Maybe we can just add if (mBrowserToolbar.onBackPressed()) return; ?

Makes sense, done.
 
> @@ +1027,5 @@
> >      public void refreshChrome() {
> >          invalidateOptionsMenu();
> >          updateSideBarState();
> >          mTabsPanel.refresh();
> > +        mBrowserToolbar.dismissSiteIdentityPopup();
> 
> I would rather this was general. mBrowserToolbar.refresh()?

Fair enough, done.

> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +473,5 @@
> >              });
> >          }
> >      }
> >  
> > +    public boolean dismissSiteIdentityPopup() {
> 
> I'd personally rather the above than having BrowserApp know anything at all
> about the site identity popup.

Agree. With the changes above, dismissSiteIdentityPopup() is not used by BrowserApp anymore.

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #14)
> Comment on attachment 8339358 [details] [diff] [review]
> Change SiteIdentityPopup's API to be package-scoped (r=wesj)
> 
> Review of attachment 8339358 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/SiteIdentityPopup.java
> @@ +41,5 @@
> > +    static final int LEVEL_UKNOWN = 0;
> > +    static final int LEVEL_IDENTIFIED = 1;
> > +    static final int LEVEL_VERIFIED = 2;
> > +    static final int LEVEL_MIXED_CONTENT_BLOCKED = 3;
> > +    static final int LEVEL_MIXED_CONTENT_LOADED = 4;
> 
> We should move these and the stuff above into a complex enum and then leave
> it public? Maybe we can put it in AppConstants? We'd have to be explicit
> about image levels I guess...
> 
> public enum SecurityLevel {
>     UNKONWN  (0, "unknown"),
>     VERIFIED (1, "verified"),
>     ...
> 
>     private final String name;
>     private final int level;
>     SecurityLevel(int imageLevel, String name) { this.level = imageLevel;
> this.name = name; }
>     public String getName() { return name; }
>     public int getLevel() { return level; }
>     public boolean matches(String name) { return this.name.equals(name); }
> }
> 
> @@ +67,1 @@
> >          if (IDENTIFIED.equals(mode)) {
> 
> Then this could be:
> 
> for (SecurityLevel level : SecurityLevel.values()) {
>   if (level.matches(mode)) {
>     return level;
>   }
> }

Yep, this is covered in bug 945212.
 
> @@ +176,5 @@
> >  
> >      /*
> >       * @param identityData A JSONObject that holds the current tab's identity data.
> >       */
> > +    void updateIdentity(JSONObject identityData) {
> 
> you could make the JSONObject final

Not a big deal but I'll update my patch for bug 945212 to cover this.

Comment 20

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #15)
> Comment on attachment 8339359 [details] [diff] [review]
> Encapsulate button click listener in SiteIdentityPopup (r=wesj)
> 
> Review of attachment 8339359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/SiteIdentityPopup.java
> @@ +56,5 @@
> >      private TextView mVerifier;
> >  
> >      private DoorHanger mMixedContentNotification;
> >  
> > +    private OnButtonClickListener mButtonClickListener;
> 
> Make this final.

Done.

Comment 21

[Lucas Rocha (:lucasr)]

Attachment: Encapsulate SiteIdentityPopup into BrowserToolbar (r=wesj)

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 8346218 [details] [diff] [review]
Encapsulate SiteIdentityPopup into BrowserToolbar (r=wesj)

BrowserApp doesn't know anything about SiteIdentityPopup anymore. Filed bug 949216 as a follow-up for further encapsulation.

Comment 23

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/7636a1597015
https://hg.mozilla.org/integration/fx-team/rev/c3dd4b9e1c7e
https://hg.mozilla.org/integration/fx-team/rev/543e0fbc22f4
https://hg.mozilla.org/integration/fx-team/rev/b73e9b911235
https://hg.mozilla.org/integration/fx-team/rev/3e5bc802df62
https://hg.mozilla.org/integration/fx-team/rev/547f2316b280
https://hg.mozilla.org/integration/fx-team/rev/371cd009024b
https://hg.mozilla.org/integration/fx-team/rev/d6f749a28b3d
https://hg.mozilla.org/integration/fx-team/rev/6812364ddbae

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7636a1597015
https://hg.mozilla.org/mozilla-central/rev/c3dd4b9e1c7e
https://hg.mozilla.org/mozilla-central/rev/543e0fbc22f4
https://hg.mozilla.org/mozilla-central/rev/b73e9b911235
https://hg.mozilla.org/mozilla-central/rev/3e5bc802df62
https://hg.mozilla.org/mozilla-central/rev/547f2316b280
https://hg.mozilla.org/mozilla-central/rev/371cd009024b
https://hg.mozilla.org/mozilla-central/rev/d6f749a28b3d
https://hg.mozilla.org/mozilla-central/rev/6812364ddbae