Bug 70203 - MediaList: Remove inheritance from StyleBase.
Summary: MediaList: Remove inheritance from StyleBase.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on: 70332
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-16 12:07 PDT by Andreas Kling
Modified: 2011-10-19 05:15 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-10-16 12:07:45 PDT
MediaList shouldn't inherit from StyleBase.
Comment 1 Andreas Kling 2011-10-16 12:19:32 PDT
Created attachment 111183 [details]
Proposed patch
Comment 2 WebKit Review Bot 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.
Comment 3 Andreas Kling 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. :(
Comment 4 Antti Koivisto 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.
Comment 5 Andreas Kling 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?
Comment 6 Antti Koivisto 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.
Comment 7 Antti Koivisto 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);
};
Comment 8 Andreas Kling 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.
Comment 9 WebKit Review Bot 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.
Comment 10 Darin Adler 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.
Comment 11 Darin Adler 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.
Comment 12 Andreas Kling 2011-10-18 08:04:57 PDT
Committed r97752: <http://trac.webkit.org/changeset/97752>
Comment 13 Andreas Kling 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.
Comment 14 Ryosuke Niwa 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
Comment 15 Ryosuke Niwa 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
Comment 16 Ryosuke Niwa 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.
Comment 17 Andreas Kling 2011-10-19 05:10:44 PDT
Committed r97849: <http://trac.webkit.org/changeset/97849>
Comment 18 Andreas Kling 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.)