626595 - Make it possible to build maemo 6 platform version without meegotouch

Status: RESOLVED FIXED

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

Description

[Jeremias Bosch (:jbos)]

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,...

Comment 1

[Jeremias Bosch (:jbos)]

Attachment: make it possible to disable meegotouch lib dependency for maemo 6

Comment 2

[Jeremias Bosch (:jbos)]

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.

Comment 3

[Jeremias Bosch (:jbos)]

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 4

[Oleg Romashin (:romaxa)]

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

Comment 5

[Jeremias Bosch (:jbos)]

Attachment: Updated Version

Updated

Comment 6

[(not currently active) Ted Mielczarek]

You'll need a build system peer to review the configure.in parts, FWIW.

Comment 7

[Oleg Romashin (:romaxa)]

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...

Comment 8

[Jeremias Bosch (:jbos)]

I think we should take another approach and remove entire meegotouch code.

Comment 9

[Jeremias Bosch (:jbos)]

Attachment: This removes MeegoTouch finaly

Remove it for good.

Comment 10

[Oleg Romashin (: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...

Comment 11

[Jeremias Bosch (:jbos)]

(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

Comment 12

[Jeremias Bosch (:jbos)]

Attachment: Added Username

Comment 13

[Oleg Romashin (:romaxa)]

can't compile on maemo6 with this patch:
http://pastebin.mozilla.org/1007145

Comment 14

[Oleg Romashin (:romaxa)]

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?

Comment 15

[Oleg Romashin (:romaxa)]

the same is related to nsLocalFileUnix.cpp

Comment 16

[Jeremias Bosch (:jbos)]

Attachment: V3 remove meegotouch

Comment 17

[Oleg Romashin (: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?

Comment 18

[Jeremias Bosch (:jbos)]

Attachment: Correct Patch

Uploaded wrong file...

Comment 19

[Oleg Romashin (: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.

Comment 20

[Jeremias Bosch (:jbos)]

Attachment: Remove MeeGo Touch

Updated

Comment 21

[Tatiana Meshkova (:tatiana)]

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

Comment 22

[Jeremias Bosch (:jbos)]

oh...

Comment 23

[Oleg Romashin (:romaxa)]

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

Comment 24

[Jeremias Bosch (:jbos)]

Attachment: Next comments fixed

Yes i fixed next comments, i misread the #19....

Comment 25

[Jeremias Bosch (:jbos)]

Attachment: That is it. V1

Comment 26

[Jeremias Bosch (:jbos)]

Attachment: That is it. V2

Comment 27

[Jeremias Bosch (:jbos)]

Attachment: That is it. V3

Removed last bit, pushed to try:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=196f464354d9

Comment 28

[Oleg Romashin (:romaxa)]

Comment on attachment 509080 [details] [diff] [review]
That is it. V3

ok, this looks good to me

Comment 29

[Oleg Romashin (:romaxa)]

Comment on attachment 509080 [details] [diff] [review]
That is it. V3

Doug this is npotb... try server green.

Comment 30

[(not currently active) Ted Mielczarek]

This patch touches a whole bunch of code, including configure, so claiming it's NPOTB is kind of...weird.

Comment 31

[Oleg Romashin (:romaxa)]

Comment on attachment 509080 [details] [diff] [review]
That is it. V3

Ted would you like double check this patch?

Comment 32

[Benjamin Smedberg]

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.

Comment 33

[Oleg Romashin (:romaxa)]

Comment on attachment 509080 [details] [diff] [review]
That is it. V3

ok, all reviews are done, requesting approval again

Comment 34

[Oleg Romashin (:romaxa)]

> 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.

Comment 35

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/7961753fcc67

Comment 36

[Andreea Pod]

Can we verify this somehow? If not can somebody verify it please?