MediaList shouldn't inherit from StyleBase.
Created attachment 111183 [details] Proposed patch
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.
(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. :(
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.
(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?
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.
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); };
Created attachment 111260 [details] Proposed patch v2 Remove the parentRule pointer and just always store the parentStyleSheet instead.
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.
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.
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.
Committed r97752: <http://trac.webkit.org/changeset/97752>
(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.
It appears that this patch broke SL debug tests: http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/2621
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
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.
Committed r97849: <http://trac.webkit.org/changeset/97849>
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.)