DevTools: polish the MQ ruler, move zoom controls to the top.

DevTools: polish the MQ ruler, move zoom controls to the top.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182327

[pfeldman, 2014-09-18 15:48:41]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org, lushnikov@chromium.org

[dgozman, 2014-09-19 08:41:26]

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/responsiveDesignView.css (right):

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/responsiveDesignView.css:430: z-index: 2;
I think, we can remove all the z-index properties now and just put
.responsive-design-media-container after canvas in DOM hierarchy.

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right):

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/MediaQueryInspector.js:316:
_highlightMarkerInContainer: function(marker, container)
This method can be greatly simplified:
container.children.forEach(function(el) {
    el.classList.toggle("media-inspector-marker-highlight", el === marker);
});

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/toolbox/ResponsiveDesignView.js (right):

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:301: var rulerHeight =
this._rulerTotalHeightDIP();
This introduces an extra layout. You should calculate it at the start of
updateUI and pass as a parameter.

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:353: if (!(index %
(rulerStepCount / 2)))
This is not needed any more.

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:466: var
cssRulerHeight = (mediaInspectorHeight ?
WebInspector.ResponsiveDesignView.RulerTopHeight :
WebInspector.ResponsiveDesignView.RulerHeight) / zoomFactor + "px";
cssRulerTopHeight

[pfeldman, 2014-09-19 09:21:48]

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/responsiveDesignView.css (right):

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/responsiveDesignView.css:430: z-index: 2;
On 2014/09/19 08:41:25, dgozman wrote:
> I think, we can remove all the z-index properties now and just put
> .responsive-design-media-container after canvas in DOM hierarchy.

Done.

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right):

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/MediaQueryInspector.js:316:
_highlightMarkerInContainer: function(marker, container)
Why don't we use hover and remove it altogether?

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/toolbox/ResponsiveDesignView.js (right):

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:301: var rulerHeight =
this._rulerTotalHeightDIP();
On 2014/09/19 08:41:25, dgozman wrote:
> This introduces an extra layout. You should calculate it at the start of
> updateUI and pass as a parameter.

Done.

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:353: if (!(index %
(rulerStepCount / 2)))
On 2014/09/19 08:41:26, dgozman wrote:
> This is not needed any more.

Done.

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:466: var
cssRulerHeight = (mediaInspectorHeight ?
WebInspector.ResponsiveDesignView.RulerTopHeight :
WebInspector.ResponsiveDesignView.RulerHeight) / zoomFactor + "px";
On 2014/09/19 08:41:25, dgozman wrote:
> cssRulerTopHeight

Is that important?

[dgozman, 2014-09-19 09:25:22]

lgtm

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right):

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/MediaQueryInspector.js:316:
_highlightMarkerInContainer: function(marker, container)
On 2014/09/19 09:21:47, pfeldman wrote:
> Why don't we use hover and remove it altogether?

I guess that works if you don't move the highlighted on up anymore.

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/toolbox/ResponsiveDesignView.js (right):

https://codereview.chromium.org/581133002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:466: var
cssRulerHeight = (mediaInspectorHeight ?
WebInspector.ResponsiveDesignView.RulerTopHeight :
WebInspector.ResponsiveDesignView.RulerHeight) / zoomFactor + "px";
On 2014/09/19 09:21:47, pfeldman wrote:
> On 2014/09/19 08:41:25, dgozman wrote:
> > cssRulerTopHeight
> 
> Is that important?

Well, I expect cssRulerHeight === rulerTotalHeight * zoomFactor.

[pfeldman, 2014-09-19 10:20:33]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-09-19 10:20:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/581133002/60001

[commit-bot: I haz the power, 2014-09-19 10:23:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 10:23:25]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/58166)

[pfeldman, 2014-09-19 12:14:33]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-09-19 12:14:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/581133002/80001

[commit-bot: I haz the power, 2014-09-19 13:52:09]

Committed patchset #5 (id:80001) as 182327