Cache web app data for home screen sites declaring "display": "fullscreen" in their manifest.

Cache web app data for home screen sites declaring "display": "fullscreen" in their manifest.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch
into fullscreen mode from the home screen.

BUG=581522
Committed: https://crrev.com/3f9ab13fcba673aa56307a9b495f821bec49160a
Cr-Commit-Position: refs/heads/master@{#394981}

[dominickn, 2016-05-10 11:29:59]

Patchset #2 (id:20001) has been deleted

[dominickn, 2016-05-10 12:13:20]

Patchset #2 (id:40001) has been deleted

[dominickn, 2016-05-10 13:13:07]

dominickn@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mlamouri@chromium.org

[dominickn, 2016-05-10 13:13:07]

PTAL, thanks!

@dfalcantara: the one thing that this CL doesn't yet do is actually launch
homescreen apps into full screen. I couldn't work out how to do it - can you
point out what I'm missing?

[gone, 2016-05-10 17:31:49]

Are you calling the function needed to kick you into fullscreen?  Only works in,
FWIW.

https://developer.android.com/training/system-ui/immersive.html

[gone, 2016-05-10 17:31:55]

Are you calling the function needed to kick you into fullscreen?  Only works in
KK, FWIW.

https://developer.android.com/training/system-ui/immersive.html

[gone, 2016-05-10 19:41:20]

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:99:
} else if (!TextUtils.equals(mWebappInfo.id(), newWebappInfo.id())) {
Probably need to update the fullscreen state here if mIsInitialized is already
true.

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121:
getActivityTab().getTabWebContentsDelegateAndroid().setDisplayMode(displayMode);
Update it here too, probably.

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:323:

Feels like version checking should be done here so that these fields are all
read only for version 1, with another block for version 2.  But I guess that
really only matters if the default value changes for a field?

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:248:
final int displayMode = 3;
Shouldn't you be using a real displayMode here?

[dominickn, 2016-05-11 04:24:21]

At this point, I'm just reimplementing chunks of FullscreenHtmlApiHandler to do
the fullscreen transition. This is sad, especially because that class works
around a bunch of annoying bugs (e.g. clearing a fullscreen flag 20ms after
entering fullscreen so that when the keyboard is shown the controls don't come
back). I don't really want to have to reimplement all of this in WebappActivity.

I propose splitting up the FullscreenHtmlApiHandler into two classes. The
superclass does the fullscreen work, and the subclass adds on the
toast/infobars, the delegate interface, etc.

 * I'll use the superclass to handle fullscreen in WebappActivity. 
 * The second class would be the one used by the FullscreenManager in existing
HTML fullscreen
 * I'll override getFullscreenManager in WebappActivity to return null if the
app is launched in full screen, since reasoning about entering/exiting HTML
fullscreen when the app is already launched in fullscreen hurts.

WDYT? The split should be quite easy since the existing interface won't change,
so I'll do it in this CL.

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:99:
} else if (!TextUtils.equals(mWebappInfo.id(), newWebappInfo.id())) {
On 2016/05/10 19:41:20, dfalcantara wrote:
> Probably need to update the fullscreen state here if mIsInitialized is already
> true.

Acknowledged.

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121:
getActivityTab().getTabWebContentsDelegateAndroid().setDisplayMode(displayMode);
On 2016/05/10 19:41:20, dfalcantara wrote:
> Update it here too, probably.

Acknowledged.

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:323:

On 2016/05/10 19:41:20, dfalcantara wrote:
> Feels like version checking should be done here so that these fields are all
> read only for version 1, with another block for version 2.  But I guess that
> really only matters if the default value changes for a field?

Yeah, that should only be an issue if we change a default value. For display,
standalone was the implicit default anyway, whilst all the other fields
shouldn't change.

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:248:
final int displayMode = 3;
On 2016/05/10 19:41:20, dfalcantara wrote:
> Shouldn't you be using a real displayMode here?

Yes, I should. However, the tests didn't compile when I imported
WebDisplayMode - it's in blink_public, and I couldn't work out
how/where to get those files included in the test binary so I gave
up. If you can point out how to do it I'd much prefer using a real
value here.

[mlamouri (slow - plz ping), 2016-05-11 10:18:57]

Looks good but I can't find where you actually make the activity go fullscreen.
Did I miss something or is that meant to be done in a follow-up?

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://codereview.chromium.org/1961313002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:248:
final int displayMode = 3;
On 2016/05/11 at 04:24:21, dominickn wrote:
> On 2016/05/10 19:41:20, dfalcantara wrote:
> > Shouldn't you be using a real displayMode here?
> 
> Yes, I should. However, the tests didn't compile when I imported
> WebDisplayMode - it's in blink_public, and I couldn't work out
> how/where to get those files included in the test binary so I gave
> up. If you can point out how to do it I'd much prefer using a real
> value here.

I think you can add:
"//third_party/WebKit/public:blink_headers_java",

to junit_binary("chrome_junit_tests") in:
chrome/android/BUILD.gn

I didn't try so it might be entirely wrong :)

https://codereview.chromium.org/1961313002/diff/60001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/1961313002/diff/60001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:43: // 'minimal-ui' is not yet
supported, so fallback in this case.
Can you point to the minimal-ui BUG?

[gone, 2016-05-11 17:51:30]

You'd have to ask Ted for his input on the FullscreenHtmlApiHandlerLongName
split.  I think he was the owner and already split it up once, so another split
will be interesting.

https://chromiumcodereview.appspot.com/1961313002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://chromiumcodereview.appspot.com/1961313002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:323:

On 2016/05/11 04:24:21, dominickn wrote:
> On 2016/05/10 19:41:20, dfalcantara wrote:
> > Feels like version checking should be done here so that these fields are all
> > read only for version 1, with another block for version 2.  But I guess that
> > really only matters if the default value changes for a field?
> 
> Yeah, that should only be an issue if we change a default value. For display,
> standalone was the implicit default anyway, whilst all the other fields
> shouldn't change.

Acknowledged.

[dominickn, 2016-05-12 06:40:41]

> Looks good but I can't find where you actually make the activity go
fullscreen.
> Did I miss something or is that meant to be done in a follow-up?

It's to be implemented in this CL. I put this up to get some feedback
while I worked out how to best do the fullscreen transition. I'm
currently asking OWNERs if I can repurpose some of the existing
fullscreen machinery since there are a bunch of bugs that it works
around. Unfortunately, I can't directly reuse it because it's
automatically wired to kick out of fullscreen when the user
navigates; I'll need to do some decoupling which should be relatively
contained.

I'll mail out again once I have this done (or if it's necessary to split things
up).

>
https://codereview.chromium.org/1961313002/diff/60001/chrome/android/junit/sr...
> File
>
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
> (right):
> 
>
https://codereview.chromium.org/1961313002/diff/60001/chrome/android/junit/sr...
>
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:248:
> final int displayMode = 3;
> On 2016/05/11 at 04:24:21, dominickn wrote:
> > On 2016/05/10 19:41:20, dfalcantara wrote:
> > > Shouldn't you be using a real displayMode here?
> > 
> > Yes, I should. However, the tests didn't compile when I imported
> > WebDisplayMode - it's in blink_public, and I couldn't work out
> > how/where to get those files included in the test binary so I gave
> > up. If you can point out how to do it I'd much prefer using a real
> > value here.
> 
> I think you can add:
> "//third_party/WebKit/public:blink_headers_java",
> 
> to junit_binary("chrome_junit_tests") in:
> chrome/android/BUILD.gn
> 
> I didn't try so it might be entirely wrong :)

Thanks for the pointer, this and one other place does it. :)

>
https://codereview.chromium.org/1961313002/diff/60001/chrome/browser/android/...
> File chrome/browser/android/shortcut_info.cc (right):
> 
>
https://codereview.chromium.org/1961313002/diff/60001/chrome/browser/android/...
> chrome/browser/android/shortcut_info.cc:43: // 'minimal-ui' is not yet
> supported, so fallback in this case.
> Can you point to the minimal-ui BUG?

Done.

[mlamouri (slow - plz ping), 2016-05-18 10:39:46]

dominickn@, do you still need a review here? It l-g-t-m but unless there is a
follow-up CL I'

[mlamouri (slow - plz ping), 2016-05-18 10:40:01]

On 2016/05/18 at 10:39:46, Mounir Lamouri wrote:
> dominickn@, do you still need a review here? It l-g-t-m but unless there is a
follow-up CL I'

... I'm not sure it should land :)

[dominickn, 2016-05-18 11:09:18]

On 2016/05/18 10:40:01, Mounir Lamouri wrote:
> On 2016/05/18 at 10:39:46, Mounir Lamouri wrote:
> > dominickn@, do you still need a review here? It l-g-t-m but unless there is
a
> follow-up CL I'
> 
> ... I'm not sure it should land :)

I'm tempted to just make display: fullscreen fall back to standalone in this CL
(that
is, store the web app data for sites declaring display: fullscreen, which
currently
doesn't happen). I'm still trying to address a very irritating bug where when
the
keyboard is triggered in a fullscreen home screen app, it's forced out of
fullscreen
(but with the top and bottom system control bars obscuring some of the content).
The Android folks haven't had an easy solution to this, so unless I can fix it
enabling
display: fullscreen to launch into fullscreen doesn't seem right.

[mlamouri (slow - plz ping), 2016-05-18 11:15:30]

On 2016/05/18 at 11:09:18, dominickn wrote:
> On 2016/05/18 10:40:01, Mounir Lamouri wrote:
> > On 2016/05/18 at 10:39:46, Mounir Lamouri wrote:
> > > dominickn@, do you still need a review here? It l-g-t-m but unless there
is a
> > follow-up CL I'
> > 
> > ... I'm not sure it should land :)
> 
> I'm tempted to just make display: fullscreen fall back to standalone in this
CL (that
> is, store the web app data for sites declaring display: fullscreen, which
currently
> doesn't happen). 

That would work for me.

[dominickn, 2016-05-19 01:11:37]

Description was changed from

==========
Support "display": "fullscreen" in web apps on the homescreen.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. This allows Chrome to
respect sites which declare "display": "fullscreen" in their manifest,
and launch those sites directly from the homescreen into fullscreen
mode.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

BUG=581522
==========

to

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch into
fullscreen mode from the home screen.

BUG=581522
==========

[dominickn, 2016-05-19 01:15:43]

Description was changed from

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch into
fullscreen mode from the home screen.

BUG=581522
==========

to

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch into
fullscreen mode from the home screen.

BUG=581522
==========

[dominickn, 2016-05-19 07:10:54]

Description was changed from

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch into
fullscreen mode from the home screen.

BUG=581522
==========

to

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch
into fullscreen mode from the home screen.

BUG=581522
==========

[dominickn, 2016-05-19 07:32:53]

PTAL - rebranded this CL as just enabling the storage of display modes
(particularly whether display is standalone or fullscreen).

I'm going to have to keep poking at the actual enter fullscreen functionality.

[mlamouri (slow - plz ping), 2016-05-19 12:56:23]

lgtm with comments applied.

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:122:
mWebappInfo.displayMode());
I would prefer us to keep pretending it is standalone if we actually show the
web app as standalone.

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(right):

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:169:
3, ScreenOrientationValues.DEFAULT, ShortcutSource.UNKNOWN,
Why 3?

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:184:
3, ScreenOrientationValues.DEFAULT, ShortcutSource.UNKNOWN,
ditto

[gone, 2016-05-19 17:28:38]

lgtm % mounir's comments

[dominickn, 2016-05-19 23:28:06]

Thanks!

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:122:
mWebappInfo.displayMode());
On 2016/05/19 12:56:23, Mounir Lamouri wrote:
> I would prefer us to keep pretending it is standalone if we actually show the
> web app as standalone.

Done.

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(right):

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:169:
3, ScreenOrientationValues.DEFAULT, ShortcutSource.UNKNOWN,
On 2016/05/19 12:56:23, Mounir Lamouri wrote:
> Why 3?

Bad rebase!

https://codereview.chromium.org/1961313002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:184:
3, ScreenOrientationValues.DEFAULT, ShortcutSource.UNKNOWN,
On 2016/05/19 12:56:23, Mounir Lamouri wrote:
> ditto

Done.

[dominickn, 2016-05-20 01:20:27]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-05-20 01:20:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1961313002/#ps120001
(title: "Addressing nits")

[commit-bot: I haz the power, 2016-05-20 01:21:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961313002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961313002/120001

[commit-bot: I haz the power, 2016-05-20 03:33:00]

Description was changed from

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch
into fullscreen mode from the home screen.

BUG=581522
==========

to

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch
into fullscreen mode from the home screen.

BUG=581522
==========

[commit-bot: I haz the power, 2016-05-20 03:33:01]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-05-20 03:34:19]

Description was changed from

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch
into fullscreen mode from the home screen.

BUG=581522
==========

to

==========
Cache web app data for home screen sites declaring "display": "fullscreen" in
their manifest.

This CL stores the display mode specified by web apps in the launch
intent and the corresponding WebappDataStorage. Currently, sites with
"display": "fullscreen" in their manifest do not have their web app data
cached, meaning they do not behave like a native app when launched
from home screen.

Additionally, the requirement for sites to declare "display":
"standalone" in order to open in the standalone web frame and have
notification deep-linking behaviour is expanded to include "fullscreen"
declarations.

A future CL will allow sites declaring "display": "fullscreen" to launch
into fullscreen mode from the home screen.

BUG=581522

Committed: https://crrev.com/3f9ab13fcba673aa56307a9b495f821bec49160a
Cr-Commit-Position: refs/heads/master@{#394981}
==========

[commit-bot: I haz the power, 2016-05-20 03:34:20]

Patchset 5 (id:??) landed as
https://crrev.com/3f9ab13fcba673aa56307a9b495f821bec49160a
Cr-Commit-Position: refs/heads/master@{#394981}