337704 - Clean up files moved from xmlextras

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Now that we're in content, we can reuse some code.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

I've kept the nsISyncLoadDOMService interface, but stopped using it from content: we're in the same module so a static function seems more logical, especially since the service has no state. I also removed loadLocalXBLDocument, I don't see a lot of use-cases for it. There's no more users of nsISyncLoadDOMService in the tree after this patch.
Not too happy about the description for nsContentUtils::CreateDocument, suggestions welcome.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I might not get to this till June... But if we're moving stuff into nsContentUtils, could we have it fail out if we've already shut down layout, perhaps?

Comment 3

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> But if we're moving stuff into
> nsContentUtils, could we have it fail out if we've already shut down layout,
> perhaps?

Yes, though all callers in the patch deal with that I think. There's two calls to nsContentUtils::CanCallerAccess in the serializer that could go wrong (it uses sSecurityManager without null-checking). I can add a check before the callers or inside nsContentUtils::CanCallerAccess if you think it's an issue.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221807 [details] [diff] [review]
v1

>Index: content/base/public/nsContentUtils.h
>+   * Creates a new XML document, setting a container on the document gotten
>+   * through the docshell from the caller.

You mean "setting the document's container to be the docshell of whatever script is on the JSContext stack", right?

And comment that this is not guaranteed to continue?  I think we want to change this behavior, in fact.

>Index: content/base/src/nsSyncLoadService.cpp
>-/*
>- * A service that provides methods for synchronously loading a DOM in various ways.
>- */

Please leave this comment.  LXR will show it, which seems like a good thing to me.  If you want to move it to the header, that's fine, but it needs to come right after the license; before the header guard, etc.

>Index: content/base/src/nsSyncLoadService.h
>+     * Synchronously load the document from the specified channel.

This method doesn't take a channel...


>+     *                   referrer header. May be null if no securitychecks

"security checks"

>+    static nsresult PushSyncStreamToListener(nsIInputStream* aIn,

Document this method?

I guess the hardcoding of chrome:// and resource:// is OK...  It'd be nice to be able to ask the protocol handler whether it supports open() in a way we care about here, though.  Maybe file a followup bug?

sr=bzbarsky; you can take this as an r+sr unless you really want sicking to look at this too.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> You mean "setting the document's container to be the docshell of whatever
> script is on the JSContext stack", right?

I wasn't sure that the prescontext's container was always a docshell. Docshell is still mostly a mystery to me :-(.

Comment 6

[Peter Van der Beken [:peterv]]

I'll take bz's r/sr, but after this bug is fixed the question becomes: do we want to keep nsISyncLoadDOMService? I'd be fine with dropping it entirely.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It seems like something that extensions would use...  Do they?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Any reason extensions couldn't just use XMLHttpReqest?

I thought a new patch was coming for this bug, but I just saw it landing?

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 221807 [details] [diff] [review]
v1

Right, I took bz's offer of r/sr.

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> And comment that this is not guaranteed to continue?  I think we want to change
> this behavior, in fact.

I didn't add that comment: if we remove that part from the method, we can simply replace all callers with NS_NewDOMDocument.

This landed last week. It shaved a couple of K from codesize and seems to have reduced Ts by 5-8%. I hope that's real and not something going wrong somewhere ;-).

Regarding the nsISyncLoadDOMService, I would prefer to limit the number of different ways that we have to do the same thing. I think XMLHttpRequest and document.load provide enough, but let's move that discussion to a separate bug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I wasn't sure that the prescontext's container was always a docshell.

It is.  ;)

> Any reason extensions couldn't just use XMLHttpReqest?

The sync load service impl has a lot more complexity than XMLHttpRequest (eg sync listeners vs async listeners, etc).  Why do we have all that complexity if it's not needed?

In fact, if XMLHttpRequest does all that's needed here, why not have even the internal consumers use it?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wouldn't we have to get rid of some security issues with using XHR from C++ first?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Mmm.... yeah.  Then what about non-JS extensions and app code?  We have such in Firefox, for sure.