Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
308003007:
[DevTools] Update media queries when resizing with emulation on. (Closed)
|
Created:
6 years, 6 months ago by dgozman Modified:
6 years, 6 months ago CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
Description[DevTools] Update media queries when resizing with emulation on.
Media query evaluator assumes that device size cannot change, which is
wrong in emulation mode.
BUG=327641, 386142
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176420
Patch Set 1 #
Total comments: 3
Patch Set 2 : With test #
Total comments: 4
Patch Set 3 : Rebased, added comment #
Total comments: 2
Patch Set 4 : Rebase, updated comment, removed test #Messages
Total messages: 19 (0 generated)
Take a look please. I tried to introduce a setting "deviceSizeMayChange" to mark device-size media features as viewport-dependent, which is more efficient approach. It turned out to be too intrusive, since MediaQueryEvaluator doesn't have a reference to frame/page/settings.
On 2014/05/30 13:22:04, dgozman wrote: > Take a look please. > > I tried to introduce a setting "deviceSizeMayChange" to mark device-size media > features as viewport-dependent, which is more efficient approach. > It turned out to be too intrusive, since MediaQueryEvaluator doesn't have a > reference to frame/page/settings. We should make sure this doesn't drastically regress resize behaviour in "responsive design" DevTools feature
https://codereview.chromium.org/308003007/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/308003007/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1623: if (page()->inspectorController().deviceEmulationEnabled() && mainFrameImpl()->frame()->document()) Not sure if it is worth it, but in our instrumentation code we use (or used to use?) something similar to: if (page()->inspectorController().deviceEmulationEnabled()) { if (Document* document = mainFrameImpl()->frame()->document()) document->mediaQueryAffectingValueChanged(); }
Is it possible to write a test for this? Also, I don't understand the change fully. Don't we only need to do this when the emulation state actually changes? e.g. when you go in/out of emulation mode or you change what device is being emulated. https://codereview.chromium.org/308003007/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/308003007/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1623: if (page()->inspectorController().deviceEmulationEnabled() && mainFrameImpl()->frame()->document()) On 2014/05/30 14:13:53, apavlov wrote: > Not sure if it is worth it, but in our instrumentation code we use (or used to > use?) something similar to: > > if (page()->inspectorController().deviceEmulationEnabled()) { > if (Document* document = mainFrameImpl()->frame()->document()) > document->mediaQueryAffectingValueChanged(); > } +1. We don't have an official style rule around this, but I prefer this style. In the future if frame() or something gets a branch, then we don't unnecessarily do the branch twice.
Please take another look. Resize in device emulation mode means that screen (device) size may have changed, so we invalidate MQs on every resize, not just when turning emulation on/off. I managed to test this, but it uses |useUnfortunateSynchronousResizeMode| and requires small changes in content for that: https://codereview.chromium.org/311823002. I can't make it work with async resize, since we have a regular window, not a popup, and resize requests are ignored. https://codereview.chromium.org/308003007/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/308003007/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1623: if (page()->inspectorController().deviceEmulationEnabled() && mainFrameImpl()->frame()->document()) On 2014/05/30 14:13:53, apavlov wrote: > Not sure if it is worth it, but in our instrumentation code we use (or used to > use?) something similar to: > > if (page()->inspectorController().deviceEmulationEnabled()) { > if (Document* document = mainFrameImpl()->frame()->document()) > document->mediaQueryAffectingValueChanged(); > } Done.
lgtm with a comment https://codereview.chromium.org/308003007/diff/20001/LayoutTests/inspector/de... File LayoutTests/inspector/device-emulation/device-emulation-mq-on-resize.html (right): https://codereview.chromium.org/308003007/diff/20001/LayoutTests/inspector/de... LayoutTests/inspector/device-emulation/device-emulation-mq-on-resize.html:137: Tests that device width is updated when resizing. This is not what this test is about, is it? :)
ojan@, take another look please.
> Resize in device emulation mode means that screen (device) size may have changed, so we invalidate MQs on every resize, not just when turning emulation on/off. I still don't really understand this. Why is the device emulation mode special? Shouldn't the code in FrameView::performPreLayoutTasks do the mediaQueryAffectingValueChanged when the window changes size. I'm not saying this patch is wrong. You just need to explain to me how device emulation works so I can understand why this patch is right. :) I think the part I'm missing is the part you say in your description "Media query evaluator assumes that device size cannot change". Where does it make this assumption? > I managed to test this, but it uses |useUnfortunateSynchronousResizeMode| and requires small changes in content for that: https://codereview.chromium.org/311823002. > I can't make it work with async resize, since we have a regular window, not a popup, and resize requests are ignored. :( I'm not too familiar with this. I believe dglazkov added this. He may have ideas on how to avoid this and/or whether it's the only way to test this.
> I think the part I'm missing is the part you say in your description "Media
> query evaluator assumes that device size cannot change". Where does it make
this
> assumption?
The [max-]device-{width,height} properties are not considered viewport dependent
(see MediaQueryExp::isViewportDependent).
Thus, |mediaQueryAffectingValueChanged| does not invalidate them ever. While
emulation is turned on, device size is set equal to
the view size (this is not exactly the same as on real device, but good enough
for most cases). This means we need a way to
invalidate device-size MQs on resize in emulation mode.
I tried to fix this directly in MediaQueryExp::isViewportDependent, but I didn't
find an easy way to pass a flag there. Another problem
of this solution - it only works after invalidating those MQs once, since
features in question are already put in the wrong non-viewport-dependent
container.
ojan@, ptal. dglazkov@, what do you think about using |useUnfortunateSynchronousResizeMode| here? Is there another way to test window resize from layout tests?
https://codereview.chromium.org/308003007/diff/20001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/308003007/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1629: document->mediaQueryAffectingValueChanged(); I can't think of a better solution, so lgtm, but this needs a comment explaining why we need this, i.e. some version of the explanation you gave in your last comment.
On 2014/06/09 at 09:46:03, dgozman wrote: > ojan@, ptal. > > dglazkov@, what do you think about using |useUnfortunateSynchronousResizeMode| here? Is there another way to test window resize from layout tests? I don't see you using it in the patch. But if you do need it, it might be good to expose reading window rect values (as renderer understands them) off the Internals object?
> > dglazkov@, what do you think about using |useUnfortunateSynchronousResizeMode| > here? Is there another way to test window resize from layout tests? > > I don't see you using it in the patch. But if you do need it, it might be good > to expose reading window rect values (as renderer understands them) off the > Internals object? The test ensures that some on-resize logic triggers, and doesn't need to verify window rect values (there should be separate tests for that). I assume this is the only way.
Thank you for review. https://codereview.chromium.org/308003007/diff/20001/LayoutTests/inspector/de... File LayoutTests/inspector/device-emulation/device-emulation-mq-on-resize.html (right): https://codereview.chromium.org/308003007/diff/20001/LayoutTests/inspector/de... LayoutTests/inspector/device-emulation/device-emulation-mq-on-resize.html:137: Tests that device width is updated when resizing. On 2014/06/04 11:21:06, apavlov wrote: > This is not what this test is about, is it? :) Done. https://codereview.chromium.org/308003007/diff/20001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/308003007/diff/20001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1629: document->mediaQueryAffectingValueChanged(); On 2014/06/10 02:38:05, ojan wrote: > I can't think of a better solution, so lgtm, but this needs a comment explaining > why we need this, i.e. some version of the explanation you gave in your last > comment. Done.
lgtm https://codereview.chromium.org/308003007/diff/40001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/308003007/diff/40001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1627: // (see MediaQueryExp::isViewportDependent), and will not be invalidated in |FrameView::performPreLayoutTasks|. I would add something like... These values are not considered viewport-dependent since they are only viewport-dependent in emulation mode...
I'm going to commit this without test. Filed a bug to create async test for this: crbug.com/386142. https://codereview.chromium.org/308003007/diff/40001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/308003007/diff/40001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:1627: // (see MediaQueryExp::isViewportDependent), and will not be invalidated in |FrameView::performPreLayoutTasks|. On 2014/06/10 17:22:37, OOO-only-code-yellow-reviews wrote: > I would add something like... > > These values are not considered viewport-dependent since they are only > viewport-dependent in emulation mode... Done.
The CQ bit was checked by dgozman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/308003007/60001
Message was sent while issue was closed.
Change committed as 176420 |
