Bugzilla

Assignee

Updated

•

9 years ago

Flags: needinfo?(gijskruitbosch+bugs)

Assignee

Comment 3

•

9 years ago

Right, this keeps biting me when doing Firefox frontend work, so I kept poking on this until I found how to do this.

It turns out nsStyleSet has an mBindingManager which has an AppendAllStylesheets thing, so I exposed that via nsStyleSet itself and used it from inDOMUtils. That seems to work, but then we get duplicates everywhere (not just skin vs. content, but things like chrome://global/skin/listbox.css show up 5 times). This kind of makes sense because the binding manager just collects sheets off all the individual bound elements.

Then I added a hashset on the cssstylesheet object pointers in order to avoid getting these multiple times. AIUI chrome:// sheets should be cached so they should have a unique object.

That didn't seem to work in that I still see duplicates. It *looks* like this is due to different bindings including the same stylesheet. Updating any of them seems to work, though...

Also, scarily, updating these affects main browser chrome (at least with e10s turned off, not sure about e10s turned on).

I'll put up the patch and get some feedback, bearing the above in mind.

Flags: needinfo?(gijskruitbosch+bugs)

Assignee

Comment 4

•

9 years ago

Attached file MozReview Request: Bug 1157292 - include XBL stylesheets in the inspector's list of stylesheets, r?dholbert,heycam — Details

Bug 1157292 - include XBL stylesheets in the inspector's list of stylesheets, r?dholbert,heycam

Attachment #8646901 - Flags: review?(dholbert)

Attachment #8646901 - Flags: review?(cam)

Assignee

Comment 5

•

9 years ago

(In reply to :Gijs Kruitbosch from comment #4)
> Created attachment 8646901 [details]
> MozReview Request: Bug 1157292 - include XBL stylesheets in the inspector's
> list of stylesheets, r?dholbert,heycam
> 
> Bug 1157292 - include XBL stylesheets in the inspector's list of
> stylesheets, r?dholbert,heycam

Daniel, could you have a look at this and check what I'm doing is sane? Please doublecheck my use of pointer types as well, because I was a bit confused about what nsStyleSet/nsBindingManager have, what we want here, and the NS_ADDREF stuff lower down in inDOMUtils::GetAllStyleSheets - maybe I shouldn't use nsCOMPtr? So confusing.

The duplication thing is interesting and makes me wonder whether there are more savings a la bug 77999 that can be had here, so asking Cameron to take a look as well.

Self-nit:

  void AppendAllXBLStyleSheets(nsTArray<mozilla::CSSStyleSheet*>& aArray) const {
    mBindingManager->AppendAllSheets(aArray);
  }

this needs a nullcheck for mBindingManager, I guess...?

Assignee

Comment 6

•

9 years ago

(In reply to :Gijs Kruitbosch from comment #5)
> The duplication thing is interesting and makes me wonder whether there are
> more savings a la bug 77999 that can be had here, so asking Cameron to take
> a look as well.

(many thanks to jryans for pointing this out!)

Comment 7

•

9 years ago

(In reply to :Gijs Kruitbosch from comment #5)
> The duplication thing is interesting and makes me wonder whether there are
> more savings a la bug 77999 that can be had here, so asking Cameron to take
> a look as well.

(I'm not familiar with inDOMUtils or XBL stylesheet management, so heycam is probably a better "primary reviewer" here than I am. (depending on how comfortable he feels with it)  I'll take a look & post my thoughts on the reviewboard, though.))

Depends on: 734861

Comment 8

•

9 years ago

[er, I'll just post review notes here, since they're just direct responses to things you asked]

(In reply to :Gijs Kruitbosch from comment #5)
> Daniel, could you have a look at this and check what I'm doing is sane?
> Please doublecheck my use of pointer types as well, because I was a bit
> confused about what nsStyleSet/nsBindingManager have, what we want here, and
> the NS_ADDREF stuff lower down in inDOMUtils::GetAllStyleSheets - maybe I
> shouldn't use nsCOMPtr? So confusing.

Yeah, there's no need to use nsCOMPtr here.  You should just be able to directly do:
  sheets.AppendElement(sheet).
('sheet' points to a CSSStyleSheet, which derives from nsIStyleSheet, which is what 'sheets.AppendElement()' expects.)

(The NS_ADDREF stuff lower down is indeed confusing -- it looks like it's because we're returning a raw array of pointers, and we're addref'ing each entry in that array to make sure they stay alive. Presumably whoever calls this method & eventually deallocates the array will call NS_RELEASE on them.)

> Self-nit:
> 
>   void AppendAllXBLStyleSheets(nsTArray<mozilla::CSSStyleSheet*>& aArray)
> const {
>     mBindingManager->AppendAllSheets(aArray);
>   }
> 
> this needs a nullcheck for mBindingManager, I guess...?

Looks like it, yes. (We nullcheck mBindingManager all over the place in nsStyleSet.cpp at least, and it doesn't look like there are any obvious guarantees about non-nullness.)

Beyond that, this looks generally fine to me, if it works (and it sounds like it does). r=me with the above & anything that heycam catches. :)

Assignee

Comment 9

•

9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #8)
> [er, I'll just post review notes here, since they're just direct responses
> to things you asked]
> 
> (In reply to :Gijs Kruitbosch from comment #5)
> > Daniel, could you have a look at this and check what I'm doing is sane?
> > Please doublecheck my use of pointer types as well, because I was a bit
> > confused about what nsStyleSet/nsBindingManager have, what we want here, and
> > the NS_ADDREF stuff lower down in inDOMUtils::GetAllStyleSheets - maybe I
> > shouldn't use nsCOMPtr? So confusing.
> 
> Yeah, there's no need to use nsCOMPtr here.  You should just be able to
> directly do:
>   sheets.AppendElement(sheet).
> ('sheet' points to a CSSStyleSheet, which derives from nsIStyleSheet, which
> is what 'sheets.AppendElement()' expects.)

This got me an error about an ambiguous conversion to nsISupports. Does that make sense? (MSVC, if it matters)

> (The NS_ADDREF stuff lower down is indeed confusing -- it looks like it's
> because we're returning a raw array of pointers, and we're addref'ing each
> entry in that array to make sure they stay alive. Presumably whoever calls
> this method & eventually deallocates the array will call NS_RELEASE on them.)

I mean, it goes into JS, which means that if the JS gets garbage collected that should happen, AIUI?

Flags: needinfo?(dholbert)

Comment 10

•

9 years ago

(In reply to :Gijs Kruitbosch from comment #9)
> > Yeah, there's no need to use nsCOMPtr here.  You should just be able to
> > directly do:
> >   sheets.AppendElement(sheet).
> > ('sheet' points to a CSSStyleSheet, which derives from nsIStyleSheet, which
> > is what 'sheets.AppendElement()' expects.)
> 
> This got me an error about an ambiguous conversion to nsISupports. Does that
> make sense? (MSVC, if it matters)

Oops, right, because 'sheets' has type 'nsCOMArray<nsISupports>'.

What if you change it to nsCOMArray<nsIStyleSheet>? I expect everything else will still work, and that should hopefully make this ambiguous cast unambiguous.

> I mean, it goes into JS, which means that if the JS gets garbage collected
> that should happen, AIUI?

(Presumably something like that happens, yeah.)

Flags: needinfo?(dholbert)

Updated

•

9 years ago

Flags: needinfo?(gijskruitbosch+bugs)

Assignee

Comment 11

•

9 years ago

Comment on attachment 8646901 [details]
MozReview Request: Bug 1157292 - include XBL stylesheets in the inspector's list of stylesheets, r?dholbert,heycam

Bug 1157292 - include XBL stylesheets in the inspector's list of stylesheets, r?dholbert,heycam

Assignee

Comment 12

•

9 years ago

Updated the patch with the nullcheck and the type change, and the compiler was fine so I believe this should work.

Considering the duplicated stylesheets, I'm still wondering if maybe I should use a hashset on the URL for the stylesheets instead to avoid these? Not sure if that's likely to break real-world cases or something.

Flags: needinfo?(gijskruitbosch+bugs)

Updated

•

9 years ago

Attachment #8646901 - Flags: review?(dholbert)

Comment 13

•

9 years ago

Comment on attachment 8646901 [details]
MozReview Request: Bug 1157292 - include XBL stylesheets in the inspector's list of stylesheets, r?dholbert,heycam

https://reviewboard.mozilla.org/r/15883/#review14209

::: layout/inspector/inDOMUtils.cpp:81
(Diff revision 2)
>    // Get the agent, then user sheets in the style set.

This comment ("Get the agent, then user sheets") probably needs an update.

(The clause that it's adjacen to previously just grabbed those two classes of sheets, but now you're making it grab a third class.)

::: layout/style/nsStyleSet.h:349
(Diff revision 2)
> +  void AppendAllXBLStyleSheets(nsTArray<mozilla::CSSStyleSheet*>& aArray) const {

Nit: insert a newline between this method and the previous method, to keep their function-bodies more clearly separated.

Comment 14

•

9 years ago

(In reply to :Gijs Kruitbosch from comment #12)
> Considering the duplicated stylesheets, I'm still wondering if maybe I
> should use a hashset on the URL for the stylesheets instead to avoid these?
> Not sure if that's likely to break real-world cases or something.

Yeah, I don't know enough about why we've got duplicates here in the first place to be sure about whether checking pointer-values is sufficient.

But, given that hashing based on pointer-value (as you do now) is working well enough at filtering out the duplicates you're seeing, then it seems worth sticking with that, since it's cheaper and easier than grabbing & hashing the URL.

If we end up discovering that this makes us report duplicate sheets (due to having multiple XBL CSSStyleSheet objects with the same URI in some circumstances), then we can revisit this & consider hashing based on URL.

(We might already be doing some de-duplication of XBL stylesheet objects at some level, which explains why you're seeing the pointer-based hashing Just Working.  heycam would be surer than I am about this.)

Comment 15

•

9 years ago

(In reply to :Gijs Kruitbosch from comment #3)
> It turns out nsStyleSet has an mBindingManager which has an
> AppendAllStylesheets thing, so I exposed that via nsStyleSet itself and used
> it from inDOMUtils. That seems to work, but then we get duplicates
> everywhere (not just skin vs. content, but things like
> chrome://global/skin/listbox.css show up 5 times). This kind of makes sense
> because the binding manager just collects sheets off all the individual
> bound elements.

Right.

> Then I added a hashset on the cssstylesheet object pointers in order to
> avoid getting these multiple times. AIUI chrome:// sheets should be cached
> so they should have a unique object.

I don't think it's anything to do with sheets having a chrome:// URI -- instead, if the sheet is loaded through an XBL binding it should be stored on an nsXBLPrototypeResources and it'll be returned from there.  Also a few style sheets (that do happen to have chrome:// URIs) have a unique instance and come from the nsLayoutStylesheetCache.  Also the CSS Loader object will cache loaded sheets for a given document and return them in certain circumstances.  So I am not sure you'll be be guaranteed you won't get different CSSStyleSheet pointers for sheets loaded in the same document.

> That didn't seem to work in that I still see duplicates. It *looks* like
> this is due to different bindings including the same stylesheet. Updating
> any of them seems to work, though...

If they are the exact same object it makes sense updating one of them would work. :-)

> Also, scarily, updating these affects main browser chrome (at least with
> e10s turned off, not sure about e10s turned on).

Also to be expected, given we sharing the same sheet in multiple places.

Comment 16

•

9 years ago

My one concern is that it looks like this will expose XBL sheets loaded for, say, HTML form controls' bindings when inspecting content documents.  Is that right, and am I right that we don't want to do this?

Comment 17

•

9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #14)
> If we end up discovering that this makes us report duplicate sheets (due to
> having multiple XBL CSSStyleSheet objects with the same URI in some
> circumstances), then we can revisit this & consider hashing based on URL.

I agree, btw.

Comment 18

•

9 years ago

To follow up on the bug 77999 comment, yes, there is scope for further rule processor sharing, for sheets at the document level.  Filed bug 1194041 for that.

Assignee

Comment 19

•

9 years ago

(In reply to Cameron McCormack (:heycam) from comment #15)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Then I added a hashset on the cssstylesheet object pointers in order to
> > avoid getting these multiple times. AIUI chrome:// sheets should be cached
> > so they should have a unique object.
> 
> I don't think it's anything to do with sheets having a chrome:// URI --
> instead, if the sheet is loaded through an XBL binding it should be stored
> on an nsXBLPrototypeResources and it'll be returned from there.  Also a few
> style sheets (that do happen to have chrome:// URIs) have a unique instance
> and come from the nsLayoutStylesheetCache.  Also the CSS Loader object will
> cache loaded sheets for a given document and return them in certain
> circumstances.  So I am not sure you'll be be guaranteed you won't get
> different CSSStyleSheet pointers for sheets loaded in the same document.

Hm, I was thinking of the XBL/XUL cache, which I ran into in my exploration of this code. Anyway, it sounds like we do a lot of caching in a lot of different places already...

> > That didn't seem to work in that I still see duplicates. It *looks* like
> > this is due to different bindings including the same stylesheet. Updating
> > any of them seems to work, though...
> 
> If they are the exact same object it makes sense updating one of them would
> work. :-)

Well, that's the thing - I meant, even after using the pointer-based hashset, I am seeing duplicates, so AIUI that means they *shouldn't* be the exact same object... even if they're pointing to the same URI.

It sounds like you and Daniel both think that it would be appropriate to hash the URIs to avoid duplicates there. I can look into doing that as part of this patch.

Flags: needinfo?(gijskruitbosch+bugs)

Comment 20

•

9 years ago

One more thing I just thought of:

It all depends what you want to do with the returned array of CSSStyleSheet objects of course, but a worry I would have about removing the "duplicates" with the same URI (but being distinct objects) is that there's a chance one sheet applies to some bound content (thinking of the ones from bindings here) while the other sheets applies to some other.  Why we might not end up using the very same CSSStyleSheet object is a good question, but there are certain conditions that make it invalid to return the same object, such as when the first has been modified.

Assignee

Comment 21

•

9 years ago

(In reply to Cameron McCormack (:heycam) from comment #20)
> One more thing I just thought of:
> 
> It all depends what you want to do with the returned array of CSSStyleSheet
> objects of course, but a worry I would have about removing the "duplicates"
> with the same URI (but being distinct objects) is that there's a chance one
> sheet applies to some bound content (thinking of the ones from bindings
> here) while the other sheets applies to some other.  Why we might not end up
> using the very same CSSStyleSheet object is a good question, but there are
> certain conditions that make it invalid to return the same object, such as
> when the first has been modified.

Right. I don't know exactly how the modifications from the style editor are fed back to gecko, and what the effects would therefore be... Patrick, can you clarify?

Flags: needinfo?(pbrosset)

Patrick Brosset <:pbro>

Reporter

Comment 22

•

9 years ago

(In reply to :Gijs Kruitbosch from comment #21)
> Right. I don't know exactly how the modifications from the style editor are
> fed back to gecko, and what the effects would therefore be... Patrick, can
> you clarify?
Whenever a change is made in the style-editor, devtools calls this:

DOMUtils.parseStyleSheet(styleSheetObject, newCssText);

Where parseStyleSheet is:
https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inIDOMUtils.idl#156
https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inDOMUtils.cpp#1212
newCssText is a string
styleSheetObject is an object part of the list of stylesheets returned either by DOMUtils.getAllStyleSheets(document) or document.styleSheets

Flags: needinfo?(pbrosset)

Assignee

Comment 23

•

9 years ago

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #22)
> (In reply to :Gijs Kruitbosch from comment #21)
> > Right. I don't know exactly how the modifications from the style editor are
> > fed back to gecko, and what the effects would therefore be... Patrick, can
> > you clarify?
> Whenever a change is made in the style-editor, devtools calls this:
> 
> DOMUtils.parseStyleSheet(styleSheetObject, newCssText);
> 
> Where parseStyleSheet is:
> https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inIDOMUtils.
> idl#156
> https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inDOMUtils.
> cpp#1212
> newCssText is a string
> styleSheetObject is an object part of the list of stylesheets returned
> either by DOMUtils.getAllStyleSheets(document) or document.styleSheets

OK, then it actually sounds like we shouldn't deduplicate the list any further inside the inDOMUtils API (ie all unique objects, even if they have duplicate URLs, should appear in it) and we should adapt the style editor to be able to handle several stylesheet objects per URI, and to update all of them.

That is, unless Gecko does magic for us in parseSheet, in which case we can still filter the list devtools-side... I would need to investigate this further with a minimal-ish testcase - most of our XUL about: pages are now styled with the in-content common styles which override almost everything from the XBL styling, so it's hard to figure out what's going on there when there are duplicates - but that also seems to be part of why there *are* duplicates. :-\

I also imagine there might be cases where (theoretically) other consumers could care about the individual objects, for instance if they want to associate them with elements / bindings again... I'll leave Cameron to review the current patch as-is, then. :-)

Flags: needinfo?(gijskruitbosch+bugs)

Assignee

Comment 24

•

9 years ago

(In reply to Cameron McCormack (:heycam) from comment #16)
> My one concern is that it looks like this will expose XBL sheets loaded for,
> say, HTML form controls' bindings when inspecting content documents.  Is
> that right, and am I right that we don't want to do this?

Oh, sorry I missed this. From the API perspective, yes, but not in user-visible devtools - it will only use document stylesheets for content pages, and only calls into inDOMUtils for chrome pages. See https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/stylesheets.js#153 .

Comment 25

•

9 years ago

(In reply to :Gijs Kruitbosch from comment #23)
> OK, then it actually sounds like we shouldn't deduplicate the list any
> further inside the inDOMUtils API (ie all unique objects, even if they have
> duplicate URLs, should appear in it) and we should adapt the style editor to
> be able to handle several stylesheet objects per URI, and to update all of
> them.

That sounds like a good enough approach.

Brian Grinstead [:bgrins]

Comment 26

•

9 years ago

Comment on attachment 8646901 [details]
MozReview Request: Bug 1157292 - include XBL stylesheets in the inspector's list of stylesheets, r?dholbert,heycam

https://reviewboard.mozilla.org/r/15883/#review14421

Just to make clear what "merging" in devtools style sheets with the same URI would mean, you could imagine content like:

  <!DOCTYPE html>
  <link rel=stylesheet href=one.css type=text/css>
  <link rel=stylesheet href=two.css type=text/css>
  <link rel=stylesheet href=one.css type=text/css>

showing only a single style sheet, even though there are actually two sheets loaded.  Might the user want to modify just one of them?  Maybe, thought it's probably a rare situation.  The server might have even returned different content for the different a.css fetches.  Still it is probably useful to show to the user that there're actually two one.css style sheets loaded there.  You'll find two of them in document.styleSheets, too.

Given comment 24, this would only happen when inspecting Chrome pages.  But if you're OK with that behaviour, then r=me.

Attachment #8646901 - Flags: review?(cam) → review+

Comment 27

•

9 years ago

(In reply to Cameron McCormack (:heycam) from comment #26)
> Just to make clear what "merging" in devtools style sheets with the same URI
> would mean, you could imagine content like:
> 
>   <!DOCTYPE html>
>   <link rel=stylesheet href=one.css type=text/css>
>   <link rel=stylesheet href=two.css type=text/css>
>   <link rel=stylesheet href=one.css type=text/css>
> 
> showing only a single style sheet, even though there are actually two sheets
> loaded.  Might the user want to modify just one of them?  Maybe, thought
> it's probably a rare situation.  The server might have even returned
> different content for the different a.css fetches.  Still it is probably
> useful to show to the user that there're actually two one.css style sheets
> loaded there.  You'll find two of them in document.styleSheets, too.
> 
> Given comment 24, this would only happen when inspecting Chrome pages.  But
> if you're OK with that behaviour, then r=me.

Would editing the 'merged' sheet end up modifying the contents of both in the page?  If so, that's probably OK for chrome only pages (where the server doesn't seem likely to have returned different content for a.css)

Assignee

Updated

•

9 years ago

Depends on: 1049199

Pulsebot

Comment 28

•

9 years ago

https://hg.mozilla.org/integration/fx-team/rev/f4cc8e8b1f19

Assignee

Updated

•

9 years ago

Depends on: 1195461

Assignee

Comment 29

•

9 years ago

Making the bug reflect reality.

Component: Developer Tools: Style Editor → Layout: Misc Code

Product: Firefox → Core