Bug 70203 – MediaList: Remove inheritance from StyleBase.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

MediaList shouldn't inherit from StyleBase.

[Andreas Kling]

Created attachment 111183 [details]
Proposed patch

[WebKit Review Bot]

Attachment 111183 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/MediaList.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

(In reply to comment #2)
> Attachment 111183 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/css/MediaList.h:27:  Alphabetical sorting problem.  [build/include_order] [4]

Alphabet is hard. :(

[Antti Koivisto]

I wonder if you actually need to store the parentRule(). The impression from this patch is that it is only ever used for getting the parent style sheet or locating the root in js bindings) Assuming that rules can never be roots, it might be possible to always just save the parent sheet.

[Andreas Kling]

(In reply to comment #4)
> I wonder if you actually need to store the parentRule(). The impression from this patch is that it is only ever used for getting the parent style sheet or locating the root in js bindings) Assuming that rules can never be roots, it might be possible to always just save the parent sheet.

So you're saying we would simply pass the parentStyleSheet along to the MediaList in the CSSImportRule and CSSMediaRule constructors.. I suppose that could work, assuming there's no way for those rules to be reparented into another CSSStyleSheet after construction. Does that makes sense?

[Antti Koivisto]

Right. I suspect reparenting to a different sheet never happens, but I'm not entirely sure. Some asserts over layout test run might tell.

The same may well be true for many other things associated with style sheets. If so, we could simplify code elsewhere too so finding this out would be valuable.

[Antti Koivisto]

CSSOM doesn't have parent for MediaList so it is purely an internal thing:

interface MediaList {
  stringifier attribute DOMString mediaText;
  readonly attribute unsigned long length;
  getter DOMString item(unsigned long index);
  void appendMedium(DOMString medium);
  void deleteMedium(DOMString medium);
};

[Andreas Kling]

Created attachment 111260 [details]
Proposed patch v2

Remove the parentRule pointer and just always store the parentStyleSheet instead.

[WebKit Review Bot]

Attachment 111260 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   ec92ce4..4af16b0  master     -> origin/master
	M	Source/JavaScriptCore/runtime/JSObject.h
	M	Source/JavaScriptCore/runtime/JSAPIValueWrapper.h
	M	Source/JavaScriptCore/ChangeLog
r97620 = 4af16b0fe9fdecaea9f8ae0ebb6f7113787b0171 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 111260 [details]
Proposed patch v2

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

> Source/WebCore/bindings/js/JSDOMBinding.h:194
> +        CSSStyleSheet* parentStyleSheet = mediaList->parentStyleSheet();
> +        if (parentStyleSheet)
> +            return root(parentStyleSheet);
> +        return 0;

I’d suggest either early return style, or defining the variable inside the if statement conditional as the function above does.

> Source/WebCore/css/MediaList.h:80
> +    CSSStyleSheet* parentStyleSheet() const;

Since the relationship of a media list to a style sheet is not a parent/child one, I do not think this terminology is quite right.

[Darin Adler]

Comment on attachment 111260 [details]
Proposed patch v2

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

> Source/WebCore/css/MediaList.cpp:260
> +CSSStyleSheet* MediaList::parentStyleSheet() const
> +{
> +    return m_parentStyleSheet;
> +}

Seems this should be in the header so it can be inlined.

> Source/WebCore/css/MediaList.cpp:267
> +void MediaList::setParentStyleSheet(CSSStyleSheet* styleSheet)
> +{
> +    // MediaList should never be moved between style sheets.
> +    ASSERT(!m_parentStyleSheet || !styleSheet);
> +    m_parentStyleSheet = styleSheet;
>  }

Seems this should be in the header so it can be inlined.

[Andreas Kling]

Committed r97752: <http://trac.webkit.org/changeset/97752>

[Andreas Kling]

(In reply to comment #10)
> (From update of attachment 111260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111260&action=review
> 
> > Source/WebCore/css/MediaList.h:80
> > +    CSSStyleSheet* parentStyleSheet() const;
> 
> Since the relationship of a media list to a style sheet is not a parent/child one, I do not think this terminology is quite right.

Truth. I'll revisit this once I get to cleaning up parent/child relations in the CSSOM classes.

[Ryosuke Niwa]

It appears that this patch broke SL debug tests:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/2621

[Ryosuke Niwa]

Some stack trace:
Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000001013f773f WebCore::MediaList::setParentStyleSheet(WebCore::CSSStyleSheet*) + 77 (MediaList.h:84)
1   com.apple.WebCore             	0x00000001020b337c WebCore::StyleSheet::setMedia(WTF::PassRefPtr<WebCore::MediaList>) + 322 (StyleSheet.cpp:77)
2   com.apple.WebCore             	0x0000000101debe77 WebCore::ProcessingInstruction::setCSSStyleSheet(WTF::String const&, WebCore::KURL const&, WTF::String const&, WebCore::CachedCSSStyleSheet const*) + 405 (ProcessingInstruction.cpp:224)
3   com.apple.WebCore             	0x0000000101325f30 WebCore::CachedCSSStyleSheet::checkNotify() + 194 (CachedCSSStyleSheet.cpp:117)
4   com.apple.WebCore             	0x000000010132612c WebCore::CachedCSSStyleSheet::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool) + 354 (CachedCSSStyleSheet.cpp:107)
5   com.apple.WebCore             	0x000000010133fac6 WebCore::CachedResourceRequest::didFinishLoading(WebCore::SubresourceLoader*, double) + 436 (CachedResourceRequest.cpp:181)
6   com.apple.WebCore             	0x00000001020b5d40 WebCore::SubresourceLoader::didFinishLoading(double) + 174 (SubresourceLoader.cpp:192)
7   com.apple.WebCore             	0x0000000101fd9e02 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 164 (ResourceLoader.cpp:452)
8   com.apple.WebCore             	0x0000000101fd4b76 -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 261 (ResourceHandleMac.mm:874)
9   com.apple.Foundation          	0x00007fff83ac7608 _NSURLConnectionDidFinishLoading + 113
10  com.apple.CFNetwork           	0x00007fff85bc21a0 URLConnectionClient::_clientDidFinishLoading(URLConnectionClient::ClientConnectionEventQueue*) + 174
11  com.apple.CFNetwork           	0x00007fff85c279ae URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) + 254
12  com.apple.CFNetwork           	0x00007fff85bae825 URLConnectionClient::processEvents() + 121

[Ryosuke Niwa]

Oops, apparently the patch had been rolled out in http://trac.webkit.org/changeset/97764 as pointed out by anttik. Sorry about the noise. r97764 didn't show up on build.webkit.org/console and I didn't notice.

[Andreas Kling]

Committed r97849: <http://trac.webkit.org/changeset/97849>

[Andreas Kling]

Relanded with the assertion fixed (setting the MediaList's parent stylesheet to the MediaList's current parent stylesheet is a no-op and not a crime.)