WIP of auto-container-origin #386

jonathanKingston · 2017-03-16T08:15:45Z

Super WIP PR just to demo the functionality for @johngruen and @groovecoder ...

STR (Assumes default search is google):

create Google container
add a new google container tab
go to google.com
right click, "Always open in this container"
open a new non container tab
Search string "test"

Tab created in step 6. Will be in "Google" container.

Loads of polish needed like:

Change context menu when already auto linking.
Show warning for paranoid users like myself on autolinking
Allow container based auto dismissal of paranoid warning
Potentially show URL autolinking in other menus

groovecoder · 2017-03-16T14:33:41Z

This is for #306? Anything for #311 in here?

groovecoder · 2017-03-16T14:56:47Z

webextension/background.js

+
+browser.contextMenus.onClicked.addListener((info, tab) => {
+  const pageUrl = new window.URL(info.pageUrl);
+  const userContextId = getUserContextIdFromCookieStore(tab.cookieStoreId);


🎉 tab.cookieStoreId Yay Firefox 52 WebExtensions!

Does this need to work for 51? Totally forgot about that :/

Yeah. I introduced cookieStoreId, but it's not in FF51.... we can still retrieve the userContextId from the tab as we do in index.js.

jonathanKingston · 2017-03-16T14:59:21Z

Sorry yea this is for #306.

Nothing for #311 here. There are other issues with doing that I need to explain there. I think we should keep them separate.

groovecoder

Looks like a good start. Some nits but overall it's coming along. Agree with the changes you already mention making.

groovecoder · 2017-03-16T15:08:19Z

webextension/background.js

+  }
+});
+
+browser.webRequest.onBeforeRequest.addListener((options) => {


Could we also add this check?

if (options.frameId !== 0) { return {} }

So we only do this logic for the requests that happen in the top frame?

I.e., we just want to check the first/top request in the tab - not all the requests?

Added the check anyway for paranoia sake.

groovecoder · 2017-03-16T15:11:14Z

webextension/background.js

+  return false;
+}
+
+const storagePrefix = "originMap@@_";


nit: more descriptive name? originContainerMap@@_ ?

Done... the @@_ is my paranoia about something merging with the key itself as it's just a boring key val store.

groovecoder · 2017-03-16T15:12:09Z

webextension/background.js

+     return {
+      cancel: true,
+     };
+     return {};


unreachable?

Yup deleted.

groovecoder · 2017-03-16T15:14:41Z

webextension/background.js

+    console.log('err', e);
+    return {};
+  });
+},{urls: ["<all_urls>"], types: ["main_frame"]}, ["blocking"]);


Ah, the main_frame RequestFilter here takes care of my concern about processing every request. 👍

groovecoder · 2017-03-16T15:26:25Z

Show warning for paranoid users like myself on autolinking

Absolutely 👍

Allow container based auto dismissal of paranoid warning

I would leave this out at first, until users tell us they're sick of the warning? Based on the experience in Blok, I imagine most users will get their origins and containers linked in the first couple weeks, and it seems we should err on the side of reminding them about the privacy & security concerns of auto-linking rather than err on the side of users easily falling back into insecure browsing.

Potentially show URL autolinking in other menus

👍 for the tab-bar context menu (is this possible in WebExtension?), the container icon in the Awesomebar (see #387 - possible in WebExtension?), and/or a new pageAction.

groovecoder · 2017-03-16T16:07:46Z

webextension/background.js

+const storagePrefix = "originMap@@_";
+const storageArea = browser.storage.local;
+
+browser.contextMenus.onClicked.addListener((info, tab) => {


Do we need to add:

if (info.menuItemId !== "open-in-this-container") { return; }

To make sure that clicks on other context menus don't trigger the assignment?

Yeah if we add any certainly. There is a click handler per each menu item too, I just didn't test that out yet.

jonathanKingston · 2017-03-18T00:52:24Z

I put in a rough warning when users are directed to a container, currently this opens in the container and I use history replace to move them into the same container. It might actually make more sense to use message passing to the background page to open in a selected container (this would allow the user flow of people who want to auto assign the container but also want to change it sometimes).

To make the never ask checkbox work I had to use message passing anyway.

I would have to put it a fair bit more work in to get any of these features:

container select box, picker whatever
container name (The background script would need to maintain/fetch just for this request the list of containers which actually would be a pain)

I think we should show the full URL here however... I also think it looks ugly :(

This really needs some of @johngruen's touch probably either way it's lower on the priorities I have on making pretty.

groovecoder · 2017-03-22T19:01:01Z

👍 to some styling work on it.

Some clearer copy may also be:

"
Are you sure you want to open <url> in your pre-assigned container? The page will have access to this container's cookies and other data.

<Open in non-Container> <Open in pre-assigned container> <Never ask me this again>
"

johngruen · 2017-03-22T20:06:48Z

hey @jonathanKingston @groovecoder work week this week has put me behind. Should be able to clean up by friday.

bakulf · 2017-03-22T20:45:23Z

webextension/background.js

+
+browser.contextMenus.onClicked.addListener((info, tab) => {
+  const pageUrl = new window.URL(info.pageUrl);
+  const userContextId = getUserContextIdFromCookieStore(tab.cookieStoreId);


Yeah. I introduced cookieStoreId, but it's not in FF51.... we can still retrieve the userContextId from the tab as we do in index.js.

bakulf · 2017-03-22T20:48:08Z

webextension/background.js

+  if (neverAsk) {
+    browser.tabs.create({url, cookieStoreId: `firefox-container-${userContextId}`});
+  } else {
+    browser.tabs.create({url: `${loadPage}?url=${url}`, cookieStoreId: `firefox-container-${userContextId}`});


What about the position? We should open the new tab in the same position where we are.
Plus, doing this we are breaks the Back-Forward feature.

Another point would be: what about if the URL is loaded from a window.open() ? This breaks also 'the web' because you will open a tab, instead of a window, and the parent window will not have a reference to the new one.

I would somewhat argue breaking window.opener is a feature. I'm not sure we can fix that through a web extension either right?

groovecoder · 2017-03-23T14:07:08Z

The Firefox sec warning page is a good style to copy:

jonathanKingston · 2017-03-24T18:30:10Z

@groovecoder @bakulf @kjozwiak and @johngruen I pushed an update for the branch auto-container-origin

This has the index mentioned by @bakulf and the new error layout as discussed with @groovecoder and demoed in the meeting.

It is still missing the ability to unassign though.

groovecoder

Mostly naming and copy nits. But a big-ish question is if we want to set a global "never ask" preference, or I thought we should start with a "Never ask again for this site".

Should we start out with the more secure/private/annoying UX - i.e., confirm for each individual assignment - and only relax it when users complain?

groovecoder · 2017-03-28T15:09:51Z

webextension/background.js

@@ -1,3 +1,174 @@
+const assignManager = {
+  closeableWindows: new Set([


nit: CLOSEABLE_WINDOWS to denote it is a constant/hard-coded set.

Using uppercase here makes me cry a little (especially when it's not a const too), much like Hungarian notation does but I will not argue ;)

groovecoder · 2017-03-28T15:10:29Z

webextension/background.js

+    "about:home",
+    "about:blank"
+  ]),
+  menuAssignId: "open-in-this-container",


MENU_ASSIGNED_ID

groovecoder · 2017-03-28T15:10:37Z

webextension/background.js

+    "about:blank"
+  ]),
+  menuAssignId: "open-in-this-container",
+  menuRemoveId: "remove-open-in-this-container",


MENU_REMOVE_ID

groovecoder · 2017-03-28T15:15:28Z

webextension/background.js

+      });
+    });
+
+    browser.runtime.onMessage.addListener((request) => {


Can we change request to something more accurate like neverAskMessage? When I first read this, I thought request was an HTTP request of some kind.

groovecoder · 2017-03-28T15:17:28Z

webextension/background.js

+  },
+
+  getOriginStoreKey(url) {
+    const storagePrefix = "originContainerMap@@_";


nit: STORAGE_PREFIX to denote hard-coded constant string.

groovecoder · 2017-03-28T15:39:18Z

webextension/background.js

+        if (store[originStoreKey]) {
+          browser.contextMenus.create({
+            id: this.menuRemoveId,
+            title: "Remove Open in This Container",


Maybe "Un-assign this site from this Container"

groovecoder · 2017-03-28T15:40:59Z

webextension/js/confirm-page.js

@@ -0,0 +1,25 @@
+const redirectUrl = new URL(window.location).searchParams.get("url");
+document.getElementById("redirect-url").textContent = redirectUrl;
+const redirectOrigin = new URL(redirectUrl).origin;


const redirectHostname = new URL(redirectUrl).hostname and textContent = redirectHostname will probably look cleaner?

groovecoder · 2017-03-28T15:42:22Z

webextension/confirm-page.html

+      </p>
+      <div id="redirect-url"></div>
+      <p>
+        You asked for <span id="redirect-origin"></span> to always open in <span id="container-name">this</span> container, are you sure you want to confirm?<br />


Maybe You assigned <span id="redirect-hostname"></span> to <span id="container-name">this</span> container, ...

Happy to change "asked for" to "added" as above however. I personally think that is less code'y.

Either copy is fine there. But "are you sure you want to confirm" is redundant? Should be "Are you sure you want to open the page in this container?" right?

groovecoder · 2017-03-28T15:46:40Z

webextension/css/popup.css

@@ -383,7 +383,7 @@ span ~ .panel-header-text {
 .panel-footer {
  align-items: center;
  background: #efefef;
-  block-size: 55px;
+  block-size: 54px;


Screw you 55th pixel!

This was unrelated but part of the menu being unaligned.

groovecoder · 2017-03-28T15:47:12Z

webextension/manifest.json

+    "storage",
+    "tabs",
+    "webRequestBlocking",
+    "webRequest"


Wow, we really are creating a tab-manager add-on.

We were just faking it till we made it before, now we are the real deal 🦊

groovecoder

Almost perfect. But, during my spot-checks, I still seemed to lose the context menu feature and the originSettings completely? I'm using jpm watchpost ... so maybe a file-change triggered an .xpi re-install? Will the origin settings survive an addon upgrade?

groovecoder · 2017-03-29T17:47:51Z

webextension/confirm-page.html

+      </p>
+      <div id="redirect-url"></div>
+      <p>
+        You asked for <span id="redirect-origin"></span> to always open in <span id="container-name">this</span> container, are you sure you want to confirm?<br />


Either copy is fine there. But "are you sure you want to confirm" is redundant? Should be "Are you sure you want to open the page in this container?" right?

groovecoder · 2017-03-29T17:50:53Z

webextension/background.js

+          }
+          resolve(null);
+        }).catch(() => {
+          reject();


Need a reason here. .catch((e) => { reject(e); }); would be fine.

groovecoder · 2017-03-29T18:46:47Z

Also, this needs a version bump to 2.1.0

groovecoder · 2017-03-29T19:41:35Z

webextension/background.js

+          browser.notifications.create({
+            type: "basic",
+            title: "Containers",
+            message: `Successfully ${actionName} ${pageUrl.origin} to always open in this container`,


What if we just remove the ${pageUrl.origin} completely? A significant origin string overflows the notification interface, and the important feedback here seems to be the action. Can we assume the user knows the origin they just assigned, since they just assigned it?

kjozwiak · 2017-03-29T20:35:55Z

Issues found while testing the auto-container-origin branch on jonathanKingston/testpilot-containers:

Bug [1] [FIXED/VERIFIED]

"Always Open in This Contianer" is being displayed under the context menu under about:privatebrowsing when opening a PB window while the currently focussed tab is a container.

STR:

open the personal container via the "+" new tab button and load any website
while the personal container is the currently focussed tab, open a new PB window
once the PB window opens, right click on the about:privatebrowsing page

Bug [2] [FIXED/VERIFIED]

Once you've assigned an origin to a specific container, opening that particular origin within PB won't load the website and will produce the following error in the browser console:

Error: Illegal to set non-private cookieStorageId in a private window

Even though an origin has been assigned to a specific container, users should still be able to open that website within PB as those are two separate features. For example, lets say I assigned google.ca to my "Google" container but I want to search for something that won't appear in my history and I don't feel like creating a "throw away" container. I won't be able to use google.ca in this situation.

STR:

open the personal container via the "+" new tab button and load any website
open a PB window and load the same website within PB

Bug [3] [FIXED/VERIFIED]

The "Hey, we heard you liked containers!" intermediate window will still appear for origins that belonged to a specific container that have been deleted.

STR:

open the personal container and load any website within that container
right click on the website and select "Always Open in This Container"
close the personal tab
remove the personal container via the Containers Test Pilot icon
load the same origin... you'll get the "Hey, we heard you liked containers!" even thought it's loading in non-container tab due to the personal container being removed

Bug [4] [FIXED/VERIFIED]

Related to the above issue (bug 3), the "Always Open in This Container" item will appear under the context menu of an origin that used to belong to a container that has now been removed. Once a user deletes a container, we need to remove all origin associations with that particular container.

STR:

open the personal container and load any website within that container
right click on the website and select "Always Open in This Container"
close the personal tab
remove the personal container via the Containers Test Pilot icon
load the same origin... you'll get the "Hey, we heard you liked containers!"
select "Take me there"
the origin will open in a non-default container.... right click on the page and you'll notice that the "Always Open in This Container" item is appearing under the context menu.

Suggestion, possible Bug [5] [currently not solvable as per @jonathanKingston]

I'm not sure if this is considered a bug or not, but I personally feel like it's going to be an issue. When clearing "everything" from FX using the "Clear Recent History" feature, should we be removing all the origin associations? Even though we've removed all the cookies, history etc... that origin association is still there and the user will get the "Hey, we heard you liked containers!" window.

STR:

associate several different origins with several containers
remove everything from FX via "Clear History"
load any of the previous origins into a non-container window and you'll still be presented with the "Hey, we heard you liked containers!" window.

Bug [6] [FIXED/VERIFIED]

Should we be saving the moz-extension:// warnings into History? I think we're just going to pollute users history data with useless data. You'll also get the following error message in the browser console when you attempt to open of the entries that belonged to a container that has been removed:

NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIWebNavigation.loadURIWithOptions]  browser.js:848
	_loadURIWithFlags chrome://browser/content/browser.js:848:7
	loadURIWithFlags chrome://browser/content/tabbrowser.xml:7370:13
	openLinkIn chrome://browser/content/utilityOverlay.js:380:5
	openUILinkIn chrome://browser/content/utilityOverlay.js:197:3
	openUILink chrome://browser/content/utilityOverlay.js:102:3
	HM__onCommand chrome://browser/content/browser-places.js:822:7
	oncommand chrome://browser/content/browser.xul:1:1

jonathanKingston · 2017-03-30T19:55:46Z

@kjozwiak Issues 1,2,6 are solved.

Issues 3,4,5 are not. I'm not actually sure 5 is solvable with the extension. 3,4 are not solvable in web extensions see: https://bugzilla.mozilla.org/show_bug.cgi?id=1352189 I will look into message passing from SDK when I get home (I was hoping to completely dog food web extensions here :( )

Solved in latest push

jonathanKingston · 2017-03-31T16:42:44Z

TypeError: types.taggedTemplates.includes is not a function
    at isSafeNode (/home/jonathan/projects/testpilot-containers/node_modules/eslint-plugin-no-unescaped/lib/rules/enforce.js:91:37)
    at EventEmitter.AssignmentExpression (/home/jonathan/projects/testpilot-containers/node_modules/eslint-plugin-no-unescaped/lib/rules/enforce.js:152:18)
    at emitOne (events.js:82:20)
    at EventEmitter.emit (events.js:169:7)
    at NodeEventGenerator.enterNode (/home/jonathan/projects/testpilot-containers/node_modules/eslint/lib/util/node-event-generator.js:39:22)
    at CodePathAnalyzer.enterNode (/home/jonathan/projects/testpilot-containers/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:607:23)
    at CommentEventGenerator.enterNode (/home/jonathan/projects/testpilot-containers/node_modules/eslint/lib/util/comment-event-generator.js:98:23)
    at Controller.enter (/home/jonathan/projects/testpilot-containers/node_modules/eslint/lib/eslint.js:928:36)
    at Controller.__execute (/home/jonathan/projects/testpilot-containers/node_modules/estraverse/estraverse.js:397:31)
    at Controller.traverse (/home/jonathan/projects/testpilot-containers/node_modules/estraverse/estraverse.js:501:28)

Linting rule I wrote is borked... should fix which is annoying as I want this rule to be replaced.

jonathanKingston · 2017-03-31T16:43:27Z

Also... this latest patch fixes 3 and 4 and rebases.

groovecoder

Code looks good. Spot-checks look good.

groovecoder · 2017-04-03T19:27:41Z

I would still be good to merge this, but I notice assigning a site to auto-open in a container now triggers a page-requests-completed-per-tab event with pageRequestCount of 0 for the confirm page being loaded in firefox-default container. 😐

kjozwiak · 2017-04-03T19:38:54Z

Verified that bug#1, 2, 3, 4 and 6 have been fixed.

jonathanKingston requested a review from groovecoder March 16, 2017 08:15

groovecoder reviewed Mar 16, 2017

View reviewed changes

groovecoder requested changes Mar 16, 2017

View reviewed changes

groovecoder reviewed Mar 16, 2017

View reviewed changes

jonathanKingston force-pushed the auto-container-origin branch from 56a05d9 to 4bb53ef Compare March 18, 2017 00:45

bakulf reviewed Mar 22, 2017

View reviewed changes

jonathanKingston force-pushed the auto-container-origin branch from 4bb53ef to c2d78c7 Compare March 24, 2017 18:28

jonathanKingston force-pushed the auto-container-origin branch 2 times, most recently from a2df62f to 90beb31 Compare March 28, 2017 01:53

groovecoder requested changes Mar 28, 2017

View reviewed changes

jonathanKingston force-pushed the auto-container-origin branch from 90beb31 to 5598e70 Compare March 29, 2017 16:47

groovecoder previously requested changes Mar 29, 2017

View reviewed changes

groovecoder reviewed Mar 29, 2017

View reviewed changes

jonathanKingston force-pushed the auto-container-origin branch 2 times, most recently from c4f261d to e333f91 Compare March 30, 2017 19:51

jonathanKingston force-pushed the auto-container-origin branch from e333f91 to b02be0b Compare March 30, 2017 19:57

kjozwiak mentioned this pull request Mar 30, 2017

Automatically open a hostname in the previously selected container (rather than Default) #322

Closed

jonathanKingston force-pushed the auto-container-origin branch from b02be0b to 0c7242e Compare March 31, 2017 16:32

jonathanKingston force-pushed the auto-container-origin branch 3 times, most recently from d2300f8 to ce8b4de Compare April 3, 2017 15:58

groovecoder approved these changes Apr 3, 2017

View reviewed changes

Auto send users to sites they assigned to a container. Fixes mozilla#306

3e657a2

jonathanKingston force-pushed the auto-container-origin branch from ce8b4de to 3e657a2 Compare April 3, 2017 20:57

groovecoder merged commit ca1a926 into mozilla:master Apr 3, 2017

		@@ -1,3 +1,174 @@
		const assignManager = {
		closeableWindows: new Set([

WIP of auto-container-origin #386

WIP of auto-container-origin #386

Conversation

jonathanKingston commented Mar 16, 2017

groovecoder commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanKingston commented Mar 16, 2017

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanKingston Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanKingston commented Mar 18, 2017

groovecoder commented Mar 22, 2017 • edited Loading

johngruen commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder commented Mar 23, 2017

jonathanKingston commented Mar 24, 2017 • edited Loading

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder commented Mar 29, 2017

groovecoder Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

kjozwiak commented Mar 29, 2017 • edited Loading

jonathanKingston commented Mar 30, 2017

jonathanKingston commented Mar 31, 2017

jonathanKingston commented Mar 31, 2017

groovecoder left a comment

Choose a reason for hiding this comment

groovecoder commented Apr 3, 2017

kjozwiak commented Apr 3, 2017

jonathanKingston Mar 16, 2017 •

edited

Loading

groovecoder commented Mar 22, 2017 •

edited

Loading

jonathanKingston commented Mar 24, 2017 •

edited

Loading

groovecoder Mar 29, 2017 •

edited

Loading

kjozwiak commented Mar 29, 2017 •

edited

Loading