Export to GitHub

chromium-os - issue #8258

FileBrowse panel is of wrong size on ToT


Posted on Oct 27, 2010 by Quick Rhino

0.9.103.x dev build.

What steps will reproduce the problem? 1. Bring up file browse panel by "Ctrl+O"; 2. Close it 3. Repeat step 1 and 2 a few times.

What is the expected output? File browse panel should be of of the same size all the time.

What do you see instead? File browse panel has wrong size.

Attachments

Comment #1

Posted on Oct 27, 2010 by Happy Camel

i don't see this in my build

Comment #2

Posted on Oct 27, 2010 by Quick Rhino

cc more panel experts.

The small FileBrowse panel seems like to have a size of 100x100. Does this ring bell for any one?

Comment #3

Posted on Oct 28, 2010 by Quick Rhino

So 100x100 is the min panel size. This happens because somehow filebrowser panel's window returns size of 1x1 in PanelBrowserView::Init and trigers the min size logic. Still don't know why it has 1x1 to begin with.

Comment #4

Posted on Oct 28, 2010 by Swift Wombat

Probably because WidgetGtk::CreateGtkWidget sets minimum size to 1x1 ?

Comment #5

Posted on Oct 29, 2010 by Quick Rhino

Dan, this could be related to window manager and needs your help.

I added a bunch of LOG statements in Chrome to track the flow of a FileBrowse panel creation and found it is always the same regardless of whether the final on screen result is correct or not. It looks like some unresolved racing cases between X and window manager.

Here's the rough flow: 1. FileBrowseUI::OpenPopup called on "Ctrl+O" to trigger FileBrowse panel creation; 2. In OpenPoup, browser->AddTabWithURL creates the browser window which contains PanelBrowserView; 3. Browser window creation will call into PanelBrowserView::Init. And because we pass emtpy bounds during browser window creation, PanelBrowserView::Init always gets 1x1 as its bounds. Per Oshima's comments, this is probably because we use a 1x1 min size for all WidgetGtk derived. And this result in a gtk_window_resize with min panel size of 100x100; 4. Then after some other code, the flow goes back to FileBrowseUI::OpenPopup and we have a params.target->window()->SetBounds that intends to change FileBrowse panel bounds to 250x300. This calls into WidgetGtk::SetBounds and fires another gtk_window_resize with size of 250x300. The window is not mapped at this moment so gdk_window_move_resize is not called. 5. Then back to FileBrowseUI::OpenPopup, we show the FileBrowse UI and somehow it ends up with the wrong 100x100 size;

Dan, let me know if this is good enough for you to investigate. Thanks.

Comment #6

Posted on Oct 29, 2010 by Massive Elephant

When this happens, Chrome is asking the window manager to resize the window to 100x100 soon after it's been mapped. Here is an excerpt from the window manager log (with particularly-boring lines removed):

[1029/104341:INFO:window_manager.cc(1595)] Handling create notify for normal window 0xa01277 at (0, 0) with size 1x1 [1029/104342:INFO:window_manager.cc(1541)] Handling configure request for 0xa01277 to pos (0, 0) and size 250x300 [1029/104342:INFO:window.cc(655)] Resizing 0xa01277's client window to 250x300 [1029/104342:INFO:window_manager.cc(1714)] Handling map request for 0xa01277 [1029/104342:INFO:window.cc(598)] Mapping 0xa01277 [1029/104342:INFO:window_manager.cc(1541)] Handling configure request for 0xa01277 to pos (undef, undef) and size 100x100 [1029/104342:INFO:window.cc(655)] Resizing 0xa01277's client window to 100x100

You can see that the second configure request is getting sent after the request to map the window. When this problem doesn't happen, we don't see the final 100x100 request.

One possible explanation would be a race in Chrome where we: 1) send a request to resize the window to 100x100 2) send a request to resize the window to 250x300 3) query the current size and send a request to resize to that size

Since the requests in 1) and 2) get routed through the window manager and applied asynchronously, there's a race as to whether X says the window is 100x100 or 250x300 in 3). I have not looked at the Chrome code; this is just a common sort of mistake -- if this were precisely what were happening, I'd expect to see a configure request to 100x100 come in before we receive the 250x300 one, which it sounds from your message like Chrome is sending but which I don't see in the WM logs.

Comment #7

Posted on Oct 29, 2010 by Quick Rhino

It is really strange that 100x100 config request comes after 250x300's. It is not inline with the chrome code.

I suspect it is related to the way we call gtk. So I tried to use different sizes with a few suspects and found out that gtk_widget_set_size_request in PanelBrowserView::Init seems be causing the problem. When the problem happens, the panel always uses the size associated with that call. And the problem does not happen if I remove that call.

Oshima, do we still need PanelBrowserView::Init now? window manager has evolved a lot and I am also not able to repro the problem in your comments after removing it.

Comment #8

Posted on Nov 1, 2010 by Quick Rhino

Talked with Oshima and we could try if we still need PanelBrowserView::Init.

Comment #9

Posted on Nov 2, 2010 by Happy Ox

Rather than eliminate it, we may want to change PanelBrowserView::Init() to just:

void PanelBrowserView::Init() { ::BrowserView::Init(); SetBounds(frame()->GetWindow()->GetBounds()); }

So that the correct sizing code is called at least?

Comment #10

Posted on Nov 2, 2010 by Quick Rhino

I am not sure whether that is necessary. PanelBrowserView::SetBounds will be called when WindowGtk's size is allocated, which always happens.

Comment #11

Posted on Nov 3, 2010 by Happy Ox

OK, cool, so long as that's true then there shouldn't be a problem.

Comment #12

Posted on Nov 3, 2010 by Quick Rhino

I'll have my finger crossed. :)

Review: http://codereview.chromium.org/4170010 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64922

Comment #13

Posted on Nov 15, 2010 by Grumpy Rhino

Build:

0.9.112.0 (dev channel official build)

Verified fixed.

Comment #14

Posted on Nov 15, 2010 by Grumpy Rhino

(No comment was entered for this change.)

Comment #15

Posted on Mar 7, 2013 by Grumpy Hippo

(No comment was entered for this change.)

Comment #16

Posted on Mar 13, 2013 by Happy Horse

Moved to: Issue chromium:190603

Status: Moved

Labels:
Type-Bug Pri-1 Area-FileBrowser Sev-2 Mstone-R9.x Iteration-16 Est-1 OS-Chrome