Bug 802895 disables the "This Frame" context menu item for srcdoc iframes. This was done to prevent "show only this frame" etc. displaying arbitrary web content in the about protocol that was previously reserved for browser-internal/addon pages A consequence of this is that it will hide the "view image"/"view video" context menu items for images/videos in srcdoc frames, which probably isn't optimal. To fix that we'd probably need to track inFrame and inSrcdocFrame separately, so that srcdoc iframes retain a "this frame" sub-menu, but without the offending menu options.

This hides the frame entries that are undesirable or do not make sense for srcdoc iframes. The question is, whether suppressing the ability to open srcdoc iframes in isolation is still undesirable. Since the decision to suppress them was made, new identity elements have been introduced to indicate secure Firefox-internal pages. If the srcdoc document in isolation displayed the standard grey globe icon (with an about:srcdoc URI), would it be clear that it is untrusted, arbitrary content?

Comment on attachment 778431 [details] [diff] [review] menu Someone else (bz?) should review the nsContentUtils change. At first glance it seems odd to be special-casing those particular schemes (perhaps it should be a check for nested URIs in general, using the innermost if present?). I'll delegate the nsContextMenu reviews to mconley.

Comment on attachment 778431 [details] [diff] [review] menu Requesting review for the nsContentUtils portion of the patch. The check is necessary to allow viewing the source of a srcdoc document to work in debug builds.

Comment on attachment 778431 [details] [diff] [review] menu r=me on the contentutils change.

Comment on attachment 778431 [details] [diff] [review] menu Review of attachment 778431 [details] [diff] [review]: ----------------------------------------------------------------- r=me with my concerns resolved. Thanks! ::: browser/base/content/test/test_contextmenu.html @@ +1060,5 @@ > select_inputtext = subwindow.document.getElementById("test-select-input-text"); > select_inputtext_password = subwindow.document.getElementById("test-select-input-text-type-password"); > plugin = subwindow.document.getElementById("test-plugin"); > longdesc = subwindow.document.getElementById("test-longdesc"); > + srcdoc = subwindow.document.getElementById("test-srcdoc"); Unless I'm mistaken, srcdoc should be added to the list of variables here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_contextmenu.html?force=1#983 ::: content/base/src/nsContentUtils.cpp @@ +6105,5 @@ > // srcdoc loads, but the URI is non-resolvable in cases where it is not). > if (aForceOwner) { > nsAutoCString uriStr; > aURI->GetSpec(uriStr); > + if(!uriStr.EqualsLiteral("about:srcdoc") && Trailing whitespace.

Changes applied

Comment on attachment 779686 [details] [diff] [review] patch You can just use the checkin-needed bug keyword in the future :)

Backed out for mochitest-bc orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/5005e426f324 https://tbpl.mozilla.org/php/getParsedLog.php?id=25718230&tree=Mozilla-Inbound

False alarm, these were due to bug 890690 which has now been backed out. This can reland - sorry for the churn!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1514cf837dbe Sorry again for the goof :(

Doesn't seem to have an impact for developers, no doc needed. Reset the keyword with an explanation if you disagree.