229285 - separate nsIXULOverlayProvider from nsIXULChromeRegistry

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Benjamin Smedberg]

per my plan at http://bdsmedberg.no-ip.org/chrome/ I'm separating the chrome
registry into a set of smaller interfaces. This bug is for nsIXULOverlayProvider
(because that's the easiest one). First-draft patch forthcoming.

Comment 1

[Benjamin Smedberg]

Attachment: nsIXULOverlayProvder (seamonkey-only)

This is a working patch for seamonkey. I want to get review comments on this
(not necessarily detailed nitpicks, but at least that the interfaces are
correct). Before checking in, of course, I'm gonna need to patch the toolkit,
as well as chromelite and perhaps the embed chrome service.

Comment 2

[Benjamin Smedberg]

oops, bryner is module owner

Comment 3

[David Hyatt]

I'd prefer if you could avoid the goto usage in nsXULDocument.cpp.

Comment 4

[Benjamin Smedberg]

darin has been pushing the goto-preventing-early-returns style as a way to cut
codesize by significant amounts. Is there an alternate coding style that
prevents the goto but doesn't have early returns? (multiple comptr and string
destructor code is the culprit).

Comment 5

[Brian Ryner (not reading)]

I'm worried that this is making things more complicated than they need be.  What
sort of code other than the chrome registry would ever implement
nsIXULOverlayProvider?  If we don't know of any, then I would say the category
thing is overkill.

Comment 6

[Benjamin Smedberg]

Comment on attachment 137888 [details] [diff] [review]
nsIXULOverlayProvder (seamonkey-only)

More work needed on this: I wasn't separating out non-chrome XUL correctly.

As for the category, I can't think of a use-case, so we can probably just use a
contractid. However, I do want to keep the category for the styleprovider (in
my proposal), because I definitely see an extension use-case for that, plus
separating the userContent/userChrome provider from the chrome provider and
perhaps the scrollbar provider.

Comment 7

[Benjamin Smedberg]

Attachment: nsIXULOverlayProvider

This appears to work nicely on seamonkey. I'm having a problem with my new
firebird build; it has an undefined entity in the help overlay. I don't know
whether this is because of this patch or is a fluke or something else.

Comment 8

[Benjamin Smedberg]

Comment on attachment 138434 [details] [diff] [review]
nsIXULOverlayProvider

My firebird bug was a fluke, I think. This should be ready for review. Hyatt,
do you want to review this, or should I ask jst?

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 138434 [details] [diff] [review]
nsIXULOverlayProvider

r=bryner

Comment 10

[Benjamin Smedberg]

I tried to check this in, but it caused orange on the tboxen that were running
the Txul test. Mozilla runs fine, it's just the -chrome flag with a file: URI
causes assertions like "no xul:window".

I have backed it out... would love suggestions on what might have caused this.

Comment 11

[Benjamin Smedberg]

FIXED on trunk. I had misplaced the close-brace in nsXULDocument::EndLoad such
that non-chrome XUL docs never called ResumeWalk...

Comment 12

[Brendan Eich [:brendan]]

Please don't add trailing whitespace.  Nagging cuz the misplaced brace (really,
misplaced PrepareToWalk/ResumeWalk code -- indentation was off) caught my eye. 
Reviewers must have blinked.  I'm happy to review XUL document/prototype code,
obviously....

/be

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This broke the minimo build (I'm not sure which option specifically).  See the
egg build on
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Testerbox&hours=36

Comment 14

[Brendan Eich [:brendan]]

Minimo's not tier 1, but it is heading that way.  Benjamin, can you look into
this?  I'll leave it to you to reopen, or not -- main thing is to fix the bustage.

/be

Comment 15

[Benjamin Smedberg]

I'm on it... this is due to --disable-xul.  dougt, do we need to build
rdf/chrome at all for minimo? Don't we use the minimo chrome registry?

Comment 16

[Benjamin Smedberg]

Attachment: Don't build chrome registry if --disable-xul is specified

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 139134 [details] [diff] [review]
Don't build chrome registry if --disable-xul is specified

If you're going to take this approach, it seems like
embedding/minimo/chromelite should be built ifndef MOZ_XUL instead of ifdef
MINIMO.

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 139134 [details] [diff] [review]
Don't build chrome registry if --disable-xul is specified

i have something similar in my tree.  however, i just changed ifndef
MOZ_XUL_APP to ifdef MOZ_XUL_APP.

Comment 19

[Brian Ryner (not reading)]

Comment on attachment 139134 [details] [diff] [review]
Don't build chrome registry if --disable-xul is specified

what dbaron said.

Comment 20

[Benjamin Smedberg]

OK, I've checked in a patch that builds embedding/minimo/chromelite when XUL_APP
is not defined, and rdf/chrome otherwise.

Comment 21

[Brendan Eich [:brendan]]

> darin has been pushing the goto-preventing-early-returns style as a way to cut
> codesize by significant amounts. Is there an alternate coding style that
> prevents the goto but doesn't have early returns? (multiple comptr and string
> destructor code is the culprit).

Why isn't such code fused by the compiler?  If you have to write

    nsCOMPtr<nsIWhatever> whatever = ...;
    nsCAutoString s;
    rv = whatever->GetSomeString(&s);
    if (NS_FAILED(rv)) return rv;

    do more stuff;
    if (NS_FAILED(rv)) return rv;

    do yet more stuff;
    return rv;

like so:

    nsCOMPtr<nsIWhatever> whatever = ...;
    nsCAutoString s;
    rv = whatever->GetSomeString(&s);
    if (NS_SUCCEEDED(rv)) {
        do more stuff;
        if (NS_SUCCEEDED(rv)) {
            do yet more stuff;
        }
    }
    return rv;

then (a) the success case takes the branch-predicted-not-taken path, which hurts
on modern (super-scalar) CPUs; (b) we might as well manually refcount, since
there is a single return and a single place to put the NS_IF_RELEASE or the
NS_RELEASE or whatever; my original point (c), we need a better compiler,
because any decent basic block analyzing optimizer should be able to fuse the
common destructor calls that pile up before early returns, along with fusing the
returns.

I don't like bending our code around weak compilers (gcc pre 3.5? ;-) -- when
the compilers get fixed, the code is still bent.  And I don't like the very real
costs in (a), either -- they show the penalty for overindenting normal-case code
is not just aesthetic.

About (b), the only thing to say is that code with manual refcounting ages
badly, assuming it was written by Top Men -- and if it wasn't, then it's leaky
from the get-go.  In past kernel hacking lives, I and a few comrades could keep
things balanced in C (no autostrings/smartptrs), but even then, the kernel
hacker gene pool eventually regressed to the mean.  We have smart pointers and
auto-strings -- let's use them as if we had them, not as if we were stuck with
manual ref-counting, which does not scale across a large project.

Sorry to spam a closed bug, but I wanted to record these thoughts and get some
reactions.

/be