Closed Bug 626595 Opened 9 years ago Closed 9 years ago

Make it possible to build maemo 6 platform version without meegotouch

Categories

(Firefox for Android Graveyard :: General, defect, P4, minor)

x86
Linux

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jbos, Unassigned)

References

Details

Attachments

(1 file, 10 obsolete files)

Meegotouch is basically deprecated. Nokia and Intel say both that its better to just implement meego platform functionality - but keep meegotouch out.

This patch allows us to select pretty specific those different platform features.

Meaning: ContentManager, ContentAction, and LibMeegoTouch

Those need meegotouch for them self (but not linking):
* ContentManager (and saveAs) are the Load&Save Dialogs of Meego Handset Release
* ContentAction allows to start software like eMail, Feedreader,...
Attachment #504655 - Flags: review?(doug.turner)
Severity: normal → minor
Priority: -- → P4
I do not agree that this is minor, actually it is really important considering that meego netbook - which is the only version currently available - in the public does not have meegotouch by default.
Attachment #504655 - Flags: review?(romaxa)
http://apidocs.meego.com/1.1/platform/html/index.html

See top frame text in red:

" The MeeGo platform libraries and components are not part of the MeeGo API and are discouraged for general use. "
Comment on attachment 504655 [details] [diff] [review]
make it possible to disable meegotouch lib dependency for maemo 6


>+         PKG_CHECK_MODULES(LIBCONTENTMANAGER, ContentManager qttracker,
>+                              _LIB_FOUND=1,
>+                              _LIB_FOUND=)
>+         if test "$_LIB_FOUND"; then
>+             MOZ_PLATFORM_MAEMO_LIBS="$MOZ_PLATFORM_MAEMO_LIBS $LIBCONTENTMANAGER_LIBS"
>+             MOZ_PLATFORM_MAEMO_CFLAGS="$MOZ_PLATFORM_MAEMO_CFLAGS $LIBCONTENTMANAGER_CFLAGS"
not applicable to latest m-c
Attached patch Updated Version (obsolete) — Splinter Review
Updated
Attachment #504655 - Attachment is obsolete: true
Attachment #507080 - Flags: review?(romaxa)
Attachment #504655 - Flags: review?(romaxa)
Attachment #504655 - Flags: review?(doug.turner)
You'll need a build system peer to review the configure.in parts, FWIW.
Comment on attachment 507080 [details] [diff] [review]
Updated Version


>+   if test $MOZ_PLATFORM_MAEMO = 6; then      
whitespaces

>+      MOZ_THUMB2=1
please remove it, the same as for maemo5...
Maemo6 thumb2 support is unclear yet... also Qt need to be fixed in order to compile with thumb2, see:
http://bugreports.qt.nokia.com/browse/QTBUG-16402


>+      if test -n "$MOZ_MEEGOCONTENTACTION"; then
..
>+         if test "$_LIB_FOUND"; then
...
>+         fi
>+         AC_DEFINE(MOZ_ENABLE_CONTENTMANAGER)
>+         AC_SUBST(MOZ_ENABLE_CONTENTMANAGER)
>+       fi
>+
>+      dnl ========================================================

please fix if/fi indentation

>+      MOZ_ARG_ENABLE_BOOL(meegotouch,
>+      [  --enable-meegotouch           Enable meegotouchcore support],
>+      MOZ_MEEGOTOUCHCORE=1,
>+      MOZ_MEEGOTOUCHCORE=)
        ^ indentation missing here I guess

>+           AC_MSG_ERROR([libcontentaction is required when build for Maemo])
This seems need to be fixed

>-
>+#ifdef MOZ_ENABLE_CONTENTACTION

Don't add or remove empty lines without any reason... here and in rest of the patch...
Attachment #507080 - Flags: review?(romaxa) → review-
I think we should take another approach and remove entire meegotouch code.
Attached patch This removes MeegoTouch finaly (obsolete) — Splinter Review
Remove it for good.
Attachment #507080 - Attachment is obsolete: true
Attachment #507826 - Flags: review?(romaxa)
Comment on attachment 507826 [details] [diff] [review]
This removes MeegoTouch finaly

>-#ifdef MOZ_ENABLE_MEEGOTOUCH
>-        // Create a snapshot of the gconf locale values into the
>-        // corresponding environment variables to obey system settings
>-        // as accurately as possible.
>-        CopyGConfToEnv("/meegotouch/i18n/language", "LANG");

I think this should be replaced with some Qt functionality, otherwise we going to have wrong Lang settings...
Depends on: 630182
Depends on: 630186
(In reply to comment #10)
> Comment on attachment 507826 [details] [diff] [review]
> This removes MeegoTouch finaly
> 
> >-#ifdef MOZ_ENABLE_MEEGOTOUCH
> >-        // Create a snapshot of the gconf locale values into the
> >-        // corresponding environment variables to obey system settings
> >-        // as accurately as possible.
> >-        CopyGConfToEnv("/meegotouch/i18n/language", "LANG");
> 
> I think this should be replaced with some Qt functionality, otherwise we going
> to have wrong Lang settings...

Send mail to mailinglist, does not look like there is yet such a functionality, need some more time to research. -> added bug

Also added bug for Rotation Functionality
Attached patch Added Username (obsolete) — Splinter Review
Attachment #507826 - Attachment is obsolete: true
Attachment #508409 - Flags: review?(romaxa)
Attachment #507826 - Flags: review?(romaxa)
can't compile on maemo6 with this patch:
http://pastebin.mozilla.org/1007145
Comment on attachment 508409 [details] [diff] [review]
Added Username

>+      dnl ========================================================
>+      dnl = Enable meego libcontentaction
>+      dnl ========================================================
>+      MOZ_ARG_ENABLE_BOOL(meegocontentaction,
>+      [  --enable-meegocontentaction           Enable meegocontentaction support],
>+      MOZ_MEEGOCONTENTACTION=1,
>+      MOZ_MEEGOCONTENTACTION=)
        ^ still indent missing here

>+         else
>+             MOZ_ENABLE_CONTENTACTION=0

just "MOZ_ENABLE_CONTENTACTION="

>+         fi
>+         AC_DEFINE(MOZ_ENABLE_CONTENTMANAGER)

you should not define this if LIBCONTENTACTION is not found...


check nsMIMEInfoUnix.cpp, nsOSHelperAppService.cpp, we are using MAEMO == 6 in and not CONTENT Meego stuff...

could you compile it also for Maemo6?
Attachment #508409 - Flags: review?(romaxa) → review-
the same is related to nsLocalFileUnix.cpp
Attached patch V3 remove meegotouch (obsolete) — Splinter Review
Attachment #508409 - Attachment is obsolete: true
Attachment #508713 - Flags: review?(romaxa)
Comment on attachment 508713 [details] [diff] [review]
V3 remove meegotouch

don't understand, what is going on, but previous patch was removing meegotouch,... this patch does something else...

Are you sure that patch is correct?
Attachment #508713 - Flags: review?(romaxa) → review-
Attached patch Correct Patch (obsolete) — Splinter Review
Uploaded wrong file...
Attachment #508713 - Attachment is obsolete: true
Attachment #508741 - Flags: review?(romaxa)
Comment on attachment 508741 [details] [diff] [review]
Correct Patch


>+      MOZ_ARG_ENABLE_BOOL(meegocontentaction,
>+         [  --enable-meegocontentaction           Enable meegocontentaction support],
>+         MOZ_MEEGOCONTENTACTION=1,
>+         MOZ_MEEGOCONTENTACTION=)

Again wrong... see any example above:
MOZ_ARG_ENABLE_BOOL(url-classifier,
[  --enable-url-classifier Enable url classifier module],
    MOZ_URL_CLASSIFIER=1,



>+            MOZ_ENABLE_CONTENTACTION=1

wondering what is the point of this setter without AC_SUBST and autoconf.mk.in declaration...
Also you don't do AC_DEFINE for this var, later trying to use it in cpp/h files:

>+#if defined(MOZ_ENABLE_CONTENTACTION)

See how MOZ_ENABLE_MEEGOTOUCHSHARE defined, that is mostly declared everywhere..



>+            AC_DEFINE(MOZ_ENABLE_CONTENTMANAGER)

wondering why we are using 2 different vars 
MOZ_ENABLE_CONTENTACTION and MOZ_ENABLE_CONTENTMANAGER





>diff -r 2982b2820040 uriloader/exthandler/unix/nsMIMEInfoUnix.cpp
>--- a/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp	Tue Feb 01 03:58:17 2011 -0500
>+++ b/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp	Tue Feb 01 06:01:04 2011 -0500
>@@ -48,9 +48,11 @@
> #include <QDesktopServices>

This all under MAEMO == 6, wondering why? 
should not it be under
#ifdef MOZ_WIDGET_QT?

and below where QDesktopServices used..


>-#ifdef MOZ_IPC
>-#ifndef MOZ_ENABLE_MEEGOTOUCH
>-    if (mIsTopLevel && XRE_GetProcessType() == GeckoProcessType_Default) {
>-        QWidget *widget = GetViewWidget();
>-        NS_ENSURE_TRUE(widget,);
>-        widget->resize(aWidth, aHeight);
>-    }
>-#endif
>-#endif

you should not remove this... see it is ifndef. have you tested this? you should see pretty broken resize stuff.

also just remove all defines and remove XRE_GetProcessType, because we are not creating nsWindow widget anymore in content process.

>-#ifdef MOZ_IPC
>-#ifndef MOZ_ENABLE_MEEGOTOUCH
>-    if (mIsTopLevel) {
>-        if (XRE_GetProcessType() == GeckoProcessType_Default) {
>-            QWidget *widget = GetViewWidget();
>-            NS_ENSURE_TRUE(widget,);
>-            widget->setGeometry(aX, aY, aWidth, aHeight);
>-        }
>-    }
>-#endif
>-#endif

same here


        // On e10s, we never want the child process or plugin process
        // to go fullscreen because if we do the window because visible
        // do to disabled Qt-Xembed
        if (widget &&
#ifdef MOZ_IPC
            (XRE_GetProcessType() == GeckoProcessType_Default) &&
#endif
            !widget->isVisible())

also remove ifdef MOZ_IPC from here too



also add -U 8 -p  to diff flags.
Attachment #508741 - Flags: review?(romaxa) → review-
Attached patch Remove MeeGo Touch (obsolete) — Splinter Review
Updated
Attachment #508741 - Attachment is obsolete: true
Attachment #509062 - Flags: review?(romaxa)
Comment on attachment 509062 [details] [diff] [review]
Remove MeeGo Touch

>-#ifdef MOZ_IPC
>-#ifndef MOZ_ENABLE_MEEGOTOUCH
>-    if (mIsTopLevel && XRE_GetProcessType() == GeckoProcessType_Default) {
>-        QWidget *widget = GetViewWidget();
>-        NS_ENSURE_TRUE(widget,);
>-        widget->resize(aWidth, aHeight);
>-    }
>-#endif
>-#endif
>-
>     mWidget->resize( aWidth, aHeight);
> 
>     if (aRepaint)
>@@ -2288,18 +2265,6 @@
>     mNeedsResize = PR_FALSE;
>     mNeedsMove = PR_FALSE;
> 
>-#ifdef MOZ_IPC
>-#ifndef MOZ_ENABLE_MEEGOTOUCH
>-    if (mIsTopLevel) {
>-        if (XRE_GetProcessType() == GeckoProcessType_Default) {
>-            QWidget *widget = GetViewWidget();
>-            NS_ENSURE_TRUE(widget,);
>-            widget->setGeometry(aX, aY, aWidth, aHeight);
>-        }
>-    }
>-#endif
>-#endif
>-

hey, you are still breaking NativeResize on desktop. There is ifNdef, see comment #19
oh...
Comment on attachment 509062 [details] [diff] [review]
Remove MeeGo Touch


>+ifeq ($(MOZ_WIDGET_TOOLKIT), qt)
>+#OS_INCLUDES	+=  -I/usr/include/qt4 -I/usr/include/qt4/QtGui -I/usr/include/qt4/QtCore -I/usr/include/qt4/QtNetwork -I/usr/include/qt4/QtOpenGL

remove debug stuff.


>+#ifdef MOZ_WIDGET_QT
>+        const char* lang = QLocale::languageToString( QLocale::system().language() ).toAscii();
>+#else
>+        const char* lang = getenv("LANG");
> #endif
>         // Get system configuration
move this comment above getenv



Comments about resize functionality are ignored, and MOZ_ENABLE_CONTENTACTION not declared in automake system properly
Attachment #509062 - Flags: review?(romaxa) → review-
Attached patch Next comments fixed (obsolete) — Splinter Review
Yes i fixed next comments, i misread the #19....
Attachment #509062 - Attachment is obsolete: true
Attachment #509070 - Flags: review?(romaxa)
Attachment #509070 - Flags: review?(romaxa) → review-
Attached patch That is it. V1 (obsolete) — Splinter Review
Attachment #509070 - Attachment is obsolete: true
Attachment #509075 - Flags: review?(romaxa)
Attached patch That is it. V2 (obsolete) — Splinter Review
Attachment #509075 - Attachment is obsolete: true
Attachment #509077 - Flags: review?(romaxa)
Attachment #509075 - Flags: review?(romaxa)
Attached patch That is it. V3Splinter Review
Removed last bit, pushed to try:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=196f464354d9
Attachment #509077 - Attachment is obsolete: true
Attachment #509080 - Flags: review?(romaxa)
Attachment #509077 - Flags: review?(romaxa)
Comment on attachment 509080 [details] [diff] [review]
That is it. V3

ok, this looks good to me
Attachment #509080 - Flags: review?(romaxa) → review+
Comment on attachment 509080 [details] [diff] [review]
That is it. V3

Doug this is npotb... try server green.
Attachment #509080 - Flags: approval2.0?
This patch touches a whole bunch of code, including configure, so claiming it's NPOTB is kind of...weird.
Comment on attachment 509080 [details] [diff] [review]
That is it. V3

Ted would you like double check this patch?
Attachment #509080 - Flags: review?(ted.mielczarek)
Comment on attachment 509080 [details] [diff] [review]
That is it. V3

Clearing approvals until reviews are done, but I'd really like to understand the reward here, since the build-config changes don't look completely trivial.
Attachment #509080 - Flags: approval2.0?
Attachment #509080 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 509080 [details] [diff] [review]
That is it. V3

ok, all reviews are done, requesting approval again
Attachment #509080 - Flags: approval2.0?
> Clearing approvals until reviews are done, but I'd really like to understand
> the reward here, since the build-config changes don't look completely trivial.

We don't need meegotouch in order to have FF working on Maemo6, it is allowing to run Pure Qt applications now, so I would like to cleanup Qt port, make it clear, and common for Qt toolkit.

So this patch is killing meegotouch library and making FF depends only from Maemo6/Meego related components.
Attachment #509080 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/7961753fcc67
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Can we verify this somehow? If not can somebody verify it please?
You need to log in before you can comment on or make changes to this bug.