Show sites opened from Android home screen bookmarks as separate entries in the Android recent apps list

NEW
Unassigned

Status

()

Firefox for Android
General
2 years ago
7 months ago

People

(Reporter: blassey, Unassigned, Mentored)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(fennec-)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Comment hidden (empty)
Blocks: 1208655

Comment 1

2 years ago
We decided to not track this for right now, because we need to do more work to flesh out ideas around custom tabs/progressive apps. However, we should do some engineering investigation here to see what's possible.
tracking-fennec: ? → -

Comment 2

2 years ago
This doesn't necessarily depend on the custom tabs work, does it? What would be required to hook this up to our current "Add to home screen" feature? This would be a nice step in the direction of "progressive apps", which is different than custom tabs.
Flags: needinfo?(snorp)
Flags: needinfo?(nalexander)
(In reply to :Margaret Leibovic from comment #2)
> This doesn't necessarily depend on the custom tabs work, does it? What would
> be required to hook this up to our current "Add to home screen" feature?
> This would be a nice step in the direction of "progressive apps", which is
> different than custom tabs.

No, it doesn't require other parts of the custom tabs work.  I skimmed the documentation for this https://developer.android.com/guide/components/recents.html and it's not clear that we can have two tasks (corresponding to Fennec and the "launched from home screen" tab) that both are rooted at the same Activity /instance/.  (The same Activity class -- of course.  The same /instance/?  Not so clear.)  The model for tasks and activities is complicated, so it's very possible my fast reading is not accurate, but that's the question we'd need to answer, since we're a long way from having multiple GeckoView-embedding Activity instances live at one time.

mcomella: you're well versed in onNewIntent() and friends.  Do you have anything to add here?
Flags: needinfo?(nalexander) → needinfo?(michael.l.comella)

Comment 4

2 years ago
With Lollipop, there comes the concept of document centric apps. We could use http://developer.android.com/reference/android/content/Intent.html#FLAG_ACTIVITY_NEW_DOCUMENT or http://developer.android.com/reference/android/R.attr.html#documentLaunchMode to have the same activity instance as root for new tasks.
I'm not too familiar with the recents list (and it confuses me every time I try). But the article you listed is quite explicit about multiple instances of the Activity being used:
  With the Android 5.0 release (API level 21), multiple instances of the same activity containing different documents may appear as tasks in the overview screen.

The Document documentation (heh) agrees.
Flags: needinfo?(michael.l.comella)

Comment 6

2 years ago
Created attachment 8670704 [details] [diff] [review]
document-1208195.patch

This is my first go at the launchMode. It shows in the recent list as desired by the bug description. However, inheriting from BrowseApp is probably not right. Is GeckoApp enough to use for a single tab?

Comment 7

2 years ago
Created attachment 8670705 [details]
device-2015-10-06-222640.png

Screenshot of several fennecs in the recent list.
Comment on attachment 8670704 [details] [diff] [review]
document-1208195.patch

Nice! This patch is quite small too. I'll flag nalexander for some feedback.
Attachment #8670704 - Flags: feedback?(nalexander)
Looks like you guys have this under control.
Flags: needinfo?(snorp)
(In reply to friedger from comment #6)
> Created attachment 8670704 [details] [diff] [review]
> document-1208195.patch
> 
> This is my first go at the launchMode. It shows in the recent list as
> desired by the bug description. However, inheriting from BrowseApp is
> probably not right. Is GeckoApp enough to use for a single tab?

GeckoApp vs BrowserApp is a good question. I think that it's probably safer to use BrowserApp in this case because we want to make sure the loaded page has all the _browser_ capabilities. We can look at specializing it later.

Comment 11

2 years ago
Probably we do not want `Intent.FLAG_ACTIVITY_MULTIPLE_TASK`? So that the same shortcut is opened in the same document/tab again.

What about already opened tabs? They appear in the new activity. Not sure whether this is the intended behaviour. In the patch I am switching from the bookmark action to view action, and we hide the information that this is a bookmarked url in the activity, the bookmark flag should still be send to gecko, I guess.

Shall I share an apk?

Updated

2 years ago
Blocks: 1212648
No longer blocks: 1208655
Nick, comment 11.
Mentor: nalexander@mozilla.com
Flags: needinfo?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #12)
> Nick, comment 11.

Status update: I've updated the patch to work locally, and have a bunch of questions and comments.  Should get to them in the next day or two.  Sorry for the delay.
Flags: needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #13)
> (In reply to Michael Comella (:mcomella) from comment #12)
> > Nick, comment 11.
> 
> Status update: I've updated the patch to work locally, and have a bunch of
> questions and comments.  Should get to them in the next day or two.  Sorry
> for the delay.

Sorry for the long delay.  I've updated the patch to work locally; I'll
push it here and make some comments as well.

This doesn't work well for me locally, and I think there are significant
barriers to making it work well.  I've recorded a screen capture on an
Android 5 device.  The key issues I see:

* switching documents doesn't work; I see a white screen in the older
  documents.  More below.

* the same bookmark doesn't open in the same window a second time; I see
  a new document each time I open the same bookmark;

* after opening a bookmark, the user can navigate away from the bookmark
  and turn the document into the regular browser.

Technically, I think the second can be addressed by identifying
documents.  The third is challenging but do-able; some of that work is
required for Custom Tabs.

The first is going to be hard to address.  I've verified that there are
multiple Activity instances (DocumentApps, really) backing each
document.  This is violating one of the fundamental assumptions
underlying Fennec: that there is a single Activity owning a single
GeckoView.  The white screens I see are because the multiple Activity's
all think they own the one true GeckoView, and we don't co-ordinate the
hand-offs of the Gecko rendering context.

The platform team is working hard on supporting multiple Gecko windows,
and through that multiple GeckoViews with multiple Gecko rendering
contexts.  Until that work lands, I don't think this approach is worth
pursuing.  If we /do/ want to pursue it, we should investigate if we
could make the DocumentApp activities somehow thin containers that save
some state and then redirect to a unique underlying BrowserApp
instance.  To me, that sounds like a lot of state and edge cases :(

Happy to hear alternative positions or counter-arguments.
Summary: tabs opened from the android home screen should be shown in the android recent apps list → Show sites opened from Android home screen bookmarks as separate entries in the Android recent apps list
Created attachment 8675878 [details]
MozReview Request: Bug 1208195 - Show sites opened from Android home screen bookmarks as separate entries in the Android recent apps list. r?nalexander

Bug 1208195 - Show sites opened from Android home screen bookmarks as separate entries in the Android recent apps list. r?nalexander
Attachment #8675878 - Flags: review?(nalexander)
Attachment #8670704 - Flags: feedback?(nalexander)
Comment on attachment 8675878 [details]
MozReview Request: Bug 1208195 - Show sites opened from Android home screen bookmarks as separate entries in the Android recent apps list. r?nalexander

Just pushing a simplified version to the ticket.
Attachment #8675878 - Flags: review?(nalexander)
friedger: sorry for the slow turn-around and for what I think is bad8 news :(  You have done more investigation into this than I have; can you comment on my thoughts?
Flags: needinfo?(mail)
jchen: is my analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=1208195#c14 reasonable?
Flags: needinfo?(nchen)

Comment 20

2 years ago
nalexander: I was expecting something like this. These are three valid points that I also experienced.

re 1) Good to have your inside, I was assuming something like that. 
re 2) There is the multi_document flag. I was not sure what the desired behavior was. Maybe it is sufficient to remove it.
re 3) Do you any pointers how to achieve that? I tried to change the color of the chrome and didn't manage. I can offer to learn more about the chrome and how it works but would need some guidance. Does it make sense at all?
Flags: needinfo?(mail)
(In reply to Nick Alexander :nalexander from comment #18)
> jchen: is my analysis in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1208195#c14 reasonable?

Yeah looks good to me.
Flags: needinfo?(nchen)
Hi friedger, sorry for the delay.  Setting flags will ensure stuff doesn't get overlooked.

(In reply to friedger from comment #20)
> nalexander: I was expecting something like this. These are three valid
> points that I also experienced.
> 
> re 1) Good to have your inside, I was assuming something like that.

It may be possible to arrange for multiple activities to transfer the Gecko context around.  jchen will know more.
 
> re 2) There is the multi_document flag. I was not sure what the desired
> behavior was. Maybe it is sufficient to remove it.

If you could do a little demo App (or modify Fennec) to try to achieve this behaviour (always opening the same bookmark in the same document window), we'd like to know how it's done.

> re 3) Do you any pointers how to achieve that? I tried to change the color
> of the chrome and didn't manage. I can offer to learn more about the chrome
> and how it works but would need some guidance. Does it make sense at all?

I don't know exactly where the colors are, but you can start digging in by looking at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/colors.xml.  I imagine that tweaking the chrome will be a long process.  I can help answer, or redirect, specific questions.
friedger: are you the friedger from https://github.com/novoda/gradle-android-command-plugin?  If you are, I was interested in using that plug-in to achieve Bug 1217228.  Can you help?  I see the write-up of that bug isn't all that clear; I'll try to clarify what we want to achieve.
Flags: needinfo?(mail)

Comment 24

2 years ago
nalexander: yes that is me and happy to help with gradle, but I didn't get the description of the bug.
Flags: needinfo?(mail)

Comment 25

2 years ago
How's work here going? Is there anything we can do to help you? It would be great to see this land!
Flags: needinfo?(mail)

Comment 26

2 years ago
My recent local test shows that the current patch (after rebased) crashes Fennec while a instance already running on Android 4.4.

I guess quite a lot work-around (likes what the OWA Runtime is using) will be required to make this work before Bug 1191028 lands.
Duplicate of this bug: 1246312
Blocks: 1201717

Comment 28

9 months ago
I won't continue on this on for now.
Flags: needinfo?(mail)

Updated

8 months ago
Assignee: mail → nobody
No longer blocks: 1212648
You need to log in before you can comment on or make changes to this bug.