Refactor CSS to be injected into FullStructure instead of using the static version.

[patco...@google.com]

Reviewers: hearnden,

Please review this at http://codereview.waveprotocol.org/617001

Affected files:
M src/org/waveprotocol/wave/client/StageOne.java
M
src/org/waveprotocol/wave/client/wavepanel/view/dom/BlipMenuItemDomImpl.java
M src/org/waveprotocol/wave/client/wavepanel/view/dom/BlipMetaDomImpl.java
M
src/org/waveprotocol/wave/client/wavepanel/view/dom/CollapsibleDomImpl.java
A src/org/waveprotocol/wave/client/wavepanel/view/dom/CssProvider.java
M src/org/waveprotocol/wave/client/wavepanel/view/dom/FullStructure.java
M
src/org/waveprotocol/wave/client/wavepanel/view/dom/InlineConversationDomImpl.java
M
src/org/waveprotocol/wave/client/wavepanel/view/dom/InlineThreadDomImpl.java
M
src/org/waveprotocol/wave/client/wavepanel/view/dom/full/ParticipantsViewBuilder.java
A
src/org/waveprotocol/wave/client/wavepanel/view/dom/full/WavePanelCssProvider.java
M
src/org/waveprotocol/wave/client/wavepanel/view/dom/full/WavePanelResourceLoader.java

[patco...@google.com]

Hey David - to summarize the changes:
1) CssProvider interface added, with WavePanelCssProvider implementation
- let me know if these should be commented more or moved to a more
appropriate package
2) FullStructure now requires a CssProvider and threads it through to
the various DomImpls, which have their static references removed.
3) StageOne#DefaultProvider now injects the wave panel css by default
into the FullStructure, but lets a subclass override it to inject its
own.
4) FullStructure and WavePanelResourceLoader had a small amount of code
structure tidying to fit better on lines and be more standardised.

let me know whether this is the ideal way to do it, and if the packages
are ok.
fwiw, ant waveharness-hosted worked fine in chrome with these changes.

http://codereview.waveprotocol.org/617001

[veg...@gmail.com]

Great patch! Thanks for doing this!
I hope that will open the way for server side rendering.

http://codereview.waveprotocol.org/617001

[hear...@google.com]

Code LGTM. Some comments below.

Before submitting, please ensure that the static evaluability of CSS
expressions is unaffected.

Here's a suggestion on how. BlipMenuItemDomImpl.java has the
expression:
CSS.menuOption() + " " + CSS.menuOptionSelected()
If you turn off CSS obfuscation for those classes (sticking @external
lines in the .css file is the only way I know how), then first check
that the string "menuOption menuOptionSelected" occurs in the compiled
JS of a clean client. If not, then static evaluability has already been
broken :(. If it is present, then do the same thing in your patch. If
those expressions are no longer static, and have become actual string
concatenations, then we should investigate what we have to do to get the
compiler to realise that those expressions are statically evaluable.

http://codereview.waveprotocol.org/617001/diff/1/6
File
src/org/waveprotocol/wave/client/wavepanel/view/dom/CssProvider.java
(right):

http://codereview.waveprotocol.org/617001/diff/1/6#newcode29
Line 29: public interface CssProvider {
Is there any reason this needs to be an interface?

Since this just represents a tuple value object, rather than an object
that has behaviour, an immutable value class would be more idiomatic.
i.e.,

final class CssResources {
public final BlipViewBuilder.Css blipCss;
public final CollapsibleBuilder.Css collapsibleCss;
...

public CssResources(...) {
this.blipCss = blipCss;
this.collapsibleCss = collapsibleCss;
...
}
}

That structure guarantees that the Css objects can't be replaced over
time, and incur no cost to retrieve, unlike getters which may be
accidentally implemented in a non-optimal way.

The only reason I can think of to have an interface is to allow some
environments to provide these resources lazily, or some environments
that want to stub out some rather than all of the css resources - is
that what you have in mind?

http://codereview.waveprotocol.org/617001/diff/1/11
File
src/org/waveprotocol/wave/client/wavepanel/view/dom/full/WavePanelCssProvider.java
(right):

http://codereview.waveprotocol.org/617001/diff/1/11#newcode24
Line 24: public class WavePanelCssProvider implements CssProvider {
I wonder if this could be merged with WavePanelResourceLoader somehow.
I get that WPRL is static, whereas CssProvider needs to be an object,
but WPRL could be made into a singleton rather than a static. Or WPRL
could have an inline static CssProvider:

class WPRL {
public static CssProvider getCss() {
return new CssProvider() {
public BlipViewBuilder.Css getBlipCss() {
return getBlip().css();
}
...
};
}
}

If you think it's worth doing, then have a go.

http://codereview.waveprotocol.org/617001

[veg...@gmail.com]

I can understand that static availability enhances performance, but by
how much? 100% 1000% ? Also, is there a way to re-use the same rendering
logic for both server and client side without compromising static
availability? If the answer is no, then I think that would make server
side rendering very confusing and hard to maintain.

[hear...@google.com]

On 2011/09/11 19:40:11, Yuri Z. wrote:
> I can understand that static availability enhances performance, but by
how much?
> 100% 1000% ?

That's a great question - I honestly have no idea.

> Also, is there a way to re-use the same rendering logic for both
> server and client side without compromising static availability?

Yes. By not including the server-side CSS provider in the glob of files
given to the GWT compiler, it will notice that there is only one class
that implements that interface (the client-side CSS provider), so it can
then devirtualize all the calls, and so as long as the client-side CSS
provider is statically evaluable, then so will be the CSS expressions in
the rendering code.

> If the answer
> is no, then I think that would make server side rendering very
confusing and
> hard to maintain.

I agree. If we're forced to make a choice, and have either server-side
rendering or static evaluability of CSS expressions, I'd pick
server-side rendering. But I'm pretty sure we can have both, and if
it's not too much effort to maintain, it would be good to keep static
evaluability.

http://codereview.waveprotocol.org/617001

[patco...@google.com]

Thanks - after building it I get the following in the JS (in PRETTY
mode, it seems):
($location_1[stackIndex] = 'BlipMetaDomImpl.java:' + '141' ,
item).self_0).className = 'menuOption menuOptionSelected'

so it appears as though it's maintained.
fwiw, changed CssProvider to be a final tuple class, now created by
WPRL.

http://codereview.waveprotocol.org/617001

[hear...@google.com]

On 2011/09/12 03:51:33, patcoleman wrote:
> Thanks - after building it I get the following in the JS (in PRETTY
mode, it
> seems):
> ($location_1[stackIndex] = 'BlipMetaDomImpl.java:' + '141' ,
> item).self_0).className = 'menuOption menuOptionSelected'

> so it appears as though it's maintained.
> fwiw, changed CssProvider to be a final tuple class, now created by
WPRL.

Great, thanks. LGTM.

http://codereview.waveprotocol.org/617001

[veg...@gmail.com]

http://codereview.waveprotocol.org/617001/diff/2003/5002
File src/org/waveprotocol/wave/client/StageOne.java (right):

http://codereview.waveprotocol.org/617001/diff/2003/5002#newcode37
Line 37: import
org.waveprotocol.wave.client.wavepanel.view.dom.full.WavePanelCssProvider;
Is it a new class? This import causes compilation error for me.

http://codereview.waveprotocol.org/617001