(In reply to Ms2ger from comment #27)
> Comment on attachment 578799 [diff] [details] [review]
> WIP - experimental prototype

Again, thanks very much for jumping on this patch and giving me feedback, it's greatly appreciated :)
 
> >--- a/content/base/public/nsIDocument.h
> >+++ b/content/base/public/nsIDocument.h
> >   /**
> >+   * Set the sandbox flags for this document. This should only happen at 
> >+   * at document load, the flags are immutable after being set at load time.
> >+   * @ see nsDocShell.idl for the possible flags
> 
> nsDocShell.idl doesn't exist. (Three times or so.)

oops, sorry, will fix.

> >--- a/content/base/src/nsFrameLoader.cpp
> >+++ b/content/base/src/nsFrameLoader.cpp
> >+  nsCOMPtr<nsIDocShellTreeItem> curDocShellItem(do_QueryInterface(mDocShell));
> 
> 'nsCOMPtr<nsIDocShellTreeItem> curDocShellItem =
> do_QueryInterface(mDocShell);'

thanks, will fix.

> >+  // is this an <iframe> with a sandbox attribute or a frame whose parent
> >+  // is sandboxed ?  
> >+  if (mOwnerContent->IsHTML(nsGkAtoms::iframe) && 
> >+      (parentSandboxFlags || mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::sandbox, sandboxAttr))) {
> 
> Instead of this, add a nsHTMLIFrameElement*
> nsHTMLIFrameElement::FromContent(nsIContent*) and expose a method on
> nsHTMLIFrameElement that returns the flags, and move the code you added to
> nsContentUtils there. Also use it in nsHTMLIFrameElement::AfterSetAttr.

ok, i took a look at an example FromContent() on another element and I think I understand
what you'd like me to do here. I put the code to parse the flags etc in nsContentUtils so it could be used by CSP Sandbox (bug 671389) - does that change your opinion at all on how to rework this ? (that's why i didn't put the flag parsing in the iframe element originally). We also discussed support @sandbox on <frame>, which it seemed like there was interest in (just as an fyi). 

> >+    PRUint32 sandboxFlags = 0;
> >+  
> >+    nsCOMPtr<nsIDOMHTMLIFrameElement> iframe = do_QueryInterface(mOwnerContent);
> >+  
> >+    if (iframe) {
> >+      nsCOMPtr<nsIDOMDOMSettableTokenList> sandbox;
> >+    
> >+      nsresult result = iframe->GetSandbox(getter_AddRefs(sandbox));
> >+      // TODO : what to do if this fails ? (see below also) 
> >+    
> >+      sandboxFlags = nsContentUtils::ParseSandboxAttributeToFlags(sandbox);
> >+      // TODO: what to do if this fails ? the content was intended to be sandboxed
> >+      // and it may not be - maybe fail closed (all restrictions) ??? 
> >+    }
> >+     
> >+    if (parentSandboxFlags) { 
> >+      // do the intersection with the current flags in the docshell!
> 
> Doesn't sandboxFlags &= parentSandboxFlags do the right thing, and if not,
> why not?

yeah. as i mentioned above, i think &= is the right thing here. i will fix this. 

> >+  }
> >+
> >+  else { 
> 
> } else {
> 
> >--- a/content/html/content/src/nsHTMLIFrameElement.cpp
> >+++ b/content/html/content/src/nsHTMLIFrameElement.cpp
> >+  virtual nsresult AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> >+                                  const nsAString* aValue,
> >+                                  bool aNotify);
> 
> Indentation

will fix, sorry.

> >--- a/docshell/base/nsDocShell.cpp
> >+++ b/docshell/base/nsDocShell.cpp
> >+    // If the content is sandboxed, force the passed in owner (which should be 
> >+    // a null principal) to be set on the channel, UNLESS the sandbox allow-same-domain flag is also
> >+    // set. In that case, allow the content to asssume its "natural domain" as it
> >+    // normally would. 
> >+    if ((mSandboxFlags & SANDBOX_FLAGS_SANDBOXED) && 
> >+        (mSandboxFlags & SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) != SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) {
> >+      nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true);
> >+    }
> >+    else {
> >+      nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false);
> >+    }
> 
> nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true,
>                                   (mSandboxFlags & SANDBOX_FLAGS_SANDBOXED)
> && 
>                                   (mSandboxFlags &
> SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) != SANDBOX_FLAGS_ALLOW_SAME_DOMAIN);

thanks, much cleaner, will fix.

> >--- a/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
> >+++ b/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
> >-
> >+  
> >+  // HTML5 sandbox attribute
> 
> This interface is specified in HTML, so no need for the comment. Also, leave
> the two empty lines between the HTML-specified members and the Mozilla
> extensions.

Thanks, will fix.

> Also check the entire patch for tabs and trailing whitespace, and make sure
> the comments you add are full sentences (including capital letters and full
> stops).

Will do - sorry, this patch wasn't totally cleaned up, thanks for the sanity check on the code, i will clean it all up ! :)