1134106 - Graphene chrome tweaks

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Paul Rouget [:paul]]

Attachment: Part 1

- hide scrollbars for the scrollgrab container
- enable window dragging
- bunch of prefs

Comment 1

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/projects/larch/rev/b02b14eafcb4

Comment 2

[Paul Rouget [:paul]]

Attachment: Part 2

Comment 3

[Paul Rouget [:paul]]

Attachment: Part 2

Comment 4

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/larch/rev/ddabaca5de60

Comment 5

[Paul Rouget [:paul]]

Attachment: Part 3

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8572523 [details] [diff] [review]
Part 3

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

:(

Comment 7

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/larch/rev/621680da2ca7

Comment 8

[Paul Rouget [:paul]]

Attachment: Part 4

To avoid flickering at startup, we want the cocoa window to be white, not black.

Comment 9

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/larch/rev/0e28e50f3ac3

Comment 10

[Paul Rouget [:paul]]

Attachment: Part 5

Fabrice, I'm not sure if this is the right way to do it. Let me know what you think.

We can't use fullscreen from content, because after a reload, it would unfullscreen the content (which is not surprising).

Also, I had to remove teh removeEventListener as we want to get events even after a reload.

Comment 11

[Paul Rouget [:paul]]

Attachment: Part 6

The persist mechanism only works with xul documents:
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp#1486

I use prefs because we to call resizeTo and moveTo synchronously.

Comment 12

[Paul Rouget [:paul]]

Attachment: Part 7

This allows the content to reload more aggressively.

Comment 13

[Paul Rouget [:paul]]

Attachment: Part 7

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8576420 [details] [diff] [review]
Part 5

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

I don't understand the event listener changes. Please provide str.

::: b2g/chrome/content/shell.js
@@ -633,5 @@
>    },
>  
>    notifyContentStart: function shell_notifyContentStart() {
> -    this.contentBrowser.removeEventListener('mozbrowserloadstart', this, true);
> -    this.contentBrowser.removeEventListener('mozbrowserlocationchange', this, true);

I don't understand this change. We don't need to run SystemAppProxy registration again, etc. on reload. What is that fixing exactly?

Comment 15

[Paul Rouget [:paul]]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> Comment on attachment 8576420 [details] [diff] [review]
> Part 5
> 
> Review of attachment 8576420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand the event listener changes. Please provide str.

I'm not sure exactly what should be done on reload. But the listeners don't work anymore:

STR:
- start B2G
- press fullscreen button
- it goes fullscreen
- press fullscreen button
- back to non-fullscreen window
- F5
- press fullscreen button
- Nothing happens

Maybe we should only call `CustomEventManager.init()` on reload.

Comment 16

[[:fabrice] Fabrice Desré]

ok, I'm gonna take a look at what we need to do and will land that with the other 2 patches.

Comment 17

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/projects/larch/rev/ed8cd765f5ee
remote:   https://hg.mozilla.org/projects/larch/rev/4fece36fd723
remote:   https://hg.mozilla.org/projects/larch/rev/483800301abc

Comment 18

[Paul Rouget [:paul]]

Attachment: Part 8

Delegating command line handling to the app. It will be used for things like `-url xxx` `-open xxx` …

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 8600803 [details] [diff] [review]
Part 8

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

This patch is a very generic pass-through. I'm not opposed if we have use cases beyond opening a new page. If not, we should just use the support for window.open(..., "_blank").

Comment 20

[Paul Rouget [:paul]]

Attachment: Part 8

Is this approach better?
What do you mean by supporting `window.open(..., "_blank")`?

Comment 21

[[:fabrice] Fabrice Desré]

What I meant is that in gaia, we have support for opening new "tabs" when a page does window.open("http://foo.com", "_blank") and that we could reuse that to trigger the tab load instead of sending the command line argument to the system app.
We also support a "view" activity, but I'm not sure if that makes sense to add that to browser.html

Comment 22

[Paul Rouget [:paul]]

Still not sure to understand.

What we're trying to achieve here is to support the `-url` command line argument that is (indirectly) used by the OS.

This is how it works:
- set Graphene as your default browser
- go to your email client, click on a link
- OS X will send a cocoa URLEvent to gecko
- gecko will translate this event into a command line argument (-url url)
- gecko run all the command line handlers

So we need a command line handler to forward this URL to, first, shell.js, then, the app.

Are you saying that instead of sending an event to the app, we should use call `window.open` from within the app? Something like: in shell.js: content.open(url, "_blank"), and the app would then open a tab?

I can think of 3 ways to build that:
- listen to mozbrowseropenwindow from shell.js, but then, you still need to send a message back to the app iframe
- implement nsIWindowProvider
- override the native `open` call

But I couldn't find any of these things in /b2g/. And I actually tried to call window.open(url, _blank) in Gaia's system app. Nothing happens.

What am I missing?

Comment 23

[[:fabrice] Fabrice Desré]

browser.html (and gaia) use the mozbrowseropenwindow event for that, so why not have shell.js dispatch such an event on the systemApp iframe?

Comment 24

[Paul Rouget [:paul]]

(In reply to Fabrice Desré [:fabrice] from comment #23)
> browser.html (and gaia) use the mozbrowseropenwindow event for that, so why
> not have shell.js dispatch such an event on the systemApp iframe?

Are you suggesting that I just rename "opentab" to "mozbrowseropenwindow" in attachment 8601257 [details] [diff] [review]?

Comment 25

[[:fabrice] Fabrice Desré]

(In reply to Paul Rouget [:paul] from comment #24)
> (In reply to Fabrice Desré [:fabrice] from comment #23)
> > browser.html (and gaia) use the mozbrowseropenwindow event for that, so why
> > not have shell.js dispatch such an event on the systemApp iframe?
> 
> Are you suggesting that I just rename "opentab" to "mozbrowseropenwindow" in
> attachment 8601257 [details] [diff] [review]?

Yes, if that works that's all we need!

Comment 26

[Paul Rouget [:paul]]

Attachment: Part 9

Comment 27

[Paul Rouget [:paul]]

Attachment: Part 8

Comment 28

[[:fabrice] Fabrice Desré]

Comment on attachment 8604455 [details] [diff] [review]
Part 8

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

looks fine, except the `url` in the js object. If that works, I don't understand why.

::: b2g/chrome/content/shell.js
@@ +708,5 @@
> +    try {
> +      // Returns null if -url is not present
> +      let url = args.handleFlagWithParam("url", false);
> +      if (url) {
> +        this.sendChromeEvent({type: "mozbrowseropenwindow", url});

url: url ??

::: b2g/components/CommandLine.js
@@ +14,5 @@
>  
>  CommandlineHandler.prototype = {
>      handle: function(cmdLine) {
>        this.cmdLine = cmdLine;
> +      let win = Services.wm.getMostRecentWindow('navigator:browser');

nit: double quotes in this file.

Comment 29

[Paul Rouget [:paul]]

(In reply to Fabrice Desré [:fabrice] from comment #28)
> Comment on attachment 8604455 [details] [diff] [review]
> Part 8
> 
> Review of attachment 8604455 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks fine, except the `url` in the js object. If that works, I don't
> understand why.
> 
> ::: b2g/chrome/content/shell.js
> @@ +708,5 @@
> > +    try {
> > +      // Returns null if -url is not present
> > +      let url = args.handleFlagWithParam("url", false);
> > +      if (url) {
> > +        this.sendChromeEvent({type: "mozbrowseropenwindow", url});
> 
> url: url ??

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Property_definitions

> ::: b2g/components/CommandLine.js
> @@ +14,5 @@
> >  
> >  CommandlineHandler.prototype = {
> >      handle: function(cmdLine) {
> >        this.cmdLine = cmdLine;
> > +      let win = Services.wm.getMostRecentWindow('navigator:browser');
> 
> nit: double quotes in this file.

ok.

Comment 30

[Paul Rouget [:paul]]

Attachment: Part 8

Comment 31

[[:fabrice] Fabrice Desré]

Actually we could land part 8 & 9 on m-c. Do you mind moving them to their own bug carrying r+ ?

Comment 32

[Paul Rouget [:paul]]

(In reply to Fabrice Desré [:fabrice] from comment #31)
> Actually we could land part 8 & 9 on m-c. Do you mind moving them to their
> own bug carrying r+ ?

ok!

Comment 33

[Paul Rouget [:paul]]

See bug 1163905 and bug 1163904.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a7a4747bdc4e

Comment 35

[Paul Rouget [:paul]]

Attachment: Part 8

Fabrice, this is required to enable testing. See https://github.com/mozilla/browser.html/pull/392

Comment 36

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/larch/rev/7f8e7c37c557

Comment 37

[Paul Rouget [:paul]]

Attachment: Part 9

Earlier, we switched from the fullscreen wep api to window.fullScreen, but I forgot to fix the way we register the fullscreen state.

Comment 38

[[:fabrice] Fabrice Desré]

Comment on attachment 8625045 [details] [diff] [review]
Part 9

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

I'll land once we merge m-c in.

Comment 39

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/larch/rev/7541cb540ff0

Comment 40

[Paul Rouget [:paul]]

Attachment: Part 10

Comment 41

[Paul Rouget [:paul]]

Attachment: Part 11

Comment 42

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/larch/rev/dbe7e97235d3

Comment 43

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/larch/rev/24429391461c

Comment 44

[Paul Rouget [:paul]]

Attachment: Part 12

We want to enable/disable APZC directly from browser.html. In the long term, I don't think we want to keep that, but for now, this will help us a lot.

Comment 45

[Paul Rouget [:paul]]

Went ahead and pushed Part 12: https://hg.mozilla.org/projects/larch/rev/45796a4b1a48

Comment 46

[Paul Rouget [:paul]]

Attachment: Part 13

Comment 47

[Paul Rouget [:paul]]

Part 13: https://hg.mozilla.org/projects/larch/rev/e0b8d95c9549

Comment 48

[Paul Rouget [:paul]]

Attachment: Part 14

Let's disable the devtools now we have the UI to enable them in browser.html (F12).

Comment 49

[Paul Rouget [:paul]]

Attachment: Part 15

Comment 50

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/projects/larch/rev/010bc244a855
remote:   https://hg.mozilla.org/projects/larch/rev/feee383db2f0

Comment 51

[Paul Rouget [:paul]]

Attachment: Part 16

More prefs.

Comment 52

[Paul Rouget [:paul]]

Attachment: Part 17

Touch caret is back.

Comment 53

[[:fabrice] Fabrice Desré]

Comment on attachment 8642812 [details] [diff] [review]
Part 16

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

::: b2g/graphene/graphene.js
@@ +47,5 @@
>  pref("b2g.nativeWindowGeometry.fullscreen", false);
>  
>  pref("media.useAudioChannelService", false);
>  
> +pref("browser.sessionhistory.max_total_viewers", -1);

I'd like this to be changed in the main b2g.js. We don't target low end by default now.

@@ +48,5 @@
>  
>  pref("media.useAudioChannelService", false);
>  
> +pref("browser.sessionhistory.max_total_viewers", -1);
> +pref("network.predictor.enabled", true);

Let's keep this one in graphene only, as I'm not sure about the consequences in terms of data consumption.

Comment 54

[Paul Rouget [:paul]]

Attachment: Part 16

See bug 1192133 for the total_viewers pref.

Comment 55

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/projects/larch/rev/ecd1bea4f241
remote:   https://hg.mozilla.org/projects/larch/rev/75ea02b6a043

Comment 56

[Kevin Grandon :kgrandon]

I believe everything here was rolled-up and landed in mozilla-central via bug 1204965.