Bug 46069 – [Chromium] User style layout tests don't pass on Chromium

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mihai Parparita]

[Chromium] User style layout tests don't pass on Chromium

[Mihai Parparita]

Created attachment 68046 [details]
Patch

[Mihai Parparita]

Aaron, cc-ing you since these user style tests don't pass because of r63243, where you added a Chromium-only codepath so that user styles are only applied after a reload. Do you have an opinion about how to best make these tests pass under the Chromium DRT? The options I saw were
1) Make the tests do a reload
2) Add a flag to PageGroup::addUserStyleSheetToWorld to do the reset, and have Chromium DRT set it, but have regular Chromium code not set it.
3) Add that flag, but expose it in the LayoutTestController.addUserStyleSheet method signature, so that the layout test would have to opt-in to that behavior.

I ended up doing 1), since it was the least work :) 2) is in some ways nicer, since the tests can be left alone, but I don't know if we want the DRT behavior to diverge from the actual Chromium code. 3) I like the least since other ports won't care about that flag, but they may have to accept it anyway.

You also alluded to "something more clever and general" in comment 2 of bug 42003. Do you happen to remember what that is? I don't mind implementing it.

[Aaron Boodman]

(In reply to comment #2)
> Aaron, cc-ing you since these user style tests don't pass because of r63243, where you added a Chromium-only codepath so that user styles are only applied after a reload. Do you have an opinion about how to best make these tests pass under the Chromium DRT? The options I saw were
> 1) Make the tests do a reload
> 2) Add a flag to PageGroup::addUserStyleSheetToWorld to do the reset, and have Chromium DRT set it, but have regular Chromium code not set it.
> 3) Add that flag, but expose it in the LayoutTestController.addUserStyleSheet method signature, so that the layout test would have to opt-in to that behavior.
> 
> I ended up doing 1), since it was the least work :) 2) is in some ways nicer, since the tests can be left alone, but I don't know if we want the DRT behavior to diverge from the actual Chromium code. 3) I like the least since other ports won't care about that flag, but they may have to accept it anyway.
> 
> You also alluded to "something more clever and general" in comment 2 of bug 42003. Do you happen to remember what that is? I don't mind implementing it.

Thank you for fixing this! I forgot that I had had a plan to make this more configurable.

I didn't think of 1). It seems a bit of a shame to have to call this Chromium-specific method in every layout test, though. It also seems easy to miss for non-Chromium developers, and then end up with a test that doesn't work in Chromium.

2/3 (3 is a superset of 2, right?) was what I was alluding to. I was imagining an enum like InjectInExistingDocuments for addUserStylesToWorld. Then layout tests could test both behaviors.

[Adam Barth]

Comment on attachment 68046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68046&action=review

> LayoutTests/userscripts/simple-stylesheet.html:9
> +    // Necessary for the Chromium DRT to pick up user style sheet changes.
> +    layoutTestController.queueReload();

Why not have DRT do this work?  It seems like we'll be making these chromium-specific changes to tests forever with this approach.

[Mihai Parparita]

Created attachment 68153 [details]
Patch

[Mihai Parparita]

Aaron, something like the new attached patch? This basically implements option 2.

+Timothy since he reviewed r63243.

[Aaron Boodman]

Comment on attachment 68153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68153&action=review

> WebCore/page/PageGroup.h:87
> +#if !PLATFORM(CHROMIUM)

The #ifdef here is not required here. Just default it to InjectInExistingDocuments and it will do the right thing for the existing ports and Chromium will override.

> WebCore/page/UserStyleSheetTypes.h:34
> +enum UserStyleInjectionTime { InjectInExistingDocuments, InjectInSubsequentDocuments };

It's weird that Level is in UserStyleSheet, but this is here. It looks like having them in User*Types.h is more consistent, so can you move Level here?

[Aaron Boodman]

(In reply to comment #6)
> Aaron, something like the new attached patch? This basically implements option 2.

Yes, that is what I had in mind. Some comments sent separately.

[Mihai Parparita]

Created attachment 68258 [details]
Patch

[Mihai Parparita]

(In reply to comment #7)
> The #ifdef here is not required here. Just default it to InjectInExistingDocuments and it will do the right thing for the existing ports and Chromium will override.

Done.
 
> > WebCore/page/UserStyleSheetTypes.h:34
> > +enum UserStyleInjectionTime { InjectInExistingDocuments, InjectInSubsequentDocuments };
> 
> It's weird that Level is in UserStyleSheet, but this is here. It looks like having them in User*Types.h is more consistent, so can you move Level here?

Move Level to be UserStyleLevel in UserStyleSheetTypes.h.

Timothy, would you be abel to review this?

[Aaron Boodman]

Adding another potential reviewer.

[David Levin]

Comment on attachment 68258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68258&action=review

I think the compile assert really should be added before committing this.

> LayoutTests/platform/chromium/drt_expectations.txt:156
> +BUG49021 : userscripts/mixed-case-stylesheet.html = PASS

Why do we have PASS lines in here as opposed to just deleting the line?

> LayoutTests/userscripts/mixed-case-stylesheet.html:-5
> -    window.layoutTestController.dumpAsText();

I think removing "window." is "clean up the test code slightly".

Ideally this would be in a different patch so one doesn't think that it has anything to do with the fix. (I was wondering how this helped fix the issue.)

> WebKit/chromium/public/WebView.h:74
> +    };

chromium/public change = Darin Fisher review needed.

> WebKit/chromium/src/WebViewImpl.cpp:2042
> +                                        static_cast<WebCore::UserStyleInjectionTime>(injectionTime));

Please add a compile assert to ensure that the enum values continue to be the same for both of these.

[David Levin]

Darin, please checkout/review change to WebKit/chromium/public/WebView.h.

I feel comfortable looking at the rest of it.

[Mihai Parparita]

Created attachment 68391 [details]
Patch

[Mihai Parparita]

(latest version of the patch incorporates these changes)

(In reply to comment #12)
> > LayoutTests/platform/chromium/drt_expectations.txt:156
> > +BUG49021 : userscripts/mixed-case-stylesheet.html = PASS
> 
> Why do we have PASS lines in here as opposed to just deleting the line?

These tests pass with the Chromium DRT but fail with test_shell (they can be made to pass with test_shell too, but that will need to wait until we roll to a revision with this patch, and then test_shell can start using UserStyleInjectInExistingDocuments too). Therefore these tests will still have a TEXT expectation in test_expectations.txt, but are in the "Pass with DRT though FAIL on test_shell" section of drt_expectations.txt with a PASS expectation (I attempted to make this clearer in the ChangeLog entry, let me know if it's not).
 
> > LayoutTests/userscripts/mixed-case-stylesheet.html:-5
> > -    window.layoutTestController.dumpAsText();
> 
> I think removing "window." is "clean up the test code slightly".
> 
> Ideally this would be in a different patch so one doesn't think that it has anything to do with the fix. (I was wondering how this helped fix the issue.)

The initial version of this patch needed test changes, so I did this clean-up while I was editing those files. Agreed that this should be moved to a separate patch now that those files don't need to be touched.

> > WebKit/chromium/src/WebViewImpl.cpp:2042
> > +                                        static_cast<WebCore::UserStyleInjectionTime>(injectionTime));
> 
> Please add a compile assert to ensure that the enum values continue to be the same for both of these.

Done.

[David Levin]

Comment on attachment 68391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68391&action=review

This looks fine to me. It may be nice to fix the text in WebKitTools/ChangeLog.

I'm not r+ b/c I want Darin to review the change in WebKit/chromium/public/WebView.h (For example: I know that WebKit allows default arguments. I know that Google style does not. I don't know what applies to WebKit/chromium/public/WebView.h but I would think it tends more toward the Google style in this regard so that may be an issue.)

> WebKitTools/ChangeLog:8
> +        Fix a typo in LayoutTestController::addUserStyleSheet that was causing a

What typo? It looks like you are adding a whole argument that wasn't possible before.

[Mihai Parparita]

(In reply to comment #16)
> > WebKitTools/ChangeLog:8
> > +        Fix a typo in LayoutTestController::addUserStyleSheet that was causing a
> 
> What typo? It looks like you are adding a whole argument that wasn't possible before.

It used to say arguments[2].toBoolean(), but arguments only has two elements in this case.

[Darin Fisher (:fishd, Google)]

(In reply to comment #16)
> (From update of attachment 68391 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68391&action=review
> 
> This looks fine to me. It may be nice to fix the text in WebKitTools/ChangeLog.
> 
> I'm not r+ b/c I want Darin to review the change in WebKit/chromium/public/WebView.h (For example: I know that WebKit allows default arguments. I know that Google style does not. I don't know what applies to WebKit/chromium/public/WebView.h but I would think it tends more toward the Google style in this regard so that may be an issue.)

The Chromium WebKit API follows WebKit coding style since it lives in the WebKit repository :-)

[WebKit Commit Bot]

Comment on attachment 68391 [details]
Patch

Clearing flags on attachment: 68391

Committed r68114: <http://trac.webkit.org/changeset/68114>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.