Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for firefox on android #1516

Merged
merged 22 commits into from
Aug 20, 2017
Merged

Added support for firefox on android #1516

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
5328e70
making options page accessible within android
lemnis Jul 16, 2017
b330291
faking popup when clicked on browser action
lemnis Jul 16, 2017
f222299
extra checks if specific function is available
lemnis Jul 16, 2017
fdaa6fb
fix for android to get tab id
lemnis Jul 16, 2017
d39af45
Merge remote-tracking branch 'EFForg/master'
lemnis Jul 18, 2017
bc231ec
Fixing small css issues, mostly visible on a phone
lemnis Jul 18, 2017
c61c0a8
cleanup
lemnis Jul 19, 2017
0b865e8
Removes whitespace at bottom of a popup in chrome
lemnis Jul 19, 2017
ab5d902
removed chrome.windows api
lemnis Jul 20, 2017
5a8d934
small fixes caused by container size
lemnis Jul 20, 2017
323580e
Moved feature checks to top of function
lemnis Jul 21, 2017
3bda018
Moved to URL api
lemnis Jul 26, 2017
ea950fd
changed fake popup behavior + cleanup
lemnis Jul 26, 2017
7884298
update feature check
lemnis Jul 26, 2017
700e92c
removed whitespace
lemnis Jul 26, 2017
ac18053
fixed accidental tab close
lemnis Aug 14, 2017
b4dd778
cleanup code
lemnis Aug 14, 2017
2c986a2
Moved all FF Android functions to sep. file
lemnis Aug 16, 2017
e73d12d
Merge branch 'EFForg/master'
lemnis Aug 16, 2017
98fbb56
small bug fixes
lemnis Aug 16, 2017
2443260
improved comments
lemnis Aug 16, 2017
1f230cf
fix for selenium test
lemnis Aug 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 36 additions & 13 deletions src/js/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,9 @@ function Badger() {
}

// Show icon as page action for all tabs that already exist
chrome.windows.getAll({populate: true}, function (windows) {
for (var i = 0; i < windows.length; i++) {
for (var j = 0; j < windows[i].tabs.length; j++) {
badger.refreshIconAndContextMenu(windows[i].tabs[j]);
}
chrome.tabs.query({}, function (tabs) {
for (var i = 0; i < tabs.length; i++) {
badger.refreshIconAndContextMenu(tabs[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
});

Expand Down Expand Up @@ -532,16 +530,22 @@ Badger.prototype = {
*/
updateBadge: function(tabId){
if (!this.showCounter()){
chrome.browserAction.setBadgeText({tabId: tabId, text: ""});
if(chrome.browserAction.setBadgeText){
chrome.browserAction.setBadgeText({tabId: tabId, text: ""});
}
return;
}
var numBlocked = this.blockedTrackerCount(tabId);
if(numBlocked === 0){
chrome.browserAction.setBadgeBackgroundColor({tabId: tabId, color: "#00cc00"});
} else {
chrome.browserAction.setBadgeBackgroundColor({tabId: tabId, color: "#cc0000"});
if(chrome.browserAction.setBadgeBackgroundColor){
if(numBlocked === 0){
chrome.browserAction.setBadgeBackgroundColor({tabId: tabId, color: "#00cc00"});
} else {
chrome.browserAction.setBadgeBackgroundColor({tabId: tabId, color: "#cc0000"});
}
}
if(chrome.browserAction.setBadgeText){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move all of these if (chrome.browserAction....) { guards to the top of this function?

Also, could we combine these into a global variable like isMobile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved feature checks.

For what would you use that global variable? All my checks are just feature checks. The best way to determine if it's mobile is chrome.runtime.getPlatformInfo((platformInfo) => {console.log(platformInfo.os)});, but i don't see a specific use case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use case is to isolate browser specific code into its own region so developers can reason about it more easily.

For example, I'm imagining someone might be trying to fix some bug on chrome, if they come across this code, it might not be obvious that this intended for use on mobile.

Eventually, it would nice if browser specific code was isolated to independent files, then we could have a shim API. That is out of scope for this PR. But to make it easier to do in the future, it would be nice to have browser specific pieces clearly distinguished.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: way to determine if it is mobile

I think we should use feature existence as a way to determine if its a mobile browser, so we could do something like:

let isMobile = !(chrome.browserAction.getPopup && chrome.browserAction.setBadgeText && etc);

This also means that we won't have to consider edge cases where some of these API's may exist while others do not.

The downside is we won't immediately get new features if these api's are added. But I think think that is okay. And when they are added all we have to do is remove them from the isMobile check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the boolean to the badger constructor, it's only the browserAction API where it is checked against.

Added in another pull request other browser specific code 😞 , bloating the source code 😇

chrome.browserAction.setBadgeText({tabId: tabId, text: numBlocked + ""});
}
chrome.browserAction.setBadgeText({tabId: tabId, text: numBlocked + ""});
},

/**
Expand Down Expand Up @@ -602,7 +606,7 @@ Badger.prototype = {
}
return true;
},

/**
* Check if privacy badger is disabled, take an origin and
* check against the disabledSites list
Expand Down Expand Up @@ -761,7 +765,7 @@ Badger.prototype = {
};
}

chrome.browserAction.setIcon({tabId: tab.id, path: iconFilename});
if(chrome.browserAction.setIcon){ chrome.browserAction.setIcon({tabId: tab.id, path: iconFilename}); }
chrome.browserAction.setTitle({tabId: tab.id, title: "Privacy Badger"});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, now this is localized properly by the "default_title" key in the manifest

},

Expand All @@ -776,6 +780,25 @@ function startBackgroundListeners() {
}
}, {urls: ["http://*/*", "https://*/*"]}, []);

// Temporary fix for android while it doesn't support `browser_action.default_popup`
// Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1330159
if(!chrome.browserAction.getPopup) {
chrome.browserAction.onClicked.addListener(() => {
chrome.tabs.query({active: true, lastFocusedWindow: true}, (tabs) => {
var url = chrome.runtime.getManifest().browser_action.default_popup + "?id=" + tabs[0].id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the "?id=" + tabs[0].id part needed?

Are we trying to find if any popup is open? In which case having the tab included might make us miss it in the url query.


// check if popup already is open
chrome.tabs.query({url}, (popupTabs) => {
if(popupTabs.length > 0){
chrome.tabs.update(popupTabs[0].id, {active: true});
chrome.tabs.reload(popupTabs[0].id);
} else {
chrome.tabs.create({url, index: tabs[0].index + 1});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get around the passing a url parameter. After you load the popup, you could try doing:

chrome.tabs.executeScript(tabId, {code: 'getTab = callback => {callback(' + tabId + ')}'});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would override the getTab function in popup with something that works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I don't think that would work... Another idea is using sendMessage. But if this is working well there is no need to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree there should be a neater solution, what do you think is the best behavior?

  • the current, update an existing popup with same parent tab or create a new popup
  • always create a new popup and close the popup directly after it was deactivated
  • keep at max 1 popup open and update its content

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that option 2 is the best! It also mimics the behavior of current popups (it isn't long running). And adding code for deactivating the popup should be simpler than adding the updating code for 3. And I think it will avoid the confusion a user might have when they switch to a popup, and don't know what tab the popup is for.

}
});
});
});
}

// Update icon if a tab changes location
chrome.tabs.onUpdated.addListener(function(tabId, changeInfo, tab) {
Expand Down
33 changes: 32 additions & 1 deletion src/js/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,26 @@ function syncUISelections() {
}
}

/**
* Get's the value of specific key within a url.
* Temporary function for android while it doesn't support `browser_action.default_popup`
*
* Source: https://stackoverflow.com/a/901144/2662749
* Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1330159
*
* @param {String} key Key to get from url
* @param {String} url
* @return {String} Value of the key
*/
function getParameterByName(key, url) {
if (!url) { url = window.location.href; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the URL API to do this nicely:

getTabFromUrl(url) {
  return parseInt(new URL(url).searchParams.get('tabId'));
}

The nice thing about working on web extensions is that we only run on newer browsers, so we can use newer API's.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used that API before but didn't bother to read. Changed it know, should have read it before though 💤

key = key.replace(/[\[\]]/g, "\\$&");
var regex = new RegExp("[?&]" + key + "(=([^&#]*)|&|#|$)"), results = regex.exec(url);
if (!results) { return null; }
if (!results[2]) { return ''; }
return decodeURIComponent(results[2].replace(/\+/g, " "));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see what this is for

}

/**
* We use this function where:
* * getting the tabId from the associatedTab id won't work because
Expand All @@ -512,7 +532,18 @@ function syncUISelections() {
* seems to be that it is synchronous.
*/
function getTab(callback) {
chrome.tabs.query({active: true, lastFocusedWindow: true}, function(t) { callback(t[0]); });
/*
* Temporary fix for android while it doesn't support `browser_action.default_popup`
* Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1330159
*/
if(!chrome.browserAction.getPopup){
chrome.tabs.query({active: true, lastFocusedWindow: true}, function(focusedTab) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use chrome.tabs.getCurrent here instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCurrent unfortunately doesn't work

var id = getParameterByName("id", focusedTab[0].url);
chrome.tabs.get(Number(id), function(t) { callback(t); });
});
} else {
chrome.tabs.query({active: true, lastFocusedWindow: true}, function(t) { callback(t[0]); });
}
}

document.addEventListener('DOMContentLoaded', function () {
Expand Down
1 change: 1 addition & 0 deletions src/skin/options.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<head>
<meta name="google" content="notranslate">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<link type="text/css" href="/lib/vendor/jquery-ui-1.12.1.custom/jquery-ui.structure.min.css" rel="stylesheet" />
<link type="text/css" href="/lib/vendor/jquery-ui-1.12.1.custom/jquery-ui.theme.min.css" rel="stylesheet" />
<link type="text/css" media="screen" href="/skin/popup.css" rel="stylesheet" />
Expand Down
25 changes: 16 additions & 9 deletions src/skin/popup.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,21 @@
/*csslint ids:ignore*/

body {
min-width: 200px;
max-width: 400px;
margin: 0;
min-width: 430px; /* Chrome */
max-width: 100%; /* FF android */
font-size: 12px;
background: #fefefe;
color: #333;
font-family: helvetica, arial, sans-serif;
padding-left: 7px;
padding-right: 7px;
overflow-y: hidden;
overflow-x: hidden;
width: 400px;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

creates a whitespace within chrome, use min-width instead

padding: 8px 15px;
box-sizing: border-box;
}
@media screen and (min--moz-device-pixel-ratio:0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firefox specific css, otherwise the popup would render too wide

body{
min-width: 200px; /* FF android */
width: 430px; /* FF desktop */
}
}

a { text-decoration: none }
Expand Down Expand Up @@ -190,11 +194,13 @@ font-size: 16px;
max-height: 290px;
overflow-y: auto;
background-color: #E8E9EA;
position: relative;
}
.key {
position: fixed;
position: absolute;
height: 25px;
width: 235px;
left: 0;
right: 0;
z-index: 30;
background: #fefefe;
padding-top: 4px;
Expand Down Expand Up @@ -296,6 +302,7 @@ font-size: 16px;
z-index: -1;
opacity: 0;
padding: 3%;
box-sizing: border-box;
color: #ccc;
background: url("/skin/background.png");
transition: opacity .1s ease;
Expand Down