WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Proposed patch v2
(18.31 KB, patch)
2011-10-17 07:33 PDT
,
Andreas Kling
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r97752
: <
http://trac.webkit.org/changeset/97752
>
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
It appears that this patch broke SL debug tests:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/2621
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
Committed
r97849
: <
http://trac.webkit.org/changeset/97849
>
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.
Top of Page
Format For Printing
XML
Clone This Bug