Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bug 783743 - addon-page should work with "index.html?…" & "index.html#…" #639

Merged
merged 1 commit into from

3 participants

Matteo Ferretti Irakli Gozalishvili Dave Townsend
Matteo Ferretti
Owner

Replaced usage of inContentWhitelist with hideChromeForLocation to have more flexibility about URLs to hide.

Matteo Ferretti ZER0 Bug 783743 - addon-page should work with "index.html?[stuff]" and "in…
…dex.html#[stuff]" (and both together)

- Replaced usage of `inContentWhitelist` with `hideChromeForLocation` to have more flexibility about URLs to hide
b6e04ca
Irakli Gozalishvili
Owner

I sort of was not comfortable with an idea of overriding window method itself. Maybe it's fine but probably @Mossop knows it better. Also I guess I'm fine with doing this as long as we also have a patch for FF to change hideChromeForLocation such that this hack won't be necessary. Maybe whitelist could contain regexps or functions ?

Matteo Ferretti
Owner

@Gozala as I wrote in the comment, this is what in MDN is suggested for hiding browser chrome. However I totally understand your uncomfortable feeling, because I had the same. Even if this is what is suggested, it feels an hack to me as well and error prone in some cases. I'm totally in to improve Firefox's hideChromeForLocation / whitelist behavior.

Dave Townsend
Owner

I'm ok with it as long as we're still calling the old function and are properly cleaning up on unload, looks like both of those are happening so looks good. Let's get a Firefox bug filed to make this better though.

Irakli Gozalishvili Gozala merged commit da55fc6 into from
Dave Townsend
Owner

I guess I was wrong, this isn't cleaning up after itself properly. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=829845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 31, 2012
  1. Matteo Ferretti

    Bug 783743 - addon-page should work with "index.html?[stuff]" and "in…

    ZER0 authored
    …dex.html#[stuff]" (and both together)
    
    - Replaced usage of `inContentWhitelist` with `hideChromeForLocation` to have more flexibility about URLs to hide
This page is out of date. Refresh to see the latest.
Showing with 118 additions and 3 deletions.
  1. +29 −3 lib/sdk/addon-page.js
  2. +89 −0 test/test-addon-page.js
32 lib/sdk/addon-page.js
View
@@ -12,14 +12,34 @@ const { isXULBrowser } = require('./window/utils');
const { add, remove } = require('./util/array');
const { getTabs, closeTab, getURI } = require('./tabs/utils');
const { data } = require('./self');
+const { ns } = require("./core/namespace");
const addonURL = data.url('index.html');
+const windows = ns();
+
WindowTracker({
onTrack: function onTrack(window) {
- if (isXULBrowser(window))
- add(window.XULBrowserWindow.inContentWhitelist, addonURL);
+ if (!isXULBrowser(window) || windows(window).hideChromeForLocation)
+ return;
+
+ let { XULBrowserWindow } = window;
+ let { hideChromeForLocation } = XULBrowserWindow;
+
+ windows(window).hideChromeForLocation = hideChromeForLocation;
+
+ // Augmenting the behavior of `hideChromeForLocation` method, as
+ // suggested by https://developer.mozilla.org/en-US/docs/Hiding_browser_chrome
+ XULBrowserWindow.hideChromeForLocation = function(url) {
+ if (url.indexOf(addonURL) === 0) {
+ let rest = url.substr(addonURL.length);
+ return rest.length === 0 || ['#','?'].indexOf(rest.charAt(0)) > -1
+ }
+
+ return hideChromeForLocation.call(this, url);
+ }
},
+
onUntrack: function onUntrack(window) {
if (isXULBrowser(window))
getTabs(window).filter(tabFilter).forEach(untrackTab.bind(null, window));
@@ -33,6 +53,12 @@ function tabFilter(tab) {
function untrackTab(window, tab) {
// Note: `onUntrack` will be called for all windows on add-on unloads,
// so we want to clean them up from these URLs.
- remove(window.XULBrowserWindow.inContentWhitelist, addonURL);
+ let { hideChromeForLocation } = windows(window);
+
+ if (hideChromeForLocation) {
+ window.XULBrowserWindow.hideChromeForLocation = hideChromeForLocation;
+ windows(window).hideChromeForLocation = null;
+ }
+
closeTab(tab);
}
89 test/test-addon-page.js
View
@@ -42,6 +42,95 @@ exports['test that add-on page has no chrome'] = function(assert, done) {
});
};
+exports['test that add-on page with hash has no chrome'] = function(assert, done) {
+ let loader = Loader(module);
+ loader.require('addon-kit/addon-page');
+
+ let window = windows.activeBrowserWindow;
+ let tab = openTab(window, uri + "#foo");
+
+ assert.ok(isChromeVisible(window), 'chrome is visible for non addon page');
+
+ // need to do this in another turn to make sure event listener
+ // that sets property has time to do that.
+ setTimeout(function() {
+ activateTab(tab);
+
+ assert.equal(isChromeVisible(window), is('Fennec'), 'chrome is not visible for addon page');
+
+ closeTab(tab);
+ assert.ok(isChromeVisible(window), 'chrome is visible again');
+ loader.unload();
+ done();
+ });
+};
+
+exports['test that add-on page with querystring has no chrome'] = function(assert, done) {
+ let loader = Loader(module);
+ loader.require('addon-kit/addon-page');
+
+ let window = windows.activeBrowserWindow;
+ let tab = openTab(window, uri + '?foo=bar');
+
+ assert.ok(isChromeVisible(window), 'chrome is visible for non addon page');
+
+ // need to do this in another turn to make sure event listener
+ // that sets property has time to do that.
+ setTimeout(function() {
+ activateTab(tab);
+
+ assert.equal(isChromeVisible(window), is('Fennec'), 'chrome is not visible for addon page');
+
+ closeTab(tab);
+ assert.ok(isChromeVisible(window), 'chrome is visible again');
+ loader.unload();
+ done();
+ });
+};
+
+exports['test that add-on page with hash and querystring has no chrome'] = function(assert, done) {
+ let loader = Loader(module);
+ loader.require('addon-kit/addon-page');
+
+ let window = windows.activeBrowserWindow;
+ let tab = openTab(window, uri + '#foo?foo=bar');
+
+ assert.ok(isChromeVisible(window), 'chrome is visible for non addon page');
+
+ // need to do this in another turn to make sure event listener
+ // that sets property has time to do that.
+ setTimeout(function() {
+ activateTab(tab);
+
+ assert.equal(isChromeVisible(window), is('Fennec'), 'chrome is not visible for addon page');
+
+ closeTab(tab);
+ assert.ok(isChromeVisible(window), 'chrome is visible again');
+ loader.unload();
+ done();
+ });
+};
+
+exports['test that malformed uri is not an addon-page'] = function(assert, done) {
+ let loader = Loader(module);
+ loader.require('addon-kit/addon-page');
+
+ let window = windows.activeBrowserWindow;
+ let tab = openTab(window, uri + 'anguage');
+
+ // need to do this in another turn to make sure event listener
+ // that sets property has time to do that.
+ setTimeout(function() {
+ activateTab(tab);
+
+ assert.ok(isChromeVisible(window), 'chrome is visible for malformed uri');
+
+ closeTab(tab);
+ loader.unload();
+ done();
+ });
+};
+
exports['test that add-on pages are closed on unload'] = function(assert, done) {
let loader = Loader(module);
loader.require('sdk/addon-page');
Something went wrong with that request. Please try again.