Closed Bug 801739 Opened 8 years ago Closed 6 years ago

Initialize mDoorHangerPopup in WebApp, not GeckoApp

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: Margaret, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → kshriram18
Status: NEW → ASSIGNED
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.
Attachment #695645 - Flags: review?(margaret.leibovic) → feedback+
Attachment #695645 - Attachment is obsolete: true
Attachment #695749 - Flags: review?(margaret.leibovic)
Comment on attachment 695749 [details] [diff] [review]
Patch that initializes mDoorHangerPopup in WebApp, not GeckoApp

Awesome, thanks!
Attachment #695749 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
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);
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?
Can you confirm that you're still working on this bug?
Flags: needinfo?(kshriram18)
Assignee: kshriram18 → nobody
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.
Flags: needinfo?(kshriram18)
Whiteboard: [mentor=margaret][lang=java]
Assignee: nobody → kshriram18
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.
Assignee: kshriram18 → nobody
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?
Flags: needinfo?(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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(margaret.leibovic)
Resolution: --- → WORKSFORME
Thank you!
You need to log in before you can comment on or make changes to this bug.