Split away from bug 1048937. Spec: https://mozilla.app.box.com/s/r36n5hqu8ji0rppo2e1j

This adds the option and adds a placeholder implementation that uses a view activity to open a minimal data uri. To work properly, this will need bug 1041620. I'd have preferred to use window.open, but that doesn't work for some reason :/ The branch can be viewed here: https://github.com/Cwiiis/gaia/tree/bug1050319-add-new-window-to-system-browser

(In reply to Chris Lord [:cwiiis] from comment #1) > Created attachment 8469423 [details] [diff] [review] > Add 'New Window' to browser system overflow menu > > This adds the option and adds a placeholder implementation that uses a view > activity to open a minimal data uri. To work properly, this will need bug > 1041620. > > I'd have preferred to use window.open, but that doesn't work for some reason > :/ window.open will end up firing a mozbrowseropenwindow in b2g/, where nobody is listening for it. Also window.open may results into opening the window into the same process as the system app, which may be unsecure, so this is a good thing that it does not work until some platform fixes make it better.

Comment on attachment 8469423 [details] [diff] [review] Add 'New Window' to browser system overflow menu Review of attachment 8469423 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits and a test for the onNewWindow thing. ::: apps/system/js/app_chrome.js @@ +241,5 @@ > this.hideOverflowMenu(); > break; > > + case this.newWindowButton: > + evt.stopImmediatePropagation(); More or less the same question than the Share bug. @@ +588,5 @@ > + // XXX Until we have a start page (Bug 1041620), open a new window with > + // the minimal valid URL that doesn't incur bandwidth use. > + var activity = new MozActivity({name: 'view', data: { > + type: 'url', > + url: 'data:1' Can't we just do a about:blank instead of this ? I guess it does not matter since it is coming soon... @@ +591,5 @@ > + type: 'url', > + url: 'data:1' > + }}); > + // For jshint > + activity.onsuccess = function() {}; Maybe /*jshint -W031 */ new MozActivity(....); ::: apps/system/style/chrome/chrome.css @@ +359,5 @@ > } > > +.option#new-window > .icon { > + background-image: url("images/menu_new_window.png"); > +} Does it miss the new image or does bugzilla lies to me ?

Addressed Vivien's review comments, but given that the method of launching a new window is now different, would appreciate review of the whole patch again really.

Comment on attachment 8472497 [details] Add 'New Window' to browser system overflow menu (v2, with test) Looks good. I am mainly concerned about adding a settings observer for every app window. Can you address that and re-flag me for review?

Comment on attachment 8472497 [details] Add 'New Window' to browser system overflow menu (v2, with test) Wow, that is a silly mistake... Corrected, thanks for spotting :)

Comment on attachment 8472497 [details] Add 'New Window' to browser system overflow menu (v2, with test) Great work as usual. Thanks Chris!

Cheers :) Merged: https://github.com/mozilla-b2g/gaia/commit/ffe6a39ce959e5d3969b5a68c307ad12a2307ee3

this commit broke unit test of build system.

filed bug 1054822 for fixing unit test