Last Comment Bug 802895 - implement <iframe srcdoc=''> to allow document content in iframe to be specified inline
: implement <iframe srcdoc=''> to allow document content in iframe to be specif...
Status: RESOLVED FIXED
[mentor=bz][lang=C++]
: dev-doc-complete, feature
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla25
Assigned To: James Kitchener (:jkitch)
: Ada [:adalucinet]
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 893537
Blocks: html5 html5test 885289
  Show dependency treegraph
 
Reported: 2012-10-17 17:24 PDT by David Baron :dbaron: ⌚️UTC-7
Modified: 2013-11-10 15:22 PST (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
25+


Attachments
patch (1.88 KB, patch)
2012-10-23 05:58 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
wip (19.56 KB, patch)
2012-11-16 04:58 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
wip v2 (28.50 KB, patch)
2012-11-25 05:16 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
wip v3 (39.34 KB, patch)
2012-12-01 04:56 PST, James Kitchener (:jkitch)
bzbarsky: feedback+
Details | Diff | Splinter Review
wip v4 (29.60 KB, patch)
2013-02-08 02:59 PST, James Kitchener (:jkitch)
bzbarsky: feedback+
Details | Diff | Splinter Review
testcase (2.61 KB, text/html)
2013-02-28 01:44 PST, James Kitchener (:jkitch)
no flags Details
add srcdoc attribute to nsIChannel (13.85 KB, patch)
2013-03-02 01:57 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
add srcdoc attribute to nsIChannel v2 (14.13 KB, patch)
2013-03-02 02:02 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Add srcdoc to about protocol handler (5.74 KB, patch)
2013-03-03 02:29 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Changes to nsI?Document to support srcdoc (6.29 KB, patch)
2013-03-13 05:42 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Changes to nsDocShell to support srcdoc (37.66 KB, patch)
2013-03-13 05:44 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Changes to HTML related code to support srcdoc iframes (8.92 KB, patch)
2013-03-13 05:46 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Testcases for srcdoc iframes (4.01 KB, patch)
2013-03-13 06:20 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 1: add srcdoc attribute to nsIChannel v2 (18.00 KB, patch)
2013-03-31 06:06 PDT, James Kitchener (:jkitch)
bzbarsky: review-
Details | Diff | Splinter Review
Part 2: Add about:srcdoc to about protocol handlers (5.74 KB, patch)
2013-03-31 06:10 PDT, James Kitchener (:jkitch)
bzbarsky: review-
Details | Diff | Splinter Review
Part 3: Changes to nsI?Document to support srcdoc (6.29 KB, patch)
2013-03-31 06:20 PDT, James Kitchener (:jkitch)
bzbarsky: review+
Details | Diff | Splinter Review
Part 4: add srcdoc support to docshell and SHEntry (43.81 KB, patch)
2013-03-31 06:21 PDT, James Kitchener (:jkitch)
bzbarsky: review-
Details | Diff | Splinter Review
Part 5: Allow sessionrestore to work on srcdoc (1.77 KB, patch)
2013-03-31 06:23 PDT, James Kitchener (:jkitch)
bzbarsky: review+
ttaubert: review+
Details | Diff | Splinter Review
Part 6: HTML related changes to about srcdoc (10.72 KB, patch)
2013-03-31 06:25 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 7: Testcases of srcdoc iframes (3.28 KB, patch)
2013-03-31 06:27 PDT, James Kitchener (:jkitch)
bzbarsky: review+
Details | Diff | Splinter Review
Part 6: HTML related changes to about srcdoc v2 (11.15 KB, patch)
2013-04-01 05:49 PDT, James Kitchener (:jkitch)
bzbarsky: review+
hsivonen: review-
Details | Diff | Splinter Review
Part 1: add srcdoc attribute to nsIChannel v3 (8.98 KB, patch)
2013-04-28 02:09 PDT, James Kitchener (:jkitch)
bzbarsky: review+
benjamin: superreview+
Details | Diff | Splinter Review
Part 2: Add about:srcdoc to about protocol handlers (4.38 KB, patch)
2013-04-28 02:13 PDT, James Kitchener (:jkitch)
bzbarsky: review+
Details | Diff | Splinter Review
Part 3: Changes to nsI?Document to support srcdoc v2 (9.81 KB, patch)
2013-04-28 02:31 PDT, James Kitchener (:jkitch)
bzbarsky: review+
Details | Diff | Splinter Review
Part 4: Docshell changes interdiff (42.43 KB, patch)
2013-04-28 02:38 PDT, James Kitchener (:jkitch)
bzbarsky: feedback+
Details | Diff | Splinter Review
Part 4: add srcdoc support to docshell and SHEntry v2 (35.60 KB, patch)
2013-04-28 02:43 PDT, James Kitchener (:jkitch)
bzbarsky: review+
Details | Diff | Splinter Review
Part 5: Allow sessionrestore to work on srcdoc v2 (1.83 KB, patch)
2013-04-28 02:45 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 6: HTML related changes to about srcdoc v3 (9.55 KB, patch)
2013-04-28 02:49 PDT, James Kitchener (:jkitch)
bzbarsky: review-
hsivonen: feedback+
Details | Diff | Splinter Review
Part 7: srcdoc iframes no longer considered frames in context menu (1.44 KB, patch)
2013-04-28 02:57 PDT, James Kitchener (:jkitch)
gavin.sharp: review+
Details | Diff | Splinter Review
Part 7: Testcases of srcdoc iframes v2 (6.61 KB, patch)
2013-04-28 02:59 PDT, James Kitchener (:jkitch)
bzbarsky: review-
Details | Diff | Splinter Review
WIP changes to parser (22.56 KB, patch)
2013-05-19 03:01 PDT, James Kitchener (:jkitch)
hsivonen: feedback-
Details | Diff | Splinter Review
Part 1: add srcdoc attribute to nsIChannel v4 (11.63 KB, patch)
2013-06-18 06:53 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 3: Changes to nsI?Document to support srcdoc v3 (8.06 KB, patch)
2013-06-18 06:56 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 4: add srcdoc support to docshell and SHEntry v3 (36.63 KB, patch)
2013-06-18 07:05 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 6: HTML related changes to about srcdoc v4 (10.04 KB, patch)
2013-06-18 07:07 PDT, James Kitchener (:jkitch)
bzbarsky: review-
Details | Diff | Splinter Review
Part 7: srcdoc iframes no longer considered frames in context menu v2 (1.45 KB, patch)
2013-06-18 07:08 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 8 parser changes (80.67 KB, patch)
2013-06-18 07:17 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Changes to the htmlparser subrepository (9.99 KB, patch)
2013-06-18 07:21 PDT, James Kitchener (:jkitch)
hsivonen: review+
Details | Diff | Splinter Review
Part 9: Testcases of srcdoc iframes v3 (7.36 KB, patch)
2013-06-18 07:27 PDT, James Kitchener (:jkitch)
bzbarsky: review+
Details | Diff | Splinter Review
part 1: channel and nsNetUtil changes (11.88 KB, patch)
2013-06-20 05:00 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 3: nsI?Document changes (8.05 KB, patch)
2013-06-20 05:01 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
part 4: docshell and SHEntry changes (36.55 KB, patch)
2013-06-20 05:03 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 5: sessionrestore changes (1.75 KB, patch)
2013-06-20 05:04 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 6: HTML related changes (11.80 KB, patch)
2013-06-20 05:06 PDT, James Kitchener (:jkitch)
bzbarsky: review+
Details | Diff | Splinter Review
Part 8: parser changes (77.46 KB, patch)
2013-06-20 05:08 PDT, James Kitchener (:jkitch)
hsivonen: review+
Details | Diff | Splinter Review
Part 9: tests (7.35 KB, patch)
2013-06-20 05:09 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 6: HTML related changes (12.57 KB, patch)
2013-06-22 06:27 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 6: HTML related changes (12.65 KB, patch)
2013-06-23 01:56 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 9: tests (7.34 KB, patch)
2013-06-23 01:57 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
part 1: channel and nsNetUtil changes (11.88 KB, patch)
2013-06-28 18:42 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
part 4: docshell and SHEntry changes (36.37 KB, patch)
2013-06-28 18:43 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 9: tests (7.36 KB, patch)
2013-06-28 18:44 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 2012-10-17 17:24:45 PDT
This bug is to track implementing the srcdoc attribute of the iframe element, which HTML5 specifies to allow authors to write the contents of the document in the iframe inline within the parent document.

(I'm not expressing an opinion on whether we should do this; it's just the only thing on html5test.com that we didn't have an existing bug report on, so I figured I'd file it.)

See:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-srcdoc
http://www.w3.org/TR/html5/the-iframe-element.html#attr-iframe-srcdoc
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-10-17 17:58:16 PDT
I do think this is worth doing.  I'm happy to mentor someone who wants to work on this, as needed.
Comment 2 James Kitchener (:jkitch) 2012-10-18 17:37:59 PDT
I'm interested in working on this.  Could you point out which files need to be modified and how to get started?
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-10-18 17:43:37 PDT
The right place to start is probably nsFrameLoader.cpp and probably nsHTMLIFrameElement.cpp, which are what decide when to load the src attribute right now.

That said, you may need to create some infrastructure in nsDocShell.cpp to do the actual load.  Though it's possible that it already has APIs that are good enough for this.

What else will need changing depends on what the differences are between scrdoc and just having a data: URI src in terms of behavior, if any.
Comment 4 James Kitchener (:jkitch) 2012-10-23 05:58:42 PDT
Created attachment 674213 [details] [diff] [review]
patch

This converts a srcdoc argument into a data URI.  I looked into alternatives, but it appears that it will require a lot of changes as everything expects a URI as an argument.

It is currently hardcoded to use charset=UTF-8.  Do I need to detect/inherit the appropriate encoding?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-10-23 06:48:20 PDT
Comment on attachment 674213 [details] [diff] [review]
patch

This gives clearly spec-incorrect behavior for the resulting document.  Most importantly, its URI and base URI are wrong, which will break uses of relative URIs in the document.

The URI should be an "about:srcdoc" URI.

The base URI should be that of the parent document.  Note that this means that the child needs to know its a srcdoc document.

We also need to know whether a document is a srcdoc document for purposes of determining referrers.

And location.reload on a window containing a srcdoc document needs to do something a bit different than a normal reload() call.

And srcdoc documents are always in standards mode per spec.

Basically, look for "srcdoc document" in http://www.whatwg.org/specs/web-apps/current-work/

I agree that there will probably need to be some core work to make all of this work (in particular, we'll need to communicate the srcdoc document state to the parser to make the standards mode thing work).
Comment 6 James Kitchener (:jkitch) 2012-10-29 04:17:30 PDT
Clearing assignment, I've made no progress.
Comment 7 James Kitchener (:jkitch) 2012-11-08 01:56:47 PST
So far I've managed to detect when srcdoc is invoked and then to set the URI to about:srcdoc.

As far as I can tell, the next step is to work out how data URIs are handled and then how to call it directly.

In this task, I've made no progress.  I've looked through nsDocShell, but so far I have been unable to locate where data URIs are treated differently.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-11-08 08:15:40 PST
When you NS_NewChannel from a data: URI, you get an nsDataChannel.  Then you call AsyncOpen on that.

But I don't think you necessarily need to pretend like this is a data URI.  Have you looked into using the loadStream method on nsIDocShell?
Comment 9 James Kitchener (:jkitch) 2012-11-16 04:58:53 PST
Created attachment 682436 [details] [diff] [review]
wip

Attached is my progress so far.  Very much a work in progress.  Reloading and relative links do not work.

I've worked out how to use LoadStream and I'm currently calling it from within nsDocShell::LoadURI.  My reason for doing so is that LoadStream lacks awareness of referrers, sandboxes etc, which would need to be duplicated.  Is this reasonable?

I still need to add additional arguments for baseURI and character encoding.  Am I correct in saying that the srcdoc iframe inherits the encoding from the parent document?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-11-19 22:09:44 PST
Hmm.  I'd have to look through the code carefully to figure out whether calling LoadStream like that is ok.

As for encoding... What does the spec say?  On the face of it, scrdoc starts out as a Unicode string, so there should be no encoding issues, except insofar as the document encoding is used for post-load stuff like URIs.
Comment 11 James Kitchener (:jkitch) 2012-11-21 20:01:27 PST
I've since found the answer in section 4.2.5.5 of the whatwg spec.

"If the document is an iframe srcdoc document, the document must not have a character encoding declaration. (In this case, the source is already decoded, since it is part of the document that contained the iframe.)"
Comment 12 James Kitchener (:jkitch) 2012-11-22 17:03:53 PST
Calling LoadStream isn't ok.  It bypasses the sandboxing and Content Security Policy code in DoURILoad.

Modifying DoURILoad to accept a srcdoc argument and create an InputStreamChannel instead of a normal one appears to work.
Comment 13 James Kitchener (:jkitch) 2012-11-25 05:16:34 PST
Created attachment 684949 [details] [diff] [review]
wip v2

Improvements since the last patch:
*DoURILoad now executes, so sandboxing and anything else present should still work (nothing has been conditionally disabled, so there may be some unnecessary code execution)
*Can now set the URI to about:srcdoc without triggering security errors.  To do this, about:srcdoc was registered as an about:page that resolves to an error page.  Given the specifications state that it should be an unresolvable URI, is returning an error page acceptable?  
If not, the alternative would be to bypass the security checks when an about:srcdoc URI is accessed.

Current work:
I'm having difficulty setting the correct baseURI.  As far as I can tell, setting the baseURI is an action performed on an nsIDocument, but I haven't been able to find where the iframe's document is created or located
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2012-11-25 05:38:16 PST
Comment on attachment 684949 [details] [diff] [review]
wip v2

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

Some nits:

::: content/base/src/nsFrameLoader.cpp
@@ +328,3 @@
>  
> +  mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc, srcdoc);
> +  if (srcdoc.IsEmpty()) {

This probably needs to check if the attribute is present, rather than if the value is empty. You can write that as

if (!mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc, srcdoc)) {

::: content/base/src/nsFrameLoader.h
@@ +385,5 @@
>    nsCOMPtr<nsIURI> mURIToLoad;
>    mozilla::dom::Element* mOwnerContent; // WEAK
>  
> +  nsString mSrcdoc;
> +  

Please remove the trailing whitespace from this line

::: docshell/base/nsDocShell.cpp
@@ +4498,5 @@
>  
>      return InternalLoad(errorPageURI, nullptr, nullptr,
>                          INTERNAL_LOAD_FLAGS_INHERIT_OWNER, nullptr, nullptr,
>                          nullptr, nullptr, LOAD_ERROR_PAGE,
> +                        nullptr, true, nullptr, nullptr,nullptr);

Space after the last comma

@@ +8701,5 @@
>              // Do this asynchronously
>              nsCOMPtr<nsIRunnable> ev =
>                  new InternalLoadEvent(this, aURI, aReferrer, aOwner, aFlags,
>                                        aTypeHint, aPostData, aHeadersData,
> +                                      aLoadType, aSHEntry, aFirstParty,aSrcdoc);

Same here
Comment 15 James Kitchener (:jkitch) 2012-11-28 05:41:51 PST
Resolving to an error page will give incorrect behaviour for
<iframe src="about:srcdoc">

I'll look into giving the security checks awareness of the srcdoc attribute and only bypassing the minimum required.
Comment 16 James Kitchener (:jkitch) 2012-12-01 04:56:06 PST
Created attachment 687384 [details] [diff] [review]
wip v3

Improvements:
This creates an exception within the security manager to permit a non resolvable about URI.  I have tried to be specific as possible.  There is an explicit opt-in flag and it will only work if the URI is "about:srcdoc".

It now correctly handles empty srcdoc attributes.



Left to do:

Ensuring compliance with the standards.
Properly setting origin, effective script origin, referrer, BaseURI etc (there is an attempt at the last one, but I doubt it works).

I am having difficulty identifying where and how to make the necessary changes.
Comment 17 Josh Matthews [:jdm] 2013-01-18 02:58:32 PST
Comment on attachment 687384 [details] [diff] [review]
wip v3

Boris, do you have any suggestions re: comment 16?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2013-01-18 09:50:31 PST
Comment on attachment 687384 [details] [diff] [review]
wip v3

The right place to set the base URI is interesting.  Per spec right now it sounds like this should return the base URL of the parent document, right?  So what needs to happen is that documents need to know when they're srcdoc documents and nsIDocument::GetBaseURI needs to branch on that and do the right thing.

Note that similar things need to happen for places where documents are used for referrer determination during fetch...  It might be good to add a getter on nsIDocument that returns the parent document for srcdoc docs and the document itself otherwise or something.

The right place to set the origin and effective script origin is by setting them on the channel when the load starts.  There's existing code to do this for data: and javascript: and about:blank loads; we just need to do it in the srcdoc case too.

I'm not sure what I think of the about:srcdoc setup in this patch.  It might make more sense to just make it a normal about: URI that can be linked to for CheckLoadURI purposes but then make any asyncOpen attempts for it throw....
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2013-01-18 09:51:05 PST
And sorry for the lack of response earlier.  Please use the "need additional information from" checkbox thing if you have questions for me!
Comment 20 James Kitchener (:jkitch) 2013-01-30 03:07:47 PST
I'm having trouble working out how/where/when the child nsIDocument is created and how to reference it from within nsFrameLoader and nsDocShell.


I've found code snippets within the files that return a nsCOMPtr<nsIDocument>, but I don't know if they return the desired one.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2013-01-30 07:25:16 PST
The child document is created when nsDSURIContentListener::DoContent calls CreateContentViewer.  That ends up calling into the document loader factory, which lands us in nsContentDLF::CreateInstance which creates the actual document.

Note that if the channel knows it's a srcdoc then the document's Reset() code can just get that information from the channel.

In terms of getting a document in docshell, mContentViewer->GetDocument() if mContentViewer is not null.

In frameloader, you there are several options, but the simplest is probably doing what nsFrameLoader::CreateStaticClone does, going via the docshell.
Comment 22 James Kitchener (:jkitch) 2013-02-08 02:59:19 PST
Created attachment 711757 [details] [diff] [review]
wip v4

Progress:
*Security manager hack has been removed and about:srcdoc is present for security manager purposes, yet unresolvable for AsyncOpen.
*Referrer and base URI now follow specifications (albeit untested)
*I think origin/effective script origin follows specifications, but I'm not convinced (again untested).

Could you please confirm whether nsContentUtils::SetUpChannelOwner is responsible for setting the origin property?

Remaining Tasks:
* Tests.  Where would be the most appropriate location for the tests to reside?
* View source without a selection doesn't work.
* Navigation is broken.  Click on a link within the iframe, then click back and it tries to load the url about:srcdoc, rather than parse the srcdoc attribute.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2013-02-09 18:50:37 PST
Sorry for the lag here; I blame the weather and trying to get to Boston...

I promise I'll comment on this no later than Monday.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2013-02-10 07:26:12 PST
SetUpChannelOwner sets up the .owner for the channel, for sure.  The security context that's used then is the .owner if present, else derived from the channel's URI and whatnot.  So the change to SetUpChannelOwner is a reasonable approach.

The change to GetReferrer seems wrong to me.  At this point the nsIDocument version is what's used from web script, so you want to be changing that one.

Tests should probably go in content/base/test or content/html/content/test.  I'd somewhat prefer the former.

View source is broken presumably because it's using the URI...  Need to think about this a bit.

Similar for history navigation.  I wonder whether using wyciwyg here like we do for document.open() is the way to go; conceptually srcdoc and document.open/write are somewhat similar...  Olli, thoughts?  That _might_ also make view-source work, by the way.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2013-02-10 07:30:11 PST
Comment on attachment 711757 [details] [diff] [review]
wip v4

Having srcdoc-related members on docshell is a bit odd, since docshells live across multiple documents.  I would much rather we stored that information on the channel or some other document-specific object.  Same for the frameloader.
Comment 26 Olli Pettay [:smaug] 2013-02-12 10:42:59 PST
srcdoc is closer to data: urls than document.open(). Why would you use wyciwyg for srcdoc?
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2013-02-12 10:56:30 PST
Because we can't stuff the data into the URI in this case: the URI is fixed by the spec.

Well, I suppose we could stuff it in there but not serialize it when you call various getters on the URI.  That would require a new URI class.
Comment 28 James Kitchener (:jkitch) 2013-02-28 01:44:18 PST
Created attachment 719399 [details]
testcase

Testcase to assist with debugging for a non-empty origin (I've yet to integrate my debugger with the mochitest setup).

It will be converted into a mochitest at some stage.
Comment 29 James Kitchener (:jkitch) 2013-03-02 01:57:15 PST
Created attachment 720257 [details] [diff] [review]
add srcdoc attribute to nsIChannel

I'm splitting the patch into multiple sub-patches as it was starting to get a little unwieldy.

The attached patch adds the ability to store srcdoc information in the channel.  It currently stores the entire srcdoc string, but I'll change it to a bool if I determine the full information isn't necessary.
Comment 30 James Kitchener (:jkitch) 2013-03-02 02:02:50 PST
Created attachment 720259 [details] [diff] [review]
add srcdoc attribute to nsIChannel v2

This one actually compiles.
Comment 31 James Kitchener (:jkitch) 2013-03-03 02:29:26 PST
Created attachment 720387 [details] [diff] [review]
Add srcdoc to about protocol handler

adds srcdoc to the list of about pages

It exists for security manager purposes yet is non-resolvable should anyone try to actually load it.
Comment 32 James Kitchener (:jkitch) 2013-03-13 05:42:56 PDT
Created attachment 724385 [details] [diff] [review]
Changes to nsI?Document to support srcdoc

The change in mChannel's location will be reverted if storing the entire srcdoc string isn't necessary.
Comment 33 James Kitchener (:jkitch) 2013-03-13 05:44:39 PDT
Created attachment 724387 [details] [diff] [review]
Changes to nsDocShell to support srcdoc

Now rewritten to embed information within an nsIChannel instance.
Comment 34 James Kitchener (:jkitch) 2013-03-13 05:46:23 PDT
Created attachment 724388 [details] [diff] [review]
Changes to HTML related code to support srcdoc iframes

Improvements since wip v4.

No longer stores srcdoc string.
Now reports srcdoc support on html5test.com.
Comment 35 James Kitchener (:jkitch) 2013-03-13 06:20:45 PDT
Created attachment 724394 [details] [diff] [review]
Testcases for srcdoc iframes

All patches have now been uploaded.

Do you know of a way I can set a non-empty referrer for the parent document from within a mochitest?  The current test compares two empty referrers and returns success, but that doesn't test for non-empty referrer propagation.

Current work:
There are still a few corner cases to address.  I'm currently looking into one involving session history.

Steps to reproduce:
Create a srcdoc iframe with a link pointing to another site.
Click on the link
Click back.

It should load the srcdoc page again.  What it actually does is display an error page.

I've identified that the session history entry (nsISHEntry) needs to be aware of srcdoc documents.  The brute force approach is to embed the entire srcdoc string within the entry and updating session restore accordingly.  Once people start using srcdoc iframes, this has the potential to add considerable bloat to session history.

Can you think of a better way?
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2013-03-13 23:20:19 PDT
> Do you know of a way I can set a non-empty referrer for the parent document from within a
> mochitest? 

Probably the simplest way is to load whatever document you want a nonempty referrer for in a subframe, right?  Then the referrer should be the URI of the document the subframe is in.

Embedding the scrdoc string in the session history entry is probably the way to go; it's no worse than what happens when people use a data: URI already...  It's tempting to try to think of a way to encapsulate the srcdoc data inside the URI somehow, but I suspect that way ends up lying madness.  So probably better to just store it explicitly.
Comment 37 James Kitchener (:jkitch) 2013-03-31 06:06:38 PDT
Created attachment 731635 [details] [diff] [review]
Part 1: add srcdoc attribute to nsIChannel v2

Adds srcdoc support to the various nsIChannel implementations.  This touches some very different places, so may need to be split up further for review.

Also adds a new function to nsIViewSourceChannel to allow the embedded channel and URI to be replaced with an arbitrary channel.  The purpose of this function is to allow the source of srcdoc iframes to be viewed.

The alternative is to pass a srcdoc argument to NS_NewChannel and change all the protocol handlers to support an extra argument.
Comment 38 James Kitchener (:jkitch) 2013-03-31 06:10:37 PDT
Created attachment 731636 [details] [diff] [review]
Part 2: Add about:srcdoc to about protocol handlers

I don't think there were any changes with this patch, but uploading a new one to be safe.

about:srcdoc exists in the listings, but returns an error if anyone tries to load it.

The use of about:mozilla as the resolvable destination was chosen to make mistaken resolving obvious.
Comment 39 James Kitchener (:jkitch) 2013-03-31 06:20:24 PDT
Created attachment 731639 [details] [diff] [review]
Part 3: Changes to nsI?Document to support srcdoc

nsCOMPtr<nsIChannel> mChannel was moved from nsDocument.h to nsIDocument.h to avoid storing the srcdoc string in both the channel and the document.
Comment 40 James Kitchener (:jkitch) 2013-03-31 06:21:58 PDT
Created attachment 731640 [details] [diff] [review]
Part 4: add srcdoc support to docshell and SHEntry
Comment 41 James Kitchener (:jkitch) 2013-03-31 06:23:15 PDT
Created attachment 731641 [details] [diff] [review]
Part 5: Allow sessionrestore to work on srcdoc
Comment 42 James Kitchener (:jkitch) 2013-03-31 06:25:17 PDT
Created attachment 731642 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc
Comment 43 James Kitchener (:jkitch) 2013-03-31 06:27:38 PDT
Created attachment 731643 [details] [diff] [review]
Part 7: Testcases of srcdoc iframes

Tests for compliance with specifications and that sandboxing works.

Session history, session restore and view-source are not tested.  Is this required and if so, how would you suggest doing it?
Comment 44 James Kitchener (:jkitch) 2013-03-31 06:33:50 PDT
I've submitted this to try 

https://tbpl.mozilla.org/?tree=Try&rev=5322c0cb73c4

(The additional patch was for fixing mach mochitest-plain)

The only issue is Win7-Opt not compiling properly, which is surprising given that I'm developing it on Windows.  It doesn't give a clear error message and I can't see what went wrong.  Can you suggest anything?
Comment 45 James Kitchener (:jkitch) 2013-04-01 05:49:11 PDT
Created attachment 731840 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v2

Forgot to modify the GUID.
Comment 46 Till Schneidereit [:till] 2013-04-03 04:14:17 PDT
@James: after you're done with this, you might be interested in taking a look at bug 631218. Nudge, nudge, hint, hint. ;)
Comment 47 Boris Zbarsky [:bz] (still a bit busy) 2013-04-03 20:33:51 PDT
I'm sorry for the review lag here.  I'll be working on this tomorrow, for sure.
Comment 48 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 13:29:00 PDT
Comment on attachment 731635 [details] [diff] [review]
Part 1: add srcdoc attribute to nsIChannel v2

Why put this on nsIChannel instead of, say, nsIInputStreamChannel?

>+    void ReplaceChannel(in nsIURI aURI,

Is this really ok to call at random points like halfway through a load?  

This is just needed for view-source on srcdoc documents, right?  How do we plan to handle "view only this frame" or "view frame in new window" or whatnot with srcdoc?
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 13:30:19 PDT
> How do we plan to handle "view only this frame" or "view frame in new window" or whatnot
> with srcdoc?

In fact, those probably need to be raised as spec issues...
Comment 50 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 14:07:26 PDT
On the other hand, the spec author is likely to claim that it's not a spec issue or something, as long as the web-observable behavior is "right" (that being that the URI is an about:srcdoc).  I'll post about it, but I'm not too hopeful.

I need to think a little about how we want to handle those parts of the UI, in any case...
Comment 51 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 14:13:06 PDT
I posted http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2013-April/039333.html

I guess what we implement here so far is no worse than what Safari does.  :(
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 19:05:11 PDT
Comment on attachment 731636 [details] [diff] [review]
Part 2: Add about:srcdoc to about protocol handlers

Why do you need to change browser/components/about/AboutRedirector.cpp at all?

Likewise for browser/components/build/nsModule.cpp

I think you just want to add this to docshell/base/nsAboutRedirector.cpp and docshell/build/nsDocShellModule.cpp, plus your change to the about: protocol handler.

And it should be fine to "redirect" srcdoc to "about:blank", not a chrome:// URI at all, I would think.
Comment 53 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 19:11:28 PDT
Comment on attachment 731639 [details] [diff] [review]
Part 3: Changes to nsI?Document to support srcdoc

>+  bool GetIsSrcdoc() {

I'd prefer we call this "IsSrcdocDocument" and make it a const method.

>+  void SetIsSrcdoc(bool aIsSrcdoc) {

And here SetIsSrcdocDocuemnt.

>+  nsresult GetSrcdoc(nsAString& aSrcdoc) {

This could check mIsSrcDocDocument and bail early, right?  Past that, it would need to QI to nsIInputStreamChannel and get the string from the channel....

Is there really a good reason to inline this?  At first glance, I suspect not.  We can just make it an nsIDocument method in nsDocument.cpp.

>+  // Included within nsIDocument to avoid storing redundent srcdoc strings

I don't follow why it's needed to avoid that.

>+  bool mIsSrcdoc;

msIsSrcdocDocument, and please put it next to the other boolean members a bit above this.

>+    mIsSrcdoc(false)

No need; documents are 0-initialized by their operator new.

>+  nsAutoString srcdoc;
>+  if (NS_SUCCEEDED(mChannel->GetSrcdoc(srcdoc))) {

Hrm.  I think we should just have a way to ask the channel for the boolean.  

r=me with the above fixed, but I'd like to see the updated patch.
Comment 54 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 20:08:32 PDT
Comment on attachment 731640 [details] [diff] [review]
Part 4: add srcdoc support to docshell and SHEntry

>+++ b/content/base/public/nsContentUtils.h
>+   * from the channel.  If aIsSrcdoc is true then the channel is for a srcdoc
>+   * iframe and the principle is set as owner.

That seems identical to aForceOwner in terms of behavior.  Why can we not just pass aForceOwner == true?

>+++ b/content/base/src/nsContentUtils.cpp
>+  // XXX Should this be demoted to an assert?

At best, or just removed if we reuse the existing aForceOwner.

>+++ b/docshell/base/nsDocShell.cpp
>+    nsXPIDLString srcdoc;

nsString, please.

>+        aLoadInfo->GetSrcdoc(getter_Copies(srcdoc));

I know other things here used "wstring" in the IDL, and I'm sorry we haven't updated them to not be late-90s code yet.  :(

Just use AString in the IDL, and then this will be aLoadInfo->GetSrcdoc(srcdoc);

>@@ -1407,18 +1414,22 @@ nsDocShell::LoadURI(nsIURI * aURI,
>+        return LoadHistoryEntry(shEntry, loadType, srcdoc.get(),flags);

Please pass the srcdoc as an nsAString or nsString reference, not a PRUnichar pointer.

And space before "flags".

If the issue is that you need to differentiate whether there's a srcdoc at all or not, either pass it as an nsString* or use a void string to indicate lack of a srcdoc...

>@@ -4667,43 +4682,57 @@ nsDocShell::Reload(uint32_t aReloadFlags
>+          nsIDocument* cvDoc = mContentViewer->GetDocument();

Why can't you just use 'doc'?

>@@ -8491,54 +8521,59 @@ void CopyFavicon(nsIURI *aOldURI, nsIURI
>-
>+  

Please undo that change.

>+                      const PRUnichar *aSrcdoc) :

Again, please pass around string objects, not PRUnichar pointers.

>@@ -9431,41 +9471,102 @@ nsDocShell::DoURILoad(nsIURI * aURI,
>+    nsAutoCString scheme;
>+    rv = aURI->GetScheme(scheme);
>+    NS_ENSURE_SUCCESS(rv,rv);
>+    bool isViewSource = scheme.LowerCaseEqualsLiteral("view-source");

Please use SchemeIs() here.

>+    if (aIsSrcdoc && isViewSource) {
>+        // Hack.  The input URI is about:srcdoc in this situation, but this
>+        // cannot be passed to the view-source constructor as it is 
>+        // non-resolvable.  Pass a dummy URI and correct for it further on
>+        rv = NS_NewURI(getter_AddRefs(chanURI),
>+                       NS_LITERAL_STRING("view-source:about:blank"));

I would almost rather we build knowledge of about:srcdoc into the view-source protocol handler so it won't bother to create the extra nested channel here.  It could then also assert that the real channel is only being set on it once...

Thoughts?

>+    else {
>+      chanURI = aURI;

Please fix the indent.  This file is 4-space indented, in general.

>+    if (aIsSrcdoc) {
>+        nsAutoCString cSrcdoc = NS_ConvertUTF16toUTF8(aSrcdoc);
>+        rv = stream->SetData(cSrcdoc.get(), cSrcdoc.Length());

So this is ending up making at least 2 copies of the srdoc data in the common case....

I think it would be better to do something like this:

  uint32_t length;
  char* utf8Bytes = ToNewUTF8String(aSrcDoc, &length);
  rv = stream->AdoptData(utf8Bytes, length);
  

>+            // XXX This could be replaced with a hardcoded about:srcdoc

Yes, please!

>+            rv = aURI->GetOriginCharset(charset);

Don't bother.  It doesn't really matter for this case.

>+            vsChan->ReplaceChannel(aURI,srcdocChannel);

Missing space after ','.

>+            channel = do_QueryInterface(srcdocChannel);

Why can't this just be a direct assignment?

>+nsDocShell::LoadHistoryEntry(nsISHEntry * aEntry, uint32_t aLoadType,
>+    if (aFlags & INTERNAL_LOAD_FLAGS_IS_SRCDOC) {

Hmm.  When would this be the case if aEntry->GetIsSrcdoc is false?

>+        srcdoc = srcdocStr.get();

That sets srcdoc to a pointer to about-to-be-deallocated memory.  Yet another reason to stick to string objects.... ;)

>+      else {
>+        // Do not inherit owner from document (security-critical!)
>+        flags = INTERNAL_LOAD_FLAGS_NONE;

And this branch never sets srcdoc at all, so it ends up with a random value.

We clearly need more testing of this codepath.  :(  Or is that value unused if the INTERNAL_LOAD_FLAGS_IS_SRCDOC flag is not set?

>+++ b/docshell/base/nsDocShellLoadInfo.cpp
>+   NS_ENSURE_ARG_POINTER(aIsSrcdoc);

Please don't bother null-checking XPCOM out params.  And again, please make this AString in the IDL.

>+++ b/docshell/base/nsIDocShell.idl
>   [noscript]void internalLoad(in nsIURI aURI,
>+                              in wstring aSrcdoc,

Once we sort out the actual type here and how it interacts with the internal flag, we should document that.  I think the type should be AString, which is ignored (or even asserted to be empty) if the srcdoc load flag is not set.

>+++ b/docshell/base/nsIDocShellLoadInfo.idl
>+    attribute wstring srcdoc;

AString

This is looking really good; most of the stuff above is little mechanical nits, but the overall approach seems totally reasonable.

When you post an updated version for this one, I'd really appreciate an interdiff; let me know if you're not sure how to produce one.
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 20:11:04 PDT
Comment on attachment 731641 [details] [diff] [review]
Part 5: Allow sessionrestore to work on srcdoc

r=me
Comment 56 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 20:16:01 PDT
Comment on attachment 731641 [details] [diff] [review]
Part 5: Allow sessionrestore to work on srcdoc

Would like a once-over from a sessionstore peer here.
Comment 57 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 20:24:11 PDT
Comment on attachment 731840 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v2

> nsFrameLoader::LoadFrame()
>+  bool isSrcdoc = mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc, srcdoc);

You're not using the value here, right?

If so, better to do:

  bool isSrcdoc = mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::srcdoc);

>@@ -470,18 +478,35 @@ nsFrameLoader::ReallyStartLoadingInterna
>+    rv = NS_NewURI(getter_AddRefs(referrer), referrerStr, charset);

I wouldn't worry about the charset here, honestly.  Especially because the document's charset may have not much bearing on the referrer string's bytes, since that string came from the _previous_ document.

r=me on the non-parser bits.

Henri, would you look over the parser changes?
Comment 58 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 20:29:08 PDT
Comment on attachment 731643 [details] [diff] [review]
Part 7: Testcases of srcdoc iframes

>* * *
>try: -b o -p all -u all -t none

Drop this part, please.  ;)

Would be good to have session history tests too, but those are a huge pain, so maybe just manual testing is OK for now.  Of course if you're up for it... ;)

>+++ b/content/base/test/test_srcdoc.html

This should maybe live under content/html/content/test

>+    var iframe = pframe.getElementById("iframe");

Shouldn't that be pframe.contentDocument.getElementById("iframe")?  Not sure how this is passing...

>+    var innerDoc = iframe.contentDocument || iframe.contentWindow.document;

We support contentDocument, so no need for the second half.  ;)  All three times, please.

r=me with those nits.

And again, this is looking really good.  Thank you for sticking with it!
Comment 59 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 20:31:19 PDT
Comment on attachment 731635 [details] [diff] [review]
Part 1: add srcdoc attribute to nsIChannel v2

I think we should just put this on input stream channels and QI where we care.
Comment 60 Boris Zbarsky [:bz] (still a bit busy) 2013-04-04 20:48:34 PDT
Ah, you asked about tests for the edge cases.

So for viewsource the only thing I can think of is a browser-chrome test or something.  I have no idea how to write one for that end of it offhand; would have to ask around on irc...  I think it's ok to pass on that for now.  Session restore is even worse.  Again, asking on irc is the best bit if you want to write a test for it.

For session history, you should be able to just write a test that loads an iframe A that has a srcdoc iframe B inside, and then does the following:

1)  Add an unload listener to the window inside iframe A (to disable bfcache for it).
2)  Modify the DOM of the document inside iframe A in some way (e.g. add an element with
    some id)
3)  Starts an about:blank load in iframe A.
4)  When that load finishes, call history.back().
5)  When _that_ load finishes, verify that the modification made in step 2 is gone, so we
    didn't load from bfcache.
6)  Check that the srcdoc bit got loaded properly.

I think that should do the trick...

> The only issue is Win7-Opt not compiling properly

That looks like infrastructure bustage to me, not your code.
Comment 61 Marco Bonardo [::mak] 2013-04-05 03:43:19 PDT
Comment on attachment 731641 [details] [diff] [review]
Part 5: Allow sessionrestore to work on srcdoc

forwarding to speed it up.
Comment 62 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-04-05 05:14:40 PDT
Comment on attachment 731840 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v2

r- on the parser changes. They appear to be incomplete. The translator does not run as part of the build, so there are C++ bits missing. Even the Java version of the AttributeName needs to regenerate its hashes for the Java version to make sense and before it can be translated to C++.

The good news is that the parser changes are unnecessary to land this. Making the parser aware of srcdoc is a mere performance optimization that can be landed separately. (I can do it.)

r+ if the changes under parser/html/ are simply omitted from this patch (or, alternatively, made complete, but as noted above, you don't need to do that).
Comment 63 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-04-05 05:25:32 PDT
What part of these patches makes sure that the doc parsed out of srcdoc reports its encoding the right way? Upon a quick look, the spec isn't clear on what the right way is. I hope it is that the doc always reports UTF-8 as its encoding. If the spec doesn't already say so, is there a spec bug to that effect on file?
Comment 64 Boris Zbarsky [:bz] (still a bit busy) 2013-04-05 08:36:54 PDT
> What part of these patches makes sure that the doc parsed out of srcdoc reports its
> encoding the right way?

The relevant bit is:

+        NS_NewInputStreamChannel(getter_AddRefs(srcdocChannel), inStreamURI, stream,
+                                 NS_LITERAL_CSTRING("text/html"),
+                                 NS_LITERAL_CSTRING("UTF-8"));

which will make us report "UTF-8".  In terms of the spec, I have no clue what it's requiring here, if anything; scrdoc is monkeypatched in all over in no particularly principled way, so I can't tell what the right behavior is for it.
Comment 65 Boris Zbarsky [:bz] (still a bit busy) 2013-04-05 08:38:13 PDT
James, looking at the spec there are three more issues that need to be dealt with as far as I can tell:

1)  Changes to @srcdoc should do the "process frame attributes" thing.
2)  Changes to @src should be ignored if @srcdoc is set.
3)  The srcdoc document should always be standards mode.

We should add tests for all three of those...
Comment 66 James Kitchener (:jkitch) 2013-04-18 04:47:30 PDT
I've addressed almost all of the non-optional comments and will submit the patches for review once the final matter is resolved.  

(In reply to Boris Zbarsky (:bz) from comment #48)
> This is just needed for view-source on srcdoc documents, right?  How do we
> plan to handle "view only this frame" or "view frame in new window" or
> whatnot with srcdoc?

I have the contents of the srcdoc attribute displaying in these cases in standards compliance mode.  What I have doubts about is what to do with domain/origin and referrer.

For a srcdoc iframe, both of these are determined based on the "Document's browsing context's browsing context container's Document".

The question is:  Does the contents of a srcdoc iframe, when viewed in isolation in a new tab/window, have a "browsing context container"?  If it doesn't, origin/referrer are undefined, and current behaviour (blank or null) should be adequate.

If it does have a browsing context container which inherits some behaviour/attributes from the parent document, the nature of this needs to be carefully specified as there is the potential for some rather perverse outcomes.

I've looked through the relevant posts on the whatwg mailing list, but was unable to come to a conclusion.
Comment 67 James Kitchener (:jkitch) 2013-04-18 05:08:05 PDT
One further complication has crossed my mind.  When a srcdoc iframe is viewed in isolation, the address bar is currently set to about:srcdoc (as that is the URL of the document according to the specifications).

Up until now, all about: documents are internal to Firefox and resolve to chrome: URIs.

I'm a little concerned about the potential for users to mistake an about:srcdoc document for an internal firefox page, rather than the arbitrary web content that it is.
Comment 68 Boris Zbarsky [:bz] (still a bit busy) 2013-04-18 10:42:47 PDT
> Does the contents of a srcdoc iframe, when viewed in isolation in a new tab/window, have
> a "browsing context container"?

Nope.

> Up until now, all about: documents are internal to Firefox and resolve to chrome: URIs.

Not necessarily; about:blank doesn't, and neither do some of the ones extensions add...

But yes, that's a bit of a worry.  The other option is to just disable those context menu options for srcdoc iframes, like roc suggested.

Gavin?
Comment 69 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-22 13:48:06 PDT
(In reply to Boris Zbarsky (:bz) from comment #68)
> But yes, that's a bit of a worry.  The other option is to just disable those
> context menu options for srcdoc iframes, like roc suggested.

This sounds reasonable. It sounds like we probably want all of the "is this a frame?" checks in nsContextMenu to return false for srcdoc iframes.
Comment 70 James Kitchener (:jkitch) 2013-04-28 02:09:56 PDT
Created attachment 742786 [details] [diff] [review]
Part 1: add srcdoc attribute to nsIChannel v3

Comments have been applied.

isSrcdocChannel/srcdocData are now nsIInputStreamChannel attributes.

One side effect is that viewSourceChannels no longer have these attributes, which limits the ability to recreate the channel at a later date.  However I am unaware of a situation in which this might be necessary.  

There is a new approach for viewSourceChannels.  Rather than creating loading view-source:about:blank and overwriting it with the srcdoc channel, the viewSourceHandler has been given an additional constructor which accepts a srcdoc string.  It still embeds a nsIViewSourceChannel, but creation of the channel is now done within nsIViewSourceChannel.
Comment 71 James Kitchener (:jkitch) 2013-04-28 02:13:26 PDT
Created attachment 742788 [details] [diff] [review]
Part 2: Add about:srcdoc to about protocol handlers

Comments have been applied
Comment 72 James Kitchener (:jkitch) 2013-04-28 02:31:59 PDT
Created attachment 742790 [details] [diff] [review]
Part 3: Changes to nsI?Document to support srcdoc v2

Comments have been applied.  Flagging for review again as you asked to see it again and the changes are non-trivial.

isSrcdocDocument has been exposed to chrome-only javascript to allow the context menu to detect srcdoc iframes.

Behaviour of GetSrcdocData has changed.  It always returns NS_OK and lack of data is indicated by a NullString outparam.
Comment 73 James Kitchener (:jkitch) 2013-04-28 02:38:29 PDT
Created attachment 742791 [details] [diff] [review]
Part 4: Docshell changes interdiff

Interdiff of the docshell changes.  I doubt it will apply cleanly to the previously submitted patch as there was a need to rebase it.

Changes were made to SHEntry to make isSrcdoc/srcdoc more explicit (now isSrcdocEntry/srcdocData)
Comment 74 James Kitchener (:jkitch) 2013-04-28 02:43:33 PDT
Created attachment 742793 [details] [diff] [review]
Part 4: add srcdoc support to docshell and SHEntry v2

This is the docshell changes folded into the original rebased patch.

The differences between this patch and the original are found in attachment 742791 [details] [diff] [review].
Comment 75 James Kitchener (:jkitch) 2013-04-28 02:45:07 PDT
Created attachment 742794 [details] [diff] [review]
Part 5: Allow sessionrestore to work on srcdoc v2

Superficial changes to work with the changes to SHEntry.

isSrcdoc -> isSrcdocEntry
srcdoc -> srcdocData
Comment 76 James Kitchener (:jkitch) 2013-04-28 02:49:16 PDT
Created attachment 742795 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v3

Changes have been applied.

changes to /parser have been removed.
Comment 77 James Kitchener (:jkitch) 2013-04-28 02:57:58 PDT
Created attachment 742796 [details] [diff] [review]
Part 7: srcdoc iframes no longer considered frames in context menu

Following the suggestion in comment 69, this prevents srcdoc iframes from being detected as frames within the context menu.
Comment 78 James Kitchener (:jkitch) 2013-04-28 02:59:59 PDT
Created attachment 742798 [details] [diff] [review]
Part 7: Testcases of srcdoc iframes v2

Test for session history has been added.

Tests for other matters have not.
Comment 79 James Kitchener (:jkitch) 2013-04-28 03:05:38 PDT
Comment on attachment 742795 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v3

On second thoughts, this might require review for the changes to standards mode and loading on src/srcdoc changes.
Comment 80 James Kitchener (:jkitch) 2013-04-28 03:06:49 PDT
(In reply to James Kitchener from comment #78)
> Created attachment 742798 [details] [diff] [review]
> Part 7: Testcases of srcdoc iframes v2
> 
> Test for session history has been added.
> 
> Tests for other matters have not.

Tests for standards compliance and loading on src/srcdoc changes have also been added.
Comment 81 Boris Zbarsky [:bz] (still a bit busy) 2013-04-30 06:59:17 PDT
Comment on attachment 742786 [details] [diff] [review]
Part 1: add srcdoc attribute to nsIChannel v3

I think we should make isScrdocChannel readonly and just have it return whether scrdocData was ever set.

>+    // about:srcdoc as this is the only permissable URL for srcdoc 

"permissible".

>+    rv = NS_NewURI(getter_AddRefs(inStreamURI),
>+                    NS_LITERAL_STRING("about:srcdoc"));

Please fix the indent.

>+    NS_ENSURE_SUCCESS(rv,rv);

Space after comma.  This comes up twice in this patch.

>+    NS_NewInputStreamChannel(getter_AddRefs(mChannel), inStreamURI, stream,

You presumably meant to assign the return value of this into rv, right?

>+    if (inStrmChan) {

It can't be null.  Feel free to assert that!

>+        inStrmChan->SetSrcdocData(nsAutoString(aSrcdoc));

Just inStrmChan->SetSrcdocData(aSrcdoc), please.

r=me with those nits.

Benjamin, would you mind doing an sr on the IDL change here?
Comment 82 Boris Zbarsky [:bz] (still a bit busy) 2013-04-30 07:00:56 PDT
Comment on attachment 742788 [details] [diff] [review]
Part 2: Add about:srcdoc to about protocol handlers

r=me
Comment 83 Boris Zbarsky [:bz] (still a bit busy) 2013-04-30 07:12:28 PDT
Comment on attachment 742790 [details] [diff] [review]
Part 3: Changes to nsI?Document to support srcdoc v2

>+++ b/content/base/public/nsIDocument.h
>+  nsCOMPtr<nsIChannel> mChannel;

You need to remove that from nsDocument.h, right?  Sorry I misseed that last time.  :(

>+++ b/content/base/src/nsDocument.cpp
>+  nsAutoString srcdocData;

This variable is unused.  Please remove it.

>+nsDocument::GetIsSrcdocDocument(bool* aIsSrcdocDocument)

Hmm.  Is this used anywhere?  If not, I would prefer that we just not add this to nsIDOMDocument.idl and not add this XPCOM method.

>+nsIDocument::GetSrcdocData(nsAString &aSrcdocData)
>+  if(mIsSrcdocDocument && mChannel) {

Space after "if", please.  Also, no need to null-check mChannel here, since do_QueryInterface is null-safe.

>+      inStrmChan->GetSrcdocData(aSrcdocData);
>+      return NS_OK;

Probably better to do:

  return inStrmChan->GetSrcdocData(aSrcdocData);

>+++ b/dom/interfaces/core/nsIDOMDocument.idl

Like I said, probably better to just drop the changes to this file.

r=me with the above nits.
Comment 84 Boris Zbarsky [:bz] (still a bit busy) 2013-04-30 07:16:56 PDT
Though I guess part 7 may be relying on the nsIDOMDocument bits for now because HTMLDocument is not on WebIDL bindings yet...  If so, it's OK to land that with a followup bug to remove it as soon as bug 855971 is fixed.
Comment 85 Boris Zbarsky [:bz] (still a bit busy) 2013-04-30 07:48:29 PDT
Comment on attachment 742795 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v3

>+++ b/content/base/src/nsFrameLoader.cpp
>+    //

Drop that line, please.

>+    nsCOMPtr<nsIDocument> doc;

No need for that temporary.  Just do:

  mOwnerContent->OwnerDoc()->GetReferrer(referrerStr);

>+++ b/content/html/content/src/nsGenericHTMLFrameElement.cpp
>@@ -219,18 +219,24 @@ nsGenericHTMLFrameElement::SetAttr(int32
>+    //Changes to src have no effect if srcdoc attribute is present

Space after "//", please.

You need to change UnsetAttr too, I think.  Specifically, unsetting @srcdoc should LoadSrc(), I would think.

Also, you want to have the srcdoc stuff here only affect <iframe>, not <frame>.  That applies for the nsFrameLoader bits too, I just realized: those should only do srcdoc stuff when mOwnerContent->IsHTML(nsGkAtoms::iframe).

>+++ b/content/html/document/src/nsHTMLDocument.cpp
> nsHTMLDocument::SetCompatibilityMode(nsCompatibility aMode)

Does this affect quirks mode stuff in the parser itself?  For example does this:

  <iframe srcdoc='<p style="border: 1px solid black"><table></table>Text</p>'></iframe>

put the text in the black box or after it?  It should go after the black box...

Furthermore, I just looked at the spec more carefully, and it looks like standards mode is only the default if no doctype token was seen.  It's possible that this was a spec change since the last time I reviewed.  :(

Henri, what's the right way of fixing the parser here?
Comment 86 Boris Zbarsky [:bz] (still a bit busy) 2013-04-30 07:58:20 PDT
Comment on attachment 742798 [details] [diff] [review]
Part 7: Testcases of srcdoc iframes v2

>+  function soon(f) {

Use SimpleTest.executeSoon.

>+  pframe.addEventListener("unload", function () { }, false);

So this assumes pframe is the window in the iframe...

>+  pframe.onload = soon(function () {

But this assumes it's the iframe itself.

The latter makes more sense to me; should the addEventListener be happening on pframe.contentWindow?

>+++ b/content/html/content/test/test_srcdoc.html

This test seems to assume that @scrdoc and @src changes take effect synchronously.  That's not right...

It should probably also be testing removeAttribute("srcdoc").

about:robots is a Firefox-specific about: URI, but this is a Gecko test.  Use some data: URI instead, please.
Comment 87 Boris Zbarsky [:bz] (still a bit busy) 2013-04-30 08:28:16 PDT
Comment on attachment 742793 [details] [diff] [review]
Part 4: add srcdoc support to docshell and SHEntry v2

>+        if(isViewSource) {

Space after "if", please.

>+            nsViewSourceHandler *vsh = new nsViewSourceHandler();

Protocol handlers are services; you don't want to create a new one.

What's probably better is to have a private |static nsViewSourceHandler* gInstance| in the nsViewSourceHandler class with a public static getter for it, set it when an instance is created, null it out when the instance is destroyed, and use it here.

>+                inStrmChan->SetSrcdocData(nsAutoString(aSrcdoc));

Same comments here as before.  Can we we share this chunk of code (e.g. by putting it in nsNetUtil.h or something)?

>@@ -10557,16 +10621,30 @@ nsDocShell::AddToSessionHistory(nsIURI *
>+    if (aChannel) {

Again, no need to null-check the argument to do_QueryInterface; just null-checking the QI result is enough.

>@@ -10703,28 +10781,43 @@ nsDocShell::LoadHistoryEntry(nsISHEntry 
>+    else {
>+      srcdoc = NullString();

Again, 4-space indent.

>+                             NullString(),              // XXX No srcdoc?

Take out the "XXX" and the question mark; there is no srcdoc for a link click, of course.  ;)

>+++ b/docshell/base/nsDocShellLoadInfo.cpp
>+NS_IMETHODIMP nsDocShellLoadInfo::SetIsSrcdocLoad(bool aIsSrcdocLoad)

Can we just automatically set this boolean to true when SetScrdocData is called, and get rid of this setter?

r=me with those nits fixed, though I'd like to take a look at the new view source handler code.  Thank you very much for the interdiff!
Comment 88 James Kitchener (:jkitch) 2013-05-02 06:44:07 PDT
(In reply to Boris Zbarsky (:bz) from comment #83)
> Comment on attachment 742790 [details] [diff] [review]
> Part 3: Changes to nsI?Document to support srcdoc v2
> 
> >+++ b/content/base/public/nsIDocument.h
> >+  nsCOMPtr<nsIChannel> mChannel;
> 
> You need to remove that from nsDocument.h, right?  Sorry I misseed that last
> time.  :(
> 
Oops.  Thought I fixed that.

> 
> >+nsDocument::GetIsSrcdocDocument(bool* aIsSrcdocDocument)
> 
> Hmm.  Is this used anywhere?  If not, I would prefer that we just not add
> this to nsIDOMDocument.idl and not add this XPCOM method.
> 
> >+++ b/dom/interfaces/core/nsIDOMDocument.idl
> 
> Like I said, probably better to just drop the changes to this file.
> 
> r=me with the above nits.

This function is used to give the context menu awareness of srcdoc iframes (see attachment 742796 [details] [diff] [review]:  Part 7: srcdoc iframes no longer considered frames in context menu).

For reasons I don't understand
"document.isSrcdocDocument" calls nsIDocument:IsSrcdocDocument()
while
"this.target.ownerDocument.isSrcdocDocument" will only call nsDocument::GetIsSrcdocDocument()

Is there a better way to detect srcdoc iframes within chrome javascript?  
(srcdoc status is otherwise stored within the parent document's iframe element, but I don't know of a way to access that from the child document object).
Comment 89 Boris Zbarsky [:bz] (still a bit busy) 2013-05-02 07:47:58 PDT
> For reasons I don't understand

It's because HTMLDocument is using WebIDL quickstubs but not actual WebIDL bindings yet, so access via an XrayWrapper goes through the XPConnect interface so far.  See comment 84.

The other option would be to use something like doc.defaultView.frameElement in the chrome JS to get at the actual <iframe> and then check whether the "srcdoc" attribute is set, I guess.  It would be slightly racy when the attribute gets changed but the new document hasn't loaded yet, and I don't think it's worth worrying about, per comment 84.

In fact, thinking about it more, once bug 855971 lands the main reason I don't like the nsIDOMDocument change (that it exposes the method unconditionally to web content) goes away, so we should just add this to nsIDOMDocument and move on.
Comment 90 James Kitchener (:jkitch) 2013-05-08 06:47:54 PDT
(In reply to Boris Zbarsky (:bz) from comment #85)
> >+++ b/content/html/document/src/nsHTMLDocument.cpp
> > nsHTMLDocument::SetCompatibilityMode(nsCompatibility aMode)
> 
> Does this affect quirks mode stuff in the parser itself?  For example does
> this:

No it doesn't.  The document still renders in quirks mode.

> 
>   <iframe srcdoc='<p style="border: 1px solid
> black"><table></table>Text</p>'></iframe>
> 
> put the text in the black box or after it?  It should go after the black
> box...
> 
> Furthermore, I just looked at the spec more carefully, and it looks like
> standards mode is only the default if no doctype token was seen.  It's
> possible that this was a spec change since the last time I reviewed.  :(
> 
> Henri, what's the right way of fixing the parser here?

So far I've identified the portion of code responsible for setting the compatibility mode in the absence of a doctype within the parser, but I haven't been able to find a way to make it srcdoc aware (a property held in nsIInputStreamChannel or nsI?Document instances).
Comment 91 Boris Zbarsky [:bz] (still a bit busy) 2013-05-08 09:04:25 PDT
Henri, what's a good way to handle the quirks mode bits here?
Comment 92 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-05-10 05:12:06 PDT
Comment on attachment 742795 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v3

> Furthermore, I just looked at the spec more carefully, and it looks like 
> standards mode is only the default if no doctype token was seen.

Eww.

> Henri, what's the right way of fixing the parser here?

To introduce an srcdoc flag on the TreeBuilder and making the doctype handling logic branch on the flag in the case of a missing doctype.

Aside: I see that the spec says: "For iframe elements in XML documents, the srcdoc attribute, if present, must have a value that matches the production labeled document in the XML specification." That's not cool. It seems to repeat the same mistake that was made with innerHTML when making the parser it invokes depend on the HTMLness flag of the document.
Comment 93 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-05-10 05:13:54 PDT
(In reply to Henri Sivonen (:hsivonen) from comment #92)
> To introduce an srcdoc flag on the TreeBuilder and making the doctype
> handling logic branch on the flag in the case of a missing doctype.

Making the TreeBuilder aware of whether it's involved in parsing the contents of an srcdoc attribute that is.
Comment 94 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-05-10 05:14:56 PDT
Then again: What's the use case for supporting non-standards modes for srcdoc? Why don't we ask for a spec chance to make the mode always be the standards mode for srcdoc?
Comment 95 Boris Zbarsky [:bz] (still a bit busy) 2013-05-10 07:24:38 PDT
> when making the parser it invokes depend on the HTMLness flag of the document.

Actually, in this case things are fine, from our pov.  The "must have a value that matches the production" stuff is an _authoring_ requirement (though why it's there, I dunno).  The UA requirement is to parse it as text/html.

> Why don't we ask for a spec chance to make the mode always be the standards mode for
> srcdoc?

I thought it used to say that and got changed to the present behavior.  But looks like it's just been this way all along.

Let's do that and I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=21998
Comment 96 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-14 17:39:10 PDT
Comment on attachment 742796 [details] [diff] [review]
Part 7: srcdoc iframes no longer considered frames in context menu

Sorry for the delay here, I lost track of this request.

One real issue: this change 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 need to track inFrame and inSrcdocFrame separately, I think. Can you file a followup bug for that?

One nit: please put a before the parenthesis in "if("
Comment 97 James Kitchener (:jkitch) 2013-05-19 03:01:29 PDT
Created attachment 751448 [details] [diff] [review]
WIP changes to parser

I've attached an attempt at giving the parser awareness of srcdoc iframes, generated as appropriate from the Java source.

It fails to work approximately half the time when run and sometimes causes non-srcdoc documents without doctypes to render in standards mode.  (With a debugger the iframe is always correctly rendered and the parent document is always incorrectly rendered)

I've identified that nsHtml5Parser correctly detects srcdoc documents, but the isSrcdocDocument flag does not report the correct value by the time it is actually read.

Is there a more appropriate location to set the isSrcdocDocument flag and a more reliable way to reset it so it doesn't affect other documents?

(Note that this is a subset of the changes.  I haven't included changes to the generated NamedCharacters and AttributeName files since they increase the patch size by roughly 400kB.  I also haven't included changes to the copyright date or ones that replace int32_t with PRInt32 which were caused by regenerating the cpp files rather than my changes.)
Comment 98 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2013-05-24 06:39:49 PDT
(In reply to James Kitchener from comment #97)
> I've attached an attempt at giving the parser awareness of srcdoc iframes,
> generated as appropriate from the Java source.

Looks good, though I'd put "else" on the same line with "}". Also, it looks like it fails to suppress some errors in the case the token that triggers the mode commitment is a character token.

> It fails to work approximately half the time when run and sometimes causes
> non-srcdoc documents without doctypes to render in standards mode.  (With a
> debugger the iframe is always correctly rendered and the parent document is
> always incorrectly rendered)

This is probably due to nsHtml5Parser and nsHtml5StreamParser having distinct instances of nsHtml5TreeBuilder.

Instead of debugging this further right now, I suggest pursuing https://www.w3.org/Bugs/Public/show_bug.cgi?id=21998


> ones that replace int32_t with PRInt32 which were caused
> by regenerating the cpp files rather than my changes.)

Interesting. I will need to follow up on that one. I thought I had landed a fix.
Comment 99 James Kitchener (:jkitch) 2013-06-18 06:53:21 PDT
Created attachment 764135 [details] [diff] [review]
Part 1: add srcdoc attribute to nsIChannel v4

A NS_NewInputStreamChannel overload has been added which takes an nsAString as input and uses it to create the InputStream.
Comment 100 James Kitchener (:jkitch) 2013-06-18 06:56:46 PDT
Created attachment 764138 [details] [diff] [review]
Part 3: Changes to nsI?Document to support srcdoc v3

The nsIDOMDocument changes have been removed as they are no longer needed.
Comment 101 James Kitchener (:jkitch) 2013-06-18 07:05:17 PDT
Created attachment 764139 [details] [diff] [review]
Part 4: add srcdoc support to docshell and SHEntry v3

An additional change has been made to SetUpChannelOwner to disable an assert for about:srcdoc URIs.  

This can cause false negatives for non-srcdoc iframes whenever the URL is set to about:srcdoc, but it shouldn't matter as the URI is non-resolvable.
Comment 102 James Kitchener (:jkitch) 2013-06-18 07:07:33 PDT
Created attachment 764141 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v4

Corrections have been made.
Comment 103 James Kitchener (:jkitch) 2013-06-18 07:08:38 PDT
Created attachment 764142 [details] [diff] [review]
Part 7: srcdoc iframes no longer considered frames in context menu v2
Comment 104 James Kitchener (:jkitch) 2013-06-18 07:17:09 PDT
Created attachment 764145 [details] [diff] [review]
Part 8 parser changes

Now renders all srcdoc iframes in standards mode.  Most doctype error messages are suppressed from within the error function itself.  (The errStrayDoctype error isn't suppressed as the standards don't allow arbitrary doctype locations.)
Comment 105 James Kitchener (:jkitch) 2013-06-18 07:21:42 PDT
Created attachment 764146 [details] [diff] [review]
Changes to the htmlparser subrepository
Comment 106 James Kitchener (:jkitch) 2013-06-18 07:27:19 PDT
Created attachment 764153 [details] [diff] [review]
Part 9: Testcases of srcdoc iframes v3

Tests should now be asynchronous.

For one of the edge cases, it was necessary to test than an event didn't occur.  
The time period before assuming success is currently 2 seconds, but that is a guess at a reasonable interval.
Comment 107 Boris Zbarsky [:bz] (still a bit busy) 2013-06-18 07:31:48 PDT
Comment on attachment 764135 [details] [diff] [review]
Part 1: add srcdoc attribute to nsIChannel v4

I assume you meant to ask for review...

What compile errors did you hit with the readable utils version of ToNewUTF8String?

>+                                  aSrcdoc,NS_LITERAL_CSTRING("text/html"),

Space after comma.
Comment 108 Boris Zbarsky [:bz] (still a bit busy) 2013-06-18 07:36:33 PDT
Comment on attachment 764138 [details] [diff] [review]
Part 3: Changes to nsI?Document to support srcdoc v3

The documentation for GetSrcDocData (about NS_ERROR_NOT_AVAILABLE) does not match the implementation....
Comment 109 Boris Zbarsky [:bz] (still a bit busy) 2013-06-18 07:38:16 PDT
The old part 4 seems to have become part 5.  What's part 4 now?
Comment 110 Boris Zbarsky [:bz] (still a bit busy) 2013-06-18 07:46:04 PDT
Comment on attachment 764139 [details] [diff] [review]
Part 4: add srcdoc support to docshell and SHEntry v3

We still have the SetIsSrcdocLoad and SetIsSrcdocEntry setters that seem like they could be handled by SetSrcdocData directly...
Comment 111 Boris Zbarsky [:bz] (still a bit busy) 2013-06-18 07:52:44 PDT
Comment on attachment 764141 [details] [diff] [review]
Part 6: HTML related changes to about srcdoc v4

The frameloader parts seem to be mis-merged (e.g. end up doing GetURL twice).

>+      // Fall back to the src element, if any

"src attribute".

This is still doing scrdoc stuff for <html:frame>.  Was there a reason for that, or just oversight?  r- pending this being explained.

Note that the spec has been updated to say that srcdoc is always standards-mode, for what that's worth...
Comment 112 Boris Zbarsky [:bz] (still a bit busy) 2013-06-18 07:58:17 PDT
Comment on attachment 764153 [details] [diff] [review]
Part 9: Testcases of srcdoc iframes v3

>+      ok(!iframe1load, "Changing scr attribute shouldn't cause a load when srcdoc is set");

s/scr/src/

r=me
Comment 113 James Kitchener (:jkitch) 2013-06-19 03:13:26 PDT
Comment on attachment 764139 [details] [diff] [review]
Part 4: add srcdoc support to docshell and SHEntry v3

Oops.  Part 4 (docshell) should remain part 4.
The old part 5 (session restore) is still part 5.
Comment 114 James Kitchener (:jkitch) 2013-06-19 03:16:31 PDT
(In reply to Boris Zbarsky (:bz) from comment #107)

> What compile errors did you hit with the readable utils version of
> ToNewUTF8String?

When compiling intl/unicharutil/tests/UnicharSelfTest.cpp and intl/unicharutil/tests/NormalizationTest.cpp:

86:32.93 c:\mozilla-central\obj-i686-pc-mingw32\dist\include\nsStringFwd.h(16) :
 fatal error C1004: unexpected end-of-file found

Which is a MSVC bug caused when the #error at line 16 is triggered.

#error Internal string headers are not available from external-linkage code.
Comment 115 Boris Zbarsky [:bz] (still a bit busy) 2013-06-19 06:56:23 PDT
OK.  Can you just ifdef MOZILLA_INTERNAL_API that function in nsNetUti.h to get rid of that problem with the tests?
Comment 116 James Kitchener (:jkitch) 2013-06-20 05:00:30 PDT
Created attachment 765302 [details] [diff] [review]
part 1: channel and nsNetUtil changes

Code paths for MOZILLA_INTERNAL_API and otherwise are now included.
Comment 117 James Kitchener (:jkitch) 2013-06-20 05:01:37 PDT
Created attachment 765303 [details] [diff] [review]
Part 3: nsI?Document changes

Comment has been corrected.
Comment 118 James Kitchener (:jkitch) 2013-06-20 05:03:26 PDT
Created attachment 765304 [details] [diff] [review]
part 4: docshell and SHEntry changes

isSrcdoc(Load|Entry) now readonly.
Comment 119 James Kitchener (:jkitch) 2013-06-20 05:04:49 PDT
Created attachment 765305 [details] [diff] [review]
Part 5: sessionrestore changes

Fallout from making isSrcdocEntry readonly.
Comment 120 James Kitchener (:jkitch) 2013-06-20 05:06:33 PDT
Created attachment 765306 [details] [diff] [review]
Part 6: HTML related changes

It was an oversight.  I believe this patch acts upon the correct file.
Comment 121 James Kitchener (:jkitch) 2013-06-20 05:08:21 PDT
Created attachment 765307 [details] [diff] [review]
Part 8: parser changes

Rebased, but otherwise no further changes.
Comment 122 James Kitchener (:jkitch) 2013-06-20 05:09:38 PDT
Created attachment 765309 [details] [diff] [review]
Part 9: tests

typo fixed
Comment 123 Boris Zbarsky [:bz] (still a bit busy) 2013-06-20 09:50:30 PDT
Comment on attachment 765306 [details] [diff] [review]
Part 6: HTML related changes

Instead of duplicating the SetAttr() code, I think we should do the following:

1)  HTMLIFrameElement::SetAttr only handles changes to the srcdoc attribute and calls into nsGenericHTMLFrameElement::SetAttr.

2)  nsGenericHTMLFrameElement::SetAttr only responds to @src changes if Tag() != nsGkAtoms::iframe || !HasAttr(kNameSpaceID_None, nsGkAtoms::srcdoc).

You also need IsHTML(nsGkAtoms::iframe) checks in the frameloader bits that look for the srcdoc attr.

r=me with the above changes.
Comment 124 James Kitchener (:jkitch) 2013-06-22 06:27:12 PDT
Created attachment 766290 [details] [diff] [review]
Part 6: HTML related changes
Comment 125 Boris Zbarsky [:bz] (still a bit busy) 2013-06-22 10:25:45 PDT
HTMLIFrameElement::SetAttr should have a two-space indent.

Also, it may be clearer in frameloader to do:

  bool isSrcdoc = mOwnerContent->IsHTML(nsGkAtoms::iframe) && 
                  mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::srcdoc);

and


  nsAutoString srcdoc;
  bool isSrcdoc = mOwnerContent->IsHTML(nsGkAtoms::iframe) &&
                  mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc, srcdoc);
Comment 126 James Kitchener (:jkitch) 2013-06-23 01:56:29 PDT
Created attachment 766392 [details] [diff] [review]
Part 6: HTML related changes
Comment 127 James Kitchener (:jkitch) 2013-06-23 01:57:33 PDT
Created attachment 766393 [details] [diff] [review]
Part 9: tests

rebased
Comment 128 James Kitchener (:jkitch) 2013-06-28 18:42:06 PDT
Created attachment 769296 [details] [diff] [review]
part 1: channel and nsNetUtil changes

One word fix to allow it to compile on other platforms.
Comment 129 James Kitchener (:jkitch) 2013-06-28 18:43:03 PDT
Created attachment 769297 [details] [diff] [review]
part 4: docshell and SHEntry changes

Slight reordering to allow it to compile on other platforms.
Comment 130 James Kitchener (:jkitch) 2013-06-28 18:44:46 PDT
Created attachment 769298 [details] [diff] [review]
Part 9: tests

and another rebase.

Green on try
https://tbpl.mozilla.org/?tree=Try&rev=519607c96b3e

Unless there are any further issues, this may be ready to land.
Comment 134 bhavana bajaj [:bajaj] 2013-08-02 13:16:15 PDT
Add under the HTML5 section.
Comment 135 Ada [:adalucinet] 2013-08-28 07:38:06 PDT
James Kitchener from comment #130)
> Created attachment 769298 [details] [diff] [review]
> Part 9: tests

Noticed that this feature is covered by automated tests. Is anything in particular you want covered by QA? If yes, please let me know how could I help.
Comment 136 James Kitchener (:jkitch) 2013-08-29 02:47:53 PDT
There are a few items that aren't covered by automated tests.

1. "View frame source" for a srcdoc iframe.  (The presence of the menu option is checked, but whether it works is not.

2.Session restore.  Navigate to a page with a srcdoc iframe, perhaps navigate the iframe to another page, close and then reopen the browser.  The iframe should then correctly display the srcdoc page (in the navigation history if the frame loaded another page).

I did check that they worked locally, but there may be corner cases I hadn't considered or things have broken since I last tested them.
Comment 137 Alex Keybl [:akeybl] 2013-09-02 12:53:56 PDT
Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.

Note You need to log in before you can comment on or make changes to this bug.