RESOLVED FIXED 105903
Clean up the loadXXXStyle() idiom in StyleResolver.
https://bugs.webkit.org/show_bug.cgi?id=105903
Summary Clean up the loadXXXStyle() idiom in StyleResolver.
Mike West
Reported 2013-01-01 16:39:59 PST
Attachments
Patch (5.15 KB, patch)
2013-01-02 02:00 PST, Mike West
no flags
Patch for landing (3.63 KB, patch)
2013-01-02 12:57 PST, Mike West
no flags
Alexey Proskuryakov
Comment 1 2013-01-01 20:57:28 PST
> See https://bugs.webkit.org/show_bug.cgi?id=90834#c11 for details. Can you post adequate information here? It's a simple copy/paste for you, but extra effort for multiple other people to do the research.
Mike West
Comment 2 2013-01-02 01:14:15 PST
(In reply to comment #1) > > See https://bugs.webkit.org/show_bug.cgi?id=90834#c11 for details. > > Can you post adequate information here? It's a simple copy/paste for you, but extra effort for multiple other people to do the research. Very fair. This was meant as a note to myself, but you're absolutely right; the bug tracker should be a source of truth. --- In https://bugs.webkit.org/show_bug.cgi?id=90834#c11, akling@ suggested: > Source/WebCore/css/StyleResolver.cpp:1361 > + if (!defaultSeamlessStyle) > + loadSeamlessStyle(); > + matchUARules(result, defaultSeamlessStyle); This pattern seems overly verbose, it would be nicer if we could just do: matchUARules(result, defaultSeamlessStyle()); (I see that we already have code like this elsewhere in StyleResolver.cpp, but I think we can do better.) > Source/WebCore/css/StyleResolver.cpp:5304 > + if (!defaultSeamlessStyle) > + loadSeamlessStyle(); > + m_features.add(defaultSeamlessStyle->features()); Again, would be nicer with just: m_features.add(defaultSeamlessStyle()->features());
Mike West
Comment 3 2013-01-02 02:00:24 PST
Mike West
Comment 4 2013-01-02 02:01:29 PST
akling@: Hopefully this is more or less what you were looking for. What do you think?
Darin Adler
Comment 5 2013-01-02 10:36:46 PST
Comment on attachment 181018 [details] Patch Looks OK. Also not sure that adding “UA” to these function names improved anything. It just seems like mysterious additional acronym/jargon. Not sure that it even helps in the name matchUARules.
Mike West
Comment 6 2013-01-02 12:29:19 PST
(In reply to comment #5) > (From update of attachment 181018 [details]) > Looks OK. Thanks. I'm dropping the seamless bit from the patch, as that got rolled back and it looks like we'll be doing things entirely differently. The view source bit is still relevant, however, so I appreciate the review. > Also not sure that adding “UA” to these function names improved anything. It just seems like mysterious additional acronym/jargon. Not sure that it even helps in the name matchUARules. In context, I think you're right. I'll drop UA from this method name. I don't think I want to touch matchUARules, however. I'll leave that decision for someone who knows what they're doing. :)
Mike West
Comment 7 2013-01-02 12:57:25 PST
Created attachment 181050 [details] Patch for landing
WebKit Review Bot
Comment 8 2013-01-02 13:15:13 PST
Comment on attachment 181050 [details] Patch for landing Clearing flags on attachment: 181050 Committed r138639: <http://trac.webkit.org/changeset/138639>
WebKit Review Bot
Comment 9 2013-01-02 13:15:17 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.