Jonathan Watt [:jwatt] Assignee
	Description • 10 years ago

We should avoid loading the xul.css UA style sheet when possible. Currently we load it for _all_ documents because ua.css (which is loaded for all documents) @import's it. This means that, despite the fact that XUL is disabled on the web, we load it for HTHL documents, SVG documents, SVG-as-an-image, etc.

	Jonathan Watt [:jwatt] Assignee
	Comment 1 • 10 years ago

This saves us slightly over 44 KB per-document (and SVG-as-image) on OS X.

	Nicholas Nethercote [inactive]
	Comment 2 • 10 years ago

Nice!

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 10 years ago

Does it really save us things for non-SVG documents?  I would have thought scrollbars would trigger the catalog sheet bit, no?

	Jonathan Watt [:jwatt] Assignee
	Comment 4 • 10 years ago

Yes. In fact even for non-SVG-as-an-image xul.css gets pulled in. I'm currently looking into a lot of things surrounding which style sheets we load, but will come back to this soon. My thinking on this at the moment is to move things around and change the current separation of rules a bit.

	Nicholas Nethercote [inactive]
	Comment 5 • 10 years ago

Does this apply to all platforms equally? If so, it'll be great on B2G.

	Jonathan Watt [:jwatt] Assignee
	Comment 6 • 10 years ago

This splits out the XUL rules that apply to XUL elements that can be implicitly created for SVG/XUL from xul.css into a separate UA sheet called minimal-xul.css.

	Jonathan Watt [:jwatt] Assignee
	Comment 7 • 10 years ago

Hopefully this fully passes Try now.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 10 years ago

Comment on attachment 8422583 [details] [diff] [review]
patch

>+++ b/content/base/public/nsIDocument.h
>+   * to say a regular HTML document with Content-Type "text/html". Returns
>+   * false for any other type of document including XHTML documents.

Sadly, no.  text/plain documents, image documents, media documents are all IsHTML() per spec and in our implementation.

I think this comment should say:

   * Returns true if this is an "HTML document" as defined by HTML 5.  Returns
   * false for documents which were parsed by the XML parser, including XHTML
   * documents.

>+  bool IsXBL() const;

As implemented this is not a time-invariant property of the document.  I think if we want such a predicate we should just have a write-once mIsXBL boolean that the XBL content sink sets...

But why are XBL documents special?  What about, say, DOMParser-created documents?  Seems like checking IsLoadedAsData() || IsLoadedAsInteractiveData() would work better here.

>+++ b/content/xul/content/src/nsXULElement.cpp
>+  if (!doc) {
>+    doc = aParent->GetCurrentDoc();

In BindToTree(), we're guaranteed that aDocument == aParent->GetCurrentDoc(), so this code is pointless.

>+      NS_ASSERTION(!needAllXULStyleRules, // XXXjwatt update to NS_ABORT_IF_FALSE
>+                        "Unexpected XUL element in non-XUL document - "

Weird indent.

I don't understand this assert.  Is this basically asserting that XUL elements of types not listed in XULElementMayBeCreatedImplicitlyByNonXULDoc are never used outside videocontrols in HTML documents?  There's no way to enforce that in the face of extensions, I'd think.  I suppose we could just break compat with extensions that do this sort of thing, but in that case we should make that clear (and cc Jorge on this bug, etc).

>+++ b/layout/base/nsDocumentViewer.cpp

So the minimal xul sheet has higher specificity than the xul sheet here.  Is that OK?  I guess it shouldn't matter much.

I didn't review the CSS changes propert; I assume Neil will do that.

	Jonathan Watt [:jwatt] Assignee
	Comment 9 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #8)
> I think this comment should say:
> 
>    * Returns true if this is an "HTML document" as defined by HTML 5. 
> Returns
>    * false for documents which were parsed by the XML parser, including XHTML
>    * documents.

Thanks.

> But why are XBL documents special?  What about, say, DOMParser-created
> documents?  Seems like checking IsLoadedAsData() ||
> IsLoadedAsInteractiveData() would work better here.

It's just a case of knowing the right functions to call. Those sound right, thanks.

> >+++ b/content/xul/content/src/nsXULElement.cpp
> >+  if (!doc) {
> >+    doc = aParent->GetCurrentDoc();
> 
> In BindToTree(), we're guaranteed that aDocument ==
> aParent->GetCurrentDoc(), so this code is pointless.

That's not exactly what the comment for nsIContent::BindToTree lead to to believe. It says

  * @param aDocument The new document for the content node.  Must match the
  *                  current document of aParent, if aParent is not null.
  *                  May not be null if aParent is null.

The "May not be null if aParent is null" seems to imply that it could be null (and if so the document can be obtained from aParent). If it can never be null, I'm happy to change this of course.

> Weird indent.

Yeah, I'm going to change it to NS_ABORT_IF_FALSE after finishing with Try, for which the indent is correct.

> I don't understand this assert.  Is this basically asserting that XUL
> elements of types not listed in XULElementMayBeCreatedImplicitlyByNonXULDoc
> are never used outside videocontrols in HTML documents?  There's no way to
> enforce that in the face of extensions, I'd think.  I suppose we could just
> break compat with extensions that do this sort of thing, but in that case we
> should make that clear (and cc Jorge on this bug, etc).

I've made sure all UA rules for XUL elements that can be created implicitly by HTML are in minimal-xul.css, except for videocontrols and its descendants. This means we only need to check if the element is a videocontrols element to know if we need to pull in xul.css dynamically, which keeps the overhead for nsXULElement::BindToTree to a minimum. The point of the assertion is to make sure that if XUL elements that are not under videocontrols or for whom the rules are not in minimal-xul.css start being used in HTML, we notice. Otherwise it would be annoying to debug why things are not styled correctly (the appropriate rules are not being loaded).

The alternative of course is to just make nsXULElement::BindToTree load xul.css dynamically if XULElementMayBeCreatedImplicitlyByNonXULDoc() returns false. It's just a little more overhead on the binding of every XUL element to do so. Maybe that's fine though...

> >+++ b/layout/base/nsDocumentViewer.cpp
> 
> So the minimal xul sheet has higher specificity than the xul sheet here.  Is
> that OK?  I guess it shouldn't matter much.

I think it shouldn't matter, but probably it's better for it to have lower specificity. I'll switch the order of the code that pulls in minimal-xul.css and xul.css.

> I didn't review the CSS changes propert; I assume Neil will do that.

That's what my request of him is for.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 10 years ago

> seems to imply that it could be null (and if so the document can be obtained from aParent)

aDocument can be null, but in that case aParent is definitely not null (though this is not obvious from the API comments) and aParent->GetCurrentDoc() is null.

What the API comments are saying is that if aParent is not null then aParent->GetCurrentDoc() == aDocument.

This is all present in assertion form in Element::BindToTree:

1085   NS_PRECONDITION(aParent || aDocument, "Must have document if no parent!");
...
1088   NS_PRECONDITION(!aParent || aDocument == aParent->GetCurrentDoc(),
1089                   "aDocument must be current doc of aParent");

> I've made sure all UA rules for XUL elements that can be created implicitly by HTML

The point is extensions can create XUL elements and stick them into web documents.

Speaking of which, what sort of elements do we use for the editor's resize handles?

> It's just a little more overhead on the binding of every XUL element to do so.

Yeah.  We can fall back to that if needed, but like you I'd like to avoid it if possible.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 10 years ago

Oh, and we should totally improve API docs as needed.

	Jonathan Watt [:jwatt] Assignee
	Comment 12 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #10)
> Speaking of which, what sort of elements do we use for the editor's resize
> handles?

A 'resizer' element, which is why XULElementMayBeCreatedImplicitlyByNonXULDoc has a line for that.

> > It's just a little more overhead on the binding of every XUL element to do so.
> 
> Yeah.  We can fall back to that if needed, but like you I'd like to avoid it
> if possible.

I think we're going to have to do that.

Try turned up one other instance where we create XUL elements that are not currently handled by XULElementMayBeCreatedImplicitlyByNonXULDoc. That's the <binding id="feedreaderUI"> binding (only pulled in by subscribe.xhtml#feedSubscribeLine) which creates XUL elements that have their own bindings. So it pulls in lots of different types of element.

Unlike for 'videocontrols' there's not a cheap way of detecting that we're pulling in feedreaderUI, so I think we will have to check new XUL elements against the list that's known to have their rules in minimal-xul.css unfortunately. :/

	Jonathan Watt [:jwatt] Assignee
	Comment 14 • 10 years ago

With the patch for bug 1011024 this passes Try.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 10 years ago

> A 'resizer' element

No, I'm not talking about the resizer on textareas.  I'm talking about the resize handles that, say, <img> gets inside a contenteditable area.

Or are those <resizer> too?

In any case, that's somewhat moot given the rest of comment 12.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 10 years ago

Comment on attachment 8424041 [details] [diff] [review]
patch

>+    if (ancestor->NodeInfo()->Equals(nsGkAtoms::videocontrols, kNameSpaceID_XUL)) {

  if (ancestor->IsXUL(nsGkAtoms::videocontrols)) {

>+ * id="feedreaderUI"> binding or one of the bindings bound to such an element.
>+ * element in one of the binding for such an element. Only

Some copy/paste duplication there?

>+  nsIContent* bindingParent = aElement->GetBindingParent();
>+  while (bindingParent->GetBindingParent()) {

This will crash if aElement->GetBindingParent() is null, no?

But in general, I don't understand why this loop is here.  What is this code trying to do?

In fact, what is the assertion trying to do, really?

>+    if (!XULElementsRulesInMinimalXULSheet()) {

Need to pass the tag, no?

	Jonathan Watt [:jwatt] Assignee
	Comment 17 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #16)
> >+  nsIContent* bindingParent = aElement->GetBindingParent();
> >+  while (bindingParent->GetBindingParent()) {
> 
> This will crash if aElement->GetBindingParent() is null, no?

Oops. Fixed.

> But in general, I don't understand why this loop is here.  What is this code
> trying to do?
>
> In fact, what is the assertion trying to do, really?

The loop is there because checking the first binding parent isn't enough when the nsXULElement is from a binding that is bound to one of the elements that is in the feedreaderUI binding. In other words it's because we bindings applied to the elements in a binding in this case.

The assertion is there in an attempt to make sure that we notice if something changes and we start to frequently create XUL elements that we don't currently know about. This patch's win will be completely negated if <p>, say, gets a binding that creates a XUL element that isn't handled by minimal-xul.css, since we'd then start pulling in xul.css for virtually every page. I'm trying to avoid that happening without us noticing.

> >+    if (!XULElementsRulesInMinimalXULSheet()) {
> 
> Need to pass the tag, no?

Yup, had that fixed locally.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 10 years ago

> Oops. Fixed.

OK.  Can I see the fixed code?  This is basically trying to find the non-anonymous element that dragged us in, right?

> I'm trying to avoid that happening without us noticing

OK.  I'm worried about this asserting in common configurations with extensions installed, but I guess that would be useful information...

In fact, perhaps we should add telemetry for this case, or even console warnings, to make sure people notice even in non-debug builds?

	Jonathan Watt [:jwatt] Assignee
	Comment 19 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #18)
> > Oops. Fixed.
> 
> OK.  Can I see the fixed code?  This is basically trying to find the
> non-anonymous element that dragged us in, right?

Yes.

> > I'm trying to avoid that happening without us noticing
> 
> OK.  I'm worried about this asserting in common configurations with
> extensions installed, but I guess that would be useful information...

I think so.

> In fact, perhaps we should add telemetry for this case, or even console
> warnings, to make sure people notice even in non-debug builds?

I'll look into that.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 10 years ago

Comment on attachment 8424091 [details] [diff] [review]
patch

>+  while (bindingParent) {
>+    bindingParent = bindingParent->GetBindingParent();
>+  }
>+  if (bindingParent) {

That if condition always tests false.

Note that the telemetry/warnings can be followups...

	Jonathan Watt [:jwatt] Assignee
	Comment 21 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #20)
> That if condition always tests false.

Argh. Is it the end of the week yet?

> Note that the telemetry/warnings can be followups...

Let's do that.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 10 years ago

Comment on attachment 8424148 [details] [diff] [review]
patch

r=me

	Jonathan Watt [:jwatt] Assignee
	Comment 24 • 10 years ago

Comment on attachment 8424148 [details] [diff] [review]
patch

Thanks!

	Neil Deakin
	Comment 25 • 10 years ago

Comment on attachment 8424148 [details] [diff] [review]
patch

>+ * THIS FILE IS LOCKED DOWN.  YOU ARE NOT ALLOWED TO MODIFY IT WITHOUT FIRST
>+ * HAVING YOUR CHANGES REVIEWED BY enndeakin@sympatico.ca
>+ */

You might as well update the email address in both places to enndeakin@gmail.com The other one hasn't worked in years.

	Jonathan Watt [:jwatt] Assignee
	Comment 26 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b42ec325e34d

	Carsten Book [:Tomcat]
	Comment 27 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/b42ec325e34d

	Jorge Villalobos [:jorgev] (he/him)
	Comment 29 • 10 years ago

This might break some add-ons, but it's difficult to identify them. I don't expect it to be a big number, since most just inject HTML code.

	Florian Bender
	Comment 30 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #11)
> Oh, and we should totally improve API docs as needed.

Did this happen?