1199466 - Update the Cookie Management UI to include userContextId

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Steven Englehardt [:englehardt]]

In Bug 1165267 originArributes support will be added to cookies. This will include the userContextId which was added in Bug 1179557.

We will want to expose user context to users in the Cookie Management UI. This may be as simple as listing the context as an additional attribute of the cookie or providing separate views of the cookie list for each context.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsICookie.originAttributes

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - UI

tanvi, I don't know how can review this patch. It's mainly UI.
With this patch I show just the numeric userContextId. I don't know how much useful could be to show the userContext name here, but feedbacks are welcome :)

Comment 3

[Tanvi Vyas[:tanvi]]

Hi baku,

For now, we only have 4 user contexts.  The user doesn't know they correspond to numbers.  So I would say show the user context name instead of the number (Personal, Work, Banking, Shopping) under a column named "Container".  What should we put for the default container (user context 0)?  Should we write "default" or write nothing into the containers column?  I'm leaning towards writing nothing and leaving it blank for the default container.

It would be nice if we could use the same font color that we use in the url bar for each container (for example, Personal is in blue and Shopping is in pink).  Enabling sorting by container name would also be nice.  But see what you can do  (i.e. if this is too hard right now then move on and we can get back to it.  these are just nice to haves).

In a future version of Contextual Identity, we will add custom user contexts and then we can revisit this decision and the UI then.

Comment 4

[Tanvi Vyas[:tanvi]]

Comment on attachment 8703738 [details] [diff] [review]
part 2 - UI

Please also see comment 3.

diff --git a/browser/components/preferences/cookies.js b/browser/components/preferences/cookies.js
--- a/browser/components/preferences/cookies.js
+++ b/browser/components/preferences/cookies.js
-              container   : false };
+              container   : false,
+              originAttributes: aCookie.originAttributes };

Fix indenting by indenting everything more.  Same in the other patch - /netwerk/cookie/nsCookie.h


diff --git a/browser/locales/en-US/chrome/browser/preferences/cookies.dtd b/browser/locales/en-US/chrome/browser/preferences/cookies.dtd
--- a/browser/locales/en-US/chrome/browser/preferences/cookies.dtd
+++ b/browser/locales/en-US/chrome/browser/preferences/cookies.dtd
@@ -16,16 +16,17 @@
 <!ENTITY     props.secure.label             "Send For:">
 <!ENTITY     props.expires.label            "Expires:">
+<!ENTITY     props.userContext.label        "User Context:">

Change to "Container" for now to match the text in the File menu.  When we get closer to releasing this, we will need to determine the right way to describe this feature.  Our UR will also help with this.

You will also need a review by a browser peer.  You could try Paolo since he is familiar with this feature.

r- for now and I will take another look at the second patch.

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to Andrea Marchesini (:baku) from comment #1)
> Created attachment 8703737 [details] [diff] [review]
> part 1 - nsICookie.originAttributes

I'm a bit surprised that nsCookieService still doesn't fully support originAttributes.  Wasn't that work supposed to be done in https://bugzilla.mozilla.org/show_bug.cgi?id=1165267?  How is usercontextid working if cookies are not supported yet?  For example, nsCookie::Create() doesn't take originAttributes as a parameter before this patch.

Comment 6

[Andrea Marchesini [:baku]]

> I'm a bit surprised that nsCookieService still doesn't fully support
> originAttributes.  Wasn't that work supposed to be done in

The originAttributes is stored in the 'key' of the hashtable. This is the reason why we don't have such information in nsICookie is because we always retrieve them from a 'key' and that key as the information of the originAttributes.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 2 - UI

Comment 8

[Tanvi Vyas[:tanvi]]

Comment on attachment 8703819 [details] [diff] [review]
part 2 - UI

diff --git a/browser/components/preferences/cookies.js b/browser/components/preferences/cookies.js
--- a/browser/components/preferences/cookies.js
+++ b/browser/components/preferences/cookies.js

+      document.getElementById("userContext-icons").setAttribute("usercontextid", aItem.originAttributes.userContextId);
     }
     else {
       var noneSelected = this._bundle.getString("noCookieSelected");
       properties = { name: noneSelected, value: noneSelected, host: noneSelected,
                      path: noneSelected, expires: noneSelected,
-                     isSecure: noneSelected };
-      for (i = 0; i < ids.length; ++i)
+                     isSecure: noneSelected, "userContext-label": noneSelected };
+      for (i = 0; i < ids.length; ++i) {
         document.getElementById(ids[i]).disabled = true;
+      }
+
+      document.getElementById("userContext-icons").removeAttribute("usercontextid");


Why do you need to removeAttribute here?

+++ b/browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ -122,16 +122,20 @@ cookiesFiltered=The following cookies ma
+containerPersonal=Personal
+containerWork=Work
+containerBanking=Banking
+containerShopping=Shopping

Can we use the strings here? http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#788


Please get browser peer review.

Comment 9

[Andrea Marchesini [:baku]]

> Why do you need to removeAttribute here?

In the CSS we have:

#userContext-icons:not([usercontextid]) { ...

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - UI

Comment 11

[:Paolo Amadini]

Thanks for working on this! It will make debugging easier for developers who want to test containers before we complete the final UI for it.

I haven't started a detailed review, I'll be able to take a closer look next week. In the meantime, I have two general indications.

First, ensure that the new column can only be made visible when the about:config preference for containers is enabled. We don't want to ship this accidentally, as it may not even be the final UI that we'll use when releasing the feature. To be clear, it should not be possible to make the column visible at all if the about:config preference is not enabled, it's not enough for it to be invisible by default.

Second, please create a module named browser/modules/UserContextUI.jsm exposing this function:

UserContextUI.getUserContextLabel(userContextId)

In addition to using the function in this patch, use it here as well:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4050

Note the use of "Context " + userContextId in the default case.

Comment 12

[:Paolo Amadini]

Comment on attachment 8704023 [details] [diff] [review]
part 2 - UI

I'm not a big fan of reusing "usercontext.inc.css" verbatim here. It's likely that people would modify it following changes in "browser.xul", without even noticing that it is used elsewhere.

The simple solution is to avoid displaying the icons here. They don't add a lot of value for debugging, the name of the container is enough.

I really suggest to just avoid the icons. They can always be added later if ever needed.

For completeness, if we wanted to reuse the container icons in other parts of the UI in the future, a better approach would be to include a file that only defines CSS variables on the ":root" pseudo-element, called "usercontext-variables.inc.css". See for example the beginning of "menupanel.inc.css" and "tabs.inc.css". You could then rename "usercontext.inc.css" to "usercontext-browser.inc.css" for clarity, and include the other file at the top. In the preferences CSS, you would only include "usercontext-variables.inc.css" and use different selectors.

So, the controls in the patch can be renamed using the local convention, like "userContextLabel". This adds consistency and also avoids the need for quotation marks in some of the JS code.

I think there's enough for the next iteration of the patch. Thanks for doing this!

Comment 13

[:Paolo Amadini]

Also, I tried the code locally but I don't seem to get different cookies even with the other patch in this bug applied. Do I need any other patches for that? In which order?

Comment 14

[:Paolo Amadini]

Jared, is there any code or CSS for windowed preferences that we should consider removing? Right now it's difficult for me and others to figure out if we should just stop updating some of the files here. I haven't found any bug on file for code removal here.

While it's not relevant right now, as we're not changing the CSS anymore, the original patch here touched a few files that seems like they could be for windowed preferences only, that maybe could be left alone.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 2 - UI

Comment 17

[Andrea Marchesini [:baku]]

(In reply to :Paolo Amadini from comment #13)
> Also, I tried the code locally but I don't seem to get different cookies
> even with the other patch in this bug applied. Do I need any other patches
> for that? In which order?

In order to test it locally, you must enable this pref: privacy.userContext.enabled

Comment 18

[:Paolo Amadini]

(In reply to Andrea Marchesini (:baku) from comment #17)
> In order to test it locally, you must enable this pref:
> privacy.userContext.enabled

I mean, I've applied the two patches in this bug, I've flipped the preference, and I've opened a webpage in a new Personal tab. When I look for its cookies in the management interface, I don't see any difference from the cookies I got when I loaded the same webpage in a normal tab.

Comment 19

[:Paolo Amadini]

Comment on attachment 8706330 [details] [diff] [review]
part 2 - UI

It would be nice if you could move the function that retrieves the string to a UserContextUI.jsm module, but if you don't want to do this right now just file a follow-up bug and reference its number in the comments of the function. In any case, ensure that the resulting string for unknown contexts says "Context 5" and not just "5".

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 2 - UI

Ok, I'll file a follow up for that.

Comment 21

[:Paolo Amadini]

Comment on attachment 8706449 [details] [diff] [review]
part 2 - UI

Review of attachment 8706449 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with two required changes.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +789,5 @@
>  usercontext.personal.label = Personal
>  usercontext.work.label = Work
>  usercontext.shopping.label = Shopping
>  usercontext.banking.label = Banking
> +usercontext.unknown.label = Container %S

Sorry for the misunderstanding, this case should not have a new localizable string, the word "Context" should be kept in English only. Take a look at what the function in browser.js does:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4064

The second required change is to write in the comments in the patch the bug number of the bug you'll file about sharing the getter for this string in a JSM. It's good for people wondering why there is duplication, and also a good way to remember to file the bug before landing the patch :-)

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Paolo Amadini from comment #14)
> Jared, is there any code or CSS for windowed preferences that we should
> consider removing? Right now it's difficult for me and others to figure out
> if we should just stop updating some of the files here. I haven't found any
> bug on file for code removal here.
> 
> While it's not relevant right now, as we're not changing the CSS anymore,
> the original patch here touched a few files that seems like they could be
> for windowed preferences only, that maybe could be left alone.

Yes, I can see how this is a bit confusing (having a themes/*/preferences/preferences.css and themes/*/preferences/in-content/preferences.css). Both files are currently used and would be merged as part of fixing bug 1201243. You can see that they are both included if you look at the includes at the top of /browser/components/preferences/in-content/preferences.xul.

With that said, bug 1027174 is the only bug I know about to remove CSS that is no longer needed, though that doesn't concern itself with the Cookies dialog.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0112b03881c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b9201165df

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0112b03881c
https://hg.mozilla.org/mozilla-central/rev/a7b9201165df

Comment 25

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected