Closed Bug 408238 Opened 17 years ago Closed 17 years ago

back out unreviewed microb changes.

Categories

(Core Graveyard :: Embedding: GTK Widget, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

()

Details

Attachments

(5 files, 1 obsolete file)

we are backing out microb changes.  the cleanest way to do this is to revert mozilla/embedding/browser/gtk to the timestap '2006-11-01 20:00', then clean up the minor bustages which include:

a few header files were renamed.
NS_STATIC_CAST => static_cast

Once we back out these changes, we will need to reland the reviewed changes:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fembedding%2Fbrowser%2Fgtk%2F&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=all&mindate=&maxdate=&cvsroot=%2Fcvsroot
Attachment #292986 - Flags: superreview?(brendan)
Attachment #292986 - Flags: review?(benjamin)
i was hoping to land this back-out patch, the land reviewed changes on top of this back-out.  In this way, it is easier to look at a patch to see what I actually did.
Adding dependencies to the bugs in the query in comment 0 back through November 1, 2006.
Adding dependencies to the bugs in the query in comment 0 back through November 1, 2006.
OS: Mac OS X → All
Hardware: PC → All
Attachment #292994 - Flags: review+
Attachment #292986 - Flags: review?(benjamin)
Comment on attachment 292986 [details] [diff] [review]
patch to back out microb.

rs=me if you still need it -- where'd the review flag go on this attachment? Oh, next patch. I'll stamp both.

/be
Attachment #292986 - Flags: superreview?(brendan) → superreview+
Who else should be involved here? Is Marco (Epiphany author) still working with branch gtk embedding code?

/be
Comment on attachment 292994 [details] [diff] [review]
diff between the clean back out and and what I want to check in. 

rs=me again.

/be
Attachment #292994 - Flags: superreview+
i checked out Epiphany.  they are pointing their users to use the distro's default install of mozilla.
I wasn't concerned about ephy, I was asking whether Marco is still a peer or candidate owner. Anyone else involved, please cc: them on this bug.

/be
CC:ing marco.

AFAIK they've written their own tiny embedding layer ('hulahop') for OLPC so probably he's not interested in peer or owner.
Flags: blocking1.9?
Maybe we should be interested in hulahop?

/be
What do you think about removing of gtkmozembed static linkage from libxul library at all?
i haven't thought about it;  I didn't accidently remove it, did i?
I'm actually interested to help out. We have been forced to write our own simple widget because of the maintenance situation (happy to see we are reverting!). I'd rather see OLPC using gtkmozembed and helping to improve it.
marco, do you have any thoughts on the above patch?
The src/ part of the patch looks good to me.

About the tests/ part:

I see you are removing gtk 1 support in the test but not in the widget implementation. Ideally we would drop it all together in a separate step. Not a big deal, if the changes are necessary to keep the build working it's fine.

+  gtk_moz_embed_push_startup();
   gtk_main();
+  gtk_moz_embed_pop_startup();

Explicit push/pop_startup didn't use to be required. We should investigate what broke exactly (it's not really clear by reading #372077). I don't think this should block the patch to land though, we can open a separate bug.

It continues to work without push/pop startup calls... but it causes assertions in debug builds and makes XPCOM leak statistics meaningless: we added the new NS_LogInit/Term API in the 1.9 timeframe so that we could more accurately get leak statistics and assert that XPCOM was not being called from static constructors/destructors.
Ah, bug 372077 says that without it we was segfaulting on exit, I haven't tested it though.
Why we cannot just re-check that code properly...

Also apply some patches from 
https://garage.maemo.org/svn/browser/mozilla/trunk/libgtkembedmoz/debian/patches

There are also many fixes...
It would be very nice to see some comments on this issue from the current GtkMozEmbed owner Josh Soref.

There are traces of some discussions between him and the other Mozilla guys:

http://groups.google.com/group/mozilla.dev.embedding/browse_thread/thread/db06da56b3f48db3/61b20bcf40abd511
http://groups.google.com/group/mozilla.dev.embedding/browse_thread/thread/c1251c42485081f5#21d3da0963458bfa

It would be very nice to hear from Josh about the code quality and status in the GtkMozEmbed component in the Mozilla trunk.
See Josh comments in the first thread you pointed out, that's the best argument to just back out all the changes. The security bug (and the fact that previous gtkmozembed users has been forced to switch away from it) is an obvious consequence of the development process.
(In reply to comment #22)

If I’ve got the point correctly, here are the reasons for this issue:

1. Security bug
2. Unhappy GtkMozEmbed users
3. Josh’s comments

AFAIK,

1. The bug was already fixed in the trunk: # 406724.
2. It would be nice to see some bug reports and bug numbers describing the reason for the “users have been forced to switch away” case(s).
3. IMO, Josh is mostly blaming himself for an inability to maintain the GtkMozEmbed module. IMO, the module owner has a full control and the responsibility to maintain the code the way she/he likes.

And I’d still like to hear the comments from Josh.


(In reply to comment #23)
> IMO, the module owner has a full control and the
> responsibility to maintain the code the way she/he likes.

That's not my understanding. I'm the module owner of mozilla/security/manager, but I always seek for review, I never check in without public review that happened in a public bugzilla (except for really small obvious or trivial changes like whitespace or comments).

See also http://www.mozilla.org/hacking/module-ownership.html
which talks about responsibilities. I don't think it says a module owner can do whatever he wants.
The module owner does not have "full control". All checkins in any module that is shipped with Firefox/XULRunner require bugs and documented reviews. Because those procedures were not carried out and have caused documented problems, it is better to back out to a known state and re-land any patches properly with bugs and documented reviews.
(In reply to comment #25)
(In reply to comment #24)
When I wrote about the module owner's responsibilities I've assumed they include the proper review process. Excuse me if I was not clear enough.
LeoZ, in the newsgroup, i proposed an alternate way to go -- it would include reviewing all the patchbombs and file/fix bugs we find.  I would be equally happy with that approach.  
romaxa, in comment #20 you ask why can't we apply the changes at some URL.  I am sure there is good stuff in some of those patches, but they have to be reviewed using the process.  File bugs, attach patches, ask for reviews.
(In reply to comment #27)
> LeoZ, in the newsgroup, i proposed an alternate way to go -- it would include
> reviewing all the patchbombs and file/fix bugs we find.  I would be equally
> happy with that approach.  
> 
That's excellent!

(In reply to comment #28)
romaxa will make the separate bug reports for the every patch we have for the GtkMozEmbed here: https://garage.maemo.org/svn/browser/mozilla/trunk/libgtkembedmoz/debian/patches

romaxa and tonikitoo are the main contributors to all the changes in the GtkMozEmbed.
Perhaps, they would be glad to help.
(In reply to comment #23)
> 1. The bug was already fixed in the trunk: # 406724.

The point is that the bug happened because of the bad development process. Patches needs to be reviewed carefully *before* going in, it's that simple.

> 2. It would be nice to see some bug reports and bug numbers describing the
> reason for the “users have been forced to switch away” case(s).

It's not matter of one or more specific bug reports. The code quality is just really low. The OLPC software is very much experimental and a work in progress, so we don't even have very high quality/stability requirements at the moment.

If we don't revert, the code will go in a stable version and hit mature projects like the several GNOME application using gtkmozembed. That's really going to suck.

Do you really expect anyone to spend time reporting bugs to a projects whose maintainer says something like: "i'm trying, it's hard, it's not a process i like at all. the direction it's taking is not the direction i want."
I fully agree with Marco. I prefer the approach "back out".

If we tried to review the existing code, we'd always have to check "is this code still in place, or has it been changed already by a newer check in?".
I suggest we try to avoid that confusion.

If we back out, you will be able to run a diff between cvs and your latest code, you will know exactly what is missing and what you require to land.

I think the "back out, attach patches, review, land again" will be much easier for everybody to understand.
What I think is cleanest is to land the patch that does the backout to pre-microb landing now.  Then over the next day or two, land the cleaned up patches for the bugs I mentioned above.  This will put us in a state where we know is valid and sound.
 
Then we can ask LeoZ, romaxa, tonikitoo and the new module owner and peers for this code to figure out what to do with microb stuff.
(In reply to comment #33)

Alternative way would be:

1. Perform the review for the first Doug's patch:
https://bugzilla.mozilla.org/attachment.cgi?id=292986.

2. Review the separate bugs and patches for the changes set from here:
https://garage.maemo.org/svn/browser/mozilla/trunk/libgtkembedmoz/debian/patches

Hi LeoZ,

I would love for your suggestion to just work.  The problem is that the patch 292986 is 64k without any sort of info about what it is suppose to do.  I think what we can do is break up this patch into the specific bugs/features it is suppose to address, file bugs against each, attach patches, and seek reviews.  I would need your help doing this.  Benjamin, what do you think -- can you review this patch as is (as the peer of this code)?


Lets not talk about the maemo.org patches here; we should discuss them in the newsgroup (or better yet follow up on the bugs blassey/I created for each patch).



No, I certainly could not review that as single 64k patch for re-landing. There are a couple reasons for this:

* If there are regressions, it is better to land individual changes one at a time... that way we can back out to know not-broken states in little pieces

* There are fixes for multiple issues here, and it would be mentally very difficult to separate during review

* There is no indication of who wrote which parts or who I could ask review questions...
(In reply to comment #35)
> what we can do is break up this patch into the specific bugs/features it is
> suppose to address, file bugs against each, attach patches, and seek reviews. 

I think since the most of the changes came from us we have to split this patch and put the description and the author or the source to every piece.

I think romaxa will be in charge of splitting and commenting the individual patches. We will try also to involve Antonio to speed up the work.
We will submit the separate patches right to this bug.
hi leoz, you can count on me, for sure.
LeoZ, Antonio --

This is great news.  I don't want to lose anything valuable in this patch, I think we just want to ensure the changes are at the quality level that we require.
patch < "292986: patch to back out microb."
patch -R < "292994: diff between the clean back out and and what I want to check in."
patch < "Missing changes for bug 363089"
I can continue and it will be ~30 (not sure) small patches or same count of bugs...

Most of that bugs will be enhancements, interface extending...

probably would be better at first to kill GTK1 pieces...
also remove non frozen API usage.
(In reply to comment #44)
> I can continue and it will be ~30 (not sure) small patches or same count of
> bugs...
> 
> Most of that bugs will be enhancements, interface extending...
> 
> probably would be better at first to kill GTK1 pieces...
> also remove non frozen API usage.
> 
Excellent! Please, continue.
If you need some help, for example, for the big tasks like "kill GTK1 pieces", please, share your work with Antonio.
(In reply to comment #38)
> hi leoz, you can count on me, for sure.
> 

Perfect! Thanks a lot, Antonio!
(In reply to comment #37)
> I think romaxa will be in charge of splitting and commenting the individual
> patches. We will try also to involve Antonio to speed up the work.
> We will submit the separate patches right to this bug.

Let's keep this bug for the backout only please, and have new, individual bugs for each patch with a clear explanation of what it does and a rationale for doing so. Right now the priority should be to get a 1.8 quality parity gtkmozembed back on trunk.
> doing so. Right now the priority should be to get a 1.8 quality parity

about quality:
Current gtkmozembed code is very old, and contain many things which should be  updated to GTK2 level.
During all this time I did not found any useful patches/bugs proposals to fix and improve this area.

Would be very good if anybody can help with this cleanup.
 
stop.  What chpe said!

File new bugs with alot more information than a summary as to what your patches intend to fix/do.

This bug is exclusively to deal with the back out of unreviewed changes to get the tree back to a known state.
Depends on: 408490
Depends on: 349382
Comment on attachment 293297 [details] [diff] [review]
Broken nsWindowCreator2 interface

Obsolete According to bug 408666
Attachment #293297 - Attachment is obsolete: true
Depends on: 408676
Depends on: 408681
"diff between the clean back out and and what I want to check in"

should contatain bugfix from:
https://bugzilla.mozilla.org/show_bug.cgi?id=227986
https://bugzilla.mozilla.org/attachment.cgi?id=284513

it looks like this change just landed: 	2007-10-12 16:19

I would like to commit this backout as soon as possible.

reed, it might have made a bit of sense to wait until this bug is resolve before landing more stuff, right?
Depends on: 408686
(In reply to comment #52)
> it looks like this change just landed:  2007-10-12 16:19
...
> reed, it might have made a bit of sense to wait until this bug is resolve
> before landing more stuff, right?

That change landed in October, way before this bug or bug 406724 was filed...
sorry sorry.

Let me see why I didn't mention these bugs in my above comment regarding what needs to land on top of this backout patch.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Backed up to 2006-11-01:

Checking in src/EmbedContentListener.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedContentListener.cpp,v  <--  EmbedContentListener.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in src/EmbedContentListener.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedContentListener.h,v  <--  EmbedContentListener.h
new revision: 1.8; previous revision: 1.7
done
Checking in src/EmbedEventListener.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedEventListener.cpp,v  <--  EmbedEventListener.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in src/EmbedEventListener.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedEventListener.h,v  <--  EmbedEventListener.h
new revision: 1.9; previous revision: 1.8
done
Checking in src/EmbedPrivate.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedPrivate.cpp,v  <--  EmbedPrivate.cpp
new revision: 1.87; previous revision: 1.86
done
Checking in src/EmbedPrivate.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedPrivate.h,v  <--  EmbedPrivate.h
new revision: 1.42; previous revision: 1.41
done
Checking in src/EmbedProgress.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedProgress.cpp,v  <--  EmbedProgress.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in src/EmbedProgress.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedProgress.h,v  <--  EmbedProgress.h
new revision: 1.7; previous revision: 1.6
done
Checking in src/EmbedPrompter.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedPrompter.cpp,v  <--  EmbedPrompter.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in src/EmbedPrompter.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedPrompter.h,v  <--  EmbedPrompter.h
new revision: 1.10; previous revision: 1.9
done
Checking in src/EmbedWindow.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedWindow.cpp,v  <--  EmbedWindow.cpp
new revision: 1.39; previous revision: 1.38
done
Checking in src/EmbedWindow.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedWindow.h,v  <--  EmbedWindow.h
new revision: 1.17; previous revision: 1.16
done
Checking in src/EmbedWindowCreator.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedWindowCreator.cpp,v  <--  EmbedWindowCreator.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in src/EmbedWindowCreator.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedWindowCreator.h,v  <--  EmbedWindowCreator.h
new revision: 1.6; previous revision: 1.5
done
Checking in src/GtkPromptService.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/GtkPromptService.cpp,v  <--  GtkPromptService.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in src/GtkPromptService.h;
/cvsroot/mozilla/embedding/browser/gtk/src/GtkPromptService.h,v  <--  GtkPromptService.h
new revision: 1.6; previous revision: 1.5
done
Checking in src/Makefile.in;
/cvsroot/mozilla/embedding/browser/gtk/src/Makefile.in,v  <--  Makefile.in
new revision: 1.75; previous revision: 1.74
done
Checking in src/gtkmozembed.h;
/cvsroot/mozilla/embedding/browser/gtk/src/gtkmozembed.h,v  <--  gtkmozembed.h
new revision: 1.44; previous revision: 1.43
done
Checking in src/gtkmozembed2.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/gtkmozembed2.cpp,v  <--  gtkmozembed2.cpp
new revision: 1.61; previous revision: 1.60
done
Checking in src/gtkmozembed_glue.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/gtkmozembed_glue.cpp,v  <--  gtkmozembed_glue.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in src/gtkmozembed_internal.h;
/cvsroot/mozilla/embedding/browser/gtk/src/gtkmozembed_internal.h,v  <--  gtkmozembed_internal.h
new revision: 1.12; previous revision: 1.11
done
Checking in src/gtkmozembedprivate.h;
/cvsroot/mozilla/embedding/browser/gtk/src/gtkmozembedprivate.h,v  <--  gtkmozembedprivate.h
new revision: 1.12; previous revision: 1.11
done

Depends on: 408823
many of the patches/bugs I listed in comment #30 do not apply to the backout (they would apply to code after the microb landing).

I relanded 398009, 363089, 362152, 364205.

Marking FIXED.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
I ran into a build failure in one of my trees, so I updated and tried to compile in embedding/browser/gtk. I got a build failure:

gmake[2]: *** No rule to make target `gtkmozembedmarshal.o', needed by `libgtkembedmoz.a'.  Stop.

I could resolve the failure by copying files mozilla/embedding/browser/gtk/src/gtkmozembedmarshal.[c|h] from a 1.8 tree, maybe those files should get added back to trunk?
versions 1.6 of both gtkmozembedmarshal.[c|h] should be on the trunk. 
>maybe those files should get added back to trunk?
May be better to fix bug 408686 and remove auto generated files from tree?
 
Depends on: 409415
Depends on: 409876
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: