Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11272015:
DevTools: “Dock to right” broken after turning a tab into a window of its own. (Closed)
|
Created:
8 years, 2 months ago by pfeldman Modified:
7 years, 8 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDevTools: “Dock to right” broken after turning a tab into a window of its own.
- Consolidates window size constraints logic in the devtools_window.*
- Makes all ports follow the same logic
- Stores docked state and split location on per-tab basis.
BUG=122772
Patch Set 1 #
Total comments: 22
Patch Set 2 : Review comments addressed. #
Total comments: 8
Patch Set 3 : More review comments addressed. #
Total comments: 8
Patch Set 4 : Cocoa review comment addressed. #
Total comments: 22
Patch Set 5 : GTK comments addressed, docs updated. #
Total comments: 6
Messages
Total messages: 28 (0 generated)
@Peter: You've seen this code already. This change further streamlines devtools docked window management. It extracts size constraints logic into devtools_window itself and uses per-tab dock and split offset values. It solves many bugs related to pulling tab with devtools opened from / into the browser + makes overall experience a little better.
I cannot review GTK or Cocoa code.
https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... File chrome/browser/debugger/devtools_window.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:339: width_ = container_width * 1 / 3; Nit: Remove "* 1" https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:342: width_ = std::min(container_width - kMinContentsSize, width_); This means that the minimum constant isn't a true minimum. Your code and constants need better documentation -- the constants to say which rules win when there's conflict, the code to document assumptions like "if GetWidth() is being called, the tools are assumed to be docked to the side". https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:344: width_ = width_ * 1 / 3; This makes no sense. If width is negative, leave it negative but make it a third as big? https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:348: int DevToolsWindow::GetHeight(int container_height) { All the same comments apply. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:363: width_ = width; These setters need to clamp to the minima. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... File chrome/browser/debugger/devtools_window.h (right): https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.h:91: int GetWidth(int container_width); These getters should be const. You need to document each of these, especially since you have implemented special behaviors like passing in -1. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2048: new_devtools_window->tab_contents()->web_contents() : NULL); Nit: Indent 4 https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2109: if (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) { Nit: Simpler: contents_split_->set_divider_offset( (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) ? (contents_split_->width() - devtools_window_->GetWidth(contents_split_->width())) : (contents_split_->height() - devtools_window_->GetHeight(contents_split_->height()))); You can make this even simpler if instead of having GetWidth()/GetHeight() return the dev tools size, they return the content size. Then you can simplify to something like: contents_split_->set_divider_offset( (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) ? devtools_window_->GetContentsWidth(contents_split_->width()) : devtools_window_->GetContentsHeight(contents_split_->height())); https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.h (right): https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:502: // Updated devtools window for given contents. Nit: Updated -> Update? What does "Update dev tools window" mean anyway? https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:511: // Updates docked devtools size. Nit: Again, "updates" is too vague. What does this do? https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:665: // Docked devtools window instance. Can this be NULL?
Thank you for your review! https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... File chrome/browser/debugger/devtools_window.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:339: width_ = container_width * 1 / 3; On 2012/10/24 21:18:09, Peter Kasting wrote: > Nit: Remove "* 1" Done. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:342: width_ = std::min(container_width - kMinContentsSize, width_); On 2012/10/24 21:18:09, Peter Kasting wrote: > This means that the minimum constant isn't a true minimum. > > Your code and constants need better documentation -- the constants to say which > rules win when there's conflict, the code to document assumptions like "if > GetWidth() is being called, the tools are assumed to be docked to the side". Done. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:344: width_ = width_ * 1 / 3; Done. Thanks for spotting it, it should have been container_width / 3. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:348: int DevToolsWindow::GetHeight(int container_height) { On 2012/10/24 21:18:09, Peter Kasting wrote: > All the same comments apply. Done. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.cc:363: width_ = width; Then scaling devtools window to say 100px will restore it upon the tab switch back and forth. I was thinking that validation on read would be sufficient. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... File chrome/browser/debugger/devtools_window.h (right): https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/debugge... chrome/browser/debugger/devtools_window.h:91: int GetWidth(int container_width); This getter will lazily fetch the value from the preferences, so it is not const strictly speaking. Also, it will never return negative values. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2048: new_devtools_window->tab_contents()->web_contents() : NULL); On 2012/10/24 21:18:09, Peter Kasting wrote: > Nit: Indent 4 Done. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2109: if (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) { I did not want to return the content size, because it is not really a size: depending on the devtools position, it holds value for either one dimention or the other. Taken together, they do not really form devtools size. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.h (right): https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:502: // Updated devtools window for given contents. Done. Added comment. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:511: // Updates docked devtools size. On 2012/10/24 21:18:09, Peter Kasting wrote: > Nit: Again, "updates" is too vague. What does this do? Done. https://chromiumcodereview.appspot.com/11272015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.h:665: // Docked devtools window instance. On 2012/10/24 21:18:09, Peter Kasting wrote: > Can this be NULL? Done.
lgtm https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/deb... File chrome/browser/debugger/devtools_window.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/deb... chrome/browser/debugger/devtools_window.cc:333: // Called only for the case when devtools window is docked to the side. I'd rather move the comment to the method declaration in the header. https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/deb... chrome/browser/debugger/devtools_window.cc:354: // Called only for the case when devtools window is docked to bottom. ditto https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/ui/... File chrome/browser/ui/cocoa/dev_tools_controller.mm (right): https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:71: // Get current devtools content. Outdated comment? https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/ui/... File chrome/browser/ui/gtk/browser_window_gtk.h (right): https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/ui/... chrome/browser/ui/gtk/browser_window_gtk.h:463: CHROMEGTK_CALLBACK_1(BrowserWindowGtk, gboolean, OnDevToolsSplitChange, Nuke this.
Adding owners for windows, gtk and cocoa. Could you please stamp? https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/deb... File chrome/browser/debugger/devtools_window.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/deb... chrome/browser/debugger/devtools_window.cc:333: // Called only for the case when devtools window is docked to the side. On 2012/10/25 15:35:24, Yury Semikhatsky wrote: > I'd rather move the comment to the method declaration in the header. Done. https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/deb... chrome/browser/debugger/devtools_window.cc:354: // Called only for the case when devtools window is docked to bottom. On 2012/10/25 15:35:24, Yury Semikhatsky wrote: > ditto Done. https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/ui/... File chrome/browser/ui/cocoa/dev_tools_controller.mm (right): https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:71: // Get current devtools content. On 2012/10/25 15:35:24, Yury Semikhatsky wrote: > Outdated comment? Done. https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/ui/... File chrome/browser/ui/gtk/browser_window_gtk.h (right): https://chromiumcodereview.appspot.com/11272015/diff/11003/chrome/browser/ui/... chrome/browser/ui/gtk/browser_window_gtk.h:463: CHROMEGTK_CALLBACK_1(BrowserWindowGtk, gboolean, OnDevToolsSplitChange, On 2012/10/25 15:35:24, Yury Semikhatsky wrote: > Nuke this. Done.
https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... File chrome/browser/ui/cocoa/dev_tools_controller.mm (right): https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:51: devToolsWindow_ = NULL; This will automatically be initialized to NULL in |-alloc|. https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:85: NSView* devToolsView = [subviews objectAtIndex:1]; Should probably check the subviews count before accessing the array. https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:94: bool shouldHide = devToolsWindow_ && (!newDevToolsWindow || Use ObjC BOOL https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:124: bool isVertical = devToolsWindow_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT; BOOL
Thanks for the review! https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... File chrome/browser/ui/cocoa/dev_tools_controller.mm (right): https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:51: devToolsWindow_ = NULL; On 2012/10/25 16:34:51, rsesek wrote: > This will automatically be initialized to NULL in |-alloc|. Done. https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:85: NSView* devToolsView = [subviews objectAtIndex:1]; Added DCHECK_EQ([subviews count], 2u); https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:94: bool shouldHide = devToolsWindow_ && (!newDevToolsWindow || On 2012/10/25 16:34:51, rsesek wrote: > Use ObjC BOOL Done. https://chromiumcodereview.appspot.com/11272015/diff/14001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:124: bool isVertical = devToolsWindow_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT; On 2012/10/25 16:34:51, rsesek wrote: > BOOL Done.
since you moved the code around it's really hard to see what's actually changed. Can you give me a hint about what I should be looking for?
https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... File chrome/browser/debugger/devtools_window.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.cc:83: // Minimal height of devotools pane or content pane when devtools are docked Nit: devotools https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.cc:348: width_ = container_width / 3; This isn't quite right. This allows a shrinking window to shrink the devtools all the way down to zero height, after which point it then suddenly springs up to nonzero again. To fix this, change "if (width_ < 0)" to "if (width_ < (kMinContentsSize / 2))" and add the words "unless the entire window is tiny" to the end of the "compromise" comment. This ends up doing what you want (I had to graph this out to see, though). https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.cc:373: width_ = width; It doesn't seem like these functions actually need to set width_ and height_. As long as they write the values to the prefs, GetWidth()/GetHeight() will do the right thing anyway. This means these could be eliminated entirely and the caller could just write the prefs directly. In fact, you could do even better than this. If the caller does something like the following (without loss of generality): int previous_width = existing_devtools_window ? existing_devtools_window->width : GetPrefs->GetInteger(...); int width = DevToolsWindow::AdjustWidthForContainer(previous_width, container_width); GetPrefs->SetInteger(..., width); // ....and set the splitter position ...then you can eliminate the height_ and width_ members entirely and make GetWidth() and GetHeight() into static methods that simply adjust existing widths/heights. Note that with this method you can optionally move the pref reading + writing around, e.g. the read and write could both move into the static method, or the write could be moved into a nonstatic method on the DevToolsWindow that gets auto-called when the size changes (if such a thing is feasible). These might be appealing if you don't want the caller to know or care about the existence of prefs. https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... File chrome/browser/debugger/devtools_window.h (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.h:92: // Returns preferred devtools window width for given |container_width|. Nit: Add more detail, like "This tries to use the saved window width, or, if none exists, 1/3 of the container width, then clamps to try and ensure both devtools and content are at least somewhat visible." https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/v... File chrome/browser/ui/views/frame/browser_view.h (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/v... chrome/browser/ui/views/frame/browser_view.h:505: // sure devtools window size and position are restored for given tab. Nit: How about: // If |tab_contents| is non-NULL and is being inspected, ensures we show the docked devtools window; otherwise, ensures it's hidden. This restores the devtools window size and position from the last devtools window for this tab. [or however this actually works, I'm not 100% sure how GetDockedInstanceForInspectedTab() works] https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/v... chrome/browser/ui/views/frame/browser_view.h:515: // it to the devtools split. Nit: How about: // Resets the splitter position based on what the current tab's devtools window says its height or width should be.
Couple of comments inline, I'll fix the rest of the nits you found. Peter, could you stamp the win logic? https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... File chrome/browser/debugger/devtools_window.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.cc:348: width_ = container_width / 3; This code is just a fuzzy logic that rescues us in case window is less than 200 pixels. But I can surely do what you are suggesting. https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.cc:373: width_ = width; The point here is that width and height should be read / written on per-tab basis. DevToolsWindow is a container for inspector tab contents and devtools window properties such as dock side and dimensions. Whenever we switch to the tab, we look up its DevToolsWindow and apply its properties to the layout.
@evan: not really much to look at, I just streamlined the code to re-use exactly the same logic in all ports and extracted the comment part into the devtools_window. The change is thoroughly tested, I was the original author of the code I moved around. So I just need a sanity check and a gtk stamp.
https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... File chrome/browser/ui/gtk/browser_window_gtk.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2311: if (!new_devtools_window || why nested ifs instead of one big if? https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2367: if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) { I might turn this into a ternary operator https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2379: if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) { I might turn this into a ternary operator
gtk lgtm with those changes
Thanks for the review! https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... File chrome/browser/ui/gtk/browser_window_gtk.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2311: if (!new_devtools_window || It is already hard to parse that logic, I thought that explicitly guarding the equal windows case would ease the pain. I'll do ternary + comment if you insist though. https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2367: if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) { I wanted this to match the other ports where it takes more lines to complete this. But I can do ternary! https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2379: if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) { ditto
lgtm https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/c... File chrome/browser/ui/cocoa/dev_tools_controller.mm (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/c... chrome/browser/ui/cocoa/dev_tools_controller.mm:82: if (devToolsWindow_) { In the views version, you ceck devToolsWindow != newDevToolsWindow here. Why is mac different?
@thakis: Thanks for the review. https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/c... File chrome/browser/ui/cocoa/dev_tools_controller.mm (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/c... chrome/browser/ui/cocoa/dev_tools_controller.mm:82: if (devToolsWindow_) { "// Store last used position" section is exactly the same in all ports. views version has additional "// Restore tab contents" section that uses that check. It is missing here because on cocoa port, managing split container (adding / removing subviews) == managing tab contents, while in gtk and views we need to handle them separately.
@Peter: I think I addressed your original concerns + answered the additional width / height vs prefs question. I'll take the risk of landing it as TBR=pkasting in order to not wait for another over-night. I'll surely follow up on any comments you make in a separate patch.
https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... File chrome/browser/ui/gtk/browser_window_gtk.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2311: if (!new_devtools_window || On 2012/10/26 03:56:47, Evan Stade wrote: > why nested ifs instead of one big if? Done. https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2367: if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) { On 2012/10/26 03:56:47, Evan Stade wrote: > I might turn this into a ternary operator Done. https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/ui/g... chrome/browser/ui/gtk/browser_window_gtk.cc:2379: if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) { On 2012/10/26 03:56:47, Evan Stade wrote: > I might turn this into a ternary operator Done.
https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... File chrome/browser/debugger/devtools_window.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.cc:83: // Minimal height of devotools pane or content pane when devtools are docked On 2012/10/26 03:22:07, Peter Kasting wrote: > Nit: devotools Done. https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.cc:348: width_ = container_width / 3; On 2012/10/26 03:22:07, Peter Kasting wrote: > This isn't quite right. This allows a shrinking window to shrink the devtools > all the way down to zero height, after which point it then suddenly springs up > to nonzero again. > > To fix this, change "if (width_ < 0)" to "if (width_ < (kMinContentsSize / 2))" > and add the words "unless the entire window is tiny" to the end of the > "compromise" comment. This ends up doing what you want (I had to graph this out > to see, though). Done. https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... File chrome/browser/debugger/devtools_window.h (right): https://chromiumcodereview.appspot.com/11272015/diff/1010/chrome/browser/debu... chrome/browser/debugger/devtools_window.h:92: // Returns preferred devtools window width for given |container_width|. On 2012/10/26 03:22:07, Peter Kasting wrote: > Nit: Add more detail, like "This tries to use the saved window width, or, if > none exists, 1/3 of the container width, then clamps to try and ensure both > devtools and content are at least somewhat visible." Done.
LGTM now that Rietveld is back *rolleyes* https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... File chrome/browser/ui/cocoa/dev_tools_controller.mm (right): https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:76: (newDevToolsWindow->dock_side() == dockSide_)) { nit: these parens are technically unnecessary https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... chrome/browser/ui/cocoa/dev_tools_controller.mm:93: // hide + show. nit: + should be written out as "and"
Come find me if you still don't understand what I'm saying below. https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/deb... File chrome/browser/debugger/devtools_window.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/deb... chrome/browser/debugger/devtools_window.cc:335: GetInteger(prefs::kDevToolsVSplitLocation); Tiny nit: I'd linebreak after '(' instead of '->' https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... chrome/browser/ui/views/frame/browser_view.cc:2084: contents_split_->width() - contents_split_->divider_offset()); The reason I said DevToolsWindow doesn't need width_ and height_ is because you have those values here and can pass them back in to GetWidth() and GetHeight() later. And you don't need to gate their calculation on whether |devtools_window_| is non-NULL, you can do it all the time. This all becomes clearer if you inline ShowDevToolsContainer() and HideDevToolsContainer() into this function.
https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... chrome/browser/ui/views/frame/browser_view.cc:2084: contents_split_->width() - contents_split_->divider_offset()); The problem is that there is 1 browser_view instance with single, _current_ width and height values. At the same time, there are N tabs and N devtools_windows, each having its own width and height. And then you select tab "i", you need to restore corresponding width, belonging to that very devtools instance.
https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://chromiumcodereview.appspot.com/11272015/diff/18001/chrome/browser/ui/... chrome/browser/ui/views/frame/browser_view.cc:2084: contents_split_->width() - contents_split_->divider_offset()); On 2012/10/29 12:24:25, pfeldman wrote: > The problem is that there is 1 browser_view instance with single, _current_ > width and height values. At the same time, there are N tabs and N > devtools_windows, each having its own width and height. And then you select tab > "i", you need to restore corresponding width, belonging to that very devtools > instance. OK, I finally figured out what was confusing me. It was a combination of interpreting contents_split_->width() as contents_split_->divider_offset() and interpreting GetWidth()'s container_width argument as what we'd set the width_ to. I still think a slight simplification is possible. It seems like GetWidth()/GetHeight() could still be static methods that calculate the appropriate value and then return it. There's no need to write width_/height_ from these methods since the block here in the caller will set them anyway before we change tabs away. This makes it clearer that these variables are only relevant for caching the window width and height during the time when it's not visible, as opposed to also being used for something while the window is showing, or causing the window's width and height to actually change as a direct result of calling GetWidth()/GetHeight(). Once this is done, a few comments could be fleshed out. width_ and height_ themselves should be commented to the effect of the above paragraph. The block here should say something more like "Tell the current devtools window, if any, to save off its current size before we change tabs, so we can restore the size on switching back later." This also raises another question. We don't write the on-disk pref until you switch away from a tab. Does this code get reached during browser shutdown as well? If not, we'll basically "forget" the last set of changes the user makes to the split location.
I actually like GetWidth and GetHeight as they are since they follow a simple "lazy getter with constraint" pattern. It easy to understand and explain. I'm happy to add more explanatory comments on the fields as you suggest when I am back in this code. Wrt saving to disk on switch away: this has been so for years. I'll need to follow up with another change to fix it. I hope to do it on a tab contents level so that it was not platform-specific.
On 2012/10/31 11:57:21, pfeldman wrote: > I actually like GetWidth and GetHeight as they are since they follow a simple > "lazy getter with constraint" pattern. It easy to understand and explain. It is not at all easy to understand. Even now that I understand what the code is doing I don't find this simple or intuitive and I don't recall seeing anything else quite like it in Chrome. I would like to think I am not stupid... But even if it were easy enough to understand this, surely a static method that doesn't touch members and a clearly reduced scope for your members is even simpler. What do you gain not going this route? I strongly urge you to consider changing this.
As discussed on chat: LGTM. I think there is win from making GetWidth() const, having it not write width_, and renaming to something that makes clearer that it's really taking a saved width, if any, and then trying to adjust appropriately to fit some constraints for a possibly-different container width. This can be done later in another change. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
