There are some use-cases for being able to specify that a docShell is "anonymous", which translates to "use the LOAD_ANONYMOUS flag for all requests made by this docShell" A little more context at https://groups.google.com/forum/#!topic/mozilla.dev.tech.network/X8t4Sob3l9U

This patch adds an isAnonymous attribute to nsIDocShell and tries to use it correctly - hopefully somewhat as described in the dev.tech.network thread. FTR, it seems likely that the same basic use-case we have might want an extra flag or 2 to be treated in the same way (eg, LOAD_BYPASS_CACHE). I only mention this incase it changes what approach we should use (eg, maybe we should call the flag something like |isIsolated| (isPrivate would be confusing WRT private-browsing mode) so additional LoadFlags could be added later behind the same single flag) Anyway - this patch is mainly to get the ball rolling...

Comment on attachment 795294 [details] [diff] [review] 0001-Bug-909218-Add-isAnonymous-flag-to-nsIDocShell-and-p.patch >+ // XXX - should we enumerate existing children like IsActive does above? My gut feeling is yes. >+ // but maybe the parent is We wouldn't need this if we propagated the set to kids, right? >+ * before the docShell loads any content. The default is that docShell's are s/docShell's/docShells/

This is an alternative, more general patch that adds 'loadFlags' to nsIDocShell, but is otherwise similar to the previous patch (but with feedback comments addressed). This patch would allow us to add flags other than LOAD_ANONYMOUS. An obvious issue with this is that the patch to nsLoadGroup still only passes LOAD_ANONYMOUS, whereas we are likely to want a few others too - eg, possibly LOAD_BYPASS_CACHE and INHIBIT_PERSISTENT_CACHING, which we could discuss in a followup. Note I haven't actually tested this yet - I just thought I'd run it up the flagpole and see if bz salutes :)

Comment on attachment 796488 [details] [diff] [review] Add 'loadFlags' attribute to nsIDocShell This seems ok, but maybe we should call these defaultLoadFlags or something?

Updated to use nsIDocShell.defaultLoadFlags

This patch adds nsILoadGroup::defaultLoadFlags, and these flags are always "merged" into new requests for this docShell. Setting nsIDocShell::defaultLoadFlags passes the flags onto the docShell's mLoadGroup. Note that LOAD_ANONYMOUS is no longer included in the "default" merged flags - that could be a followup if desired, but with this patch you get the same effect by adding LOAD_ANONYMOUS to the docShell's defaultLoadFlags. First cut at tests following...

Tests - we load an iframe containing a HTML file that references .css, .js and .png resources. Before loading this, we add a progress listener and check the request flags on each of the requests have been propagated correctly from the docShell. I'll probably expand the test to include a XHR request and a sub-iframe to ensure the flags also end up correctly on them (but I've no reason to believe they will not get them too)

Minor fix - unsaved editor buffer!

Comment on attachment 797679 [details] [diff] [review] 0003-Bug-909218-tests.patch Seems fine.

Comment on attachment 797847 [details] [diff] [review] 0002-add-defaultLoadFlags-to-nsILoadGroup.patch Seems ok, but I'd like a necko peer to review.

Comment on attachment 797847 [details] [diff] [review] 0002-add-defaultLoadFlags-to-nsILoadGroup.patch Honza, I picked you (almost) at random as a Necko peer. Can you give high-level feedback on this patch so I can be confident it's worth spending more time fleshing out the tests etc? Obviously, feel free to pass this on to another Necko peer if desired.

Comment on attachment 797847 [details] [diff] [review] 0002-add-defaultLoadFlags-to-nsILoadGroup.patch Review of attachment 797847 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDocShell.cpp @@ +5404,5 @@ > + // and if we have a load group, we add these bits to the flags that should > + // be set on all requests in this group. > + if (mLoadGroup) { > + mLoadGroup->SetDefaultLoadFlags(aDefaultLoadFlags); > + } We may want an NS_WARNING() when the load group is not here (however, it shouldn't happen according that it is created in docshell's xpcom constructor - NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsDocShell, Init)). ::: netwerk/base/public/nsILoadGroup.idl @@ +82,5 @@ > readonly attribute nsILoadGroupConnectionInfo connectionInfo; > + > + /** > + * The set of load flags that will be added to all requests in this group. > + */ I lack a bit more documentation here (or it just needs to be made more clear): - are the flags added to all request currently in the group? - or are the flags added only to new requests only? - when I set a flag with this API and then remove it later again with this API, what happens? This is all clear when looking into the code, but not when looking only at this comment. Few words about the flags being inherited would be good too..

Comment on attachment 797675 [details] [diff] [review] 0001-first-cut-at-adding-nsIDocShell.loadFlags.patch Review of attachment 797675 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDocShell.cpp @@ +5391,5 @@ > +{ > + mDefaultLoadFlags = aDefaultLoadFlags; > + // Recursively tell all of our children. We *do not* skip > + // <iframe mozbrowser> children - if someone sticks custom flags in this > + // docShell then they too get treated as anonymous. if someone sticks custom flags in this // docShell then they too get treated as anonymous Hmm... this doesn't sound right :) and also opens a question whether we should somehow filter the load flags, I personally don't have an opinion tho...

Comment on attachment 797679 [details] [diff] [review] 0003-Bug-909218-tests.patch Thanks! Any thoughts on the tests - eg, what else should be covered or a better approach?

Comment on attachment 797679 [details] [diff] [review] 0003-Bug-909218-tests.patch Review of attachment 797679 [details] [diff] [review]: ----------------------------------------------------------------- Yeah: - write reasonable detailed description (or list of steps) what the test is exactly doing, so understanding it is simpler - mark where the tests start (entry point) ::: docshell/test/chrome/test_bug909218.html @@ +43,5 @@ > +// an nsIWebProgressListener that checks all requests made by the docShell > +// have the flags we expect. > +RequestWatcher = { > + init: function(docShell, url, expectedRequests, callback) { > + this.url = url; maybe call 'this.url' like 'this.documentURL' or 'rootURL' or so... @@ +52,5 @@ > + getInterface(Ci.nsIWebProgress). > + addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATE_REQUEST | > + Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT); > + this.requestCounts = {}; > + for (var url of EXPECTED_REQUESTS) { so, EXPECTED_REQUESTS or expectedRequests? @@ +61,5 @@ > + finalize: function() { > + ok(Object.keys(this.requestCounts).length, "we expected some requests"); > + for (var url in this.requestCounts) { > + var count = this.requestCounts[url]; > + ok(count >= 1, url + " saw " + count + " requests"); while exactly not == 1 ? @@ +99,5 @@ > + // downloadable stops loading. > + function basic() { > + var iframe = insertIframe(); > + var docShell = docshellForWindow(iframe.contentWindow); > + RequestWatcher.init(docShell, TEST_URL, EXPECTED_REQUESTS, function() { Instead EXPECTED_REQUESTS just inline the urls?

Very similar to previous patch with feedback addressed.

This too is very similar to the previous one, with feedback addressed.

This test is cleaned up significantly from the previous version. Unfortunately I couldn't work out a reasonable way to ensure XHR requests made by the page also have the correct flags as the docShell's progress listener doesn't report them - but I did verify in the debugger that they do.

Comment on attachment 799261 [details] [diff] [review] 0001-Bug-909218-add-defaultLoadFlags-to-nsIDocShell.-r-bz.patch r=me

Comment on attachment 799262 [details] [diff] [review] 0002-Bug-909218-add-defaultLoadFlags-to-nsILoadGroup-and-.patch Review of attachment 799262 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab thanks. ::: docshell/base/nsDocShell.cpp @@ +5402,5 @@ > docshell->SetDefaultLoadFlags(aDefaultLoadFlags); > } > + NS_ASSERTION(mLoadGroup, "We must have a load group"); > + // Tell the load group to set these flags all requests in the group > + mLoadGroup->SetDefaultLoadFlags(aDefaultLoadFlags); I liked the non-null check. Nit: maybe for consistency set the load group flags before you iterate the children. You first set default load flags on the docshell, then on children, they set in on their load groups, and after that you set it on the root load group. But this is just a detail and if you have a reason to keep it like this, then do so.

Comment on attachment 799263 [details] [diff] [review] 0003-Bug-909218-test-defaultLoadFlags-make-it-to-all-requ.patch Review of attachment 799263 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/chrome/test_bug909218.html @@ +124,5 @@ > + > +</script> > +</head> > +<body> > +<p id="display"> What's this for?

Thanks! Try at https://tbpl.mozilla.org/?tree=Try&rev=2c01a45f1fd4 https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd87c75e840 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e289a695226 https://hg.mozilla.org/integration/mozilla-inbound/rev/59926e3b88a0

https://hg.mozilla.org/mozilla-central/rev/1cd87c75e840 https://hg.mozilla.org/mozilla-central/rev/9e289a695226 https://hg.mozilla.org/mozilla-central/rev/59926e3b88a0

Mark, please make sure to actually note *all* the reviewers in the commit message.

(In reply to :Ms2ger (away 11-21 September) from comment #24) > Mark, please make sure to actually note *all* the reviewers in the commit > message. I believe I did. Each patch had only one formal reviewer. bz gave f+ to a number of patches, but only r+ to one of them and I mentioned that in that commit. Honza gave r+ in 2 and I correctly attributed him in them (and in the non-test patch reviewed by Honza, bz explicitly asked for a Necko peer to review)