Pierre, could you see if we could use the css parser to reparse the selector when this method is called?

Sure. Could you give me a testcase? My JS skills are weak.

Moving nsbeta2 nomination from Status Summary to Keywords field. This is where the nomination belongs. NOT Status Summary...my query won't pick it up that way.

Marking M16 because it is marked beta2, but why is it beta2?

Pierre, this functionality is a DOM Level 2 feature so if possible it should be done before the feature freeze, the priority for getting this feature in is not very high tho. If you have more important features to add then go ahead and do them first and if you don't think you'll get to this in time for the feature freeze then remove the nsbeta2 keyword. Sorry about misplacing the keyword.

Don't worry about the definition of feature, as long as it doesn't have a UI which takes a whole chapter in the user manual. Removing beta2 tag, pushing to M17 and shamelessly renaming to "CSSStyleRuleImpl::SetSelectorText() doesn't work".

This bug has been marked "future" because the original netscape engineer working on this is over-burdened. If you feel this is an error, that you or another known resource will be working on this bug, or if it blocks your work in some way -- please attach your concern to the bug for reconsideration.

Taking QA Contact on all open or unverified DOM Style bugs...

Nominating this bug for nsbeta1 on behalf of gerardok@netscape.com.

Removing nsbeta1 nomination -- there was a misunderstanding and some "approved out features" were nominated by mistake! Sorry!

Assigning pierre's remaining Style System-related bugs to myself.

This bug is still present, even in the nightlies. 11 years, wow! Here are the 2 test cases I was going to submit in my bug report: http://jsfiddle.net/leaverou/g4sDc/show http://jsfiddle.net/leaverou/g4sDc/1/show

Do other browsers implement this? Should it get removed from the spec or implemented?

What needs to be implemented here, for what it's worth, is the function mozilla::css::StyleRule::SetSelectorText, at the very end of layout/style/StyleRule.cpp . Given nsCSSParser::ParseSelectorString, it should be relatively straightforward, though triggering the necessary dynamic updates might be a little work. (Note that for implementing SetCssText, also not implemented, it's more work, since that requires swapping out the StyleRule itself, which suggests the current empty stub shouldn't even be there and it should be fixed at the caller.)

(In reply to David Baron [:dbaron] from comment #16) > Do other browsers implement this? Should it get removed from the spec or > implemented? Yes, they do. At least Opera and Chrome that I tested.

Firebug could need this. See https://github.com/firebug/firebug/blob/a92ac094fe07d9bb17387143ed0d25b74c8a6eb2/extension/content/firebug/css/cssPanel.js#L1927. Sebastian

(In reply to Lea Verou from comment #18) > (In reply to David Baron [:dbaron] from comment #16) > > Do other browsers implement this? Should it get removed from the spec or > > implemented? > > Yes, they do. At least Opera and Chrome that I tested. 2 years later they still do. MSIE does not despite the MSDN stating it's a settable property (http://msdn.microsoft.com/en-us/library/ie/ff974660.aspx)

What is preventing this from being implemented? If reloading is tricky, why not execute a remove and add internally?... Maybe improving performance later?

Nothing in particular; it's just work, and other work is important too. Implementing in terms of a remove and add would probably be harder.

I see... If I knew how firefox works I'd help with this, unfortunately, I don't have enough availability to read and understand the code and to implement this myself...

It has become an important issue for the jss project (https://github.com/jsstyles/jss). We are working on a plugin for jss which will isolate rules from inherited props. For this we need a fast way to modify selectorText: https://github.com/jsstyles/jss-isolate/pull/1/ Otherwise we need to rerender the entire rule and this might be a big perf. overhead because there are thousands of rules.

I would like to work on this bug and have a preliminary patch to offer.

Comment on attachment 8842114 [details] [diff] [review] 0001-stylerule-selectortext-setter.patch This is mostly reasonable, but a few comments: >+void >+StyleRule::SetSelector(nsCSSSelectorList* aSelector) >+{ >+ delete mSelector; >+ mSelector = aSelector; >+} Presumably this requires a change to the header file. Though I'm not sure there's a reason to have it as a public method; it's probably better to just manipulate mSelector directly in SetSelectorText. > NS_IMETHODIMP > StyleRule::SetSelectorText(const nsAString& aSelectorText) >-{ >- // XXX TBI - get a parser and re-parse the selectors, >- // XXX then need to re-compute the cascade >- // XXX and dirty sheet >+{ >+ auto doc = this->GetDocument(); Here, and later, probably better to write out the type rather than using auto. Also, don't use the "this->". >+ if (!doc) { >+ return NS_OK; >+ } This seems less than optimal, since the old selector will remain. We should figure out whether GetDocument() being null means that no notifications need to be sent -- I suspect that's the case. If so, can the rest of the function be made to function without the document? >+ // Hold strong ref to the CSSLoader in case the document update >+ // kills the document >+ RefPtr<css::Loader> loader; >+ if (doc) { >+ loader = doc->CSSLoader(); >+ NS_ASSERTION(loader, "Document with no CSS loader!"); >+ } >+ >+ auto sheet = GetStyleSheet(); >+ nsCSSParser css(loader, sheet); For selectors, maybe the parser can be initialized with a null loader? (What things in the parser require a loader, and can selector parsing trigger any of them?) >+ nsCSSSelectorList *selectorList = new nsCSSSelectorList(); This is wrong; it will leak the object created here. selectorList is an out parameter; you should initialize to null, or maybe even use UniquePtr (though I don't see offhand how to use UniquePtr with out-parameters). >+ if (NS_FAILED(result)) >+ return result; {} are required, and indent is 2 spaces. I think it's fine that this doesn't change the pointer identity of the Declaration, since nothing changes about the declaration. (That's normally required for rule manipulation, but it's not needed if only the selector changes.) Some tests (ideally web-platform-tests) are needed. Though it's possible there are already web-platform-tests that this will fix. The bigger concern is how this interacts with the Stylo work. needinfo? to xidorn for an opinion there.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #26) > The bigger concern is how this interacts with the Stylo work. needinfo? to > xidorn for an opinion there. We'll need to implement this in Stylo as well. But it doesn't seem to me adding this to Stylo is very hard, so I think it's fine. We didn't added it to Stylo simply because Gecko doesn't implement it.

I have made the requested changes with the following notes: 1. I have found out by reading code that GetDocument() being null means we do not have to update a document, hence we are not exiting early when it is null. 2. Loader is only necessary when reporting errors(ErrorReporter.cpp mLoader) and setting quirks mode(nsCSSParser.cpp SetQuirkMode). Loader is removed and nullptr is passed to CSSParser. 3. Tests still missing, I could not find existing tests, will add as a separate patch if it is ok.

Comment on attachment 8842541 [details] [diff] [review] 0002-stylerule-selectortext-setter.patch Sorry, been very busy today, but a quick-ish review (leaving you to look into a few of the issues that I might look into if I had a little more time today): >+ nsresult result = css.ParseSelectorString(aSelectorText, sheet->GetSheetURI(), 0, &selectorList); >+ if (NS_FAILED(result)) { >+ return result; >+ } So ParseSelectorString returns (throws) NS_ERROR_DOM_SYNTAX_ERR for syntax errors. That makes sense. But is that exception supposed to be thrown from the selectorText setter? (What do other browsers do?) >+ sheet->WillDirty(); So, I was thinking about this a little bit. The primary thing WillDirty() does is that it ensures that the style sheet's inner (which is what holds most of the data) is unique. This is because style sheets use copy-on-write sharing for when a document links to the same stylesheet multiple times; this is also used for stylesheet preloading. It's sort of unsafe to call WillDirty from a style rule, since the style rule lives inside the inner. So, really, this WillDirty call should probably be preceded by an assertion that the style sheet already has a unique inner. This should be true because it shouldn't be possible for a caller to get to a StyleRule object (via DOM APIs) without this. But we still need to add this assertion (*before* the WillDirty call), which means adding a method AssertHasUniqueInner() to CSSStyleSheet. From comment 28: >2. Loader is only necessary when reporting errors(ErrorReporter.cpp mLoader) and setting quirks mode(nsCSSParser.cpp SetQuirkMode). Loader is removed and nullptr is passed to CSSParser. What happens with a null loader in these cases? >3. Tests still missing, I could not find existing tests, will add as a separate patch if it is ok. Separate patch is good, but the first patch shouldn't land without the tests. And the tests ought to test things like invalid selectors (see above).

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #29) > Comment on attachment 8842541 [details] [diff] [review] > 0002-stylerule-selectortext-setter.patch > > Sorry, been very busy today, but a quick-ish review (leaving you > to look into a few of the issues that I might look into if I had a > little more time today): > > >+ nsresult result = css.ParseSelectorString(aSelectorText, sheet->GetSheetURI(), 0, &selectorList); > >+ if (NS_FAILED(result)) { > >+ return result; > >+ } > > So ParseSelectorString returns (throws) NS_ERROR_DOM_SYNTAX_ERR for > syntax errors. That makes sense. But is that exception supposed to > be thrown from the selectorText setter? (What do other browsers do?) > I have tested setSelectorText in Chrome, IE 11 and Edge: * Chrome ignores invalid values of selectorText without any console log. * IE 11 and Edge ignores all values without any console log, as they do not support setSelectorText as of writing. > I have changed the patch to ignore any ParseSelectorString errors. > >+ sheet->WillDirty(); > > So, I was thinking about this a little bit. The primary thing WillDirty() > does is that it ensures that the style sheet's inner (which is what holds > most of the data) is unique. This is because style sheets use > copy-on-write sharing for when a document links to the same stylesheet > multiple times; this is also used for stylesheet preloading. It's sort of > unsafe to call WillDirty from a style rule, since the style rule lives inside > the inner. So, really, this WillDirty call should probably be preceded > by an assertion that the style sheet already has a unique inner. This > should be true because it shouldn't be possible for a caller to get > to a StyleRule object (via DOM APIs) without this. But we still need to > add this assertion (*before* the WillDirty call), which means adding > a method AssertHasUniqueInner() to CSSStyleSheet. > I have added an AssertHasUniqueInner invocation with in SetSelectorText method with a comment. > > From comment 28: > >2. Loader is only necessary when reporting errors(ErrorReporter.cpp mLoader) and setting quirks mode(nsCSSParser.cpp SetQuirkMode). Loader is removed and nullptr is passed to CSSParser. > > What happens with a null loader in these cases? > In ErrorReporter, null loader causes InnerWindowId=0 to be passed into nsScriptErrorBase::InitWithWindowID, as a result, mInnerWindowID will be retrieved via nsCSSParser::mSheet->FindOwningWindowInnerID method when ErrorReporter::OutputError method is called. nsCSSParser::mSheet will be the sheet passed in from ctor at "nsCSSParser css(nullptr, sheet);". In other words, passing a sheet with an nsIDocument in CSSStyleSheet::mDocument to get a WindowId from, is equivalent to passing a loader. In nsCSSParser, CSS quirk mode will be disabled at all times, causing CSS_PROPERTY_HASHLESS_COLOR_QUIRK and CSS_PROPERTY_UNITLESS_LENGTH_QUIRK to be disabled. * Hashless color quirk: // try 'xxyyzz' without '#' prefix for compatibility with IE and Nav4x (bug 23236 and 45804) * Unitless length quirk: // NONSTANDARD: Nav interprets unitless numbers as px While parsing a selector text, we do not pass through code paths that are affected by these quirks. Short version is; it turns out that passing a sheet when mLoader is null has no effect if sheet has a document, and being in quirk mode does not affect ParseSelectorString method. > >3. Tests still missing, I could not find existing tests, will add as a separate patch if it is ok. > > Separate patch is good, but the first patch shouldn't land without the > tests. > > And the tests ought to test things like invalid selectors (see above). I have read about web-platform-tests and found out that it excludes CSS, in light of this I think mochitests are suitable. If you agree, I would continue to write mochitests somewhere in \layout\style\test folder. web-platform-tests: http://web-platform-tests.org/introduction.html Thank you for your detailed directions, much appreciated.

Aforementioned patch.

(In reply to Kerem from comment #30) > I have read about web-platform-tests and found out that it excludes CSS, in > light of this I think mochitests are suitable. If you agree, I would > continue to write mochitests somewhere in \layout\style\test folder. > > web-platform-tests: http://web-platform-tests.org/introduction.html > > Thank you for your detailed directions, much appreciated. Please use web-platform-tests. The CSS test suite will be merging into web-platform-tests in the next few weeks.

Comment on attachment 8843003 [details] [diff] [review] 0003-stylerule-selectortext-setter.patch Review of attachment 8843003 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/CSSStyleSheet.h @@ +179,5 @@ > // called GetOwnerRule because that would be ambiguous with the ImportRule > // version. > css::Rule* GetDOMOwnerRule() const final; > > + void AssertHasUniqueInner(); Please define this simple function inline here, so that there is a higher possibility that compilers would completely remove the function call in a release build.

For what it's worth, I think the patch is in pretty good shape although I still need to do a final review to double-check a few things that we discussed. (And comment 33 should be fixed.) However, it can't land without tests, so given that it's not ready to land yet, I'm going to prioritize other reviews over this one for now.

https://bugzilla.mozilla.org/show_bug.cgi?id=37468#c33 fixed.

web-platform tests added to selectorText property. I have tried to cover many cases , including valid and invalid CSS selectors. I cannot ask :dbaron: to review this (bugzilla states that my request is blocked), could you review these patches :gl please?

Comment on attachment 8845010 [details] [diff] [review] 0004-stylerule-selectortext-setter.patch >+ inline void AssertHasUniqueInner() const >+ { >+ MOZ_ASSERT(mInner->mSheets.Length() == 1, This line should be indented two spaces more than "inline", not four. >+ "expected unique inner"); The " in this line should line up with the mInner in the previous line. So the one thing I noticed is that, as a result of having a null loader, the quirks mode doesn't get set correctly on the parser. Maybe that's OK for selector parsing, but at the very least SetSelectorText should have a comment such as: // NOTE: Passing a null loader means that the parser is always in // standards mode and never in quirks mode. It also means that the loader doesn't get associated correctly with the document, which I think means the console won't group error reports correctly. Given the latter, I think you should try to get a non-null loader, as you did in the first patch, but just continue on if it's null (with a variant of the above comments). r=dbaron with the above changes, although I should probably look at the revised patch

Comment on attachment 8845015 [details] [diff] [review] 0004-stylerule-selectortext-tests.patch Did you run the lint and manifest update as described in: https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests ? I think the filename of the new test should be CSSStyleRule-set-selectorText.html rather than CSS-setSelectorText.html. >+ .style1 { background-color: rgb(255, 0, 0); } I'd suggest using a different color here. red is often used to indicate failure, and green to indicate success. If you're using a color for some other purpose, you should use a color that's not red or green. >+ var styleSheet = document.getElementById("styleElement").sheet; >+ var rule = styleSheet.cssRules[0]; >+ >+ var divContainer = document.getElementById("container"); >+ >+ const originalStyleSelector = ".style0"; These first 6 lines should be indented less, to match the rest of the script. (Or the rest indented more.) >+ "::selection", Don't include this in a list of invalid selector values, since it should be valid in other browsers. r=dbaron with those changes, but again, I should probably look at the revised patch

And I'd note that I did review the normalizedSelector bits in the test relative to https://drafts.csswg.org/cssom/#serializing-selectors (and the definitions of selector parts in both selectors-4 (which dropped pseudo-elements from the definitions) and selectors-3 (which has pseudo-elements, but uses different terms)).

I think that, at least, something should be logged in the page console or in the browser console when there's a syntax error instead of failing silently. Just failing silently is a developer's nightmare. I know chrome just fails silently but I think they did it wrong. There's no need to throw an exception but the developer should be warned of such thing if he makes a mistake. A log in the right console is enough. I'm telling this as a web developer I am.

(In reply to brunoais from comment #40) > I think that, at least, something should be logged in the page console or in > the browser console when there's a syntax error instead of failing silently. > Just failing silently is a developer's nightmare. > I know chrome just fails silently but I think they did it wrong. There's no > need to throw an exception but the developer should be warned of such thing > if he makes a mistake. A log in the right console is enough. > I'm telling this as a web developer I am. This is relatively simple to do -- it requires adding a boolean parameter to ParseSelectorString so that it can decide whether to OUTPUT_ERROR() or CLEAR_ERROR().

(That change would best be done as an additional patch rather than adding them to the existing patch, though.)

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #41) > (In reply to brunoais from comment #40) > > I think that, at least, something should be logged in the page console or in > > the browser console when there's a syntax error instead of failing silently. > > Just failing silently is a developer's nightmare. > > I know chrome just fails silently but I think they did it wrong. There's no > > need to throw an exception but the developer should be warned of such thing > > if he makes a mistake. A log in the right console is enough. > > I'm telling this as a web developer I am. > > This is relatively simple to do -- it requires adding a boolean parameter to > ParseSelectorString so that it can decide whether to OUTPUT_ERROR() or > CLEAR_ERROR(). I have made this change in a separate patch and it only outputs the string to debug output, not JS console for some reason. Debug output message: [JavaScript Warning: "Selector expected." {file: "http://localhost/fox/css.htm" line: 0 column: 0 source: "{!"}]

(In reply to Kerem from comment #43) > I have made this change in a separate patch and it only outputs the string > to debug output, not JS console for some reason. - did you check that the document is non-null so that you're passing through a loader (see comment 37) - were you looking at the console in devtools (Tools -> Web Developer -> Web Console) and not some other console?

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #46) > (In reply to Kerem from comment #43) > > I have made this change in a separate patch and it only outputs the string > > to debug output, not JS console for some reason. > > - did you check that the document is non-null so that you're passing through > a loader (see comment 37) I have checked it, we pass a null loader only if doc is null: RefPtr<css::Loader> loader; if (doc) { loader = doc->CSSLoader(); } Also, loader is not null when inspected with MSVC debugger. > - were you looking at the console in devtools (Tools -> Web Developer -> Web > Console) and not some other console? devtools console it is.

typo: s/doc is null/doc is not null

Maybe try seeing what happens in ErrorReporter::OutputError (in layout/style/ErrorReporter.cpp) when the error is reported? It should end up setting mInnerWindowID to something useful (nonzero), which I think is what allows the console to associate it with the correct document. (If that ends up zero, I'd expect the error not to show up in the devtools console because devtools doesn't know which page it's associated with.)

ErrorReporter::OutputError finds a non-zero inner window ID and eventually invokes nsConsoleService::LogMessageWithMode method only to succeed dispatching the message at nsConsoleService.cpp:330 ( SystemGroup::Dispatch("LogMessageRunnable", TaskCategory::Other, r.forget()); ). Truncated call stack that returns NS_OK for OutputError: xul.dll!nsThread::PutEvent(already_AddRefed<nsIRunnable> aEvent, nsThread::nsNestedEventTarget * aTarget) Line 748 C++ xul.dll!nsThread::DispatchInternal(already_AddRefed<nsIRunnable> aEvent, unsigned int aFlags, nsThread::nsNestedEventTarget * aTarget) Line 805 C++ xul.dll!nsThread::Dispatch(already_AddRefed<nsIRunnable> aEvent, unsigned int aFlags) Line 858 C++ xul.dll!NS_DispatchToCurrentThread(already_AddRefed<nsIRunnable> && aEvent) Line 205 C++ xul.dll!mozilla::Dispatcher::UnlabeledDispatch(const char * aName, mozilla::TaskCategory aCategory, already_AddRefed<nsIRunnable> && aRunnable) Line 130 C++ xul.dll!mozilla::ValidatingDispatcher::LabeledDispatch(const char * aName, mozilla::TaskCategory aCategory, already_AddRefed<nsIRunnable> && aRunnable) Line 231 C++ [Inline Frame] xul.dll!mozilla::ValidatingDispatcher::Dispatch(const char *) Line 148 C++ xul.dll!mozilla::SystemGroup::Dispatch(const char * aName, mozilla::TaskCategory aCategory, already_AddRefed<nsIRunnable> && aRunnable) Line 85 C++ xul.dll!nsConsoleService::LogMessageWithMode(nsIConsoleMessage * aMessage, nsConsoleService::OutputMode aOutputMode) Line 332 C++ xul.dll!nsConsoleService::LogMessage(nsIConsoleMessage * aMessage) Line 210 C++ xul.dll!mozilla::css::ErrorReporter::OutputError() Line 222 C++ The message is observed by ContentChild.cpp ConsoleListener::Observe method, invoking ContentChild::SendScriptError method (ipdl generated). ContentChild::SendScriptError method returns true. PContent.ipdl tells us that SendScriptError is an async message which should be handled by ContentParent::RecvScriptError. I placed a breakpoint in ContentParent::RecvScriptError and it never triggered. What else can I check to find out why the message is not being logged to the devtools console?

So one thing that bz reminds me of (from http://logs.glob.uno/?c=mozilla%23content#c431228): in the devtools console, you need to hit the Filter icon to cause CSS errors to show up at all. Once you do that, does it help? Otherwise, I think things sound ok. (Was the breakpoint in ContentParent::RecvScriptError in the child process or the parent process? It should have been in the parent process -- and it should have been hit, too!)

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #51) > So one thing that bz reminds me of (from > http://logs.glob.uno/?c=mozilla%23content#c431228): in the devtools > console, you need to hit the Filter icon to cause CSS errors to show up at > all. Once you do that, does it help? > Enabling CSS filter did the trick, I can see logs in the console as "Selector expected." now, thanks. > Otherwise, I think things sound ok. > > (Was the breakpoint in ContentParent::RecvScriptError in the child process > or the parent process? It should have been in the parent process -- and it > should have been hit, too!) (it wasn't in the parent process, my bad). Can you help me get these patches to try?

Here's a single-platform try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89d62315406d6cb8b9a590dc5845d546bcbb8fee although I had to fix the fact that the 0001-output-warning.patch was malformed: https://hg.mozilla.org/users/dbaron_mozilla.com/patches-clean/raw-rev/e0290129eae7

Looks like you have: * a bunch of TEST-UNEXPECTED-PASS in WPT selectors/attribute-selectors/attribute-case/cssom.html which require that you adjust the test expectations * TEST-UNEXPECTED-TIMEOUT | /cssom/CSSStyleRule-set-selectorText.html | expected OK, which is a problem with your new test (ignore the dark blue [retry] wpt7)

(In reply to David Baron :dbaron: ⌚️UTC+8 from comment #54) > Looks like you have: > > * a bunch of TEST-UNEXPECTED-PASS in WPT > selectors/attribute-selectors/attribute-case/cssom.html which require that > you adjust the test expectations > I have adjusted the expectations. > * TEST-UNEXPECTED-TIMEOUT | /cssom/CSSStyleRule-set-selectorText.html | > expected OK, which is a problem with your new test > > (ignore the dark blue [retry] wpt7) I could not reproduce the timeout locally, can it be an intermittent issue?

Comment on attachment 8857586 [details] [diff] [review] 0006-stylerule-selectortext-tests.patch So it's a little unfortunate, but I think we've now gotten to the point where new style system features need to work in both Gecko and Stylo. That is, it also needs to work in the variant where much of the guts of the style system are being replaced by Servo's style system. So I think this also needs a patch that fixes this for stylo. I'm probably not the best person to help with that, but :xidorn could probably provide pointers.

Comment on attachment 8857586 [details] [diff] [review] 0006-stylerule-selectortext-tests.patch :xidorn said he could do the Stylo part easily, so I'll look at this review later.

Comment on attachment 8857586 [details] [diff] [review] 0006-stylerule-selectortext-tests.patch You don't need to create the file testing/web-platform/meta/cssom/CSSStyleRule-set-selectorText.html.ini It is only needed if there are known failures in the test. (Here you're just duplicating information that's in a different file, in a confusing way. Instead, just don't add this file.) I presume you made the edits to testing/web-platform/meta/MANIFEST.json by running "mach wpt-manifest-update"? If not, that's how you should make the edit. >+<!doctype html> I would: * prefer "DOCTYPE" to be uppercase * remove the byte-order-mark >+<title></title> Please add a title. >+ var divContainer = document.getElementById("container"); Instead of just storing this in a variable, you could instead store getComputedStyle(document.getElementById("container")) in a variable and save the extra call to getComputedStyle every time. >+ var assertColors = function(selectorMatches){ Space between ) and {, please. >+ ].forEach(function(selector) { >+ test(function() { >+ assert_equals(rule.selectorText, originalStyleSelector); >+ >+ rule.selectorText = selector; >+ >+ assert_equals(rule.selectorText, originalStyleSelector); >+ }, "CSSStyleRule: Invalid CSS selector: " + selector); >+ }); At the end of this, inner function it's probably best to set: rule.selectorText = originalStyleSelector; so that if somebody using the test hits a parsing bug and sees a failure, they only get one test failure instead of failing all the remaining tests as well. (Or you could also use add_cleanup() like the later test; that's probably even preferable.) >+ this.add_cleanup(function() { rule.selectorText = originalStyleSelector; }); Fix the two spaces after the "{" to be just one. Otherwise this looks good, but I'd like to look at the revised patch. When you post that, you can actually request review on both patches, since I believe the code should be ready to land, assuming you've addressed the review comments above.

Patch rebased.

Patch rebased.

Rebased and comment #58 review comments implemented.

(needinfo request changed to review request)

Comment on attachment 8904221 [details] [diff] [review] 0007-1-output-warning.patch >nsCSSParser.ParseSelectorString supports outputting error messages to console. Commit messages should describe the change you're making, not describe a state before or after the change. Thus this commit message should be: Bug 37468 - Add parameter to nsCSSParser::ParseSelectorString to control whether to output console error message. r=dbaron with that change

Comment on attachment 8904222 [details] [diff] [review] 0007-2-StyleRule-SetSelectorText.patch AssertHasUniqueInner should be an inline method in StyleSheet.h like it was when I reviewed the patch in comment 37, rather than being implemented in StyleSheet.cpp. r=dbaron with that fixed

Comment on attachment 8904223 [details] [diff] [review] 0007-3-StyleRule-SetSelectorText-tests.patch >Bug 37468 - Added web-platform tests for StyleRule selectorText property setter. The "Added" should be "Add", based on the convention we use for commit messages. >diff --git a/testing/web-platform/meta/MANIFEST.json b/testing/web-platform/meta/MANIFEST.json I assume you made the edits to testing/web-platform/meta/MANIFEST.json by running "mach wpt-manifest-update"? If so, r=dbaron with the change to the commit message

I did run "mach wpt-manifest-update" to update manifest.json originally. On the tip, these are the changes without including the new tests in this bug.

Added layout.css.servo.enabled:false pref to tests.

Comment on attachment 8913264 [details] [diff] [review] 0008-5-StyleRule-SetSelectorText-tests-with-pref.patch I do not have try access, could you push these to try?

Comment on attachment 8913264 [details] [diff] [review] 0008-5-StyleRule-SetSelectorText-tests-with-pref.patch This looks like something I reviewed before. Does it need re-review? Or are you just using the review? flag to ask me to push to try? (The needinfo? bug flag would be better for that.) If you need me to push patches to try, could you say exactly which patches, and in which order?

The reason for 0008-5-StyleRule-SetSelectorText-tests-with-pref.patch review is the preference setting which was added recently - sorry for not giving information about this in previous comments. ( https://bugzilla.mozilla.org/attachment.cgi?id=8913264&action=diff#a/testing/web-platform/meta/cssom/CSSStyleRule-set-selectorText.html.ini_sec1 ) I took the liberty of carrying the review+ to 0008-5 patch: Could you push the following 4 patches to try in this order: 0008-1-output-warning.patch 0008-2-StyleRule-SetSelectorText.patch 0008-3-wpt-manifest-update.patch 0008-5-StyleRule-SetSelectorText-tests-with-pref.patch

The failure in wpt1 looks like the fault of the patches here.

Disabled servo in selectors/attribute-selectors/attribute-case/cssom.html tests.

(FWIW, I think implementing this with proper invalidation so we don't restyle the whole page in Stylo would be very nice, and would allow us to reuse the same mechanism for making fast the tweak of the declaration blocks inside the rules as well)

Comment on attachment 8916390 [details] [diff] [review] 0008-6-StyleRule-disableservo-cssomselectors.patch I don't think this is a good idea; we should be testing what we ship. It's good for you to check that the tests work with this pref flip, but the bit we really need to implement is the stylo part.

Would you still be interested in implementing this bug in stylo? If not, could you point me to the right direction in stylo code?

I can do that, but it would be great if you can help implementing that in stylo as well. So the entry point for this in Stylo is ServoStyleRule::SetSelectorText [1]. To implement this function, you first need to add a new binding function (probably called Servo_StyleRule_SetSelectorText). You need to add a declaration of it in ServoBindingList.h after Servo_StyleRule_GetSelectorText [2] and then implement this function in Rust in servo/ports/geckolib/glue.rs, again after GetSelectorText [3]. For the binding function Servo_StyleRule_SetSelectorText, you can take Servo_KeyframesRule_SetName [4] as an example for how to change a rule, and maybe Servo_ParseEasing [5] for how to take a string from C++ side and setup parser. You would probably want to call SelectorList::parse [6] to do the actual parsing. FWIW, the definition of StyleRule in Servo is in style_rule.rs [7]. In addition to the binding function for parsing the selector text and set it in the Rust side, the C++ side ServoStyleRule::SetSelectorText also needs some of the updating logic like what you wrote for StyleRule::SetSelectorText, e.g. WillDirty / DidDirty, StyleRuleChange, etc. Tell me if you have any question or when you get into any trouble with implementing it. [1] https://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/layout/style/ServoStyleRule.cpp#238-245 [2] https://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/layout/style/ServoBindingList.h#216-217 [3] https://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/servo/ports/geckolib/glue.rs#1441-1446 [4] https://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/servo/ports/geckolib/glue.rs#1616-1621 [5] https://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/servo/ports/geckolib/glue.rs#2207-2233 [6] https://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/servo/components/selectors/parser.rs#183-195 [7] https://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/servo/components/style/stylesheets/style_rule.rs#18-27

After four months, I have a patch that compiles :) The selectorText is being updated but styling does not work, I am triggering StyleRuleChange from ServoStyleRule::SetSelectorText. Also, this would be my first rust commit if successful. Is there an easy way to print variables to the OS shell or JS console from rust?

Comment on attachment 8952494 [details] [diff] [review] 0009-5-servo-StyleRule-SetSelectorText.patch Review of attachment 8952494 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/ServoStyleRule.cpp @@ +243,5 @@ > + Servo_StyleRule_SetSelectorText(mRawRule, &aSelectorText, loader, aExtraData); > + > + sheet->DidDirty(); > + > + if (doc) { This needs to go through StyleSheet::RuleChanged to be sound. That should also update the styling. See SetCSSDeclaration.

Comment on attachment 8952494 [details] [diff] [review] 0009-5-servo-StyleRule-SetSelectorText.patch Review of attachment 8952494 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this! I'll review again when you fix the issue below. ::: layout/style/ServoBindingList.h @@ +244,5 @@ > RawServoDeclarationBlockBorrowed declarations) > SERVO_BINDING_FUNC(Servo_StyleRule_GetSelectorText, void, > RawServoStyleRuleBorrowed rule, nsAString* result) > +SERVO_BINDING_FUNC(Servo_StyleRule_SetSelectorText, void, > + RawServoStyleRuleBorrowed rule, const nsAString* result, Please remove this trailing whitespace. ::: layout/style/ServoStyleRule.cpp @@ +230,5 @@ > + > + if (doc) { > + loader = doc->CSSLoader(); > + } > + Trailing whitespace again. @@ +234,5 @@ > + > + // StyleRule lives inside of the Inner, it is unsafe to call WillDirty > + // if sheet does not already have a unique Inner. > + sheet->AssertHasUniqueInner(); > + sheet->WillDirty(); You need a null-check here for sheet. GetStyleSheet() may return null, and in that case, there is no need to notice the stylesheet about this change. @@ +237,5 @@ > + sheet->AssertHasUniqueInner(); > + sheet->WillDirty(); > + > + // TODO Do we need to pass extra data? > + URLExtraData* aExtraData = nullptr; From the code in `Servo_StyleRule_SetSelectorText`, it seems it's better to pass the extra data which will be used by the error reporter. You should be able to do this with `sheet->AsServo()->URLData()`. And don't forget the null-check. @@ +240,5 @@ > + // TODO Do we need to pass extra data? > + URLExtraData* aExtraData = nullptr; > + > + Servo_StyleRule_SetSelectorText(mRawRule, &aSelectorText, loader, aExtraData); > + Trailing whitespace again. @@ +241,5 @@ > + URLExtraData* aExtraData = nullptr; > + > + Servo_StyleRule_SetSelectorText(mRawRule, &aSelectorText, loader, aExtraData); > + > + sheet->DidDirty(); Null-check here as well. @@ +244,5 @@ > + > + sheet->DidDirty(); > + > + if (doc) { > + mozAutoDocUpdate updateBatch(doc, UPDATE_STYLE, true); It's probably better move this before `sheet->WillDirty()`. It may not matter a lot for now, but that matches convention among the code better and is also safer (in case WillDirty/selector parsing/DidDirty tries to run some script in the future). @@ +246,5 @@ > + > + if (doc) { > + mozAutoDocUpdate updateBatch(doc, UPDATE_STYLE, true); > + > + doc->StyleRuleChanged(sheet, this); As emilio mentioned, you should use `sheet->RuleChange(this);` here instead. ::: servo/ports/geckolib/glue.rs @@ +7,5 @@ > use env_logger::LogBuilder; > use malloc_size_of::MallocSizeOfOps; > use selectors::NthIndexCache; > use selectors::matching::{MatchingContext, MatchingMode, matches_selector}; > +use selectors::SelectorList; This would break Servo's lint for alphabetic `use` statements, I believe. It should probably better be merged with with the `NthIndexCache` line above. @@ +131,5 @@ > use style::properties::animated_properties::AnimationValue; > use style::properties::animated_properties::compare_property_priority; > use style::rule_cache::RuleCacheConditions; > use style::rule_tree::{CascadeLevel, StrongRuleNode, StyleSource}; > +use style::selector_parser::{SelectorParser, PseudoElementCascadeType, SelectorImpl}; It may be better to put `SelectorParser` to the last in this line, for alphabetic order. @@ +138,5 @@ > use style::style_adjuster::StyleAdjuster; > use style::stylesheets::{CssRule, CssRules, CssRuleType, CssRulesHelpers, DocumentRule}; > use style::stylesheets::{FontFeatureValuesRule, ImportRule, KeyframesRule, MediaRule}; > use style::stylesheets::{NamespaceRule, Origin, OriginSet, PageRule, StyleRule}; > +use style::stylesheets::{StylesheetContents, SupportsRule, Namespaces}; Probably put `Namespaces` to the previous line, and if that line grows too long, move the last one down to this line. @@ +1643,5 @@ > }) > } > > #[no_mangle] > +pub extern "C" fn Servo_StyleRule_SetSelectorText(rule: RawServoStyleRuleBorrowed, This whole function should probably be marked `unsafe` since it accepts arbitrary pointers and make assumption on that. There are many functions in this file don't have that, but that's probably something we should fix at some point. For new stuff like this, let's do it the right way at the beginning. @@ +1645,5 @@ > > #[no_mangle] > +pub extern "C" fn Servo_StyleRule_SetSelectorText(rule: RawServoStyleRuleBorrowed, > + text: *const nsAString, > + loader: *mut Loader, Trailing whitespace on these three lines. @@ +1650,5 @@ > + raw_extra_data: *mut URLExtraData) { > + let value_str = unsafe { (*text).to_string() }; > + > + write_locked_arc(rule, |rule: &mut StyleRule| { > + let namespaces = Namespaces::default(); This doesn't look correct. You may need to pass in the stylesheet as an argument of this binding function, and try getting namespace from that. You should only use `Namespaces::default()` when there is no stylesheet. And There should probably be a test checking it behaves correctly with @namespace rules. You can read for example `Servo_StyleSheet_GetSourceMapURL` for how to pass stylesheet data across FFI and read data from it. Actually, since stylesheet in Servo side also contains the URL extra data needed here, you don't really need to pass `raw_extra_data` from Gecko side. @@ +1654,5 @@ > + let namespaces = Namespaces::default(); > + let parser = SelectorParser { > + stylesheet_origin: Origin::Author, > + namespaces: &namespaces, > + url_data: None, This should be set to something when possible, since there are selectors only valid in some url (e.g. chrome:// urls). @@ +1663,5 @@ > + match SelectorList::parse(&parser, &mut Parser::new(&mut parser_input)) { > + Ok(selectors) => { > + rule.selectors = selectors > + } > + Err(parseError) => { `parse_error` or just `error`. Rust variables should be using snake_case.

I have removed CSSStyleSheet related patches as they no longer seem to be relevant.

Comment on attachment 8966001 [details] [diff] [review] 0011-1-servo-StyleRule.patch Review of attachment 8966001 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. It generally looks good. Please fix the issues below. ::: layout/style/ServoStyleRule.cpp @@ +237,5 @@ > + > + ServoStyleSheet* servoSheet = sheet->AsServo(); > + > + RefPtr<RawServoStyleSheetContents> contents; > + contents = const_cast<RawServoStyleSheetContents*>(servoSheet->RawContents()); A weak pointer `const RawServoStyleSheetContents*` should be enough here since the sheet cannot go away. That way you can also avoid this `const_cast`. `X##Borrowed` type in binding is a `const X*` in C++ side. @@ +241,5 @@ > + contents = const_cast<RawServoStyleSheetContents*>(servoSheet->RawContents()); > + > + if (Servo_StyleRule_SetSelectorText(contents, mRawRule, > + &aSelectorText, loader)) > + { ``` if (Servo_StyleRule_SetSelectorText(contents, mRawRule, &aSelectorText, loader)) { // ... } ``` ::: layout/style/StyleSheet.h @@ +255,5 @@ > void WillDirty(); > virtual void DidDirty() {} > + inline void AssertHasUniqueInner() const > + { > + MOZ_ASSERT(mInner->mSheets.Length() == 1, "expected unique inner"); MOZ_ASSERT(HasUniqueInner()); Probably also move the function body to StyleSheetInlines.h so that compilers wouldn't complain about `HasUniqueInner` is declared but not defined. ::: servo/ports/geckolib/glue.rs @@ +1877,5 @@ > #[no_mangle] > +pub unsafe extern "C" fn Servo_StyleRule_SetSelectorText(sheet: RawServoStyleSheetContentsBorrowed, > + rule: RawServoStyleRuleBorrowed, > + text: *const nsAString, > + loader: *mut Loader) -> bool { Please format the signature here this way: ``` pub unsafe extern "C" fn Servo_StyleRule_SetSelectorText( sheet: RawServoStyleSheetContentsBorrowed, rule: RawServoStyleRuleBorrowed, text: *const nsAString, loader: *mut Loader, ) -> bool { ``` @@ +1904,5 @@ > + let location = error.location; > + let mut_url_data = (&mut url_data.clone()).get(); > + let reporter = ErrorReporter::new(ptr::null_mut(), loader, mut_url_data); > + let error = ContextualParseError::InvalidRule(&value_str, error); > + reporter.report(location, error); We generally don't report parsing error for CSSOM setting, so for keeping things consistent we probably shouldn't do so here either. Please remove the reporting code and just return false here. By doing that, you would not need to pass `loader` into this binding function.

Comment on attachment 8966002 [details] [diff] [review] 0011-2-servo-StyleRule-tests.patch Review of attachment 8966002 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/cssom/CSSStyleRule-set-selectorText.html @@ +1,1 @@ > +<!DOCTYPE html> This file should be put in css/cssom/ rather than cssom directly.

Comment on attachment 8966002 [details] [diff] [review] 0011-2-servo-StyleRule-tests.patch Review of attachment 8966002 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/cssom/CSSStyleRule-set-selectorText.html @@ +23,5 @@ > + > + const originalStyleSelector = ".style0"; > + > + var assertColors = function(selectorMatches) { > + assert_equals(divContainerStyle.getPropertyValue('background-color'), selectorMatches ? "rgb(0, 0, 255)" : "rgb(255, 0, 255)") `divContainerStyle.backgroundColor` is enough. It isn't a big problem, though.

Comment on attachment 8966003 [details] [diff] [review] 0011-3-servo-StyleRule-namespace-tests.patch Review of attachment 8966003 [details] [diff] [review]: ----------------------------------------------------------------- This test doesn't seem to be very useful. As far as we have the other test, I don't think we need to worry about checking valid/invalid parsing in this test. We should focusing on checking the behavior of namespace in this test. Consider changing the stylesheet to ```css @namespace url(http://www.w3.org/1999/xhtml); @namespace svg url(http://www.w3.org/2000/svg); svg|*.style0 { background-color: rgb(0, 0, 255) !important; } svg|*.style1 { background-color: rgb(255, 0, 255); } ``` The default namespace is added so that selectors without namespace prefix would not match the <svg> element. And then you can probably just check the following selectors: * ".style1" doesn't match (due to default namespace) * "svg|*.style1" and "*|*.style1" matches * "p" matches the <p> because it is in the default namespace

Feel free to drop the namespace test if you don't feel comfortable figuring out namespace stuffs. In that case, just file a followup and cc me there. I can probably find a time to write the test.

Comment on attachment 8967426 [details] [diff] [review] 0012-1-servo-SetSelectorText.patch Review of attachment 8967426 [details] [diff] [review]: ----------------------------------------------------------------- Almost there! Just some code style nits left :) ::: layout/style/ServoStyleRule.cpp @@ +216,5 @@ > > void > ServoStyleRule::SetSelectorText(const nsAString& aSelectorText) > { > + if (RefPtr<StyleSheet> sheet = GetStyleSheet()) { Sorry that I somehow missed this problem before, but the indentation should be two spaces in C++ code, not four. (Rust code uses four spaces indentation, though.) @@ +219,5 @@ > { > + if (RefPtr<StyleSheet> sheet = GetStyleSheet()) { > + ServoStyleSheet* servoSheet = sheet->AsServo(); > + MOZ_ASSERT(servoSheet, "Servo style rules should have " > + "servo stylesheets."); This assertion is unnecessary. AsServo() asserts this when there were two backends. And now there is only one backend, and ServoStyleSheet and StyleSheet may get merged soonish. @@ +220,5 @@ > + if (RefPtr<StyleSheet> sheet = GetStyleSheet()) { > + ServoStyleSheet* servoSheet = sheet->AsServo(); > + MOZ_ASSERT(servoSheet, "Servo style rules should have " > + "servo stylesheets."); > + nsCOMPtr<nsIDocument> doc = sheet->GetAssociatedDocument(); A weak pointer `nsIDocument*` here should be enough. @@ +232,5 @@ > + > + const RawServoStyleSheetContents* contents = servoSheet->RawContents(); > + > + if (Servo_StyleRule_SetSelectorText(contents, mRawRule, > + &aSelectorText)) It seems to me if you reduce the indentation to two spaces each level, the line is short enough that don't need split. Also even if the line gets split, the argument should aligned to the right of `(`, and the bracket should be in the same line, like > if (Servo_StyleRule_SetSelectorText(contents, mRawRule, > &aSelectorText)) { ::: layout/style/StyleSheet.h @@ +254,5 @@ > // However, the calls do not need to be matched; it's ok to call > // WillDirty and then make no change and skip the DidDirty call. > void WillDirty(); > virtual void DidDirty() {} > + Trailing whitespaces. ::: layout/style/StyleSheetInlines.h @@ +133,5 @@ > { > return mInner->mSheets.Length() == 1; > } > > +void Trailing whitespaces.

Comment on attachment 8967427 [details] [diff] [review] 0012-2-servo-SetSelectorText-tests.patch Review of attachment 8967427 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/cssom/CSSStyleRule-set-selectorText-namespace.html @@ +1,1 @@ > +<!DOCTYPE html> Why this file is still in the patch? I suppose it should be removed.

Comment on attachment 8967427 [details] [diff] [review] 0012-2-servo-SetSelectorText-tests.patch Review of attachment 8967427 [details] [diff] [review]: ----------------------------------------------------------------- Please remove the old CSSStyleRule-set-selectorText-namespace.html file, otherwise it looks good to me.

I expect that you've run this test locally to ensure that it works. Tell me if you don't know how to do that. Also you may want to also run this test with other browsers to ensure that we don't miss anything.

Comment on attachment 8967426 [details] [diff] [review] 0012-1-servo-SetSelectorText.patch Review of attachment 8967426 [details] [diff] [review]: ----------------------------------------------------------------- drive-by uber-nit :) ::: layout/style/StyleSheetInlines.h @@ +136,5 @@ > > +void > +StyleSheet::AssertHasUniqueInner() const > +{ > + MOZ_ASSERT(mInner->mSheets.Length() == 1, "expected unique inner"); This can also just become MOZ_ASSERT(HasUniqueInner()), which is a bit nicer to read.

* Removed testing/web-platform/tests/cssom/CSSStyleRule-set-selectorText-namespace.html * Fixed nits * I ran wpt tests in the patch before uploading them before and including the results now. Test Results for the 0013 patches: * Firefox (patched) : 82/82 pass, namespace: 5/5 pass. * Chrome 65 : 81/82 pass, namespace: 5/5 pass. * Opera 52 : 81/82 pass, namespace: 5/5 pass. * IE 11 : 20/82 pass, namespace: 0/5 pass. (Only invalid CSS tests pass). * Edge 41(EdgeHTML 16) : 20/82 pass, namespace: 0/5 pass. (Only invalid CSS tests pass). The same test fails in Chrome and Opera: >CSSStyleRule: selectorText value: |[|lang]| isMatch: true >{selector: "[|lang]", isMatch: true, normalizedSelector: "[lang]"}, with the error: >assert_equals: expected "[lang]" but got "[|lang]" Firefox normalizes "[|lang]" to "[lang]", Opera and Chromium doesn't do this. According to https://drafts.csswg.org/selectors-4/#attrnmsp two representations are equal. https://drafts.csswg.org/cssom/#serializing-selectors states under "attribute selector 2." that: > If the namespace prefix maps to a namespace that is not the null namespace (not in a namespace) append the serialization of the namespace prefix as an identifier, followed by a "|" (U+007C) to s. This seems like a CSS serialization bug in Opera and Chrome.

:xidorn, thank you for your patience so far :)

https://hg.mozilla.org/integration/autoland/rev/bd0a3fdb037c308ae7eab0be3c68d00fcb9b6056 Bug 37468 - Implement ServoStyleRule::SetSelectorText method for setting CSS rule selectorText from JS. r=xidorn https://hg.mozilla.org/integration/autoland/rev/5e7a1db60298306b7c55e5c2ebd0075d7e77c258 Bug 37468 - Add web-platform tests for StyleRule selectorText property setter. r=xidorn

Backed out 2 changesets (bug 37468) for linting failure at builds/worker/checkouts/gecko/tools/lint/wpt.yml Backout: https://hg.mozilla.org/integration/autoland/rev/93723d7f125b8f9edfdc148b398b238e6d1e9705 Failure push:https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5e7a1db60298306b7c55e5c2ebd0075d7e77c258 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=173726946&repo=autoland&lineNumber=254 [vcs 2018-04-15T00:09:27.032Z] 239092 files updated, 0 files merged, 0 files removed, 0 files unresolved [vcs 2018-04-15T00:09:27.222Z] updated to 5e7a1db60298306b7c55e5c2ebd0075d7e77c258 [vcs 2018-04-15T00:09:27.226Z] PERFHERDER_DATA: {"framework": {"name": "vcs"}, "suites": [{"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "clone", "shouldAlert": false, "subtests": [], "value": 108.11901497840881}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "pull", "shouldAlert": false, "subtests": [], "value": 6.043957948684692}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "update", "shouldAlert": false, "subtests": [], "value": 91.18330097198486}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "overall", "shouldAlert": false, "subtests": [], "value": 206.4468059539795}]} [vcs 2018-04-15T00:09:27.530Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/autoland/rev/5e7a1db60298306b7c55e5c2ebd0075d7e77c258 title='Built from autoland revision 5e7a1db60298306b7c55e5c2ebd0075d7e77c258'>5e7a1db60298306b7c55e5c2ebd0075d7e77c258</a> [task 2018-04-15T00:09:27.530Z] executing ['bash', '-cx', 'cd /builds/worker/checkouts/gecko && ./mach lint -l wpt -l wpt_manifest -f treeherder'] [task 2018-04-15T00:09:27.539Z] + cd /builds/worker/checkouts/gecko [task 2018-04-15T00:09:27.539Z] + ./mach lint -l wpt -l wpt_manifest -f treeherder [task 2018-04-15T00:09:28.192Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenv/bin/python2.7 [task 2018-04-15T00:09:28.192Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenv/bin/python [task 2018-04-15T00:09:29.637Z] Installing setuptools, pip, wheel...done. [task 2018-04-15T00:09:30.617Z] running build_ext [task 2018-04-15T00:09:30.617Z] building 'psutil._psutil_linux' extension [task 2018-04-15T00:09:30.617Z] creating build [task 2018-04-15T00:09:30.617Z] creating build/temp.linux-x86_64-2.7 [task 2018-04-15T00:09:30.617Z] creating build/temp.linux-x86_64-2.7/psutil [task 2018-04-15T00:09:30.617Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o [task 2018-04-15T00:09:30.617Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o [task 2018-04-15T00:09:30.618Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o [task 2018-04-15T00:09:30.618Z] creating build/lib.linux-x86_64-2.7 [task 2018-04-15T00:09:30.618Z] creating build/lib.linux-x86_64-2.7/psutil [task 2018-04-15T00:09:30.618Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so [task 2018-04-15T00:09:30.618Z] building 'psutil._psutil_posix' extension [task 2018-04-15T00:09:30.619Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o [task 2018-04-15T00:09:30.619Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o [task 2018-04-15T00:09:30.619Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so [task 2018-04-15T00:09:30.619Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil [task 2018-04-15T00:09:30.619Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil [task 2018-04-15T00:09:30.619Z] [task 2018-04-15T00:09:30.620Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt) [task 2018-04-15T00:09:30.638Z] No specific files specified, running the full wpt lint (this is slow) [task 2018-04-15T00:09:42.352Z] 0:11.71 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/mozilla/meta/MANIFEST.json [task 2018-04-15T00:09:42.360Z] 0:11.72 INFO Diffing old and new manifests /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json [task 2018-04-15T00:09:46.789Z] 0:16.15 WARNING Manifest /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json contains correct tests but file hashes changed; please update [task 2018-04-15T00:13:40.731Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/tools/lint/wpt.yml:None | Lint process exited with return code 2 (wpt) [task 2018-04-15T00:13:40.731Z] TEST-UNEXPECTED-ERROR | testing/web-platform/tests/css/cssom/CSSStyleRule-set-selectorText-namespace.html:None | Testcase file must have a link to a spec (MISSING-LINK) [task 2018-04-15T00:13:40.731Z] TEST-UNEXPECTED-ERROR | testing/web-platform/tests/css/cssom/CSSStyleRule-set-selectorText.html:None | Testcase file must have a link to a spec (MISSING-LINK) [taskcluster 2018-04-15 00:13:41.106Z] === Task Finished === [taskcluster 2018-04-15 00:13:41.106Z] Unsuccessful task run with exit code: 1 completed in 487.176 seconds

https://hg.mozilla.org/integration/autoland/rev/4d7067c22f47dc071f085d27139a296d6bfc1f69 Bug 37468 - Implement ServoStyleRule::SetSelectorText method for setting CSS rule selectorText from JS. r=xidorn https://hg.mozilla.org/integration/autoland/rev/7343e39bf9854651c4f8c2525767b825666d38fd Bug 37468 - Add web-platform tests for StyleRule selectorText property setter. r=xidorn

My miss. Addressed the lint failure.

Looks like we already have a test for selectorText change in /css/cssom/selectorText-modification-restyle-001.html which shows up unexpected pass with this patch. There is another test /css/cssom/selectorText-modification-restyle-002.html which I suppose should also pass, but it's not in the same task so still unknown so far. I should really have had a try run before pushing it :/ (I thought it is unlikely to affect any existing test as far as there is no test checking the setter, but apparently I was wrong.) Luckly it isn't too bad this time, we just need to remove some expected fail annotations :)

https://hg.mozilla.org/integration/autoland/rev/65f61ee6d17e8a49676bcd98396bf43a4511be3e Bug 37468 followup - Update test expectations of wpt which checks setting selectorText on a CLOSED TREE.

https://hg.mozilla.org/mozilla-central/rev/4d7067c22f47 https://hg.mozilla.org/mozilla-central/rev/7343e39bf985 https://hg.mozilla.org/mozilla-central/rev/65f61ee6d17e

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10481 for changes under testing/web-platform/tests

Upstream PR merged

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2b7cbea0291d followup: Cleanup line terminators. r=me

Thanks for fixing this Kerem!

https://developer.mozilla.org/en-US/Firefox/Releases/61#DOM https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleRule/selectorText