[CEF 2623] Upgrade to latest CEF #544

Merged
merged 9 commits into from May 13, 2016

Projects

None yet

7 participants

@nethip
Contributor
nethip commented Dec 22, 2015 edited

This PR updates CEF version inside Brackets to 2357 2623. I could not upgrade to latest version as the latest MAC binaries are not yet posted to cebuilds.com. I will update this PR with the latest verison once they are built and available at cefbuilds.com.

One important thing to note is that, starting from 2357, CEF is going to support only 64 bit version of MAC. If you are building out of this branch, 64 bit binary is going to be generated on MAC.

@nethip
Contributor
nethip commented Dec 22, 2015

@MarcelGerber We might want to revert the mouse scroll hack once this PR is merged with master.

@ingorichter
Contributor

I think we need to make some changes to the xcode project. I'm currently on 10.11.2 with Xcode 7.2 and compilation failed, because C++ Exceptions are currently not explicitly enabled for the project. I've changed it locally and now it compiles and brackets is able to start. I can't verify the changes on older OSX versions right now.

@nethip
Contributor
nethip commented Dec 23, 2015

@ingorichter Thanks for trying out this branch and for the workaround as well. I tested this with XCode 6. It seems to pass fine.

Let me setup XCode 7.0 and try building this branch.

@ficristo
Member

I tried this PR on Windows and cannot find the Brackets executable in Release/Debug folders, only the wow_helper.exe.
Is this because I used VS2015?
I simply run grunt, I need additional commands?

PS: Will this works on Linux? I think it would be nice to update it...

@ficristo
Member

The version 2526 of cef should support VS2015. I'll wait for it.

@nethip
Contributor
nethip commented Dec 24, 2015

@ficristo That is right! We have not ported our build scripts to be able to build on VS2015. I will update the scripts and will let you know once it is ready. Thanks for building and posting the results.

@petetnt petetnt referenced this pull request in adobe/brackets Jan 6, 2016
Closed

Brackets susceptible for Chromium bug #533361 #11727

@ficristo
Member
ficristo commented Jan 9, 2016

If I want to test the new version of CEF, but that version it should be uploaded to a private server first. Right?
@nethip How I can point to a public source? Or can you upload the new version?

@nethip
Contributor
nethip commented Jan 11, 2016

@ficristo

Refer to https://github.com/adobe/brackets-shell/blob/master/Gruntfile.js#L226. This is where we set the download location. You can change this to any other domain (maybe dropbox). But be sure to name the binary in this format.

cef_binary_3.2171.1902_macosx32.zip

Notice the "3.2171.1902" part. This version number would be got from https://github.com/adobe/brackets-shell/blob/master/Gruntfile.js#L227. So you might want to change the version number as well. At the end, both the URL location and the version number are combined along with _<platform32/64> to form the final download URL.

But I am afriad just this is not enough. The app-shell code needs to be changed too. I would suggest you could branch out of prashant/cef-upgrade-latest and do your changes on top of it.

Oh, by the way you could also look at this PR #440 which adds support for VS 2013, which I think is a mandate, if we want to shift to the latest CEF.

Let me know how it goes.

@ficristo
Member

@nethip Thanks for the answer. I'll see if I have the time to look at it.

@ficristo
Member

It seems easy enough to add support to vs2015: I've updated the gyp in tree to latest master plus I've removed a couple of warning.
I've tried to update to CEF 2526 but I got lost trying to update follow the new structure...
Just to realize that CEF 2357 has changed it folders structure too.
I don't see any changes here: any reason to not reflect that structure here?
Will everything work without any change?

@nethip
Contributor
nethip commented Jan 25, 2016

@ficristo Could you mention which folders have changed? I have VS 2010 and the windows build seems to go fine. I will try updating to 2526 as well (now that the builds are available at https://cefbuilds.com).

@ficristo
Member

More or less like this:

In cef 3.2171

  • cef_binary
    • cef_client/
      • res/
      • cefclient_win.cpp

In cef 3.2526

  • cef_binary
    • cef_client/
      • browser/
      • common/
      • renderer/
      • resources/
        • win/
      • cefclient_win.cc
@nethip
Contributor
nethip commented on 25ddc3a Apr 19, 2016 edited

CEF upgrade related changes. Fixed the following issues

  1. developer tools URL is now changed to "chrome-devtools://devtools/inspector.html" because of which shortcuts were not working as we are explicitly checking for this URL before forwarding shortcuts to Brackets.
  2. Enabled exceptions in XCode as the build was failing on latest versions of XCode
@nethip
Contributor
nethip commented May 2, 2016

@ingorichter @vdhawal I have fixed all the issues and now the PR is ready for review. We want to merge this for Brackets 1.7. Will you guys be able to review this PR?

@ficristo
Member
ficristo commented May 2, 2016

Hi @nethip I have a couple of questions:

  • so can you confirm that aren't necessary any changes to the structure of the folders / files?
  • were it too much work to update to a newer release of CEF?
  • if OSX users were able to install Brackets on a 32bit platform what will happen to them? (I tried to see if the architecture were available in the HealtData Report but seems not)
    Thank you.
@vdhawal
vdhawal commented May 2, 2016

Changes look good to me. I did build on 10.11.4 with Xcode 7.3 and the app launched seamlessly. OnBeforePopup is also getting invoked when a popup window is launched. 👍

@nethip
Contributor
nethip commented May 2, 2016

Thanks @vdhawal for reviewing this trying to build it .

And thanks to @ingorichter for pointing out the issue with exceptions.

@ficristo

so can you confirm that aren't necessary any changes to the structure of the folders / files?

I think the grunt setup is going to take of creating necessary structure. We did a good job at segregating CEF specific files with our appshell specific files. You could have a look at this.

were it too much work to update to a newer release of CEF?

That is a good point you brought up. I will give it a spin.

if OSX users were able to install Brackets on a 32bit platform what will happen to them? (I tried to see if >the architecture were available in the HealtData Report but seems not) Thank you.

Unfortunately, CEF stopped supporting 32bit binaries for Mac OS. Hence have to switch to 64 bit only for MAC, from 2357. One solution I can think of is,the user can build Brackets shell from a prior commits, once this PR is merged. We will be retaining the CEF 2171 binaries on our S3 server.

@ficristo
Member
ficristo commented May 2, 2016

That is a good point you brought up. I will give it a spin.

I was only interested for the vs2015 compatibility. I think this will already bring some fixes (I think at you scroll issue!).
I can wait the next release.

For the platform thing, I kinda hope that the upgrade will fail gracefully, maybe with some sort of message. But without numbers is rough to know if is an important issue.

@nethip Thanks fo the answers!

@nethip
Contributor
nethip commented May 3, 2016 edited

@ficristo I have created a new branch c32ee49 for upgrading to CEF 2623. Would you mind building this branch on Windows? I have verified this on MAC.

@ficristo
Member
ficristo commented May 3, 2016

I've gave a try.
I've merged #546 and #549 to solve some issues.
But I still have some errors, all of them are LNK2019 and LNK2001.
The following are a couple of examples:
appshell_extensions.obj : error LNK2019: unresolved external symbol __imp__GetParent@4 referenced in function "void * __cdecl getMenuParent(class CefRefPtr<class CefBrowser>)" (?getMenuParent@@YAPAXV?$CefRefPtr@VCefBrowser@@@@@Z)
appshell_extensions_win.obj : error LNK2001: unresolved external symbol __imp__GetParent@4

@nethip nethip changed the title from [Review only] CEF Upgrade to CEF Upgrade to 2623 May 11, 2016
@nethip nethip referenced this pull request in adobe/brackets May 11, 2016
Merged

[CEF 2623] Upgrade to latest CEF #12415

@nethip nethip changed the title from CEF Upgrade to 2623 to [CEF 2623] Upgrade to latest CEF May 11, 2016
@nethip
Contributor
nethip commented May 11, 2016

This PR is now ready for review. This PR has dependency on Brackets PR as well.

adobe/brackets#12415

@ingorichter @vdhawal Will you guys be able to review this PR?

@nethip nethip referenced this pull request in adobe/brackets May 11, 2016
Closed

Scrolling sometimes reverts to being too fast #11083

@nethip
Contributor
nethip commented May 11, 2016

@ficristo Yeah this PR was not meant to be mixed with VS2015 changes.

Is it OK if we can work on your VS2015 related PRs post 1.7? One of the major hurdles we are facing is to do with upgrading our build system, as the system is currently equipped to churn out builds using VS2010.

@ficristo
Member

@nethip yes, no problem 😄

@nethip
Contributor
nethip commented May 12, 2016

Tagging @abhijitapte

@MarcelGerber
Member

Will CEF 2623 come to Linux as well? Would be very nice to finally have the Linux version updated!

@nethip
Contributor
nethip commented May 12, 2016

@MarcelGerber unfortunately not. :(. I guess Linux has to be worked ground up since lot has changed in cef-client.

@abhijitapte abhijitapte commented on the diff May 12, 2016
appshell/cefclient_mac.mm
@@ -650,7 +662,6 @@ - (void)createApp:(id)object {
content_rect = [mainWnd contentRectForFrameRect:[mainWnd frame]];
// Configure the rest of the window
- [mainWnd setTitle:WINDOW_TITLE];
@abhijitapte
abhijitapte May 12, 2016

@nethip where is this being set now?

@nethip
nethip May 12, 2016 edited Contributor

Drawing of window title is going to be taken care of CustomToolBarView. Actually Apple officially dropped support for installing views in NSThemeFrame and CustomToolBarView is getting added as a subview inside NSThemFrame. I think going forward we need to investigate and figure out how to officially get a dark toolbar. There is a [mainWnd setAppearance: NSAppearanceNameVibrantLight] but that is going to give UI toolbar a more darker look. But this is the actual way to do it. Also there is a way to generate custom appearances and install it via setAppearance. That is something I will investigate in the future.

For now to answer your question, OS is drawing the title along with CustomTitleBarView and hence need to suppress one of them. We require CustomTitleBarView for dark toolbar.

@nethip
Contributor
nethip commented May 12, 2016

@MarcelGerber Will you be able to build out of this branch and pull adobe/brackets#12415 to check if the fast scrolling issue is resolved?

@MarcelGerber
Member

I have already done so and I can say, the scroll issue is finally gone! Good job on your side!

@ingorichter
Contributor

@MarcelGerber did you test on Windows or OSX?

@nethip
Contributor
nethip commented May 12, 2016

@MarcelGerber Oh that is a relief. And thanks to you for fixing the actual problem via adobe/brackets#12415. Really appreciate it!

@ingorichter I guess he would have built this on Windows itself.

@abhijitapte abhijitapte commented on the diff May 12, 2016
appshell/cefclient_mac.mm
@@ -334,8 +334,15 @@ - (void)windowWillEnterFullScreen:(NSNotification *)notification {
}
if (customTitlebar) {
[customTitlebar setHidden:YES];
+ NSWindow *window = [notification object];
+
+ // Since we have nuked the title, we will have
+ // to set the string back as we are hiding the
+ // custom title bar.
+ [window setTitle:[customTitlebar titleString]];
@abhijitapte
abhijitapte May 12, 2016 edited

@nethip I did run into a console message viz., the following. Was that happening earlier also?

2016-05-12 22:52:11.306 Brackets[2370:84493] NSWindow warning: adding an unknown subview: <CustomTitlebarView: 0x608000131440>. Break on NSLog to debug.
2016-05-12 22:52:11.351 Brackets[2370:84493] Call stack:
(
0 AppKit 0x00007fff851c88bb -[NSThemeFrame addSubview:] + 107
1 AppKit 0x00007fff851c8600 -[NSView addSubview:positioned:relativeTo:] + 211
2 Brackets 0x00000001000588bb -[ClientWindowDelegate windowDidBecomeKey:] + 603
3 CoreFoundation 0x00007fff9768ebbc CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER + 12
4 CoreFoundation 0x00007fff9768eb4f ___CFXRegistrationPost_block_invoke + 63
5 CoreFoundation 0x00007fff9768eac7 _CFXRegistrationPost + 407
6 CoreFoundation 0x00007fff9768e832 ___CFXNotificationPost_block_invoke + 50
7 CoreFoundation 0x00007fff9764b5e2 -[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1922
8 CoreFoundation 0x00007fff9764a835 _CFXNotificationPost + 693
9 Foundation 0x00007fff88399fda -[NSNotificationCenter postNotificationName:object:userInfo:] + 66
10 AppKit 0x00007fff852e6048 -[NSWindow becomeKeyWindow] + 1425
11 AppKit 0x00007fff852e5a75 _NXSendWindowNotification + 252
12 AppKit 0x00007fff852e5378 -[NSWindow _changeKeyAndMainLimitedOK:] + 868
13 AppKit 0x00007fff853af173 -[NSWindow _makeKeyRegardlessOfVisibility] + 98
14 AppKit 0x00007fff852e8429 NSPerformVisuallyAtomicChange + 147
15 AppKit 0x00007fff853af0af -[NSWindow makeKeyAndOrderFront:] + 79
16 Brackets 0x0000000100059a5f -[ClientAppDelegate createApp:] + 2575
17 Foundation 0x00007fff883c7f4e -[NSObject(NSThreadPerformAdditions) performSelector:onThread:withObject:waitUntilDone:modes:] + 1115
18 Foundation 0x00007fff883c7a75 -[NSObject(NSThreadPerformAdditions) performSelectorOnMainThread:withObject:waitUntilDone:] + 131
19 Brackets 0x000000010005b276 main + 3030
20 Brackets 0x0000000100003d54 start + 52
21 ??? 0x0000000000000003 0x0 + 3
)
Got xpc error message: Connection interrupted
2016-05-12 22:54:16.043 Brackets[2370:85805] Communications error: <OS_xpc_error: <error: 0x7fff76257b90> { count = 1, contents =
"XPCErrorDescription" => <string: 0x7fff76257f40> { length = 22, contents = "Connection interrupted" }
}>

@nethip
nethip May 12, 2016 Contributor

This was happening earlier with 10.11 sdk. Please refer to my comment https://github.com/adobe/brackets-shell/pull/544/files/d3cd41c8fb964cd5634b3babdeec9dd25e681861#r63060349

for more explanation on this,

@abhijitapte
abhijitapte May 12, 2016 edited

@nethip
There seems to be an issue with custom title bar view in case "Debug > New Brackets Window" is selected and the same is maximized and/or restored. See screenshot below -
screen shot 2016-05-12 at 11 04 14 pm

@nethip
nethip May 12, 2016 Contributor

Which SDK are you on? I have built this using 10.11 sdk and it seems to work fine.
screen shot 2016-05-12 at 11 22 00 pm

@nethip
nethip May 12, 2016 Contributor

Oh I see that you are using OS dark theme. Let me give that a try.

@nethip
nethip May 12, 2016 Contributor

Nope! working fine on my system. Are you using 10.10 sdk to build this?

@nethip
nethip May 12, 2016 edited Contributor

Also I verified this on our build system which is using 10.9 sdk.

@abhijitapte
abhijitapte May 12, 2016 edited

I'm on the latest SDK 10.11.
image

@abhijitapte
abhijitapte May 12, 2016

And on OSX 10.11.4
image

@abhijitapte
abhijitapte May 12, 2016

@nethip
I also see that

OS X Deployment Target is 10.6.

Shouldn't that be updated? Changing that causes appshell build to fail. The errors and deprecations should be fixed.
Also npm install for Brackets throws these deprecations -

npm WARN deprecated graceful-fs@1.2.3: graceful-fs v3.0.0 and before will fail on node releases >= v7.0. Please update to graceful-fs@^4.0.0 as soon as possible. Use 'npm ls graceful-fs' to find it in the tree.
npm WARN deprecated npmconf@2.1.1: this package has been reintegrated into npm and is now out of date with respect to npm
npm WARN deprecated graceful-fs@1.1.14: graceful-fs v3.0.0 and before will fail on node releases >= v7.0. Please update to graceful-fs@^4.0.0 as soon as possible. Use 'npm ls graceful-fs' to find it in the tree.
npm WARN deprecated graceful-fs@3.0.8: graceful-fs v3.0.0 and before will fail on node releases >= v7.0. Please update to graceful-fs@^4.0.0 as soon as possible. Use 'npm ls graceful-fs' to find it in the tree.
npm WARN deprecated lodash@1.0.2: lodash@<3.0.0 is no longer maintained. Upgrade to lodash@^4.0.0.

@nethip
nethip May 12, 2016 Contributor

This is weird. can you bring your laptop to the office tomorrow?

@nethip
nethip May 12, 2016 Contributor

About the deprecation and 10.6 target, I think that is something we can work on post 1.7. Agree with you updating the target as well as the removing the npm deprecation warnings.

@abhijitapte abhijitapte commented on the diff May 12, 2016
appshell/client_handler_mac.mm
@@ -56,9 +56,12 @@
NSWindow* window = [view window];
std::string titleStr(title);
NSString* str = [NSString stringWithUTF8String:titleStr.c_str()];
- [window setTitle:str];
@abhijitapte
abhijitapte May 12, 2016

@nethip
Title disappeared while working with two windows (via Debug > New Brackets Window).
Both windows had two separate projects open. Getting Started folder in one window and some other folder in the other window. Entered full screen mode in both windows. This is what happened thereafter.
screen shot 2016-05-12 at 11 19 29 pm

@abhijitapte
abhijitapte May 12, 2016

message in XCode console -
2016-05-12 23:14:31.524 Brackets Helper[2782:100768] Couldn't set selectedTextBackgroundColor from default ()

@nethip
nethip May 12, 2016 Contributor

Are you able to repro this scenario? I will check with you tomorrow in the office.

@abhijitapte
abhijitapte May 12, 2016

This is on my personal laptop.

@abhijitapte
abhijitapte May 12, 2016 edited

Wasn't able to reproduce the error message :(

But reproducing the title disappearance is very straight forward.

@nethip
nethip May 13, 2016 edited Contributor

I think you missed telling me you were in full screen mode 😄 But that is fine as I figured that out from the video that you had posted.

@abhijitapte abhijitapte commented on the diff May 12, 2016
Gruntfile.js
@@ -224,7 +224,7 @@ module.exports = function (grunt) {
},
"cef": {
"url" : "http://s3.amazonaws.com/files.brackets.io/cef",
- "version" : "3.2171.1902"
+ "version" : "3.2623.1397"
@abhijitapte
abhijitapte May 12, 2016

@nethip cefbuilds.com has a link to 3.2623.1399. why is this difference? this has a mention to 3.2623.1397.

@nethip
nethip May 12, 2016 Contributor

I think cefbuilds.com is still churning out builds out of 2623 branch and the branch would have moved by some commits by now. The build date is showing 2016-05-11 so this must have got built yesterday. That reminds me if I have to update 2623 to the latest commit. Let me go check CEF branch to see if anything important has been merged.

@nethip
nethip May 12, 2016 Contributor

I have verified all the recent commits. Here is the list

b90a3be (Call RenderProcessHost::Send from correct thread (issue #1881))
5b27df3
64e2fe1
93f8103

Out of these, b90a3be seems to be whole lot changes relating to using render process id for identification. I kind of backtracked and looks like these are OSR related bugs. So I am not sure if we want to refresh the binaries.

@nethip
Contributor
nethip commented May 12, 2016

Thanks @abhijitapte in taking some time to review this PR.

@abhijitapte

@nethip please have a look at the video capture. This has the steps for reproducing blank title. I am using the brackets dist folder built using grunt buildafter syncing from origin master by the way.
BracketsShellCapture.zip

@nethip nethip Fixing the title drawing issue for popup window. This was fixed for t…
…he main window and the fix had to be migrated to popup window as well
25a643c
@nethip
Contributor
nethip commented May 13, 2016

Great catch @abhijitapte . I fixed the issue and the change should be reflecting in this PR. Thanks for testing this PR so extensively. 👍

@abhijitapte

looks good to me @nethip

@swmitra
Contributor
swmitra commented May 13, 2016

LGTM 👍.
Kudos to @nethip in pulling this out and resolving the long pending scrolling issue finally. Great job by @abhijitapte and @MarcelGerber for helping in critical review and testing.
Merging...

@swmitra swmitra merged commit 0894803 into master May 13, 2016
@swmitra swmitra deleted the prashant/cef-upgrade-latest branch May 13, 2016
@nethip
Contributor
nethip commented May 15, 2016

Thanks to @ingorichter @ficristo @vdhawal @abhijitapte who helped in shaping this PR really good. Really appreciate your efforts!

@abhijitapte

@nethip Linux build doesn't work. The CEF binary 2623 zip folder itself is not accessible.
cef_on_linux

@nethip
Contributor
nethip commented May 18, 2016

@abhijitApte unfortunately app shell on Linux is going to get built from an older branch as it requires some more work. I could upload the binaries to S3. Let me know if you want to have a look at it (which would be great :))

@nethip nethip restored the prashant/cef-upgrade-latest branch May 24, 2016
@MarcelGerber MarcelGerber deleted the prashant/cef-upgrade-latest branch Aug 18, 2016
@MarcelGerber MarcelGerber restored the prashant/cef-upgrade-latest branch Aug 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment