Closed Bug 824161 Opened 11 years ago Closed 11 years ago

Homescreen widget doesn't open awesomebar

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: wesj, Assigned: capella)

Details

Attachments

(1 file, 1 obsolete file)

The homescreen widget only seems to actually open the awesomebar if Fennec was already running when I tapped it. It should load the Awesombar activity regardless.
Wow! new stuff I hadn't seen yet ... finally tracked it to https://bugzilla.mozilla.org/show_bug.cgi?id=708707.

fwiw - this gives me my first reason to use widgets :D
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
This seems to work on my device ...
Attachment #696979 - Flags: review?(wjohnston)
Comment on attachment 696979 [details] [diff] [review]
Patch (v1)

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

Missed this in my queue :( Looks pretty good, but I want to make sure we're not leaving blank tabs scattered around.

::: mobile/android/base/GeckoApp.java
@@ +1947,5 @@
> +        // Homescreen Widget requests awesome bar
> +        if (ACTION_WIDGET.equals(action)) {
> +            addTab();
> +        }
> +

In this case I think we actually want to use the current tab... unless we're doing session restore or something. i.e. do a check like this:

http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/GeckoApp.java#1747

and alternate between addTab() and onSearchRequested()
Attachment #696979 - Flags: review?(wjohnston) → review-
Attached patch Patch (v2)Splinter Review
meh - small delay, and I figured you were busy with the release... 

I like the suggestion you made here. I've really not gotten into the onCreate() and initialize() functions too deeply yet, and had made a best-guess where to place the addTab(). 

This seems to work in my testing. I even ran eatmem crashes to test the restore situation. Let me know if you see a further refinement I should try.
Attachment #696979 - Attachment is obsolete: true
Attachment #699551 - Flags: review?(wjohnston)
Comment on attachment 699551 [details] [diff] [review]
Patch (v2)

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

Great! There's still a flash of the normal UI before the awesomescreen appears, but not showing that is probably going to require moving things around a lot in GeckoApp (i.e. we need to ensure that we know about the restore state and have initialized BrowserDB before we can even show UI). There's a risk for startup regressions there that I don't want to tackle here.
Attachment #699551 - Flags: review?(wjohnston) → review+
Comment on attachment 699551 [details] [diff] [review]
Patch (v2)

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

I asked sriram to give this a look too, since he's the widget writer.
Attachment #699551 - Flags: review+ → review?(sriram)
I have a weird feeling that the intent goes to onNewIntent() and that's how we load an external URL (say from twitter app). Could you please check that?
I can look again, but that's where I started my initial research. (I had a bunch of logcats in there to very the processing.) It's kinda not like a "new" intent if the activity hasn't been instantiated yet, so onCreate() grabs it.

I'll look again >)
I'm pretty sure you're right about that too capella. ram was a bit worried about calling this before we call Tabs.getInstance().attachToActivity(this). Can you just move it towards the end of onCreate and put up a new patch for him to review?

Technically, I think we're fine because we don't actually create a new tab until the Awesomebar returns (in case you just hit back we don't make and then kill a tab), but its good to be safe.
Help me here ... moving the code as suggested fails with a null pointer ... 

Moving the code from the end of |private initialize()| in geckoapp to the end of onCreate() doesn't move it downstream processing wise, but previously in the scheme of things. After onCreate() finishes is where onWindowFocusChanged() is called and in turn calls the initialize(). 

Maybe I mis-understand completely? The initialize() function in onCreate() referred to before the Tabs.getInstance().attachToActivity(this) seems to be to GeckoApplication.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1511
Oh you're right. I thought that's how things worked and was surprised to see the one early in onCreate(). If this is firing after all of onCreate() in geckoApp has run, I think we're fine then.
cooool :D

I'll plan on moving the patch as already review+'ed ... thanks !!
Ah sorry, forgot I'm waiting now on sriram.
Comment on attachment 699551 [details] [diff] [review]
Patch (v2)

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

Looks good to me.
Attachment #699551 - Flags: review?(sriram) → review+
https://hg.mozilla.org/mozilla-central/rev/ca753cb40bb5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.