515847 - [SeaMonkey] xpcshell-tests: 11 failures since 'asutherland Wed Sep 09 18:15:56 2009 -0700' major import

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Serge Gautherie (:sgautherie)]

Regression timeframe:
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=8931dc002164&tochange=0b161ed73eb6

Filing new bug, per bug 515805 comment 4.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

The test failures appear to fall into two categories:

1) Something involving viewWrapperTestUtils.js not having been found.  I don't understand that... it looks like it's getting installed:

/builds/slave/comm-1.9.1-linux-unittest/build/objdir/mozilla/config/nsinstall -R /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/abSetup.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/alertTestUtils.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/asyncTestUtils.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/mailDirService.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/mailShutdown.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/mailTestUtils.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/messageGenerator.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/messageModifier.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/msgFolderListenerSetup.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/POP3pump.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/searchTestUtils.js /builds/slave/comm-1.9.1-linux-unittest/build/mailnews/test/resources/viewWrapperTestUtils.js ../mozilla/_tests/xpcshell/mailnews/resources

2) gloda is not fully initializing because it is missing string bundles that it really wants.

I will spin a bug off about this one.

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> The test failures appear to fall into two categories:
> 
> 1) Something involving viewWrapperTestUtils.js not having been found.  I don't
> understand that... it looks like it's getting installed:

Not quite. The mailnews/base ones are due to the new quickSearchManager.js being included in viewWrapperTestUtils.js, as a result, SeaMonkey now requires quickSearch.properties even though its not using it yet...

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Attachment: move the dbViewWrapper abstraction and dependencies into mail/base

Per discussion with Standard8, the strategy is to move the dbViewWrapper abstraction (which includes quickSearchManager.js) into mail/base and out of mailnews/base.

As a result of changes made for bug 474711 the quickSearchManager abstraction has become somewhat more feature-centric, and is better homed in Thunderbird.  By feature-centric I mean that the set of quick searches exposed to the UI is now explicitly provided by the quickSearchManager.  While this tighter coupling makes things more clear and reduces the number of files that need to be touched if a change in the set of quicksearches is made, it also makes the code in the file explicitly driven by Thunderbird features.  So into Thunderbird it goes.

Since the FolderDisplayWidget already lives in mail/ this is reasonably consistent.

Comment 4

[Robert Kaiser]

Everything that is backend should be mailnews/ really and (almost) everything that is frontend/UI should be mail/ and suite/ - backend abstraction to be used by the UI should also be shared.

What category does this fall into?
What does the move mean in terms of SeaMonkey porting Thunderbird functionality over and possibly just integrating it differently in UI? Does it mean we need to do more identical copies and back/forth bugporting regardless of how the UI actually looks? If so, we are moving into the wrong direction with this.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

To be extremely honest and painfully blunt while raising a more general issue, I don't see why Thunderbird's engineering decisions should be based around the needs of SeaMonkey.

While good programming practices result in reusable abstraction layers so that you can theoretically hang multiple user interfaces off a single back-end, that doesn't mean we need to actually hang multiple users interfaces off of it.

Thunderbird is not trying to be a platform to write mail clients.  Thunderbird is trying to be a good mail/messaging client, and it's debatable whether we even have the resources to be doing that, let alone making sure every change we make can be reused in another mail client whose UI design principles are diametrically opposed to Thunderbird's.

If Thunderbird's backend was a static thing, there would be no harm in hanging SeaMonkey off of it.  But continual refactoring is required.  If doing the right thing in Thunderbird means changing both Thunderbird's front-end and the back-end and SeaMonkey also is using that back-end, then I need to change SeaMonkey too.  I don't see any benefit to that but I do see lots of downsides.


So, from that perspective, yes, moving this code into mail/ is the right thing.  It obviously sucks for SeaMonkey since that leaves cloning/forking as the only option, but I don't see a way to pull a win/win situation out of the reality we are confronted with.  SeaMonkey has made a very conscious decision to not embed Thunderbird but instead have its own mailnews UI.  That is an expensive decision and it is up to SeaMonkey to pay the cost, not Thunderbird.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Attachment: move the dbViewWrapper abstraction and dependencies into mail/base v2

whoops, previous patch did not fix test-folder-display-helpers.js.

Comment 7

[Robert Kaiser]

It's interesting that working together seems to work mostly - it just fails when someone comes in and is pissing off the partners he should try to work with and could profit from himself.

Comment 8

[Robert Kaiser]

Can't block on something that goes the wrong way, sorry.

Comment 9

[Mark Banner (:standard8)]

Maybe I'm not quite understanding the code well enough yet, however what I do know is this:

- Some of the code in mailnews/ is now dependent on the specific implementation of the search bar that Thunderbird has done - so Thunderbird UI code has entered mailnews/.
- SeaMonkey may or may not want to include that as-is and SeaMonkey may or may not want to change its design when it does.

Moving code from mailnews/ to mail/ does not stop sharing of code. All it does is admit that the current implementation is specific on the Thunderbird UI design.

There is nothing to stop SeaMonkey examining this code later and moving some or all of it back to mailnews/ with any changes necessary to better support a backend/frontend split if it is appropriate.

This is exactly what happened with the RSS backend.

Comment 10

[Serge Gautherie (:sgautherie)]

Standard8: ping for reviews.

Comment 11

[Robert Kaiser]

(In reply to comment #5)
> To be extremely honest and painfully blunt while raising a more general issue,
> I don't see why Thunderbird's engineering decisions should be based around the
> needs of SeaMonkey.

You're right, this *is* extremely blunt - and it turns things much farther around than wanted or needed. I'm trying hard to make us live together and share code as much as reasonable and at the same time not break each other. This sometimes means tradeoffs, either on the side of not sharing some code or on the side of sparing a few minutes to help the other project to not break. And so far, this has worked well, I hope that will continue.

What I said in bug 515855 comment #6 is still true, we want to ship the gloda backend in SeaMonkey 2.0 in a state that an extension can easily enough implement e.g. faceted search on top of it.

If comment #9 assesses things correctly, that all sounds reasonable. If everything would be fixed by just shipping one L10n file in SeaMonkey as well, so be it, it's not much of a hassle.

Let's just put out the flames and get us to continue working together, and both projects can profit from the other as we're showing continuously.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 400021 [details] [diff] [review]
move the dbViewWrapper abstraction and dependencies into mail/base v2

Transferring review to bienvenu since he is more familiar with the code.  Original review was targeted at Standard8 for time reasons.

In regards to this patch, Standard8's summary is apt.  The code being migrated to mail/ is effectively Thunderbird UI-specific logic at this point.

When originally developed, I was on the line as to whether DBViewWrapper should live in mailnews/ or mail/.  It is now quite clear that the functional unit is FolderDisplayWidget (which lives in mail/).  Additionally, much of our testing is accomplished in Thunderbird-specific mozmill tests.

Comment 13

[David :Bienvenu]

this seems to have bit-rotted - qimporting failed in mail/base/modules/Makefile.in

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Attachment: move the dbViewWrapper abstraction and dependencies into mail/base v3 [Checkin: Comment 16]

de-bit-rotted

Comment 15

[David :Bienvenu]

Comment on attachment 402008 [details] [diff] [review]
move the dbViewWrapper abstraction and dependencies into mail/base v3
[Checkin: Comment 16]

As Standard8 says, if and when SM wants to use this code, we can revisit this.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

attachment 402008 [details] [diff] [review] pushed:
http://hg.mozilla.org/comm-central/rev/294e8a69a83c

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> The test failures appear to fall into two categories:

Ftr,

> 1) Something involving viewWrapperTestUtils.js not having been found.  I don't
> understand that... it looks like it's getting installed:

I confirm that, per tinderboxes, comment 16 push fixed the 3 test_viewWrapper_*.js failures.

> 2) gloda is not fully initializing because it is missing string bundles that it
> really wants.
> 
> I will spin a bug off about this one.

Bug 515855.