713564 - Add an API to declare a stylesheet obsolete

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

We have no API to tell the CSS Loader a given stylesheet is obsolete. This
is needed by web developer tools and standalone editors if they modify
a linked stylesheet to make the browser or the editor reload the stylesheet
w/o having to reload the document. Reloading the document is not an option
if the document has been modified by DOM editing tools or an editor.

Comment 1

[Srinath N [:srinath]]

Hi BZ,

I'm interested in working on this. Are you available for mentoring?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here.  I am, yes.

Comment 3

[Mina Almasry]

Hey bz, it's Mina from bug776032. I think I can work on this too since I've been reading the CSS Parser, if that's okay.

Where do I start with this? Where's the CSS Loader defined? Or did glazou mean CSS Parser? Is there a mechanism that the parser reports a stylesheet is obsolete and I just have to expose it, or do I have to find out if a sheet is obsolete?

Comment 4

[Boris Zbarsky [:bzbarsky]]

The CSS loader is in layout/style/Loader.cpp

There is no mechanism for this; one needs to be added.

Comment 5

[Mina Almasry]

Attachment: proposed patch

Can you please assign me this?

How about this mechanism? parser.SheetIsObsolete() tells you if the last sheet parsed is obsolete. parser.ObsoleteDeclarations() tells you what exactly was obsolete.

Those would be checked right after a call to parser.ParseSheet()

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 759353 [details] [diff] [review]
proposed patch

Why is the parser involved?  Why not just nuke the sheet from the object cache in the CSS loader of the relevant document and be done?

Comment 7

[Mina Almasry]

Attachment: proposed patch v2

My bad, I completely misunderstood what was needed. Here is another go at it. Is this what you want?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 769585 [details] [diff] [review]
proposed patch v2

No, the idea here is not to remove things from the document but to remove things from the hashtables the css Loader has mapping URIs to stylesheets.

Comment 9

[Mina Almasry]

Attachment: patch

Sincerely sorry about that. I'm sometimes not familiar with the terms you use. 

When you say the hashtables, I am guessing you mean this: http://dxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#l1092

I have made another patch that removes the sheet from that table. Here it is.

If it's still not what you want, and you still want me working on this, do you mind breaking it down for me a little farther. What exactly is needed and where do I start?

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

This is much closer, yes.  The mCompleteSheets is the right data structure to remove from.

That said, there are a few more things to be done here:

1)  Nothing ensures mCompleteSheets is initialized here.  The function should just do nothing if it's not, since in that case there is nothing to do.

2)  Probably better to throw or silently do nothing on null aURI instead of crashing/asserting.

3)  The goal of this bug is to have a scriptable API (probably a ChromeOnly API on Document) for this.  So that needs to be added too.

4)  The caller shouldn't need to worry about the principal and the CORS mode (esp. because the latter can be hard to determine).  Just have the caller pass in the URI, then enumerate the hashtable and remove all the entries that have a matching URI, no matter what their principal and CORS mode are.

You can see an example of an enumeration over a hashtable that removes some entries if you look at the nsPreflightCache::RemoveExpiredEntries function in nsCrossSiteListenerProxy.cpp.

Comment 11

[Mina Almasry]

Attachment: patch

Okay I've done all the things in your last review. I wasn't sure where to put the scriptable API. I added it to nsIStyleSheetService.idl

I wasn't sure how to get to the right loader from nsStyleSheetService.cpp, so right now this patch just creates a new loader.

Is this what you want? I assume this will need a test too. Can you point me to where to start with writing those?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

The right place to put the API is in Document.webidl.  That will make the "how to get to the loader" bit trivial.  Creating a new loader can't work right, because it won't have anything in the hashtable...

As for writing a test...  I guess what you can do is a mochitest that uses an sjs that serves up a different stylesheet every time it's accessed (e.g. something with "color(accesscount, 0, 0)" in it somewhere) and sends no-cache headers.  Then you link to that sheet, remove the link from the DOM, insert a new link to the same url and check that there was no new access, then call our new method, insert _another_ <link> to the same url, and check that this time we hit the server.

>+  nsIURI* sheetURI = dynamic_cast<nsURIHashKey*>(aKey)->GetKey();

You can't use dynamic_cast in Gecko code, last I checked: we build without RTTI.  Luckily, I don't think you need to: Just add a GetURI() method on URIPrincipalAndCORSModeHashKey and use it here.

Also, Equals() on a URI can return an error code; in that case you do not want to examine areEqual; better to just assume it's false.

>+Loader::ObsoleteSheet(nsIURI* aURI) {

Curly brace on next line, please.

Comment 13

[Mina Almasry]

Attachment: patch

So I made the changes you asked for, and added a test. But I have two questions and a problem:

- I had to remove [ChromeOnly] to access the function document.obsoleteSheet in the mochitest. Is that okay, or should I be doing something else?
- do you want the javascript api to take a uri, or take a DOMString and internally turn that into a uri? so far I've assumed it takes a uri.

problem: I wrote the test like you told me, and it fails. I tried stepping into the code with gdb. It's a bit hard, but I figured that before the call to document.obsoleteSheet, the mCompleteSheets table has 4 entries (which is strange because at that point in the code, only 3 sheets are there). And after the call, only 2 style sheets remain in the table. (side note, in the test I call document.obsoleteSheet twice just to be able to step through the code again). And yet, the code doesn't behave the way you thought it would.

Did I write the test wrong?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Is that okay, or should I be doing something else?

It's not OK.  Leave the ChromeOnly and do SpecialPowers.wrap(document).obsoleteSheet.

Also, why does obsoleteSheet return a boolean?  What does that boolean mean?

> do you want the javascript api to take a uri, or take a DOMString and internally turn
> that into a uri? so far I've assumed it takes a uri.

This is WebIDL, so we might as well be all user-friendly and have two overloads, one taking a string and one taking an nsIURI.  Internally the string version can call NS_NewURI and then invoke the nsIURI version.

>+                 date.getMilliseconds() % 256 +

This is super-fragile.  Why not do what I said with a counter (declared at global scope in the sjs) and incrementing it?

>+    is(content.style.backgroundColor, previousBgColor, "sheet has changed");

You need to wait for the sheet to load, no?  <link> elements have a "load" event that fires when the sheet is don

>+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

There should be no new enablePrivilege stuff in mochitests.  Yet another reason to have a string overload.

>+    isnot(content.style.backgroundColor, previousBgColor, "sheet has not changed");

Again, you have to wait for the sheet to load...

>+    boolean obsoleteSheet(URI sheetURI);

This should be in the "Mozilla extensions of various sorts" partial interface, not in the partial interface defined in the CSSOM spec.

Comment 15

[Mina Almasry]

Attachment: patch

Okay so I have made all the changes we talked about, and it looks like this patch does what we want. It passes the test I had previously with the date.getMilliseconds()

But I am having trouble writing the test. Declaring accessCount at global scope, incrementing it, and returning it just returns rgb(1, 0, 0) all the time. I feel a bit helpless debugging this because I have no idea what mochitest does to the sjs. Here is the patch. Any clues?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Er, looks like I was wrong about using a global.  See https://developer.mozilla.org/en-US/docs/Mochitest#How_do_I_keep_state_across_loads_of_different_server-side_scripts.3F for what you really want here.

Comment 17

[Mina Almasry]

Attachment: patch

Alright, with that the test works fine. This patch builds, and passes the test written for it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

>+  void ObsoleteSheet(nsIURI *aSheetURI, mozilla::ErrorResult& rv);
>+nsIDocument::ObsoleteSheet(nsIURI *aSheetURI, ErrorResult& rv) {

Curly on next line, please.

>+  return;

No need for that line.

>+nsIDocument::ObsoleteSheet(const nsAString& aSheetURI, ErrorResult& rv) {

Again, curly on next line.

>+  nsCOMPtr<nsIURI> URI;

"uri" is better as a name, I think.  Sadly.

>+++ b/content/base/test/test_declare_stylesheet_obsolete.html
>+    SimpleTest.finish();

This should be right after the "Sheet should change after obsoleted and reinserted" test.  Otherwise this finish() call will happen before the async sheet stuff runs, and the test won't actually test those bits.

Why are we ending up with exceptions for URIs that happen to not map to a stylesheet, by the way?  That seems like it just makes this API hard to use...  And I can't tell why that's happening on the C++ side, which confuses me. Why does passing "" or "foo" to this API throw??

>+++ b/layout/style/Loader.cpp
>+Loader::RemoveEntriesWithURI(URIPrincipalAndCORSModeHashKey* aKey,
>+  if (areEqual && NS_SUCCEEDED(rv)) {

Those tests should be reversed, because you don't want to examine areEqual at all if rv failed.

r=me with the above fixed and a clear explanation of what's going on with the exceptions.

Comment 19

[Mina Almasry]

> Why does passing "" or "foo" to this API throw??

The call to NS_NewURI can fail; if it does, nsIDocument::ObsoleteSheet throws whatever error NS_NewURI came up with. So passing in a DOMString to the API that NS_NewURI can't construct a URI with, fails.

"": fails
"foo": fails
"www.mozilla.org": fails
"http://www.mozilla.org": succeeds... 

I've updated the test to show that. Is that behaviour that we want? I think we'd like www.mozilla.org to succeed, but I'm not sure what to do about it. Do you want me to look into changing NS_NewURI so that it accepts something like www.mozilla.org, or mozilla.org?

> Why are we ending up with exceptions for URIs that happen to not map to a stylesheet, by the
> way?

That's actually not what happens. A call to www.mozilla.org in the test succeeds, but removes no stylesheet, I think (since we don't have a stylesheet at mozilla.org in the test).

Which makes me wonder... wouldn't we want to tell people using the API that it didn't remove any sheets? One idea I have is to make the API return the number of sheets removed from mCompleteSheets. We can do that by querying mCompleteSheets.Count() before and after the call to mCompleteSheets.Enumerate. How about that?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> The call to NS_NewURI can fail

Oh, I missed that you're not passing the document URI as a base URI there.  OK, that makes sense.

> Is that behaviour that we want?

I think it's a reasonable behavior, yeah.

> wouldn't we want to tell people using the API that it didn't remove any sheets?

I don't see a really good reason to, honestly.  Unless the idea is to let people catch typos in their URIs or something....  Let's keep it simple for now.

Comment 21

[Mina Almasry]

Attachment: final patch candidate

Alrighty then. This patch has all the changes you asked and should be ready for check-in.

Would you vouch for me on commit access level 1? :mounir has mentored me on a couple of patches too, and bbondy on 1 in #windev. I can point you to all my work so far, if you'd like to see it first. :-)

Comment 22

[Mina Almasry]

I think I don't need a review since the last one was granted, I have made the changes, and explained the exceptions, no? I'm marking the patch checkin-needed. Feel  free to reverse if that's wrong.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> I think I don't need a review since the last one was granted

That's correct.  Fwiw, I looked at the changes in that last patch, and they look good.

> Would you vouch for me on commit access level 1?

Sure. Bug#?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0087957a1d

Comment 25

[Mina Almasry]

> Sure. Bug#?


Bug 894079. Thanks!

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8d0087957a1d

Comment 27

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Superb! Thank you everyone! I'll use it in BlueGriffon right away!