This align better with the UX spec, while providing more features to the chrome UI. The patch still need more tests and some clean up thought.

I need to add some tests for all the new features. But in the meantime I believe that the non-tests part can be reviewed.

Same patch + tests.

Oups. I made a git commit mistake in the previous patch. Sorry for the noise.

I fixed some of the config flags in order to make them more explicit.

Comment on attachment 8463125 [details] [diff] [review] chromebar-additional.patch Review of attachment 8463125 [details] [diff] [review]: ----------------------------------------------------------------- r=me with those comments addressed (mainly test-related) ::: apps/system/js/app_chrome.js @@ +447,5 @@ > > + AppChrome.prototype.useCombinedChrome = function ac_useCombinedChrome(evt) { > + return this.app.config.chrome && > + this.app.config.chrome.navigation && > + this.app.config.chrome.bar; are we actually using the combined chrome? Looks like we could remove a bunch of code (keyboard handling/hiding closing) by getting rid of the navigation at the bottom of the screen. Let's track it in a follow up! ::: apps/system/js/home_searchbar.js @@ +134,5 @@ > // XXX: fix the WindowManager coupling > // but currently the transition sequence is crucial for performance > var app = AppWindowManager.getActiveApp(); > + if (app && !app.manifestURL) { > + this.setInput(app.config.url); can you add a small test for this case? ::: apps/system/js/rocketbar.js @@ +520,5 @@ > + * @memberof Rocketbar.prototype > + */ > + selectAll: function() { > + this.input.select(); > + }, can you add a tiny test for this, it will make the homesearchbar/rocketbar merge easier. ::: apps/system/style/chrome/chrome.css @@ +49,5 @@ > +} > + > +.controls .back-button:not([data-disabled]) + .forward-button:not([data-disabled]) + .urlbar { > + width: calc(100% - 9rem); > +} :) @@ +120,5 @@ > +} > + > + > +/* Progress */ > +.appWindow.navigation .progress { I think we need to specify .scrollable here. In an app with bottom navigation it's weird to have the loading in the middle of the app display. ::: apps/system/test/unit/app_chrome_test.js @@ +311,5 @@ > + var app = new AppWindow(fakeAppConfigBoth); > + var chrome = new AppChrome(app); > + chrome.handleEvent({ type: 'mozbrowserloadstart' }); > + assert.isTrue(chrome.containerElement.classList.contains('loading')); > + assert.isFalse(chrome._titleChanged); this is testing internal stuffs, can you instead * add a titlechange test where we get a `looadstart` in between * add a test for the titlechange with an empty value (this will cover the _currentURL stuff)

(In reply to Etienne Segonzac (:etienne) (PTO -> August 12) from comment #5) > Comment on attachment 8463125 [details] [diff] [review] > chromebar-additional.patch > > Review of attachment 8463125 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with those comments addressed (mainly test-related) > > ::: apps/system/js/app_chrome.js > @@ +447,5 @@ > > > > + AppChrome.prototype.useCombinedChrome = function ac_useCombinedChrome(evt) { > > + return this.app.config.chrome && > > + this.app.config.chrome.navigation && > > + this.app.config.chrome.bar; > > are we actually using the combined chrome? > Looks like we could remove a bunch of code (keyboard handling/hiding > closing) by getting rid of the navigation at the bottom of the screen. Let's > track it in a follow up! > I'm keeping the 'navigation' only chrome for now in case some of the dogfood use cases are regressing. So instead of backouting everything I can just adjust some of the 'config' flags and fallback on those. But yeah, we should definitively get rid of all this code before the end of 2.1. > @@ +120,5 @@ > > +} > > + > > + > > +/* Progress */ > > +.appWindow.navigation .progress { > > I think we need to specify .scrollable here. > > In an app with bottom navigation it's weird to have the loading in the > middle of the app display. > Oups! Good catch.

https://github.com/mozilla-b2g/gaia/commit/792836776749808fec8c6e7ee47999747ed83dec with requested tests.