1276412 - Enable containers in Nightly only

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Enable containers for use in Nightly only starting in Firefox 50 (i.e. #ifdef NIGHTLY).  Marking this as dependent on a list of implementation bugs.  We may decide that a few of those (ex: favicon loads) don't actually block enabling on nightly.

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Bug1276412.patch

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19ac9881aa5e
Enable experimental containers feature for Nighlty to test the OriginAttributes platform work and get some validation on the idea. r=past

Comment 3

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

Backed out for bc7 failures on OSX and Windows in browser_contextmenu.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/79747869072b3b72c0484e79238d05bfb8efb1ac

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=19ac9881aa5eff2be6dbd50723fc36a110222715
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30116152&repo=mozilla-inbound

07:08:40     INFO -  445 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #0 (context-openlinkintab) name -
07:08:40     INFO -  446 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #0 (context-openlinkintab) enabled state -
07:08:40     INFO -  447 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_contextmenu.js | checking item #1 (context-openlink) name - Got context-openlinkinusercontext-menu, expected context-openlink

Looks like the test needs just an update.

Comment 4

[Tanvi Vyas[:tanvi]]

Attachment: Bug1276412-test.patch

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8762718 [details] [diff] [review]
Bug1276412-test.patch

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

I think this fix is ok. I'm not a reviewer for this component but I take the responsibility to give a r+.

Comment 6

[Tanvi Vyas[:tanvi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=192b06312a86

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Bug1276412-test-06-14-16.patch

Comment 8

[Tanvi Vyas[:tanvi]]

Still one failure with this patch:
2100 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_contextmenu.js | menuitem context-copy has same accesskey as context-openlinkinusercontext-menu - 

Stack trace:
    chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:getVisibleMenuItems:70
    chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:checkMenu:187
    chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:checkContextMenu:133
    chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:test_contextmenu:305

Comment 9

[Tanvi Vyas[:tanvi]]

https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#288
<!ENTITY copyCmd.accesskey          "C"> 

https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#460
<!ENTITY openLinkCmdInContainerTab.accesskey "C">

Comment 10

[Tanvi Vyas[:tanvi]]

Attachment: Bug1276412-test-06-14-16B.patch

Changed the access key to "z", since most letters were taken.

Gijs, can you take a look at this patch?  I'm not sure why I need to put the blank line after the menu item, but without it, the patch fails. Maybe it is because the Open Link In Container Tab has a submenu.

I need to land this as soon as possible, so if you could review sooner than later, that would be very appreciated!  Thank you!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a82fa07a9ff

Comment 11

[Tanvi Vyas[:tanvi]]

Attachment: Bug1276412-test-06-14-16C.patch

Added some comments.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8762754 [details] [diff] [review]
Bug1276412-test-06-14-16C.patch

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

Looks good to me.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c488fb21ff
Fix context menu test that doesn't account for New Container Tab option. r=past
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce3120ab50c
Enable experimental containers feature for Nighlty to test the OriginAttributes platform work and get some validation on the idea. r=past

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96d250dc0d85
Fix a typo in a comment, r=me

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/027adaea2de2
Fix a typo in a comment - part 2, r=me

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9c488fb21ff
https://hg.mozilla.org/mozilla-central/rev/cce3120ab50c
https://hg.mozilla.org/mozilla-central/rev/96d250dc0d85
https://hg.mozilla.org/mozilla-central/rev/027adaea2de2

Comment 17

[Francesco Lodolo [:flod]]

The text fix makes en-US use an external accesskey (a character that is not available in the original string), so the label will be displayed as "Open Link in New Container Tab (z)".

No other characters available? I don't see any explanation for that in the bug.

Comment 18

[Tanvi Vyas[:tanvi]]

(In reply to Francesco Lodolo [:flod] from comment #17)
> The text fix makes en-US use an external accesskey (a character that is not
> available in the original string), so the label will be displayed as "Open
> Link in New Container Tab (z)".
> 
> No other characters available? I don't see any explanation for that in the
> bug.

So many of the access keys were already taken; every letter I thought of.  So I went with z, but we can easily change this to another one if you have a suggestion.  If this is something we need to change sooner than later, please let me know and I will have someone look into it.

Comment 19

[Francesco Lodolo [:flod]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #18)
> So many of the access keys were already taken; every letter I thought of. 
> So I went with z, but we can easily change this to another one if you have a
> suggestion.  If this is something we need to change sooner than later,
> please let me know and I will have someone look into it.

Not really my call, but I think it's good to fix it, especially if you plan to enable the feature by default at some point.

Comment 20

[Tanvi Vyas[:tanvi]]

(In reply to Francesco Lodolo [:flod] from comment #19)
> (In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #18)
> > So many of the access keys were already taken; every letter I thought of. 
> > So I went with z, but we can easily change this to another one if you have a
> > suggestion.  If this is something we need to change sooner than later,
> > please let me know and I will have someone look into it.
> 
> Not really my call, but I think it's good to fix it, especially if you plan
> to enable the feature by default at some point.

Okay, we will file a followup to get it fixed in 50.  Thanks!

Comment 21

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Doesn't need uplift.

Comment 22

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #20)
> Okay, we will file a followup to get it fixed in 50.  Thanks!

https://bugzilla.mozilla.org/show_bug.cgi?id=1285853