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

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Comment 1

[:Margaret Leibovic]

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.

Comment 2

[:Margaret Leibovic]

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.

Comment 3

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

(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?

Comment 4

[friedger]

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.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 6

[friedger]

Attachment: 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

[friedger]

Attachment: device-2015-10-06-222640.png

Screenshot of several fennecs in the recent list.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8670704 [details] [diff] [review]
document-1208195.patch

Nice! This patch is quite small too. I'll flag nalexander for some feedback.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Looks like you guys have this under control.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

[friedger]

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?

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Nick, comment 11.

Comment 13

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

(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.

Comment 14

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

(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.

Comment 15

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

Attachment: 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

Comment 16

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

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.

Comment 17

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

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?

Comment 18

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

jchen: is my analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=1208195#c14 reasonable?

Comment 19

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

Screen capture is at: http://people.mozilla.org/~nalexander/New.Document.mp4

Comment 20

[friedger]

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?

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

(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.

Comment 22

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

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.

Comment 23

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

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.

Comment 24

[friedger]

nalexander: yes that is me and happy to help with gradle, but I didn't get the description of the bug.

Comment 25

[:Margaret Leibovic]

How's work here going? Is there anything we can do to help you? It would be great to see this land!

Comment 26

[SUN Haitao]

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.

Comment 28

[friedger]

I won't continue on this on for now.

Comment 29

[SUN Haitao]

As far as I can tell, it has already worked fine for a while.