Bug 105903 - Clean up the loadXXXStyle() idiom in StyleResolver.
Summary: Clean up the loadXXXStyle() idiom in StyleResolver.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Trivial
Assignee: Mike West
URL:
Keywords:
Depends on: 90834
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-01 16:39 PST by Mike West
Modified: 2013-01-02 13:15 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.15 KB, patch)
2013-01-02 02:00 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (3.63 KB, patch)
2013-01-02 12:57 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2013-01-01 16:39:59 PST
See https://bugs.webkit.org/show_bug.cgi?id=90834#c11 for details.
Comment 1 Alexey Proskuryakov 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.
Comment 2 Mike West 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());
Comment 3 Mike West 2013-01-02 02:00:24 PST
Created attachment 181018 [details]
Patch
Comment 4 Mike West 2013-01-02 02:01:29 PST
akling@: Hopefully this is more or less what you were looking for. What do you think?
Comment 5 Darin Adler 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.
Comment 6 Mike West 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. :)
Comment 7 Mike West 2013-01-02 12:57:25 PST
Created attachment 181050 [details]
Patch for landing
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2013-01-02 13:15:17 PST
All reviewed patches have been landed.  Closing bug.