64290 2011-07-11 09:42:23 -0700 The layout of RenderBox should be calculated using fixedLayoutSize instead of viewport when the fixedLayoutSize is set. 2011-12-01 13:41:50 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified UNCONFIRMED P2 Normal --- 70559 0 joybro201 webkit-unassigned ahf eric fsamuel hyatt kenneth manyoso simon.fraser tonikitoo oldest_to_newest 434937 0 joybro201 2011-07-11 09:42:23 -0700 The layout of an RenderBox should be calculated using fixedLayoutSize instead of viewport when the fixedLayoutSize is set. 434942 1 100314 joybro201 2011-07-11 09:51:56 -0700 Created attachment 100314 Patch 452417 2 eric 2011-08-17 11:35:04 -0700 This would be a simple patch for simon or hyatt to review. 452419 3 100314 eric 2011-08-17 11:36:09 -0700 Comment on attachment 100314 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100314&action=review > Source/WebCore/ChangeLog:18 > + No tests because this change affects only when fixedLayoutSize is set. Since our dumpRenderTree doesn't set it, we can't test this. DumpRenderTree has a layoutConstroller.overridePreference() which can be used to toggle a setting for a single test. Seems if your DRT supports that (which ideally it should), it would be easy to write a test for this. 452421 4 100314 simon.fraser 2011-08-17 11:36:52 -0700 Comment on attachment 100314 Patch r- for lack of test. 454403 5 joybro201 2011-08-21 08:30:05 -0700 (In reply to comment #3) > (From update of attachment 100314 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100314&action=review > > > Source/WebCore/ChangeLog:18 > > + No tests because this change affects only when fixedLayoutSize is set. Since our dumpRenderTree doesn't set it, we can't test this. > > DumpRenderTree has a layoutConstroller.overridePreference() which can be used to toggle a setting for a single test. Seems if your DRT supports that (which ideally it should), it would be easy to write a test for this. No. layoutController.overridePreference() is not helpful because it toggles only settings of WebSettings. To turn on the fixedLayoutSize feature, to test this patch, FrameView::setUseFixedLayout(true) have to be called. I think there is no easy way to turn on this feature for a single test. 454406 6 tonikitoo 2011-08-21 08:55:53 -0700 > No. layoutController.overridePreference() is not helpful because it toggles only settings of WebSettings. > > To turn on the fixedLayoutSize feature, to test this patch, FrameView::setUseFixedLayout(true) have to be called. > > I think there is no easy way to turn on this feature for a single test. It raises an interesting point. I do not think it should be necessary to set the fixedLayoutSize and set useFixedLayoutSize to TRUE. Maybe once it is set to something different than 0x0, it should be used. There could be a logicalLayoutSize in ScrollView that abstracts this for the call sites. IntSize ScrollView::logicalLayoutSize() const { if (m_fixedLayoutSize.isValid()) // != 0x0, and has non zero values return m_fixedLayoutSize; return viewportSize; } 455451 7 joybro201 2011-08-23 09:34:26 -0700 (In reply to comment #6) > > No. layoutController.overridePreference() is not helpful because it toggles only settings of WebSettings. > > > > To turn on the fixedLayoutSize feature, to test this patch, FrameView::setUseFixedLayout(true) have to be called. > > > > I think there is no easy way to turn on this feature for a single test. > > It raises an interesting point. I do not think it should be necessary to set the fixedLayoutSize and set useFixedLayoutSize to TRUE. > > Maybe once it is set to something different than 0x0, it should be used. > > There could be a logicalLayoutSize in ScrollView that abstracts this for the call sites. > > IntSize ScrollView::logicalLayoutSize() const > { > if (m_fixedLayoutSize.isValid()) // != 0x0, and has non zero values > return m_fixedLayoutSize; > > return viewportSize; > } Here is the implementation of the layoutWidth(). int ScrollView::layoutWidth() const { return m_fixedLayoutSize.isEmpty() || !m_useFixedLayout ? visibleWidth() : m_fixedLayoutSize.width(); } As you can see, m_fixedLayoutSize is not returned, even if it isn't empty, when m_useFixedLayout is not TRUE. Unfortunately there is also no function like you said in ScrollView. For now, we have to set both fixedLayoutSize and useFixedLayoutSize. 455480 8 kenneth 2011-08-23 10:29:38 -0700 > It raises an interesting point. I do not think it should be necessary to set the fixedLayoutSize and set useFixedLayoutSize to TRUE. Alex Faeroy has a patch up for review that removes the need for useFixedLayoutSize. 456019 9 joybro201 2011-08-24 01:56:33 -0700 (In reply to comment #8) > > It raises an interesting point. I do not think it should be necessary to set the fixedLayoutSize and set useFixedLayoutSize to TRUE. > > Alex Faeroy has a patch up for review that removes the need for useFixedLayoutSize. Good. Can you point me to the patch? If it is true, I should wait for the patch to land. 467322 10 tonikitoo 2011-09-14 14:05:36 -0700 (In reply to comment #8) > > It raises an interesting point. I do not think it should be necessary to set the fixedLayoutSize and set useFixedLayoutSize to TRUE. > > Alex Faeroy has a patch up for review that removes the need for useFixedLayoutSize. Kenneth, Alex, will this patch be submitted? Otherwise I can submit it myself? 467366 11 kenneth 2011-09-14 14:56:05 -0700 (In reply to comment #10) > (In reply to comment #8) > > > It raises an interesting point. I do not think it should be necessary to set the fixedLayoutSize and set useFixedLayoutSize to TRUE. > > > > Alex Faeroy has a patch up for review that removes the need for useFixedLayoutSize. > > Kenneth, Alex, will this patch be submitted? Otherwise I can submit it myself? I can talk to Alex tomorrow; he is ahf on the #qtwebkit channel. I'm afraid that he is winded up in something pretty critical though. Did you have the bug id for the patch he was working on? 513058 12 100314 fsamuel 2011-12-01 13:41:50 -0800 Comment on attachment 100314 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100314&action=review >>> Source/WebCore/ChangeLog:18 >>> + No tests because this change affects only when fixedLayoutSize is set. Since our dumpRenderTree doesn't set it, we can't test this. >> >> DumpRenderTree has a layoutConstroller.overridePreference() which can be used to toggle a setting for a single test. Seems if your DRT supports that (which ideally it should), it would be easy to write a test for this. > > No. layoutController.overridePreference() is not helpful because it toggles only settings of WebSettings. > > To turn on the fixedLayoutSize feature, to test this patch, FrameView::setUseFixedLayout(true) have to be called. > > I think there is no easy way to turn on this feature for a single test. It just so happens that I have two layout tests that address precisely this problem and I added fixedLayout to the chromium DRT (I'm not sure if it's in other DRTs at the moment). I will move this to window.internals, I guess. 100314 2011-07-11 09:51:56 -0700 2011-12-01 13:41:50 -0800 Patch bug-64290-20110712015156.patch text/plain 3200 joybro201 U3VidmVyc2lvbiBSZXZpc2lvbjogOTA1NjAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA2YWY5OTFjNTMwNjczNWYw NGZhMTJiOTM3MDM3YTkyZGQyZjc0ZmI5Li4yMTEyMmIxYjg3YmU1MmMyOGE3ODBjMGRkNTZhYmYw Mjk0YWM1NGM3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjYgQEAKKzIwMTEtMDctMTEgIFlvdW5n IEhhbiBMZWUgIDxqb3licm9AY29tcGFueTEwMC5uZXQ+CisKKyAgICAgICAgVGhlIGxheW91dCBv ZiBSZW5kZXJCb3ggc2hvdWxkIGJlIGNhbGN1bGF0ZWQgdXNpbmcgZml4ZWRMYXlvdXRTaXplIGlu c3RlYWQgb2Ygdmlld3BvcnQgd2hlbiB0aGUgZml4ZWRMYXlvdXRTaXplIGlzIHNldC4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY0MjkwCisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gY2hhbmdlc2V0IDM5 NTgwLCBhbGwgdmlzaWJsZVdpZHRoIGFuZCB2aXNpYmxlSGVpZ2h0IGluIHJlbmRlcmluZyByb3V0 aW5lIGFyZSByZXBsYWNlZAorICAgICAgICB3aXRoIGxheW91dFdpZHRoIGFuZCBsYXlvdXRIZWln aHQgcmVzcGVjdGl2ZWx5IGZvciBmaXhlZExheW91dFNpemUgZmVhdHVyZS4gQnV0IGl0IG1pc3Nl ZAorICAgICAgICBzb21lIGluIFJlbmRlckJveCwgYW5kIHRoZSBtaXNzaW5nIHZpc2libGVXaWR0 aCBhbmQgdmlzaWJsZUhlaWdodCBhcmUgY2F1c2luZyBtaXMtcmVuZGVyaW5nCisgICAgICAgIHdo ZW4gdGhlIGZpeGVkTGF5b3V0U2l6ZSBpcyBzZXQgYW5kIGl0IGlzIGRpZmZlcmVudCBmcm9tIHRo ZSB2aWV3cG9ydFNpemUgb2YgU2Nyb2xsVmlldy4KKyAgICAgICAgVGhpcyBjaGFuZ2UgcmVwbGFj ZXMgdGhlIG1pc3NpbmcgdmlzaWJsZVdpZHRoLCB2aXNpYmxlSGVpZ2h0IHdpdGggbGF5b3V0V2lk dGgsIGxheW91dEhlaWdodAorICAgICAgICByZXNwZWN0aXZlbHkuCisKKyAgICAgICAgUmVsYXRl ZCBjaGFuZ2VzZXQ6CisgICAgICAgIGh0dHA6Ly90cmFjLndlYmtpdC5vcmcvY2hhbmdlc2V0LzM5 NTgwCisKKyAgICAgICAgTm8gdGVzdHMgYmVjYXVzZSB0aGlzIGNoYW5nZSBhZmZlY3RzIG9ubHkg d2hlbiBmaXhlZExheW91dFNpemUgaXMgc2V0LiBTaW5jZSBvdXIgZHVtcFJlbmRlclRyZWUgZG9l c24ndCBzZXQgaXQsIHdlIGNhbid0IHRlc3QgdGhpcy4KKworICAgICAgICAqIHJlbmRlcmluZy9S ZW5kZXJCb3guY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyQm94OjpwZXJwZW5kaWN1bGFy Q29udGFpbmluZ0Jsb2NrTG9naWNhbEhlaWdodCk6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJC b3g6OmF2YWlsYWJsZUxvZ2ljYWxIZWlnaHRVc2luZyk6CisKIDIwMTEtMDctMDcgIE5pa29sYXMg WmltbWVybWFubiAgPG56aW1tZXJtYW5uQHJpbS5jb20+CiAKICAgICAgICAgTW92ZSByZW1haW5p bmcgZW51bXMgb3V0IG9mIFNWRypFbGVtZW50IGNsYXNzZXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCb3guY3BwIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5n L1JlbmRlckJveC5jcHAKaW5kZXggZjI5OTU2YzMwMWFlMDYyNzUyY2VhZGNiYmZjMTUzNzIzNWNk NzRkZC4uYTUwYTY0MGUxYThhOTZkMmIyNzQ5NWEzOGVjZjk2MTEzYTg1YjkzNyAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckJveC5jcHAKKysrIGIvU291cmNlL1dl YkNvcmUvcmVuZGVyaW5nL1JlbmRlckJveC5jcHAKQEAgLTExOTgsNyArMTE5OCw3IEBAIGludCBS ZW5kZXJCb3g6OnBlcnBlbmRpY3VsYXJDb250YWluaW5nQmxvY2tMb2dpY2FsSGVpZ2h0KCkgY29u c3QKICAgICAgICAgLy8gUmF0aGVyIHRoYW4gbWFraW5nIHRoZSBjaGlsZCBiZSBjb21wbGV0ZWx5 IHVuY29uc3RyYWluZWQsIFdpbklFIHVzZXMgdGhlIHZpZXdwb3J0IHdpZHRoIGFuZCBoZWlnaHQK ICAgICAgICAgLy8gYXMgYSBjb25zdHJhaW50LiAgV2UgZG8gdGhhdCBmb3Igbm93IGFzIHdlbGwg ZXZlbiB0aG91Z2ggaXQncyBsaWtlbHkgYmVpbmcgdW5jb25zdHJhaW5lZCBpcyB3aGF0IHRoZSBz cGVjCiAgICAgICAgIC8vIHdpbGwgZGVjaWRlLgotICAgICAgICByZXR1cm4gY29udGFpbmluZ0Js b2NrU3R5bGUtPmlzSG9yaXpvbnRhbFdyaXRpbmdNb2RlKCkgPyB2aWV3KCktPmZyYW1lVmlldygp LT52aXNpYmxlSGVpZ2h0KCkgOiB2aWV3KCktPmZyYW1lVmlldygpLT52aXNpYmxlV2lkdGgoKTsK KyAgICAgICAgcmV0dXJuIGNvbnRhaW5pbmdCbG9ja1N0eWxlLT5pc0hvcml6b250YWxXcml0aW5n TW9kZSgpID8gdmlldygpLT5mcmFtZVZpZXcoKS0+bGF5b3V0SGVpZ2h0KCkgOiB2aWV3KCktPmZy YW1lVmlldygpLT5sYXlvdXRXaWR0aCgpOwogICAgIH0KICAgICAKICAgICAvLyBVc2UgdGhlIGNv bnRlbnQgYm94IGxvZ2ljYWwgaGVpZ2h0IGFzIHNwZWNpZmllZCBieSB0aGUgc3R5bGUuCkBAIC0y MDQ2LDcgKzIwNDYsNyBAQCBMYXlvdXRVbml0IFJlbmRlckJveDo6YXZhaWxhYmxlTG9naWNhbEhl aWdodFVzaW5nKGNvbnN0IExlbmd0aCYgaCkgY29uc3QKICAgICAgICAgcmV0dXJuIGNvbXB1dGVD b250ZW50Qm94TG9naWNhbEhlaWdodChoLnZhbHVlKCkpOwogCiAgICAgaWYgKGlzUmVuZGVyVmll dygpKQotICAgICAgICByZXR1cm4gaXNIb3Jpem9udGFsV3JpdGluZ01vZGUoKSA/IHRvUmVuZGVy Vmlldyh0aGlzKS0+ZnJhbWVWaWV3KCktPnZpc2libGVIZWlnaHQoKSA6IHRvUmVuZGVyVmlldyh0 aGlzKS0+ZnJhbWVWaWV3KCktPnZpc2libGVXaWR0aCgpOworICAgICAgICByZXR1cm4gaXNIb3Jp em9udGFsV3JpdGluZ01vZGUoKSA/IHRvUmVuZGVyVmlldyh0aGlzKS0+ZnJhbWVWaWV3KCktPmxh eW91dEhlaWdodCgpIDogdG9SZW5kZXJWaWV3KHRoaXMpLT5mcmFtZVZpZXcoKS0+bGF5b3V0V2lk dGgoKTsKIAogICAgIC8vIFdlIG5lZWQgdG8gc3RvcCBoZXJlLCBzaW5jZSB3ZSBkb24ndCB3YW50 IHRvIGluY3JlYXNlIHRoZSBoZWlnaHQgb2YgdGhlIHRhYmxlCiAgICAgLy8gYXJ0aWZpY2lhbGx5 LiAgV2UncmUgZ29pbmcgdG8gcmVseSBvbiB0aGlzIGNlbGwgZ2V0dGluZyBleHBhbmRlZCB0byBz b21lIG5ldwo=