Bug 107779 - Split CSSOMWrapper data and functions out from StyleResolver into its own class
Summary: Split CSSOMWrapper data and functions out from StyleResolver into its own class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 89879 107777
  Show dependency treegraph
 
Reported: 2013-01-23 20:24 PST by Hayato Ito
Modified: 2013-01-30 21:05 PST (History)
10 users (show)

See Also:


Attachments
factor CSSOMWrapper logic into a CSSOMWrapper (14.89 KB, patch)
2013-01-23 22:50 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Rename CSSOMWrapper to CSSOMWrapperForInspector. (14.82 KB, patch)
2013-01-28 01:32 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Fixed typo. Renamed to InspectorCSSOMWrappers. (14.76 KB, patch)
2013-01-28 23:56 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
No longer overloading. (15.61 KB, patch)
2013-01-29 23:17 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Addressed. (15.67 KB, patch)
2013-01-30 20:26 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2013-01-23 20:24:52 PST
This will be a separated patch from https://bugs.webkit.org/show_bug.cgi?id=107777.
Comment 1 Hayato Ito 2013-01-23 22:50:55 PST
Created attachment 184407 [details]
factor CSSOMWrapper logic into a CSSOMWrapper
Comment 2 Andreas Kling 2013-01-24 02:19:44 PST
Comment on attachment 184407 [details]
factor CSSOMWrapper logic into a CSSOMWrapper

View in context: https://bugs.webkit.org/attachment.cgi?id=184407&action=review

Why should this be in a separate class? What is the purpose of this change?
CSSOMWrapper is a bad name for this class, as we already have CSSOM wrappers in WebKit, and this class has very little to do with them.

> Source/WebCore/css/StyleResolver.cpp:296
> +    , m_cssomWrapper()

This line is not needed.

> Source/WebCore/css/StyleResolver.h:455
> +    CSSOMWrapper& cssomWrapepr() { return m_cssomWrapper; }

This name is wrong.
Comment 3 Hayato Ito 2013-01-28 00:02:09 PST
Thank you for the review.

(In reply to comment #2)
> (From update of attachment 184407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184407&action=review
> 
> Why should this be in a separate class? What is the purpose of this change?
> CSSOMWrapper is a bad name for this class, as we already have CSSOM wrappers in WebKit, and this class has very little to do with them.

Good point. Sorry for the lack of the explanation in this patch.

This patch was a separated patch from bug 107777 as mentioned in comment #0. The WIP patch in 107777 requires this change so we can pass CSSOMWrapper's instance, instead of StyleResolver, to DocumentRuleSet::appendAuthorStyleSheets().
I'd like to avoid passing StyleResolver here. I'd like to minimize the dependency from the separated class to the StyleResolver.

Another motivation is to collect related functions and related data into its own class so we can avoid having static functions in StyleResolver. I think the reason is similar to https://bugs.webkit.org/show_bug.cgi?id=89879, which is a META bug.


> 
> > Source/WebCore/css/StyleResolver.cpp:296
> > +    , m_cssomWrapper()
> 
> This line is not needed.
> 
> > Source/WebCore/css/StyleResolver.h:455
> > +    CSSOMWrapper& cssomWrapepr() { return m_cssomWrapper; }
> 
> This name is wrong.

Let me re-think good naming. If you have any ideas, please let me know it.
Comment 4 Hayato Ito 2013-01-28 01:32:17 PST
Created attachment 184953 [details]
Rename CSSOMWrapper to CSSOMWrapperForInspector.
Comment 5 Dominic Cooney 2013-01-28 06:40:00 PST
Comment on attachment 184953 [details]
Rename CSSOMWrapper to CSSOMWrapperForInspector.

View in context: https://bugs.webkit.org/attachment.cgi?id=184953&action=review

I am not a reviewer, but I caught a mispelling noted inline. Also some comments inline.

> Source/WebCore/ChangeLog:23
> +        (WebCore::CSSOMWrapperForInspector::collectFromSheet):

I notice that a lot of inspector-related types start with Inspector, so what about

InspectorCSSOMWrappers

? It is two chars shorter :)

Also makes me wonder if we can deport it to WebCore/inspector in a future change… what do you think?

> Source/WebCore/css/StyleResolver.cpp:-229
> -static void collectCSSOMWrappers(HashMap<StyleRule*, RefPtr<CSSStyleRule> >&, ListType*);

Looks like this code already used the term "wrapper", but it does look confusing, these are CSSOM objects and not JavaScript wrappers. Might make sense to do a renaming in a follow-up patch. (Are CSSOM objects referred to as "wrappers" elsewhere? Maybe there is precedent I am just not aware of.)

> Source/WebCore/css/StyleResolver.h:139
> +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);

I think moving this to its own class is a huge win. Of course this WARNING is not visible at call sites, and

StyleResolver::ensureAll

looks innocuous;

CSSOMWrapperForInspector::ensureAll

is way more likely to be detected as bogus if used in the wrong context.

> Source/WebCore/css/StyleResolver.h:455
> +    CSSOMWrapperForInspector& cssomWrapeprForInspector() { return m_cssomWrapperForInspector; }

Wrapepr => Wrapper
Comment 6 Dimitri Glazkov (Google) 2013-01-28 09:05:21 PST
Comment on attachment 184953 [details]
Rename CSSOMWrapper to CSSOMWrapperForInspector.

View in context: https://bugs.webkit.org/attachment.cgi?id=184953&action=review

>> Source/WebCore/css/StyleResolver.cpp:-229
>> -static void collectCSSOMWrappers(HashMap<StyleRule*, RefPtr<CSSStyleRule> >&, ListType*);
> 
> Looks like this code already used the term "wrapper", but it does look confusing, these are CSSOM objects and not JavaScript wrappers. Might make sense to do a renaming in a follow-up patch. (Are CSSOM objects referred to as "wrappers" elsewhere? Maybe there is precedent I am just not aware of.)

Kling/Antti recently made a pretty cool optimization, where CSSOM objects are actually created on demand, rather than by default, allowing most of CSS machinery function without lugging the CSSOM objects around. The "wrapper" here refers to the object that is created on demand.

> Source/WebCore/css/StyleResolver.h:135
> +class CSSOMWrapperForInspector {

Nitpicking the name: it's not really a wrapper, it's a bunch of wrappers. So, at the very least, it's CSSOMWrappersForInspector. I like Dominic's InspectorCSSOMWrappers suggestion.

>> Source/WebCore/css/StyleResolver.h:139
>> +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> 
> I think moving this to its own class is a huge win. Of course this WARNING is not visible at call sites, and
> 
> StyleResolver::ensureAll
> 
> looks innocuous;
> 
> CSSOMWrapperForInspector::ensureAll
> 
> is way more likely to be detected as bogus if used in the wrong context.

Yup, I agree.

>> Source/WebCore/css/StyleResolver.h:455
>> +    CSSOMWrapperForInspector& cssomWrapeprForInspector() { return m_cssomWrapperForInspector; }
> 
> Wrapepr => Wrapper

Derp!
Comment 7 Hayato Ito 2013-01-28 23:52:58 PST
Thank you for the review. Let me upload a new patch soon.

(In reply to comment #5)
> (From update of attachment 184953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184953&action=review
> 
> I am not a reviewer, but I caught a mispelling noted inline. Also some comments inline.
> 
> > Source/WebCore/ChangeLog:23
> > +        (WebCore::CSSOMWrapperForInspector::collectFromSheet):
> 
> I notice that a lot of inspector-related types start with Inspector, so what about
> 
> InspectorCSSOMWrappers
> 
> ? It is two chars shorter :)

That makes sense to me. A nice name. Done.

> 
> Also makes me wonder if we can deport it to WebCore/inspector in a future change… what do you think?

Sounds good. I think we can. Let me do that later in a future change.

> 
> > Source/WebCore/css/StyleResolver.cpp:-229
> > -static void collectCSSOMWrappers(HashMap<StyleRule*, RefPtr<CSSStyleRule> >&, ListType*);
> 
> Looks like this code already used the term "wrapper", but it does look confusing, these are CSSOM objects and not JavaScript wrappers. Might make sense to do a renaming in a follow-up patch. (Are CSSOM objects referred to as "wrappers" elsewhere? Maybe there is precedent I am just not aware of.)
> 
> > Source/WebCore/css/StyleResolver.h:139
> > +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> 
> I think moving this to its own class is a huge win. Of course this WARNING is not visible at call sites, and
> 
> StyleResolver::ensureAll
> 
> looks innocuous;
> 
> CSSOMWrapperForInspector::ensureAll
> 
> is way more likely to be detected as bogus if used in the wrong context.
> 
> > Source/WebCore/css/StyleResolver.h:455
> > +    CSSOMWrapperForInspector& cssomWrapeprForInspector() { return m_cssomWrapperForInspector; }
> 
> Wrapepr => Wrapper

Ops. Fixed.

(In reply to comment #6)
> (From update of attachment 184953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184953&action=review
> 
> >> Source/WebCore/css/StyleResolver.cpp:-229
> >> -static void collectCSSOMWrappers(HashMap<StyleRule*, RefPtr<CSSStyleRule> >&, ListType*);
> > 
> > Looks like this code already used the term "wrapper", but it does look confusing, these are CSSOM objects and not JavaScript wrappers. Might make sense to do a renaming in a follow-up patch. (Are CSSOM objects referred to as "wrappers" elsewhere? Maybe there is precedent I am just not aware of.)
> 
> Kling/Antti recently made a pretty cool optimization, where CSSOM objects are actually created on demand, rather than by default, allowing most of CSS machinery function without lugging the CSSOM objects around. The "wrapper" here refers to the object that is created on demand.
> 
> > Source/WebCore/css/StyleResolver.h:135
> > +class CSSOMWrapperForInspector {
> 
> Nitpicking the name: it's not really a wrapper, it's a bunch of wrappers. So, at the very least, it's CSSOMWrappersForInspector. I like Dominic's InspectorCSSOMWrappers suggestion.
> 
> >> Source/WebCore/css/StyleResolver.h:139
> >> +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> > 
> > I think moving this to its own class is a huge win. Of course this WARNING is not visible at call sites, and
> > 
> > StyleResolver::ensureAll
> > 
> > looks innocuous;
> > 
> > CSSOMWrapperForInspector::ensureAll
> > 
> > is way more likely to be detected as bogus if used in the wrong context.
> 
> Yup, I agree.
> 
> >> Source/WebCore/css/StyleResolver.h:455
> >> +    CSSOMWrapperForInspector& cssomWrapeprForInspector() { return m_cssomWrapperForInspector; }
> > 
> > Wrapepr => Wrapper
> 
> Derp!
Comment 8 Hayato Ito 2013-01-28 23:56:40 PST
Created attachment 185180 [details]
Fixed typo. Renamed to InspectorCSSOMWrappers.
Comment 9 Hajime Morrita 2013-01-29 01:06:18 PST
Comment on attachment 185180 [details]
Fixed typo. Renamed to InspectorCSSOMWrappers.

View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review

> Source/WebCore/css/StyleResolver.h:137
> +    // WARNING. This will construct CSSOM wrappers for all style rules and cache then in a map for significant memory cost.

Nit: s/then/them/
Actually, this message is explaining not only this function but also this whole class.

Probably we could do this collect thing in the constructor and have this class as a OwnPtr. Then lifecycle will be clearer.
Comment 10 Hayato Ito 2013-01-29 03:20:24 PST
(In reply to comment #9)
> (From update of attachment 185180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> 
> > Source/WebCore/css/StyleResolver.h:137
> > +    // WARNING. This will construct CSSOM wrappers for all style rules and cache then in a map for significant memory cost.
> 
> Nit: s/then/them/
> Actually, this message is explaining not only this function but also this whole class.
> 
> Probably we could do this collect thing in the constructor and have this class as a OwnPtr. Then lifecycle will be clearer.

Good point.
Although I don't have an intention to introduce any behavior changes in this patch, the mentioned point can be another good reason to factor this into itw own class.

Let me investigate how InspectorCSSOMWrappers is used in real case later. I think that can be done in a following patch.
Comment 11 Dimitri Glazkov (Google) 2013-01-29 09:38:18 PST
Comment on attachment 185180 [details]
Fixed typo. Renamed to InspectorCSSOMWrappers.

View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review

> Source/WebCore/ChangeLog:21
> +        (WebCore):

It's good WebKit practice to describe changes in the ChangeLog.

> Source/WebCore/css/StyleResolver.h:139
> +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);

Still nitpicking names, sorry. ensureAll and collectFromSheet are complementary. The first one makes wrappers for all available stylesheets. The other one makes additional wrappers for a newly added stylesheet -- but only if the first one ran beforehand.

So maybe collectFromSheet's name should reflect that? ensureNewSheet?

Or at least, collectFromSheetIfNeeded?

> Source/WebCore/css/StyleResolver.h:150
> +    void collect(ListType*);
> +
> +    void collect(DocumentStyleSheetCollection*);
> +    void collect(const Vector<RefPtr<CSSStyleSheet> >&);
> +    void collect(HashSet<RefPtr<CSSStyleSheet> >& sheetWrapperSet, StyleSheetContents*);

These are probably fine, but the same name overloaded over different arguments seems sad, especially given they are all private. Overloading might be a good API strategy, but here it's just not necessary.
Comment 12 Dimitri Glazkov (Google) 2013-01-29 09:44:25 PST
Comment on attachment 185180 [details]
Fixed typo. Renamed to InspectorCSSOMWrappers.

View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review

> Source/WebCore/inspector/InspectorCSSAgent.cpp:974
> +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));

Looking at the callsite, "ensureAll" is the one that has the wrong name. It should just be a "get". Then what do we do with the StyleSheetCollection argument? :-\
Comment 13 Hayato Ito 2013-01-29 23:17:54 PST
Created attachment 185404 [details]
No longer overloading.
Comment 14 Hayato Ito 2013-01-29 23:28:52 PST
Thank you for the review.

(In reply to comment #11)
> (From update of attachment 185180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        (WebCore):
> 
> It's good WebKit practice to describe changes in the ChangeLog.

Done.

> 
> > Source/WebCore/css/StyleResolver.h:139
> > +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> 
> Still nitpicking names, sorry. ensureAll and collectFromSheet are complementary. The first one makes wrappers for all available stylesheets. The other one makes additional wrappers for a newly added stylesheet -- but only if the first one ran beforehand.
> 
> So maybe collectFromSheet's name should reflect that? ensureNewSheet?
> 
> Or at least, collectFromSheetIfNeeded?

I like collectFromStyleSheet.

> 
> > Source/WebCore/css/StyleResolver.h:150
> > +    void collect(ListType*);
> > +
> > +    void collect(DocumentStyleSheetCollection*);
> > +    void collect(const Vector<RefPtr<CSSStyleSheet> >&);
> > +    void collect(HashSet<RefPtr<CSSStyleSheet> >& sheetWrapperSet, StyleSheetContents*);
> 
> These are probably fine, but the same name overloaded over different arguments seems sad, especially given they are all private. Overloading might be a good API strategy, but here it's just not necessary.

Done. I've named these functions explicitly.

I don't have a strong opinion about that. I guess the original intention about overloading is to use the same name to the template<ListType*> function.
Maybe explicit is better than implicit here.
Comment 15 Hayato Ito 2013-01-29 23:29:34 PST
(In reply to comment #14)
> Thank you for the review.
> 
> (In reply to comment #11)
> > (From update of attachment 185180 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> > 
> > > Source/WebCore/ChangeLog:21
> > > +        (WebCore):
> > 
> > It's good WebKit practice to describe changes in the ChangeLog.
> 
> Done.
> 
> > 
> > > Source/WebCore/css/StyleResolver.h:139
> > > +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> > 
> > Still nitpicking names, sorry. ensureAll and collectFromSheet are complementary. The first one makes wrappers for all available stylesheets. The other one makes additional wrappers for a newly added stylesheet -- but only if the first one ran beforehand.
> > 
> > So maybe collectFromSheet's name should reflect that? ensureNewSheet?
> > 
> > Or at least, collectFromSheetIfNeeded?
> 
> I like collectFromStyleSheet.

s/collectFromStyleSheet/collectFromStyleSheetIfNeeded/.

> 
> > 
> > > Source/WebCore/css/StyleResolver.h:150
> > > +    void collect(ListType*);
> > > +
> > > +    void collect(DocumentStyleSheetCollection*);
> > > +    void collect(const Vector<RefPtr<CSSStyleSheet> >&);
> > > +    void collect(HashSet<RefPtr<CSSStyleSheet> >& sheetWrapperSet, StyleSheetContents*);
> > 
> > These are probably fine, but the same name overloaded over different arguments seems sad, especially given they are all private. Overloading might be a good API strategy, but here it's just not necessary.
> 
> Done. I've named these functions explicitly.
> 
> I don't have a strong opinion about that. I guess the original intention about overloading is to use the same name to the template<ListType*> function.
> Maybe explicit is better than implicit here.
Comment 16 Hayato Ito 2013-01-29 23:31:09 PST
(In reply to comment #12)
> (From update of attachment 185180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> 
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:974
> > +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));
> 
> Looking at the callsite, "ensureAll" is the one that has the wrong name. It should just be a "get". Then what do we do with the StyleSheetCollection argument? :-\

Hmm.. 'ensureAllAndGet'? or 'getWithEnsuringAll'? What is the best name? :)
Comment 17 Dominic Cooney 2013-01-30 08:15:35 PST
Comment on attachment 185404 [details]
No longer overloading.

View in context: https://bugs.webkit.org/attachment.cgi?id=185404&action=review

> Source/WebCore/ChangeLog:8
> +        Factored CSSOMWrapper logic and data out from StyleResolver into a

Does this sentence add anything that is not in the summary line?

> Source/WebCore/ChangeLog:15
> +        in a following patch (bug 107780), I'll move a

Nit: No need for the trailing "a" here.
Comment 18 Dominic Cooney 2013-01-30 08:19:48 PST
(In reply to comment #16)
> (In reply to comment #12)
> > (From update of attachment 185180 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> > 
> > > Source/WebCore/inspector/InspectorCSSAgent.cpp:974
> > > +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));
> > 
> > Looking at the callsite, "ensureAll" is the one that has the wrong name. It should just be a "get". Then what do we do with the StyleSheetCollection argument? :-\
> 
> Hmm.. 'ensureAllAndGet'? or 'getWithEnsuringAll'? What is the best name? :)

What about getWrapperForRuleInSheets?
Comment 19 Dimitri Glazkov (Google) 2013-01-30 10:07:18 PST
Comment on attachment 185404 [details]
No longer overloading.

View in context: https://bugs.webkit.org/attachment.cgi?id=185404&action=review

> Source/WebCore/css/StyleResolver.h:135
> +class InspectorCSSOMWrappers {

The obvious thing to do here is to let this guy hold a ref to DocumentStyleSheetCollection. But that's extra weight :-\

> Source/WebCore/inspector/InspectorCSSAgent.cpp:962
> +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));

The next easiest thing is to split up this callsite to first "collectAllSheets", and then "get".
Comment 20 Hayato Ito 2013-01-30 20:25:16 PST
Thank you for the review. Let me upload a new patch soon.

(In reply to comment #17)
> (From update of attachment 185404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185404&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Factored CSSOMWrapper logic and data out from StyleResolver into a
> 
> Does this sentence add anything that is not in the summary line?

That's redundant. Removed.

> 
> > Source/WebCore/ChangeLog:15
> > +        in a following patch (bug 107780), I'll move a
> 
> Nit: No need for the trailing "a" here.

Done.

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #12)
> > > (From update of attachment 185180 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> > > 
> > > > Source/WebCore/inspector/InspectorCSSAgent.cpp:974
> > > > +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));
> > > 
> > > Looking at the callsite, "ensureAll" is the one that has the wrong name. It should just be a "get". Then what do we do with the StyleSheetCollection argument? :-\
> > 
> > Hmm.. 'ensureAllAndGet'? or 'getWithEnsuringAll'? What is the best name? :)
> 
> What about getWrapperForRuleInSheets?

Okay. I've renamed it.


(In reply to comment #19)
> (From update of attachment 185404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185404&action=review
> 
> > Source/WebCore/css/StyleResolver.h:135
> > +class InspectorCSSOMWrappers {
> 
> The obvious thing to do here is to let this guy hold a ref to DocumentStyleSheetCollection. But that's extra weight :-\
> 
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:962
> > +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));
> 
> The next easiest thing is to split up this callsite to first "collectAllSheets", and then "get".

I don't want to split this into two functions until there is a strong requirement.
If we split this into two functions, we have to check whether all callers call them sequentially.
I am afraid that will cause potential misusage of APIs.
Comment 21 Hayato Ito 2013-01-30 20:26:37 PST
Created attachment 185667 [details]
Addressed.
Comment 22 Dimitri Glazkov (Google) 2013-01-30 20:37:10 PST
Comment on attachment 185667 [details]
Addressed.

you wore me down.
Comment 23 WebKit Review Bot 2013-01-30 21:05:31 PST
Comment on attachment 185667 [details]
Addressed.

Clearing flags on attachment: 185667

Committed r141373: <http://trac.webkit.org/changeset/141373>
Comment 24 WebKit Review Bot 2013-01-30 21:05:37 PST
All reviewed patches have been landed.  Closing bug.