1244795 - adding a string for the default container under cookie manager

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: defaultContext.png

Currently, the container field under the cookie manager for the default context is blank (see attached image). I think we should add a string so we're in parity with the other containers. From a UX perspective, I think having a string representing the default context is a lot better than leaving it blank.

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Bram, thoughts?

Comment 2

[Bram Pitoyo [:bram]]

Yes. We should add a string to indicate default context. Although what to call this context I’m not so sure about. Some obvious names: default, none, n/a, no container, main.

Another interesting proposal is to hide the “Container” field entirely, but only when its value are default.

I like hiding the best, but in case we can’t do it, “none” works best. If you are using containers, it tells you that it belongs nowhere. If you aren’t using containers, you don’t need to worry about it (if we call it default, user may wonder “what is not default, then?”).

Thoughts?

Comment 3

[Tanvi Vyas[:tanvi]]

If the user doesn't have containers enabled, the column will be hidden (right baku?).

So the question is what to label default container cookies with when they are along side container cookies.

none, n/a, no container, and main all seem like good options.  Bram, short term, which one should we pick and run with for the expert users who turn the feature on?  In the long term, how do we validate which one is the best?

Comment 4

[Bram Pitoyo [:bram]]

In the short term, let’s use “none”.

I have no idea how to test the cookies viewer UI, since it’s a feature that – I suspect – only technical users will utilise, and it’s very hard to recruit them using tools like usertesting.com.

Comment 5

[Kamil Jozwiak [:kjozwiak]]

> If the user doesn't have containers enabled, the column will be hidden

Yup, looks like that's the current behavior. Once containers are turned off, the "Container" column is removed from the UI.

> In the short term, let’s use “none”.

Agreed, I think that's the best choice, at least for the short term.

> I have no idea how to test the cookies viewer UI

We could try using Mozilla's QA One and Done platform [1] but similarly to usertesting.com, we might have a difficult time getting feedback from more technical users. We could always try adding a test case and see if we get any participation/feedback? Doesn't take very long to get one posted.

[1] https://oneanddone.mozilla.org/

Comment 6

[Andrea Marchesini [:baku]]

Attachment: a.patch

Comment 7

[Andrea Marchesini [:baku]]

Attachment: a.patch

Comment 8

[Bram Pitoyo [:bram]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #5)
> We could try using Mozilla's QA One and Done platform […]

This sounds like a good idea. I am wary of publicising this feature too much before we can articulate its value to users, but we can certainly try coming up with a One and Done task.

Comment 9

[:Paolo Amadini]

Comment on attachment 8715423 [details] [diff] [review]
a.patch

I'd make the string local to the preferences dialog, and display it conditionally when !userContextId. It's likely different UI might want to display different strings for the no user context case, and an overload of the shared function only makes the flow more complicated than necessary.

For terminology to use in the code, the comments, and the string identifiers, "no user context" is better than "default user context" since the latter can apply to the four other ones as well. If you notice, the comment in the IDL says "These strings specify the four default contexts...".

Comment 10

[Kamil Jozwiak [:kjozwiak]]

> This sounds like a good idea. I am wary of publicising this feature too much
> before we can articulate its value to users, but we can certainly try coming
> up with a One and Done task.

Talked to Tanvi quickly on IRC and decided that we'll wait to get some feedback from the usertesting study before creating anything under One and Done. This way, we can refine our test case(s) and questions using the feedback that we get from usertesting.

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to :Paolo Amadini from comment #9)
> Comment on attachment 8715423 [details] [diff] [review]
> a.patch
> 
> If you notice, the
> comment in the IDL says "These strings specify the four default contexts...".
Maybe s/default/predefined/ in the idl.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: a.patch

Comment 13

[Andrea Marchesini [:baku]]

Attachment: a.patch

Comment 14

[:Paolo Amadini]

Comment on attachment 8716900 [details] [diff] [review]
a.patch

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

Thanks for the update, looks good!

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +128,5 @@
>  # you can use #1 in your localization as a placeholder for the number.
>  # For example this is the English string with numbers:   
>  # removeSelectedCookied=Remove #1 Selected;Remove #1 Selected
>  removeSelectedCookies=Remove Selected;Remove Selected
> +userContextDefaultLabel = None

No spaces around the equal sign.

nit: I'd prefer this to be called noUserContextLabel but this name is also fine.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9494bdf52cc6

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc1cda7b777c

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for browser chrome failures.

Backouts:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb60f9e8ffe
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a08e77f75c0

Push with failing jobs: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9494bdf52cc6
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=21276152&repo=mozilla-inbound

08:16:48     INFO -  104 INFO TEST-START | browser/components/preferences/in-content/tests/browser_bug705422.js
08:16:48     INFO -  JavaScript error: chrome://browser/content/preferences/cookies.js, line 509: ReferenceError: aItem is not defined
08:16:48     INFO -  TEST-INFO | started process screenshot
08:16:48     INFO -  TEST-INFO | screenshot: exit 0
08:16:48     INFO -  105 INFO checking window state
08:16:48     INFO -  106 INFO Console message: [JavaScript Error: "TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram: "FX_PAGE_LOAD_MS", key: "null"" {file: "resource://gre/modules/TelemetryStopwatch.jsm" line: 297}]
08:16:48     INFO -  107 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Number of cookies match injected cookies -
08:16:48     INFO -  108 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Remove all cookies button has correct state: enabled -
08:16:48     INFO -  109 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Correct number of cookies shown after filter is applied -
08:16:48     INFO -  110 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Remove all cookies button has correct state: enabled -
08:16:48     INFO -  111 INFO TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_bug705422.js | Deleted selected cookie - Got 6, expected 5
08:16:48     INFO -  Stack trace:
08:16:48     INFO -  chrome://mochikit/content/browser-test.js:test_is:979
08:16:48     INFO -  chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug705422.js:runTest:90
08:16:48     INFO -  chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug705422.js:initTest/</<:49
08:16:48     INFO -  chrome://mochikit/content/browser-test.js:testScope/test_executeSoon/<.run:1005
08:16:48     INFO -  JavaScript error: chrome://browser/content/preferences/cookies.js, line 509: ReferenceError: aItem is not defined
08:16:48     INFO -  Not taking screenshot here: see the one that was previously logged
08:16:48     INFO -  112 INFO TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_bug705422.js | Deleted selected two adjacent cookies - Got 6, expected 3
08:16:48     INFO -  Stack trace:
08:16:48     INFO -  chrome://mochikit/content/browser-test.js:test_is:979
08:16:48     INFO -  chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug705422.js:runTest:104
08:16:48     INFO -  chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug705422.js:initTest/</<:49
08:16:48     INFO -  chrome://mochikit/content/browser-test.js:testScope/test_executeSoon/<.run:1005
08:16:48     INFO -  113 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Remove all cookies button has correct state: enabled -
08:16:48     INFO -  114 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Deleted all matching cookies -
08:16:48     INFO -  115 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Remove all cookies button has correct state: disabled -
08:16:48     INFO -  116 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Unmatched cookies remain -
08:16:48     INFO -  117 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Remove all cookies button has correct state: enabled -
08:16:48     INFO -  118 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Deleted all cookies -
08:16:48     INFO -  119 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Zero cookies remain -
08:16:48     INFO -  120 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_bug705422.js | Remove all cookies button has correct state: disabled -
08:16:48     INFO -  121 INFO Console message: [JavaScript Error: "ReferenceError: aItem is not defined" {file: "chrome://browser/content/preferences/cookies.js" line: 509}]
08:16:48     INFO -  gCookiesWindow._getUserContextString@chrome://browser/content/preferences/cookies.js:509:1
08:16:48     INFO -  gCookiesWindow._updateCookieData@chrome://browser/content/preferences/cookies.js:528:35
08:16:48     INFO -  gCookiesWindow.onCookieSelected@chrome://browser/content/preferences/cookies.js:554:5
08:16:48     INFO -  onselect@chrome://browser/content/preferences/cookies.xul:1:1
08:16:48     INFO -  gCookiesWindow.filter@chrome://browser/content/preferences/cookies.js:912:7
08:16:48     INFO -  gCookiesWindow.setFilter@chrome://browser/content/preferences/cookies.js:920:5
08:16:48     INFO -  runTest@chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug705422.js:75:5
08:16:48     INFO -  initTest/</<@chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug705422.js:49:70
08:16:48     INFO -  testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:1005:9
08:16:48     INFO -  122 INFO Console message: [JavaScript Error: "ReferenceError: aItem is not defined" {file: "chrome://browser/content/preferences/cookies.js" line: 509}]
08:16:48     INFO -  gCookiesWindow._getUserContextString@chrome://browser/content/preferences/cookies.js:509:1
08:16:48     INFO -  gCookiesWindow._updateCookieData@chrome://browser/content/preferences/cookies.js:528:35
08:16:48     INFO -  gCookiesWindow.onCookieSelected@chrome://browser/content/preferences/cookies.js:554:5
08:16:48     INFO -  onselect@chrome://browser/content/preferences/cookies.xul:1:1
08:16:48     INFO -  onxblclick@chrome://global/content/bindings/tree.xml:1156:13
08:16:48     INFO -  doApply@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:143:10
08:16:48     INFO -  apply@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:208:30
08:16:48     INFO -  synthesizeMouseAtPoint@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:339:7
08:16:48     INFO -  synthesizeMouse@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:291:10
08:16:48     INFO -  runTest@chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug705422.js:100:5
08:16:48     INFO -  initTest/</<@chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug705422.js:49:70
08:16:48     INFO -  testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:1005:9

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60e27c59a375

Comment 19

[Tanvi Vyas[:tanvi]]

(In reply to :Paolo Amadini from comment #14)
> Comment on attachment 8716900 [details] [diff] [review]
> a.patch
> 
> Review of attachment 8716900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the update, looks good!
> 
> ::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
> @@ +128,5 @@
> >  # you can use #1 in your localization as a placeholder for the number.
> >  # For example this is the English string with numbers:   
> >  # removeSelectedCookied=Remove #1 Selected;Remove #1 Selected
> >  removeSelectedCookies=Remove Selected;Remove Selected
> > +userContextDefaultLabel = None
> 
> No spaces around the equal sign.
> 
> nit: I'd prefer this to be called noUserContextLabel but this name is also
> fine.

defaultUserContextLabel is better, since we refer to the regular browsing experience as the default context.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28b8b4aec235

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60e27c59a375

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28b8b4aec235