Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frame: false not working on Win2k8 Server VM (Chrome provided frame) #799

Closed
frankhale opened this issue Nov 7, 2014 · 19 comments
Closed

Comments

@frankhale
Copy link
Contributor

I have a Win2k8 VM and Atom-Shell doesn't use native frames for windows but instead uses the Chrome provided blue frame. That is all fine and dandy and doesn't bother me but if I set the frame to false the Chrome provided frame still shows. I've modified the default_app to set frame: false. (see screenshot below)

Atom-Shell 0.19.1

frame_false_but_has_chrome_provided_frame

@frankhale frankhale changed the title frame: false not working on Win2k8 Server VM frame: false not working on Win2k8 Server VM (Chrome provided frame) Nov 7, 2014
@frankhale
Copy link
Contributor Author

Okay so I just played around with this on Windows 8.1 and I put the Atom-Shell exe in compatibility mode for XP Service Pack 3 and when I run a frameless window it shows that blue Chrome provided frame even though frame: false is set.

@thedaniel
Copy link
Contributor

Isn't Server 2k8 basically Vista? Atom officially supports Win 7 +

@frankhale
Copy link
Contributor Author

I'm not sure. Atom-Shell seems to work fine on Win2k8. That frame though is coming from Chrome right? I've seen that same frame on Linux on the debug tools window. It's a non-native frame as far as I can tell.

@thedaniel
Copy link
Contributor

Whoops, sorry. I was looking at a page full of notifications and thought this was on the atom/atom repo, not atom-shell. Ignore me!

@frankhale
Copy link
Contributor Author

Here is the requisite issue for the Linux dev tools that I posted a while back. It's been closed and I'm going to go dig into the code and see how it was fixed.

#646

bdbbf662-3891-11e4-852a-4aecb7d46406

@frankhale
Copy link
Contributor Author

No worries @thedaniel =)

@anaisbetts
Copy link
Contributor

I suspect that this bug would also repro on other modern Server SKUs of Windows too (i.e. Server 2012 or 2012 R2)

@frankhale
Copy link
Contributor Author

In native_window_views.cc the remove_standard_frame is being set based on if it's frameless or not. This seems to have no effect and Chrome uses it's window frame to decorate the window. Apparently there is a TYPE_WINDOW_FRAMELESS option for the InitParams. I've tried this and it indeed makes the Chrome blue frame go away and the window is frameless but it has some nasty side effects of crashing Atom.

reference: http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/widget/widget.h

struct VIEWS_EXPORT InitParams {
enum Type {
  TYPE_WINDOW,      // A decorated Window, like a frame window.
                    // Widgets of TYPE_WINDOW will have a NonClientView.
  TYPE_PANEL,       // Always on top window managed by PanelManager.
                    // Widgets of TYPE_PANEL will have a NonClientView.
  TYPE_WINDOW_FRAMELESS,
                    // An undecorated Window.
  TYPE_CONTROL,     // A control, like a button.
  TYPE_POPUP,       // An undecorated Window, with transient properties.
  TYPE_MENU,        // An undecorated Window, with transient properties
                    // specialized to menus.
  TYPE_TOOLTIP,
  TYPE_BUBBLE,
  TYPE_DRAG,        // An undecorated Window, used during a drag-and-drop to
                    // show the drag image.
};

What I tried in the NativeWindowViews constructor

views::Widget::InitParams params;
params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
params.bounds = bounds;
params.delegate = this;

if(has_frame_)
  params.type = views::Widget::InitParams::TYPE_WINDOW;
else
  params.type = views::Widget::InitParams::TYPE_WINDOW_FRAMELESS;

// Tried it with this guy commented out as well
params.remove_standard_frame = !has_frame_;

I'm going to keep digging and see what I can find out.

@frankhale
Copy link
Contributor Author

I may be making some progress here to suppress the Chrome frame. I can at least get it to force a system frame but I can't tell the Chrome frame to go away yet.

progress

@frankhale
Copy link
Contributor Author

Success... And it doesn't break Windows 8.1. w00t!

success

@anaisbetts
Copy link
Contributor

@frankhale Nice! We were hitting that bug too afaik

@frankhale
Copy link
Contributor Author

@zcbenz, I need a sanity check on this alleged fix.

What I did,,, at the bottom of the NativeWindowViews constructor I added a couple lines of code to force native window. Should this be done for all operating systems or just Windows? I'm assuming you don't ever want an Atom-Shell app to display the Chrome blue window titlebar?

atom\browser\native_window_views.cc

NativeWindowViews::NativeWindowViews(content::WebContents* web_contents,
                                     const mate::Dictionary& options)
    : NativeWindow(web_contents, options),
      window_(new views::Widget),
      web_view_(inspectable_web_contents()->GetView()->GetView()),
      menu_bar_autohide_(false),
      menu_bar_visible_(false),
      menu_bar_alt_pressed_(false),
      keyboard_event_handler_(new views::UnhandledKeyboardEventHandler),
      use_content_size_(false),
      resizable_(true) {

  // CODE OMITTED...

  if(has_frame_) {
    window_->set_frame_type(views::Widget::FrameType::FRAME_TYPE_FORCE_NATIVE);
    window_->FrameTypeChanged();
  }

  window_->UpdateWindowIcon();
  window_->CenterWindow(bounds.size());
  Layout();
}

In addition to that on Windows the frame view was not being initialized unless AeroGlass is enabled. I think we can't assume that Aero Glass will be enabled especially if people are running Atom-Shell on a VM where it may not be possible to enable Aero Glass due to graphical limitations.

views::NonClientFrameView* NativeWindowViews::CreateNonClientFrameView(
    views::Widget* widget) {
#if defined(OS_WIN)
  //if (ui::win::IsAeroGlassEnabled()) {
    WinFrameView* frame_view =  new WinFrameView;
    frame_view->Init(this, widget);
    return frame_view;
  //}
#elif defined(OS_LINUX)
  // CODE OMITTED...
#endif

  return NULL;
}

This change doesn't seem to cause any ill effects on Windows 8.1 for me.

@frankhale
Copy link
Contributor Author

Although I did not mention it, got carried away this also resolves the frameless view as well.

@zcbenz
Copy link
Member

zcbenz commented Nov 10, 2014

What I did,,, at the bottom of the NativeWindowViews constructor I added a couple lines of code to force native window. Should this be done for all operating systems or just Windows? I'm assuming you don't ever want an Atom-Shell app to display the Chrome blue window titlebar?

If it works on Linux then it is fine to keep it for all operating systems.

In addition to that on Windows the frame view was not being initialized unless AeroGlass is enabled. I think we can't assume that Aero Glass will be enabled especially if people are running Atom-Shell on a VM where it may not be possible to enable Aero Glass due to graphical limitations.

Using WinFrameView without AeroGlass used to crash on older Chrome versions, if it doesn't crash anymore I think it is good to remove that condition.

And pull requests are welcomed :)

@frankhale
Copy link
Contributor Author

I didn't want to submit too many pull requests because I know you are busy. I'll submit one tomorrow for this issue.

@thedaniel
Copy link
Contributor

Definitely err on the side of more PRs, the worst thing that can happen is that they sit for a while before they get reviewed.

@frankhale
Copy link
Contributor Author

Before I send a PR for this I'll try it on Linux.

@frankhale
Copy link
Contributor Author

Okay my ArchLinux system has the current version of Clang and I cannot build Atom-Shell. I'm going to submit this PR with the exception that I need somebody with a working Linux system to build and test this. The change is quite simple so should be very simple to see if it works.

I'm currently building the change on my Windows box before I submit a PR.

@zcbenz
Copy link
Member

zcbenz commented Nov 12, 2014

Fixed by #815.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants