DevTools: place DevTools WebContents underneath inspected WebContents.

Place DevTools WebContents underneath inspected WebContents.

DevTools window: split width and height are gone, and instead
contents insets are introduced. These insets are used for layout
in browser window. Having insets instead of rect allows for faster
regular-case resize occuring immediately in layout:
we keep DevTools of the same size and resize the page contents.

Views: removed SplitView and ContentsContainer, but instead added
ContentsLayoutManager which layouts DevTools and page contents taking
insets into account. It also supports active top margin, moved from
ContentsContainer.

Mac: removed NSSplitView, but instead added DevToolsContainerView
which resizes subviews manually taking page insets into account.

Gtk: removed both contents_vsplit and contents_hsplit, but instead
added devtools_floating_container which positions page contents
taking insets into account.

Next step will remove dock side knowledge from the browser, and leave it
on frontend only.

BUG=318751
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242071

[dgozman, 2013-11-13 15:09:42]

Take a first look please.

[pfeldman, 2013-11-18 14:18:26]

Overall, it looks promising!

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
File chrome/browser/devtools/devtools_embedder_message_dispatcher.h (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_embedder_message_dispatcher.h:31: virtual void
SetContentsOffsets(
I'd do SetContentsRect(x, y, width, height)

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_window.cc:488: int min_width = 150;
Extract constants for these.

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_window.cc:491: if (dock_side_ ==
DEVTOOLS_DOCK_SIDE_BOTTOM) {
drop {}

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_window.cc:496: return gfx::Size(min_width,
min_height);
DevTools window should not be aware of dock side anymore. Docking concept will
move to the front-end entirely. Just return fixed value gfx::Size(150, 50) /
gfx::Size(200, 150) or send it along with SetContentsRect;

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_window.h:115: gfx::Size
GetTopLeftContentsOffset() const;
gfx::Rect GetContentsRect

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.h:606: // | Contents
(contents_container_)                                   |
Now contents_container only contains devtools_container. We only need one of
them.

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:550: // if
(contents_split_->child_at(1) && contents_split_->child_at(1)->visible())
Drop?

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/devtools_container.h (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/devtools_container.h:14: // DevToolsContainer is
managing WebContents atop of the DevTools if present.
I am not sure you should introduce it. it does not do much and is 1:1 with its
container in size.

[dgozman, 2013-11-18 14:37:14]

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_window.h:115: gfx::Size
GetTopLeftContentsOffset() const;
On 2013/11/18 14:18:27, pfeldman wrote:
> gfx::Rect GetContentsRect

The whole idea of offsets is that on resize views layout will change the
webcontents size to the expected one (so that devtools stays of the same
height/width), thus making webcontents resizing faster and only once. Opposite
solution requires devtools resize, round-trip to frontend, and then another
relayout in views.
I'd say it's worth it.

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.h:606: // | Contents
(contents_container_)                                   |
On 2013/11/18 14:18:27, pfeldman wrote:
> Now contents_container only contains devtools_container. We only need one of
> them.

Contents container has top offset, which is used for at least to proper position
find bar, and maybe for something else. I'm not confident enough to remove it,
and it should be separate change anyway.

[pfeldman, 2013-11-20 15:02:08]

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_window.h:115: gfx::Size
GetTopLeftContentsOffset() const;
Ok, sgtm.

Did you measure the overhead though? I like the optimization, but it means that
devtools no longer controls the layout dynamically. For example, we will not be
able to achieve the behavior when resize leads to the sidebar resize. That might
be handly when we are emulating and don't need much real estate to the inspected
contents.

[dgozman, 2013-11-21 15:08:54]

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/63173016/diff/40001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_window.h:115: gfx::Size
GetTopLeftContentsOffset() const;
On 2013/11/20 15:02:08, pfeldman wrote:
> Ok, sgtm.
> 
> Did you measure the overhead though? I like the optimization, but it means
that
> devtools no longer controls the layout dynamically. For example, we will not
be
> able to achieve the behavior when resize leads to the sidebar resize. That
might
> be handly when we are emulating and don't need much real estate to the
inspected
> contents.

I didn't measure the overhead. But this scenario will still work - devtools
window gets resize event, decides to grow the sidebar, and then requests a new
rect for the inspected contents. This will lead to double-resize for contents,
which should not be a big deal in emulating mode - blink does not know about
resizes while emulate is on.

[pfeldman, 2013-12-05 15:54:07]

Overall looks good. I don't think landing this w/o GTK on board is a good idea,
so you should remove the checks and either make GTK work or land it later.

https://codereview.chromium.org/63173016/diff/220001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/63173016/diff/220001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:700: #if defined(OS_MACOSX) ||
defined(USE_AURA)
You probably want this everywhere or nowhere.

https://codereview.chromium.org/63173016/diff/220001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:736: #if !defined(OS_MACOSX) &&
!defined(USE_AURA)
ditto

https://codereview.chromium.org/63173016/diff/220001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:1473: #if !defined(OS_MACOSX) &&
!defined(USE_AURA)
ditto

[dgozman, 2013-12-05 15:54:36]

Hello,

pfeldman@, please take a look at devtools side.

avi@, please take a look at mac side.

sky@, please take a look at views side. I'll also need your 
owner review for the whole CL.

Thank you,
Dmitry

[dgozman, 2013-12-05 16:21:45]

Hi,

erg@, please look at the GTK stuff.

tsepez@, please look at c/b/devtools/devtools_embedder_message_dispatcher.h

Thanks,
Dmitry

[pfeldman, 2013-12-05 17:23:24]

Overall approach and devtools lgtm

[Tom Sepez, 2013-12-05 18:37:56]

devtools_embedder_message_dispatcher LGTM.

[Elliot Glaysher, 2013-12-05 20:17:50]

gtk lgtm

[sky, 2013-12-05 20:56:40]

https://codereview.chromium.org/63173016/diff/240001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/devtools_container.h (right):

https://codereview.chromium.org/63173016/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/devtools_container.h:14: // DevToolsContainer is
managing WebContents atop of the DevTools if present.
This should be a LayoutManager attached to a normal view (since you need no
customization on the view). I don't see a compelling reason to keep
ContentsContainer either, fold the logic it has into your new layoutmanager.
Then you can name this class ContentsLayoutManager.

https://codereview.chromium.org/63173016/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/devtools_container.h:18: explicit
DevToolsContainer(
no explicit.

[dgozman, 2013-12-06 13:24:00]

avi@, sky@, PTAL.

https://codereview.chromium.org/63173016/diff/240001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/devtools_container.h (right):

https://codereview.chromium.org/63173016/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/devtools_container.h:14: // DevToolsContainer is
managing WebContents atop of the DevTools if present.
On 2013/12/05 20:56:41, sky wrote:
> This should be a LayoutManager attached to a normal view (since you need no
> customization on the view). I don't see a compelling reason to keep
> ContentsContainer either, fold the logic it has into your new layoutmanager.
> Then you can name this class ContentsLayoutManager.

Done.

https://codereview.chromium.org/63173016/diff/240001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/devtools_container.h:18: explicit
DevToolsContainer(
On 2013/12/05 20:56:41, sky wrote:
> no explicit.

Done.

[sky, 2013-12-06 17:25:44]

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.h:96: public
views::SingleSplitViewListener,
I don't believe this is needed anymore. Make sure you also nuke any includes
that are now obsolete.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/contents_layout_manager.cc (right):

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.cc:10: views::View*
devtools_view, views::View* contents_view)
nit: when you wrap, each param on its own line.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.cc:20: const gfx::Size&
top_left, const gfx::Size& bottom_right) {
nit: when you wrap, each param on its own line.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/contents_layout_manager.h (right):

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.h:13: //
ContainerLayoutManager is managing WebContents atop of the DevTools.
ContentsLayoutManager positions the WebContents and devtools WebContents.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.h:27: bool
SetContentsViewOffsets(
This can InvalidateLayout is you need it to? The View is provided to
LayoutManagers by way of Installed().

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.h:28: const gfx::Size&
top_left, const gfx::Size& bottom_right);
How come this is two sizes? Isn't an Insets what you want?

[dgozman, 2013-12-09 13:19:18]

I've changed everything from offsets to insets.

avi@, sky@, PTAL.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.h:96: public
views::SingleSplitViewListener,
On 2013/12/06 17:25:44, sky wrote:
> I don't believe this is needed anymore. Make sure you also nuke any includes
> that are now obsolete.

Done.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/contents_layout_manager.cc (right):

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.cc:10: views::View*
devtools_view, views::View* contents_view)
On 2013/12/06 17:25:44, sky wrote:
> nit: when you wrap, each param on its own line.

Done.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.cc:20: const gfx::Size&
top_left, const gfx::Size& bottom_right) {
On 2013/12/06 17:25:44, sky wrote:
> nit: when you wrap, each param on its own line.

Done.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/contents_layout_manager.h (right):

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.h:13: //
ContainerLayoutManager is managing WebContents atop of the DevTools.
On 2013/12/06 17:25:44, sky wrote:
> ContentsLayoutManager positions the WebContents and devtools WebContents.

Done.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.h:27: bool
SetContentsViewOffsets(
On 2013/12/06 17:25:44, sky wrote:
> This can InvalidateLayout is you need it to? The View is provided to
> LayoutManagers by way of Installed().

Done.

https://codereview.chromium.org/63173016/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/contents_layout_manager.h:28: const gfx::Size&
top_left, const gfx::Size& bottom_right);
On 2013/12/06 17:25:44, sky wrote:
> How come this is two sizes? Isn't an Insets what you want?

Missed that class. Thank you! Done.

[sky, 2013-12-09 15:05:11]

https://codereview.chromium.org/63173016/diff/340001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/63173016/diff/340001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:849: // Layout for DevTools
_before_ setting the both main and devtools WebContents
Is this comment still correct here? I'm wondering if you can get rid of the
boolean to UpdateDevToolsForContents and always invoke
UpdateDevToolsForContents?

[dgozman, 2013-12-09 15:09:51]

https://codereview.chromium.org/63173016/diff/340001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/63173016/diff/340001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:849: // Layout for DevTools
_before_ setting the both main and devtools WebContents
On 2013/12/09 15:05:12, sky wrote:
> Is this comment still correct here? I'm wondering if you can get rid of the
> boolean to UpdateDevToolsForContents and always invoke
> UpdateDevToolsForContents?

UpdateDevToolsForContents may change the size of both DevTools and page
WebContents. We want first to layout, and then set both WebContents to WebViews
to avoid unnecessary resizes. Those may happen if you switch between tabs
with/without infobars and bookmark bar.

[sky, 2013-12-09 15:14:00]

Ok, LGTM then. Wait for Avi to review Cocoa side.

[Avi (use Gerrit), 2013-12-09 16:13:35]

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (left):

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:636: // Tests that
status bubble's base frame does move when devTools are docked.
Is there an equivalent test you can add, to test that the status bubble moves
when the devtools encloses the web page? Losing a test makes me sad,
particularly because it's a regression test for http://crbug.com/54882 .

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/dev_tools_controller.mm (right):

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:28: NSView* contentsView_;
Comment these as being weak (ownership via -subviews).

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:34: -
(void)resizeSubviewsWithOldSize:(NSSize)oldBoundsSize;
unused; remove

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:43: - (DevToolsContainerView*)
initWithFrame:(NSRect)frame {
No space between ) and i.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:48: bottomRightContentsOffset_ =
NSZeroSize;
This initialization isn't needed; ivars start out zero-filled.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:59: -
(void)resizeSubviewsWithOldSize:(NSSize)oldBoundsSize {
unused; remove

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:65: DCHECK_EQ([subviews count],
1u);
In DCHECK land, expected values are the first param. Switch the params here and
in several other places in this file.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:108: contentsHeight)];
Weird wrapping; can you get this to be on one line?

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:148: gfx::Insets insets =
devToolsWindow_->GetContentsInsets();;
Too many semicolons.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:150: contents->GetView(),
gfx::Point(insets.left(), insets.top()));
Yikes. SetOverlayView takes a flipped Y? Can you add a comment to the interface
in content, as well as a comment here?

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:152: // Swap top and bottom
offsets for NSView geometry.
:(

If you are calling parameters "top left" and "right bottom" then you should
actually pass in the top left and right bottom values, and do the appropriate
flipping inside the function.

If you swap them here and lie in the names of the parameters, you're only asking
for trouble.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:155:
bottomRight:NSMakeSize(insets.right(), insets.top())];
Oh wow, this is awkward. Cocoa's equivalent to gfx::Insets is NSEdgeInsets, but
it's 10.7 so you can't use it. You might as well use gfx::Insets for the
DevToolsContainerView and make less busy-work for yourself.

And do the flipping inside of DevToolsContainerView, not here!

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:158:
setContentsOffsetsTopLeft:NSZeroSize
Surely this fits on the previous line.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:174: // |devToolsView| is a
WebContentsViewCocoa object, whose ViewID was
:)

[dgozman, 2013-12-09 21:09:10]

Thank you for review, I will fix everything. One question below though.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (left):

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:636: // Tests that
status bubble's base frame does move when devTools are docked.
On 2013/12/09 16:13:36, Avi wrote:
> Is there an equivalent test you can add, to test that the status bubble moves
> when the devtools encloses the web page? Losing a test makes me sad,
> particularly because it's a regression test for http://crbug.com/54882 .

What do you think, if we use performSelector: here to call methods from
implementation?
Otherwise, It looks undoable here in unit tests.
Or maybe, there is a browser test which does something similar, so I can mimic
it? I'm not familiar with Mac-specific tests at all :(

[dgozman, 2013-12-10 14:51:46]

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (left):

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:636: // Tests that
status bubble's base frame does move when devTools are docked.
On 2013/12/09 16:13:36, Avi wrote:
> Is there an equivalent test you can add, to test that the status bubble moves
> when the devtools encloses the web page? Losing a test makes me sad,
> particularly because it's a regression test for http://crbug.com/54882 .

I've added BrowserWindowControllerTest.StatusBubblePositioning instead of this.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/dev_tools_controller.mm (right):

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:28: NSView* contentsView_;
On 2013/12/09 16:13:36, Avi wrote:
> Comment these as being weak (ownership via -subviews).

Done.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:34: -
(void)resizeSubviewsWithOldSize:(NSSize)oldBoundsSize;
On 2013/12/09 16:13:36, Avi wrote:
> unused; remove

This is an NSView override. Removed from interface.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:43: - (DevToolsContainerView*)
initWithFrame:(NSRect)frame {
On 2013/12/09 16:13:36, Avi wrote:
> No space between ) and i.

Done.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:48: bottomRightContentsOffset_ =
NSZeroSize;
On 2013/12/09 16:13:36, Avi wrote:
> This initialization isn't needed; ivars start out zero-filled.

Done.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:59: -
(void)resizeSubviewsWithOldSize:(NSSize)oldBoundsSize {
On 2013/12/09 16:13:36, Avi wrote:
> unused; remove

This is NSView override invoked when view is resized.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:65: DCHECK_EQ([subviews count],
1u);
On 2013/12/09 16:13:36, Avi wrote:
> In DCHECK land, expected values are the first param. Switch the params here
and
> in several other places in this file.

Done.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:108: contentsHeight)];
On 2013/12/09 16:13:36, Avi wrote:
> Weird wrapping; can you get this to be on one line?

Done.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:148: gfx::Insets insets =
devToolsWindow_->GetContentsInsets();;
On 2013/12/09 16:13:36, Avi wrote:
> Too many semicolons.

Done.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:150: contents->GetView(),
gfx::Point(insets.left(), insets.top()));
On 2013/12/09 16:13:36, Avi wrote:
> Yikes. SetOverlayView takes a flipped Y? Can you add a comment to the
interface
> in content, as well as a comment here?

SetOverlayView takes the usual coordinates (top-left corner is (0;0)).
DevToolsWindow operates has the same idea, so we just directly pass left/top to
SetOverlayView.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:152: // Swap top and bottom
offsets for NSView geometry.
On 2013/12/09 16:13:36, Avi wrote:
> :(
> 
> If you are calling parameters "top left" and "right bottom" then you should
> actually pass in the top left and right bottom values, and do the appropriate
> flipping inside the function.
> 
> If you swap them here and lie in the names of the parameters, you're only
asking
> for trouble.

Ok, so the right way is to always use logical top, and only flip when passing to
Cocoa? Done.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:155:
bottomRight:NSMakeSize(insets.right(), insets.top())];
On 2013/12/09 16:13:36, Avi wrote:
> Oh wow, this is awkward. Cocoa's equivalent to gfx::Insets is NSEdgeInsets,
but
> it's 10.7 so you can't use it. You might as well use gfx::Insets for the
> DevToolsContainerView and make less busy-work for yourself.
> 
> And do the flipping inside of DevToolsContainerView, not here!

Done.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:158:
setContentsOffsetsTopLeft:NSZeroSize
On 2013/12/09 16:13:36, Avi wrote:
> Surely this fits on the previous line.

Done.

[Avi (use Gerrit), 2013-12-10 16:32:39]

Cocoa LGTM.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (left):

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:636: // Tests that
status bubble's base frame does move when devTools are docked.
On 2013/12/10 14:51:47, dgozman wrote:
> On 2013/12/09 16:13:36, Avi wrote:
> > Is there an equivalent test you can add, to test that the status bubble
moves
> > when the devtools encloses the web page? Losing a test makes me sad,
> > particularly because it's a regression test for http://crbug.com/54882 .
> 
> I've added BrowserWindowControllerTest.StatusBubblePositioning instead of
this.

Sweet, thanks.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/dev_tools_controller.mm (right):

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:59: -
(void)resizeSubviewsWithOldSize:(NSSize)oldBoundsSize {
On 2013/12/10 14:51:47, dgozman wrote:
> On 2013/12/09 16:13:36, Avi wrote:
> > unused; remove
> 
> This is NSView override invoked when view is resized.

Oh.

https://codereview.chromium.org/63173016/diff/360001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/dev_tools_controller.mm:152: // Swap top and bottom
offsets for NSView geometry.
On 2013/12/10 14:51:47, dgozman wrote:
> On 2013/12/09 16:13:36, Avi wrote:
> > :(
> > 
> > If you are calling parameters "top left" and "right bottom" then you should
> > actually pass in the top left and right bottom values, and do the
appropriate
> > flipping inside the function.
> > 
> > If you swap them here and lie in the names of the parameters, you're only
> asking
> > for trouble.
> 
> Ok, so the right way is to always use logical top, and only flip when passing
to
> Cocoa? Done.

Well, if you have a parameter called "top left" then it should be the top left.
Otherwise it's really confusing.

[commit-bot: I haz the power, 2013-12-13 13:12:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/63173016/400001

[commit-bot: I haz the power, 2013-12-13 15:09:04]

Change committed as 240653

[commit-bot: I haz the power, 2013-12-16 12:38:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/63173016/420001

[commit-bot: I haz the power, 2013-12-16 14:20:33]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-16 19:31:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/63173016/420001

[commit-bot: I haz the power, 2013-12-17 00:34:35]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-12-17 05:56:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/63173016/420001

[commit-bot: I haz the power, 2013-12-17 07:58:42]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-17 08:45:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/63173016/420001

[commit-bot: I haz the power, 2013-12-17 08:47:36]

Change committed as 241223

[commit-bot: I haz the power, 2013-12-20 10:59:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/63173016/430001

[commit-bot: I haz the power, 2013-12-20 12:24:02]

Change committed as 242071