Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319863006:
Change image document zooming logic. (Closed)
DescriptionChange image document zooming logic.
The current zoom logic in image documents interacts badly with use-zoom-for-dsf,
in addition to having some weird behavior already.
This patch changes three things:
1. When loading a large image when page zoom was not 100% the image would start
at a strange scaling factor until it was clicked. This is fixed.
2. When page zoom is at 100% a large image will always scale to fit the window
regardless of the device scale factor. Previously with use-zoom-for-dsf on it
would scale too large for the window.
3. When an image that fits in the window is zoomed in until it doesn't it is no
longer clickable. Previously it would start acting like a large image, which
was strange.
Also added a test suite for ImageDocument.
BUG=324086, 644059
Committed: https://crrev.com/f5ff44ae79899d114b4bbf5abdd6e960f856c0d9
Cr-Commit-Position: refs/heads/master@{#419867}
Patch Set 1 #Patch Set 2 : rewrite fix #Patch Set 3 : formatting #Patch Set 4 : add tests #
Total comments: 6
Patch Set 5 : bruce comment #Patch Set 6 : enforce Desktop mode #
Total comments: 7
Patch Set 7 : remove raw pointer for compile #Patch Set 8 : use parser in test #Patch Set 9 : use implicitOpen #Patch Set 10 : rebaseline #
Total comments: 5
Patch Set 11 : minor edits #
Messages
Total messages: 38 (20 generated)
Description was changed from ========== Fix zooming large image files. This patch fixes two issues: 1. With use-zoom-for-dsf enabled images were not scaling down to the correct size because the scaling factor was being calculated in the wrong coordinate system. 2. When page zoom was >100% the image would start at 1:1 pix:dip scaling (not fully zoomed in or out) until you clicked the image. It seems like this was intentional but I can't tell why and everything works if I remove this logic. BUG=324086,644059 ========== to ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. BUG=324086,644059 ==========
Description was changed from ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. BUG=324086,644059 ========== to ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. Also added a test suite for ImageDocument. BUG=324086,644059 ==========
bsep@chromium.org changed reviewers: + japhet@chromium.org
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
I am not an owner but lgtm https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.cpp:256: return 1; Would be nice to change this to return 1.0f, and maybe add a comment explaining what this function is supposed to do, especially now that its semantics are more subtle than before. https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.h:59: // These methods are for m_shrinkToFitMode == Desktop. Is this comment accurate?
https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.cpp:256: return 1; On 2016/09/15 23:21:21, brucedawson wrote: > Would be nice to change this to return 1.0f, and maybe add a comment explaining > what this function is supposed to do, especially now that its semantics are more > subtle than before. Done. Added the comment to the header file. https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.h:59: // These methods are for m_shrinkToFitMode == Desktop. On 2016/09/15 23:21:21, brucedawson wrote: > Is this comment accurate? Yes. It's not enforced, but windowSizeChanged won't call them if the mode is Viewport.
https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.h:59: // These methods are for m_shrinkToFitMode == Desktop. On 2016/09/16 00:21:41, Bret Sepulveda wrote: > On 2016/09/15 23:21:21, brucedawson wrote: > > Is this comment accurate? > > Yes. It's not enforced, but windowSizeChanged won't call them if the mode is > Viewport. Consider making it an enforced comment (i.e.; a DCHECK), if that seems reasonable.
https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.h:59: // These methods are for m_shrinkToFitMode == Desktop. On 2016/09/16 00:23:01, brucedawson wrote: > On 2016/09/16 00:21:41, Bret Sepulveda wrote: > > On 2016/09/15 23:21:21, brucedawson wrote: > > > Is this comment accurate? > > > > Yes. It's not enforced, but windowSizeChanged won't call them if the mode is > > Viewport. > > Consider making it an enforced comment (i.e.; a DCHECK), if that seems > reasonable. Done.
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This l-g-t-m % nits but I don't speak scaling well enough to be super confident. bokan, are you a good sanity check for this scaling logic? https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.h:85: friend class ImageDocumentTest; I don't think is necessary? The only private member I see ImageDocumentTest accessing is m_imageElement, for which an accessor exists. https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/DummyPageHolder.cpp (right): https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/DummyPageHolder.cpp:81: m_frame = LocalFrame::create(m_frameLoaderClient.get(), &m_page->frameHost(), nullptr); While this is a reasonable change, it seems completely unrelated to this patch?
Also, compile failure looks legit.
bsep@chromium.org changed reviewers: + bokan@chromium.org
Adding bokan@ to reviewers. Also included a fix for the compile failure. https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.h:85: friend class ImageDocumentTest; On 2016/09/16 21:48:24, Nate Chapin wrote: > I don't think is necessary? The only private member I see ImageDocumentTest > accessing is m_imageElement, for which an accessor exists. Yes, but ImageDocumentTest is setting m_imageElement as part of the test (not just reading it). Since that's violating class encapsulation I'd rather add the test as a friend than add a setter. https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/DummyPageHolder.cpp (right): https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/DummyPageHolder.cpp:81: m_frame = LocalFrame::create(m_frameLoaderClient.get(), &m_page->frameHost(), nullptr); On 2016/09/16 21:48:24, Nate Chapin wrote: > While this is a reasonable change, it seems completely unrelated to this patch? Oh yeah oops, I thought I changed more in DummyPageHolder. I can revert it if you want but I think it's okay to sneak in.
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.h:85: friend class ImageDocumentTest; On 2016/09/16 22:24:24, Bret Sepulveda wrote: > On 2016/09/16 21:48:24, Nate Chapin wrote: > > I don't think is necessary? The only private member I see ImageDocumentTest > > accessing is m_imageElement, for which an accessor exists. > > Yes, but ImageDocumentTest is setting m_imageElement as part of the test (not > just reading it). Since that's violating class encapsulation I'd rather add the > test as a friend than add a setter. Hrm, I can live with this, but I'd like to be certain the alternatives are way worse. Is there a way to make loadImage() do something a little closer to the standard way of loading an ImageDocument, so that we can just use the public functions?
https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.h:85: friend class ImageDocumentTest; On 2016/09/16 22:28:12, Nate Chapin wrote: > On 2016/09/16 22:24:24, Bret Sepulveda wrote: > > On 2016/09/16 21:48:24, Nate Chapin wrote: > > > I don't think is necessary? The only private member I see ImageDocumentTest > > > accessing is m_imageElement, for which an accessor exists. > > > > Yes, but ImageDocumentTest is setting m_imageElement as part of the test (not > > just reading it). Since that's violating class encapsulation I'd rather add > the > > test as a friend than add a setter. > > Hrm, I can live with this, but I'd like to be certain the alternatives are way > worse. Is there a way to make loadImage() do something a little closer to the > standard way of loading an ImageDocument, so that we can just use the public > functions? I took a second look and found I can use ImageDocumentParser instead, which is a less flagrant violation of encapsulation but the "friend class ImageDocumentTest" is still necessary because createParser() is private.
On 2016/09/16 23:00:01, Bret Sepulveda wrote: > https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/ImageDocument.h (right): > > https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/ImageDocument.h:85: friend class > ImageDocumentTest; > On 2016/09/16 22:28:12, Nate Chapin wrote: > > On 2016/09/16 22:24:24, Bret Sepulveda wrote: > > > On 2016/09/16 21:48:24, Nate Chapin wrote: > > > > I don't think is necessary? The only private member I see > ImageDocumentTest > > > > accessing is m_imageElement, for which an accessor exists. > > > > > > Yes, but ImageDocumentTest is setting m_imageElement as part of the test > (not > > > just reading it). Since that's violating class encapsulation I'd rather add > > the > > > test as a friend than add a setter. > > > > Hrm, I can live with this, but I'd like to be certain the alternatives are way > > worse. Is there a way to make loadImage() do something a little closer to the > > standard way of loading an ImageDocument, so that we can just use the public > > functions? > > I took a second look and found I can use ImageDocumentParser instead, which is a > less flagrant violation of encapsulation but the "friend class > ImageDocumentTest" is still necessary because createParser() is private. Document::createParser() is public, isn't it? Failing that, call Document::implicitOpen()?
https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2319863006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.h:85: friend class ImageDocumentTest; On 2016/09/16 23:00:01, Bret Sepulveda wrote: > On 2016/09/16 22:28:12, Nate Chapin wrote: > > On 2016/09/16 22:24:24, Bret Sepulveda wrote: > > > On 2016/09/16 21:48:24, Nate Chapin wrote: > > > > I don't think is necessary? The only private member I see > ImageDocumentTest > > > > accessing is m_imageElement, for which an accessor exists. > > > > > > Yes, but ImageDocumentTest is setting m_imageElement as part of the test > (not > > > just reading it). Since that's violating class encapsulation I'd rather add > > the > > > test as a friend than add a setter. > > > > Hrm, I can live with this, but I'd like to be certain the alternatives are way > > worse. Is there a way to make loadImage() do something a little closer to the > > standard way of loading an ImageDocument, so that we can just use the public > > functions? > > I took a second look and found I can use ImageDocumentParser instead, which is a > less flagrant violation of encapsulation but the "friend class > ImageDocumentTest" is still necessary because createParser() is private. Oh hey, that works.
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not familiar with this code but I'm a good double check for scaling/coordinate issues. Code is lgtm. Please wrap the change description at 80 cols. https://codereview.chromium.org/2319863006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocumentTest.cpp (right): https://codereview.chromium.org/2319863006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:60: float windowToViewportScalar(const float s) const override { return m_scaleFactor; } Nit: remove name from parameter. https://codereview.chromium.org/2319863006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:166: EXPECT_EQ(10, imageWidth()); Just to confirm my understanding, in this case the image will load larger than the viewport, right? Since the viewport height is 10 physical pixels, the image height is 10 CSS pixels and the zoom factor is 2 so the image will be 20 physical pixels. Is that right? Is that expected?
https://codereview.chromium.org/2319863006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocumentTest.cpp (right): https://codereview.chromium.org/2319863006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:60: float windowToViewportScalar(const float s) const override { return m_scaleFactor; } On 2016/09/19 15:31:06, bokan wrote: > Nit: remove name from parameter. Oops, that's supposed to be "return s * m_scaleFactor" but it's only called with s==1 in this suite. Fixed. https://codereview.chromium.org/2319863006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:166: EXPECT_EQ(10, imageWidth()); On 2016/09/19 15:31:06, bokan wrote: > Just to confirm my understanding, in this case the image will load larger than > the viewport, right? Since the viewport height is 10 physical pixels, the image > height is 10 CSS pixels and the zoom factor is 2 so the image will be 20 > physical pixels. Is that right? Is that expected? Yes, that's correct. If you zoom in on an image it should get bigger even if it's being fit to the screen. This is just applying that principle on an initial load too. I suppose it could be a little strange if someone has a permanent zoom set (or just forgets they zoomed in that image previously on load) but I'm not sure what else we would do. Plus it's definitely better that the current behavior. Changed the name of the test to be more specific.
Description was changed from ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. Also added a test suite for ImageDocument. BUG=324086,644059 ========== to ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. Also added a test suite for ImageDocument. BUG=324086,644059 ==========
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2319863006/#ps200001 (title: "minor edits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. Also added a test suite for ImageDocument. BUG=324086,644059 ========== to ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. Also added a test suite for ImageDocument. BUG=324086,644059 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. Also added a test suite for ImageDocument. BUG=324086,644059 ========== to ========== Change image document zooming logic. The current zoom logic in image documents interacts badly with use-zoom-for-dsf, in addition to having some weird behavior already. This patch changes three things: 1. When loading a large image when page zoom was not 100% the image would start at a strange scaling factor until it was clicked. This is fixed. 2. When page zoom is at 100% a large image will always scale to fit the window regardless of the device scale factor. Previously with use-zoom-for-dsf on it would scale too large for the window. 3. When an image that fits in the window is zoomed in until it doesn't it is no longer clickable. Previously it would start acting like a large image, which was strange. Also added a test suite for ImageDocument. BUG=324086,644059 Committed: https://crrev.com/f5ff44ae79899d114b4bbf5abdd6e960f856c0d9 Cr-Commit-Position: refs/heads/master@{#419867} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f5ff44ae79899d114b4bbf5abdd6e960f856c0d9 Cr-Commit-Position: refs/heads/master@{#419867}
Message was sent while issue was closed.
https://codereview.chromium.org/2319863006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocumentTest.cpp (right): https://codereview.chromium.org/2319863006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:166: EXPECT_EQ(10, imageWidth()); On 2016/09/20 20:20:54, Bret Sepulveda wrote: > On 2016/09/19 15:31:06, bokan wrote: > > Just to confirm my understanding, in this case the image will load larger than > > the viewport, right? Since the viewport height is 10 physical pixels, the > image > > height is 10 CSS pixels and the zoom factor is 2 so the image will be 20 > > physical pixels. Is that right? Is that expected? > > Yes, that's correct. If you zoom in on an image it should get bigger even if > it's being fit to the screen. This is just applying that principle on an initial > load too. I suppose it could be a little strange if someone has a permanent zoom > set (or just forgets they zoomed in that image previously on load) but I'm not > sure what else we would do. Plus it's definitely better that the current > behavior. > > Changed the name of the test to be more specific. Sgtm, thanks for explaining. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
