Bug 793055 addon-page tests now pass on fennec #585

Merged
merged 4 commits into from Oct 6, 2012

2 participants

@erikvold

No description provided.

@Gozala Gozala was assigned Sep 21, 2012
@Gozala Gozala commented on an outdated diff Oct 5, 2012
packages/addon-kit/lib/addon-page.js
add(window.XULBrowserWindow.inContentWhitelist, addonURL);
},
onUntrack: function onUntrack(window) {
- if (isBrowser(window))
- getTabs(window).
- filter(function(tab) { return getURI(tab) === addonURL; }).
- forEach(function(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);
- closeTab(tab);
- });
+ if (isBrowser(window) && window.XULBrowserWindow)
+ getTabs(window).filter(tabFilter).forEach(untrackTab.bind(null, window));
@Gozala
Mozilla member
Gozala added a note Oct 5, 2012

Could you make name of predicate function little more descriptive so that reading it gives an idea what it does.

For example: isAddonPageTab

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on an outdated diff Oct 5, 2012
packages/addon-kit/lib/addon-page.js
@@ -18,18 +17,22 @@ const addonURL = data.url('index.html');
WindowTracker({
onTrack: function onTrack(window) {
- if (isBrowser(window))
+ if (isBrowser(window) && window.XULBrowserWindow)
@Gozala
Mozilla member
Gozala added a note Oct 5, 2012

It's not clear what this change does or why is it necessary, could you elaborate ?

@Gozala
Mozilla member
Gozala added a note Oct 5, 2012

Ok I think I understand now. What about implementing isXULBrowser function, either in window/utils or locally ?
This would make code easier to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on the diff Oct 5, 2012
doc/dev-guide-source/tutorials/mobile.md
@@ -28,6 +28,7 @@ and `cfx xpi` when targeting Firefox Mobile.
Right now only the following modules are fully functional:
+* [addon-page](packages/addon-kit/addon-page.html)
@Gozala
Mozilla member
Gozala added a note Oct 5, 2012

I'm confused. My understanding is that nothing will happen on fennec at all ?
If so is that a expected behavior ? If it is we should note this in docs.
Also comment about this in addon-page module would be useful IMO.

@erikvold
erikvold added a note Oct 6, 2012

Yep, on Fennec nothing will happen, but it can be used in a add-on that would be expected to work on Firefox as well is all that I wanted to point out, so I agree that more information about the expected behavior on Fennec is a good idea. I will do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff Oct 5, 2012
packages/addon-kit/tests/test-addon-page.js
let uri = require('self').data.url('index.html');
-function isChromeVisible(window)
- window.document.documentElement.getAttribute('disablechrome') !== 'true'
+function isChromeVisible(window) {
+ let x = window.document.documentElement.getAttribute('disablechrome')
+ console.log('x is '+x)
@Gozala
Mozilla member
Gozala added a note Oct 5, 2012

Please remove logging.

@erikvold
erikvold added a note Oct 6, 2012

oops thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on the diff Oct 5, 2012
packages/api-utils/lib/tabs/utils.js
@@ -92,7 +92,11 @@ function openTab(window, url, options) {
exports.openTab = openTab;
function isTabOpen(tab) {
- return !!tab.linkedBrowser;
@Gozala
Mozilla member
Gozala added a note Oct 5, 2012

It would be a lot easier to understand if it was just:

function isTabOpen(tab) {
  return !!tab.linkedBrowser || !!getWindowHoldingTab(tab)
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala
Mozilla member

Looks good, just make sure to address comments before landing.

Thanks!

@erikvold erikvold merged commit af174a3 into mozilla:master Oct 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment