RESOLVED FIXED 70203
MediaList: Remove inheritance from StyleBase.
https://bugs.webkit.org/show_bug.cgi?id=70203
Summary MediaList: Remove inheritance from StyleBase.
Andreas Kling
Reported 2011-10-16 12:07:45 PDT
MediaList shouldn't inherit from StyleBase.
Attachments
Proposed patch (18.73 KB, patch)
2011-10-16 12:19 PDT, Andreas Kling
no flags
Proposed patch v2 (18.31 KB, patch)
2011-10-17 07:33 PDT, Andreas Kling
darin: review+
Andreas Kling
Comment 1 2011-10-16 12:19:32 PDT
Created attachment 111183 [details] Proposed patch
WebKit Review Bot
Comment 2 2011-10-16 12:20:50 PDT
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
Comment 3 2011-10-16 12:25:07 PDT
(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
Comment 4 2011-10-16 13:20:49 PDT
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
Comment 5 2011-10-16 14:16:11 PDT
(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
Comment 6 2011-10-16 14:58:00 PDT
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
Comment 7 2011-10-16 15:01:41 PDT
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
Comment 8 2011-10-17 07:33:20 PDT
Created attachment 111260 [details] Proposed patch v2 Remove the parentRule pointer and just always store the parentStyleSheet instead.
WebKit Review Bot
Comment 9 2011-10-17 07:34:57 PDT
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 10 2011-10-17 08:22:38 PDT
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 11 2011-10-17 08:24:10 PDT
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
Comment 12 2011-10-18 08:04:57 PDT
Andreas Kling
Comment 13 2011-10-18 08:06:42 PDT
(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
Comment 14 2011-10-18 09:42:08 PDT
Ryosuke Niwa
Comment 15 2011-10-18 09:42:41 PDT
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
Comment 16 2011-10-18 10:07:00 PDT
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
Comment 17 2011-10-19 05:10:44 PDT
Andreas Kling
Comment 18 2011-10-19 05:15:29 PDT
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.)
Note You need to log in before you can comment on or make changes to this bug.