Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1930363002:
Avoid clobbering state in viewport tag initialization. (Closed)
|
Created:
4 years, 7 months ago by aelias_OOO_until_Jul13 Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid clobbering state in viewport tag initialization.
The overriding logic for meta viewport and @viewport tags worked by clobbering
mutable state in a nonreversible way. This doesn't work properly when meta viewport
support is turned on and off dynamically, which happens in devtools
device emulation. Switch the logic to store the different values
separately and compute the final result at time of use.
BUG=604619
Committed: https://crrev.com/899f1773bcbe08bce6c3e0e1238cae1b073b440a
Cr-Commit-Position: refs/heads/master@{#392554}
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 4
Patch Set 3 : Code review comments #
Messages
Total messages: 21 (12 generated)
Description was changed from ========== Avoid clobbering state in viewport tag initialization. BUG=604619 ========== to ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state, and doesn't work probably when meta viewport support setting is turned on and off dynamically, which happens particularly in devtools device emulation scenarios. Switch the logic to store legacy and not separately and compute the final result at time of use. BUG=604619 ==========
Description was changed from ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state, and doesn't work probably when meta viewport support setting is turned on and off dynamically, which happens particularly in devtools device emulation scenarios. Switch the logic to store legacy and not separately and compute the final result at time of use. BUG=604619 ========== to ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state, and doesn't work probably when meta viewport support setting is turned on and off dynamically, which happens particularly in devtools device emulation scenarios. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ==========
Description was changed from ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state, and doesn't work probably when meta viewport support setting is turned on and off dynamically, which happens particularly in devtools device emulation scenarios. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ========== to ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens particularly in devtools device emulation scenarios. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ==========
Description was changed from ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens particularly in devtools device emulation scenarios. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ========== to ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens particularly in devtools device emulation scenarios. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ==========
Description was changed from ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens particularly in devtools device emulation scenarios. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ========== to ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens in devtools device emulation scenarios. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ==========
Description was changed from ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens in devtools device emulation scenarios. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ========== to ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens in devtools device emulation. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ==========
aelias@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
Hi, dgozman@ for general review, pfeldman@ for core/ OWNERS.
On 2016/05/06 14:06:06, aelias_OOO_until_may9 wrote: > Hi, dgozman@ for general review, pfeldman@ for core/ OWNERS. dgozman@ owns core/ now, deferring all to him.
Thank you for the fix! lgtm https://codereview.chromium.org/1930363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1930363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:3160: if (viewportDescription == m_viewportDescription) This check looks strange now (and was before). Why do we only compare with |m_viewportDescription| if we can set |m_legacyViewportDescription| instead? I think it's safe to remove it. https://codereview.chromium.org/1930363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:3165: m_viewportDefaultMinWidth = viewportDescription.minWidth; Should we do something similar to |m_viewportDefaultMinWidth|? It could possibly mess up "request desktop" scenario, couldn't it?
https://codereview.chromium.org/1930363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1930363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:3160: if (viewportDescription == m_viewportDescription) On 2016/05/09 at 17:36:19, dgozman wrote: > This check looks strange now (and was before). Why do we only compare with |m_viewportDescription| if we can set |m_legacyViewportDescription| instead? I think it's safe to remove it. Agreed it's weird now. I'd like to preserve a safeguard against unnecessary calls to updateViewportDescription() though which could do expensive stuff like layouts, so I split this into two early-returns, one for legacy and one for not. https://codereview.chromium.org/1930363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:3165: m_viewportDefaultMinWidth = viewportDescription.minWidth; On 2016/05/09 at 17:36:19, dgozman wrote: > Should we do something similar to |m_viewportDefaultMinWidth|? It could possibly mess up "request desktop" scenario, couldn't it? I don't think there's a risk of bugs with this one since it's basically just a fixed constant of 980 in all cases, but anyway I moved it within the non-legacy block which is the only place where it may appear.
The CQ bit was checked by aelias@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1930363002/#ps40001 (title: "Code review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930363002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930363002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by aelias@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930363002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930363002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens in devtools device emulation. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 ========== to ========== Avoid clobbering state in viewport tag initialization. The overriding logic for meta viewport and @viewport tags worked by clobbering mutable state in a nonreversible way. This doesn't work properly when meta viewport support is turned on and off dynamically, which happens in devtools device emulation. Switch the logic to store the different values separately and compute the final result at time of use. BUG=604619 Committed: https://crrev.com/899f1773bcbe08bce6c3e0e1238cae1b073b440a Cr-Commit-Position: refs/heads/master@{#392554} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/899f1773bcbe08bce6c3e0e1238cae1b073b440a Cr-Commit-Position: refs/heads/master@{#392554} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
