-
Notifications
You must be signed in to change notification settings - Fork 614
Conversation
…ed grunt updation
Just to verify if I understood correctly: are you going to update macOS and Windows with cef 2704 ? |
@ficristo What I meant was that when we upgrade we will upgrade CEF on all the platforms to the latest version but for now only Brackets on Linux would be on 2704. Also I did not upgrade Win and Mac CEF versions to 2704 as I did not want Brackets to be effected on other platforms so kept the changes as minimal as possible and restricted only to Linux platform. I tried updating to the latest CEF version, but was not successful building Linux atleast. The build mechanism has changed for building CefClient in the most recent versions of CEF(they moved away from gyp). So I guess we need to be update our build mechanism to that of the latest CefClient. We could do this later in a different PR. |
💯 👍 🥇 |
@ficristo Will you be able to review this PR and let me know if you have any comments? |
Sorry, I cannot really review C++ code. @pratts you seemed interested in the past in Linux development of Brackets. Do you want to give a try to this PR ? |
In my light testing I can confirm the crash on quit is fixed. I don't know how much it is important but when you scroll an element (the editor area, the file tree or the extension manager) I see the following line printed in the terminal: |
Thank you for this update @nethip. I really appreciate the effort you put in 💯 🥇 !
Running brackets for the first time from terminal gives the following error: Aborted I changed the file permission using chmod 4755 /opt/brackets/chrome-sandbox One more problem, I cannot see the menu bar :File,Edit,View, etc. Is it intentional ? |
I can close the window using Ctrl+Q. It gives an error and window closes but I see an exception on console and brackets process does not terminate.
|
Thanks @ficristo @pratts for trying this branch out and testing! Appreciate your efforts 👍 @ficristo I will dig deeper into it and see which module is reporting this error. @pratts Thanks for your efforts. I will look into the issues you have mentioned above. I have done all the development on Ubuntu and there I have not seen this error. I will try to introduce the
And about the Menus not being shown, are they currently shown if you build brackets-shell from linux-1547? Is this happening with this branch alone? |
#endif | ||
|
||
#ifndef OS_LINUX | ||
browser->GetHost()->ShowDevTools(wi, browser->GetHost()->GetClient(), settings, CefPoint()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement seems to be executed in case of MAC, so can we have a separate #elif
case in line 406 only.
Something like #ifdef OS_MAC
, it would make it more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement needs to be executed for MAC as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will see if I can simplify this for better readability.
} | ||
else { | ||
} | ||
else if (message_name == "ReadDirWithStats") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please highlight the place where this message is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here is the thing. In my testing what I figured out was readDir
leads to lot of shell calls. Here is what happens for readDir
. First we get plain contents of a directory and for every file entry, we then make a stat
call to shell. With our Brackets repository open, this led to close to 33,000 shell calls. So if all we want is to get directory contents and file stats we could very well do that in a single shell call. Using this has reduced the no of shell calls to 5000 for readDir, for Brackets repo. So I guess this should pretty much be made mainstream. For now, I have just kept it added and in another PR, I would replace "ReadDir" shell call with "ReadDirWithStats". Does it make sense to remove this here and raise another PR for this?
@@ -72,24 +93,9 @@ | |||
#define KEY_BACK_QUOTE "`" | |||
#define KEY_COMMA "," | |||
|
|||
typedef char s8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used anywhere. I guess we can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -3,15 +3,16 @@ | |||
// can be found in the LICENSE file. | |||
|
|||
#include "appshell/common/client_app.h" | |||
#include "appshell/common/scheme_test_common.h" | |||
//#include "appshell/common/scheme_test_common.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
|
||
namespace client { | ||
|
||
// static | ||
void ClientApp::RegisterCustomSchemes( | ||
CefRefPtr<CefSchemeRegistrar> registrar, | ||
std::vector<CefString>& cookiable_schemes) { | ||
scheme_test::RegisterCustomSchemes(registrar, cookiable_schemes); | ||
// Brackets specific change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
@@ -4,14 +4,16 @@ | |||
|
|||
#include "appshell/renderer/client_app_renderer.h" | |||
#include "appshell/renderer/client_renderer.h" | |||
#include "appshell/renderer/performance_test.h" | |||
//#include "appshell/renderer/performance_test.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
|
||
namespace client { | ||
|
||
// static | ||
void ClientAppRenderer::CreateDelegates(DelegateSet& delegates) { | ||
renderer::CreateDelegates(delegates); | ||
performance_test::CreateDelegates(delegates); | ||
// Brackets specific change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
Hi @nethip I had to install libgcrypt11 library and saw the following error in terminal: [0807/190453:ERROR:connection.cc(799)] sqlite error 11, errno 0: malformed database schema (is_transient) - near "where": syntax error |
@nethip did you ask to test on Ubuntu? Which version of Ubuntu? 16/04, 16/10, 17/04? Older versions or other Linux distros? |
I am in the process of creating a wiki for building brackets-shell on linux with the upgraded CEF and I will update the page with all possible errors that one might run into while building and the possible solutions for the same. |
brackets 1.11, gcrypt.so file no longer needed, /usr included in source, remove seperate /usr packaging section fails to run, no errors, possible curl3 issue scala 2.12.3.1 plasma-integration added new dep, prep next build
CEF verison upgrade on Linux to 2704.
Note: CEF version on Windows and Mac will still remain to be 2623. At a later time we will upgrade CEF on all platforms to one version.
The main reason why we could not update CEF on linux to 2171/2623 was because of app quit blockage. Upon App quit, the process was completely blocked as the app was waiting for one of the threads to join. After lot of debugging and analysis, I found out that this was because of Linux sandboxing,
gtk_init()
needs to be called afterCefInitialize()
.The other major change is the removal of
libcrypto
dependency, users now don't have to install this package for Brackets to run on Linux.Here are other noteworthy changes.
cefclient_gtk.cc
will be the entry point for the app on LinuxCefClient
test app.client::ClientHandler
instead ofappshell/ClientHandler
. The difference is that the former will now derive fromappshell/ClientHandler
.RootWindowGtk
close.client::RootWindowGtk
is part ofCefClient
. All issues relating to app quit/ closing individual windows are now fixed.(Yay!!!).g_handler
is not going to be used anymore as a new client handler is created for every window creation byRootWindow
itself. This improves our multiple windows workflow. AlsoDebug->New Window
, will now have it's own menu bar (finally!). So each of the windows now have their own life time.RootWindowGtk
.RootWindowGtk
will further deliver specific command to the respective browser.appshell_extensions_gtk.cpp
had an outdated version onmaster
as it was always updated in thelinux-1547
branch alone. So migrated all changes, relating toappshell_extensions_gtk.cpp
present inlinux-1547
branch tomaster
.#ifdef OS_LINUX
macrostartNodeProcess()
at the start itself. If the thread, that spawns the node process, is called afterCefInitialize()
, it blocks the app quit.cef_paths.gypi
inside the redistributable for Linux 32 bit, zipped and uploaded that to S3.control
file to remove thelibcrypto
dependencyNote to the committers: Whoever is merging, feel free to squash all the commits and merge.
Tasks
@ficristo, @ingorichter, @eyelash, @zaggino It would be great if you can review this PR.