801739 - Initialize mDoorHangerPopup in WebApp, not GeckoApp

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We currently initialize mDoorHangerPopup with a BrowserApp-specific DoorHangerPopup in BrowserApp, and a generic fallback DoorHangerPopup in GeckoApp. Instead, we should delegate doorhanger popup initialization to the WebApp/BrowserApp subclasses.

To do this, we need to make sure that GeckoApp can handle a null mDoorHangerPopup, but it looks like that might already be true.

See bug 800199 comments 5-8 for more context.

Comment 1

[Shriram (irc: Mavericks)]

Attachment: Patch that initializes mDoorHangerPopup in WebApp, not GeckoApp

Comment 2

[:Margaret Leibovic]

Comment on attachment 695645 [details] [diff] [review]
Patch that initializes mDoorHangerPopup in WebApp, not GeckoApp

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

Thanks for the patch! There's just one change I'd like you to make before we land this.

Note to myself: I double-checked that we have null checks before ever referencing mDoorHangerPopup, so this is a safe change to make.

::: mobile/android/base/BrowserApp.java
@@ +261,5 @@
>  
>      @Override
>      protected void initializeChrome(String uri, boolean isExternalURL) {
>          super.initializeChrome(uri, isExternalURL);
> +        mDoorHangerPopup = new DoorHangerPopup(this, null);

Now that this is moved into BrowserApp, you can actually call the constructor with the correct anchor view:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java#42

@@ +266,4 @@
>          mBrowserToolbar.updateBackButton(false);
>          mBrowserToolbar.updateForwardButton(false);
>  
>          mDoorHangerPopup.setAnchor(mBrowserToolbar.mFavicon);

So you could just replace this line with a call to the constructor that passes in mBrowserToolbar.mFavicon.

Comment 3

[Shriram (irc: Mavericks)]

Attachment: Patch that initializes mDoorHangerPopup in WebApp, not GeckoApp

Comment 4

[:Margaret Leibovic]

Comment on attachment 695749 [details] [diff] [review]
Patch that initializes mDoorHangerPopup in WebApp, not GeckoApp

Awesome, thanks!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f50cfa3fe1

Comment 6

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/2b45a99e0108 for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=18448746&tree=Mozilla-Inbound :

WebApp.java:94: DoorHangerPopup(org.mozilla.gecko.GeckoApp,android.view.View) is not public in org.mozilla.gecko.DoorHangerPopup; cannot be accessed from outside package
        mDoorHangerPopup = new DoorHangerPopup(this, null);

Comment 7

[:Margaret Leibovic]

Mavericks, sorry I missed you on IRC. Are you having any luck looking into this? Does it build properly for you locally? It seems like a weird error because all these classes should be in the org.mozilla.gecko package.

Maybe something is going wrong with pre-processing the package name in WebApp.java.in? Wes, do you know anything about this stuff?

Comment 8

[Shriram (irc: Mavericks)]

May be http://stackoverflow.com/questions/7047173/class-constructor-cannot-be-accessed-by-outside-package  could help solve.

Comment 9

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 10

[:Margaret Leibovic]

The patch here doesn't apply anymore, and it looks like the WebApp.java.in change needs to be moved to WebAppImpl.java.

However, we still need to figure out what caused the failure on inbound, so this probably isn't a good mentor bug anymore.

Comment 11

[:Margaret Leibovic]

Mavericks, let me know if you need more help here. Do you have commit access to try server? It might be good to rebase this patch and push it to try to see if it still fails.

Comment 12

[Mike Hoye [:mhoye]]

Margaret, I'd like to start showing this bug around - in your judgement can this work be salvaged, or would it be better to start from scratch? Is it still relevant?

Comment 13

[:Margaret Leibovic]

(In reply to Mike Hoye [:mhoye] from comment #12)
> Margaret, I'd like to start showing this bug around - in your judgement can
> this work be salvaged, or would it be better to start from scratch? Is it
> still relevant?

Looking through the code, I think this issue was actually addressed by bug 943915.

I don't think this is actually an issue anymore, so I'm going to close this as WORKSFORME.

Comment 14

[Mike Hoye [:mhoye]]

Thank you!