[Tizen] Refactor XWalkLauncher for usage of RESET event. #2563
Conversation
|
Testing patch series with jakegeno/crosswalk@be59b19 as its head.
|
| 'xwalk_launcher_tizen.h', | ||
| 'xwalk_launcher.cc', | ||
| 'xwalk_launcher.h', | ||
| '../tizen/xwalk_launcher_tizen.cc', |
There was a problem hiding this comment.
the file location rules are following: if the file makes platform specific customization of a common functionality than it's located in the same folder as generic file but has "_platform" postfix. If the implements something platform-specific stuff which does not have generic implementation, it should be put to "platform" folder.
In this case we have Linux-specific shared process model implementation, so we have xwalk_launcher in "linux" folder, on Tizen however some more tricks are needed, so we have xwalk_launcher_tizen for it.
|
We've a strong cocktail with glib C and C++11 here (I know it was already there for "historical" reasons but maybe it's time to re-consider it since this PR is doing refactoring), would be nice IMO to pick one and drop the other. I personally would prefer having C++11 and chromium base classes (we may keep glib in isolated places where it's convenient, like cmd line parsing). |
|
@pozdnyakov do you suggest to move all the code that uses glib (g_main_loop, argument parsing) to xwalk_launcher_main.cc and replace GDBus with chromium's dbus implementation? |
|
@jizydorczyk considering that xwalk-launcher becomes an app's extension process (so it has code dependencies to xwalk/chromium (has to create XWalkExtensionProcessLauncher)) anyway, I would do so.. (initially the idea of xwalk-launcher was that it's just a thin client making only dbus calls, that's why its implementation had started using glib). |
|
@pozdnyakov : The split of rpm packages for reduce build time is avaible here : #2575 |
|
@pozdnyakov : I'm not opposed to this at all. |
|
So, @jizydorczyk would you swipe glib stuff out (except where it's really more convenient, like cmd line parsing)? |
|
Yes, I will. |
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
be59b19
to
ded7b36
Compare
|
Testing patch series with jakegeno/crosswalk@ded7b36 as its head.
|
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
ded7b36
to
3ad0002
Compare
|
Testing patch series with jakegeno/crosswalk@3ad0002 as its head.
|
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
3ad0002
to
8b67e6b
Compare
|
Testing patch series with jakegeno/crosswalk@8b67e6b as its head.
|
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
8b67e6b
to
bfd177d
Compare
|
Testing patch series with jakegeno/crosswalk@bfd177d as its head.
|
|
Updated commit according to your comments. |
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
bfd177d
to
31a70e6
Compare
|
Testing patch series with jakegeno/crosswalk@31a70e6 as its head.
|
|
@pozdnyakov could you take a look at this? |
|
@jizydorczyk The patch looks good in general (there are some nits but those can be fixed quite fast) and it definitely makes xwalk-launcher code look much better (Thanks! ). But now we've a new circumstance: https://lists.crosswalk-project.org/pipermail/crosswalk-dev/2014-December/002308.html (off course I did not know we'll go this way when I recommended the solution for this PR) -- xwalk-runner will just go away. So now I'm not sure how to proceed with this PR. Do you want me to proceed with the review and merge it even though it'll go away soon? |
|
I think we can proceed with this PR and later maybe you'll want to reuse some parts of this code? |
|
ok, let's proceed |
| const char kServiceName[] = "org.crosswalkproject.Runtime1"; | ||
| const char kRunningManagerIface[] = "org.crosswalkproject.Running.Manager1"; | ||
| const char kRunningAppIface[] = "org.crosswalkproject.Running.Application1"; | ||
| const dbus::ObjectPath kRunningManagerDBusPath("/running1"); |
There was a problem hiding this comment.
let's make it const char[] as well (the vars that are initialized before main() are better to be POD )
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
31a70e6
to
5c9a393
Compare
|
Testing patch series with jakegeno/crosswalk@5c9a393 as its head.
|
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
5c9a393
to
75164e0
Compare
|
Testing patch series with jakegeno/crosswalk@75164e0 as its head.
|
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
75164e0
to
b49c571
Compare
|
Testing patch series with jakegeno/crosswalk@b49c571 as its head.
|
|
@pozdnyakov could you take a look at this PR? |
| const std::string& interface_name) { | ||
| if (object_path != app_proxy_->object_path()) | ||
| return; | ||
| LOG(INFO) << "Application '" << object_path.value().c_str() |
There was a problem hiding this comment.
no need in .c_str() call, right?
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
b49c571
to
89e08bc
Compare
|
Testing patch series with jakegeno/crosswalk@89e08bc as its head.
|
| XWalkLauncherTizen::XWalkLauncherTizen(bool query_running, | ||
| base::MessageLoop* main_loop) | ||
| : main_loop_(main_loop), | ||
| XWalkLauncher(query_running, main_loop) { |
There was a problem hiding this comment.
wrong initialization order. Cannot you get main_loop from the base class?
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
89e08bc
to
3771c0b
Compare
|
Testing patch series with jakegeno/crosswalk@3771c0b as its head.
|
|
lgtm |
After this change execute xwalk-launcher on Tizen will cause initialization of app core and launch of g_main_loop. Application will be actually launched after xwalk-launcher receives RESET event from platform. BUG=XWALK-2779
jizydorczyk
force-pushed
the
launcher-reset-event
branch
from
3771c0b
to
a32332d
Compare
|
Testing patch series with jakegeno/crosswalk@a32332d as its head.
|
|
Anyone else? |
[Tizen] Refactor XWalkLauncher for usage of RESET event.
After this change execute xwalk-launcher on Tizen will cause
initialization of app core and launch of g_main_loop. Application
will be actually launched after xwalk-launcher receives RESET
event from platform.
BUG=XWALK-2779