Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2523553002:
Consolidate how ImageDocument sets image styling (Closed)
DescriptionConsolidate how ImageDocument sets image styling
* Consolidate how the image sets the cursor styling inside of
updateImageStyle(), which now checks for whether the cursor
or checkerboard size has changed during styling updates.
* Fix how the magnifying glass cursor is initialized on desktop
versions of the page.
BUG=664782
Committed: https://crrev.com/893f3dcf906b25e13e9d4951a52aa30843875ef5
Cr-Commit-Position: refs/heads/master@{#434020}
Patch Set 1 #Patch Set 2 : DCHECK_EQ #
Total comments: 2
Patch Set 3 : Consolidate #
Total comments: 4
Patch Set 4 : Remove space #Patch Set 5 : Remove extra enum state #Patch Set 6 : Rebaseline test expectation #Patch Set 7 : Rebaseing again #
Messages
Total messages: 44 (26 generated)
The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run
dfalcantara@chromium.org changed reviewers: + pdr@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2523553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2523553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.cpp:404: imageStyle.append(m_shouldShrinkImage ? "cursor: zoom-in; " I see we're using "m_imageElement->setInlineStyleProperty(CSSPropertyCursor, CSSValueZoomOut);" to do this elsewhere. Is the style code in this function overwriting that? I wonder if we should just have this function be in charge of managing the image style (i.e., have the other callers of m_imageElement->setInlineStyleProperty call updateImageStyle())?
https://codereview.chromium.org/2523553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2523553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.cpp:404: imageStyle.append(m_shouldShrinkImage ? "cursor: zoom-in; " On 2016/11/21 23:26:47, pdr. wrote: > I see we're using "m_imageElement->setInlineStyleProperty(CSSPropertyCursor, > CSSValueZoomOut);" to do this elsewhere. Is the style code in this function > overwriting that? I wonder if we should just have this function be in charge of > managing the image style (i.e., have the other callers of > m_imageElement->setInlineStyleProperty call updateImageStyle())? Yeah, on desktop this is called during ::imageLoaded(), after the cursor is initially set via setInlineStyleProperty(), effectively erasing it. This function isn't called anywhere else after initialization, though, and I'm not sure if it's cheaper overall to edit the whole style tag than to just remove/change the single style property explicitly when we know it needs to be updated. It might make sense to just pull out the cursor-setting code specifically, then have everything call that. WDYT?
On 2016/11/21 at 23:43:26, dfalcantara wrote: > https://codereview.chromium.org/2523553002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): > > https://codereview.chromium.org/2523553002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/ImageDocument.cpp:404: imageStyle.append(m_shouldShrinkImage ? "cursor: zoom-in; " > On 2016/11/21 23:26:47, pdr. wrote: > > I see we're using "m_imageElement->setInlineStyleProperty(CSSPropertyCursor, > > CSSValueZoomOut);" to do this elsewhere. Is the style code in this function > > overwriting that? I wonder if we should just have this function be in charge of > > managing the image style (i.e., have the other callers of > > m_imageElement->setInlineStyleProperty call updateImageStyle())? > > Yeah, on desktop this is called during ::imageLoaded(), after the cursor is initially set via setInlineStyleProperty(), effectively erasing it. This function isn't called anywhere else after initialization, though, and I'm not sure if it's cheaper overall to edit the whole style tag than to just remove/change the single style property explicitly when we know it needs to be updated. It might make sense to just pull out the cursor-setting code specifically, then have everything call that. WDYT? I'm not sure we could extract out the cursor code into a helper because it's difficult to mix code that uses setInlineStyleProperty and code that uses setAttribute('style', ...). Would the cost of calling updateImageStyle in imageLoaded be any more than creating/parsing the inline style string? If not, I'd sort of lean towards that just to keep the style code in one place.
Description was changed from ========== ImageDocument cursor fix Fix how the magnifying glass cursor is initialized on desktop versions of the page. BUG=664782 ========== to ========== Consolidate how ImageDocument sets image styling * Consolidate how the image sets the cursor styling inside of updateImageStyle(), which now checks for whether the cursor or checkerboard size has changed during styling updates. * Fix how the magnifying glass cursor is initialized on desktop versions of the page. BUG=664782 ==========
What do you think about this approach? I was trying to keep it small to cherry-pick back to M56, but we're still fairly close to branch that I think we'll be fine.
The CQ bit was checked by dfalcantara@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...
Looks great! Because we're not adding tests and plan to merge this, can you run through your testcases with this patch and double-check that everything works (e.g., during loading, etc)? https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.cpp:445: imageStyle.append("cursor: zoom-in; "); nit: is the trailing space needed? https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.h:93: enum MouseCursorMode { Uninitialized, Default, ZoomIn, ZoomOut }; Is the uninitialized case needed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Everything seems fine on Android and Linux using the old test cases during loading and after window resizing; not sure what else to check but it _seems_ OK. https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.cpp:445: imageStyle.append("cursor: zoom-in; "); On 2016/11/22 04:15:12, pdr. wrote: > nit: is the trailing space needed? Nope, removed. https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/ImageDocument.h:93: enum MouseCursorMode { Uninitialized, Default, ZoomIn, ZoomOut }; On 2016/11/22 04:15:12, pdr. wrote: > Is the uninitialized case needed? Yeah, it's needed to get past the difference check in ImageDocument.cpp:420 for initialization. Alternatively, we could store an initialization boolean, or check if the style string is empty. Not sure what's the most intuitive.
On 2016/11/22 at 18:51:29, dfalcantara wrote: > Everything seems fine on Android and Linux using the old test cases during loading and after window resizing; not sure what else to check but it _seems_ OK. Thanks! > > https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): > > https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/ImageDocument.cpp:445: imageStyle.append("cursor: zoom-in; "); > On 2016/11/22 04:15:12, pdr. wrote: > > nit: is the trailing space needed? > > Nope, removed. > > https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/ImageDocument.h (right): > > https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/ImageDocument.h:93: enum MouseCursorMode { Uninitialized, Default, ZoomIn, ZoomOut }; > On 2016/11/22 04:15:12, pdr. wrote: > > Is the uninitialized case needed? > > Yeah, it's needed to get past the difference check in ImageDocument.cpp:420 for initialization. Alternatively, we could store an initialization boolean, or check if the style string is empty. Not sure what's the most intuitive. I still don't quite follow. The Uninitialized value is for handling when updateImageStyle is called for the first time, but couldn't we use Default instead? If the initial value is Default and the updated value is Default, wouldn't it be okay to skip the style setting code?
On 2016/11/22 19:23:30, pdr. wrote: > On 2016/11/22 at 18:51:29, dfalcantara wrote: > > Everything seems fine on Android and Linux using the old test cases during > loading and after window resizing; not sure what else to check but it _seems_ > OK. > > Thanks! > > > > > > https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): > > > > > https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/ImageDocument.cpp:445: > imageStyle.append("cursor: zoom-in; "); > > On 2016/11/22 04:15:12, pdr. wrote: > > > nit: is the trailing space needed? > > > > Nope, removed. > > > > > https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/ImageDocument.h (right): > > > > > https://codereview.chromium.org/2523553002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/ImageDocument.h:93: enum MouseCursorMode { > Uninitialized, Default, ZoomIn, ZoomOut }; > > On 2016/11/22 04:15:12, pdr. wrote: > > > Is the uninitialized case needed? > > > > Yeah, it's needed to get past the difference check in ImageDocument.cpp:420 > for initialization. Alternatively, we could store an initialization boolean, or > check if the style string is empty. Not sure what's the most intuitive. > > I still don't quite follow. The Uninitialized value is for handling when > updateImageStyle is called for the first time, but couldn't we use Default > instead? If the initial value is Default and the updated value is Default, > wouldn't it be okay to skip the style setting code? Chrome ate my previous response, but after running through the scenarios again, I think we're OK. I was worried that because all image styling goes through here now (including the one that actually adds the checkerboard and sets the cursor after the image has loaded) that we'd get hung up in the "not changed" conditional. I think we're fine for two reasons: 1) When the image is first loaded and the styling is being added, the checker size member field hasn't been set either, so it'd fail the conditional and update all the styling. 2) A "Default" cursor means that we're not changing the styling to show any sort of magnifying glass, so if it fails to update there nothing would have been set anyway. I've tested and uploaded a new version that doesn't have the Uninitialized state and it still seems to work fine.
On 2016/11/22 at 19:36:00, dfalcantara wrote: > Chrome ate my previous response Arg, hate when that happens. >:( > I've tested and uploaded a new version that doesn't have the Uninitialized state and > it still seems to work fine. LGTM
The CQ bit was checked by dfalcantara@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
kbr@chromium.org changed reviewers: + bsep@chromium.org, esprehn@chromium.org, kbr@chromium.org
Bret, Elliott: could you both help review this as well? Philip asked me but I don't know this code. Dan: looks like some related layout tests are failing with this change. I suggest some new tests should be added, unless there's a strong reason not to.
Actually, Philip pointed out this was the wrong review. Never mind.
kbr@chromium.org changed reviewers: - bsep@chromium.org, esprehn@chromium.org, kbr@chromium.org
The CQ bit was checked by dfalcantara@chromium.org
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 dfalcantara@chromium.org
The CQ bit was checked by dfalcantara@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2523553002/#ps100001 (title: "Rebaseline test expectation")
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
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dfalcantara@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2523553002/#ps120001 (title: "Rebaseing again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479852491973060,
"parent_rev": "49d9b4a5bdd30231ecd73c0d99b72fd0b7f1bc94", "commit_rev":
"02294434d5b1329c495d06e05655286b885f1040"}
Message was sent while issue was closed.
Description was changed from ========== Consolidate how ImageDocument sets image styling * Consolidate how the image sets the cursor styling inside of updateImageStyle(), which now checks for whether the cursor or checkerboard size has changed during styling updates. * Fix how the magnifying glass cursor is initialized on desktop versions of the page. BUG=664782 ========== to ========== Consolidate how ImageDocument sets image styling * Consolidate how the image sets the cursor styling inside of updateImageStyle(), which now checks for whether the cursor or checkerboard size has changed during styling updates. * Fix how the magnifying glass cursor is initialized on desktop versions of the page. BUG=664782 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Consolidate how ImageDocument sets image styling * Consolidate how the image sets the cursor styling inside of updateImageStyle(), which now checks for whether the cursor or checkerboard size has changed during styling updates. * Fix how the magnifying glass cursor is initialized on desktop versions of the page. BUG=664782 ========== to ========== Consolidate how ImageDocument sets image styling * Consolidate how the image sets the cursor styling inside of updateImageStyle(), which now checks for whether the cursor or checkerboard size has changed during styling updates. * Fix how the magnifying glass cursor is initialized on desktop versions of the page. BUG=664782 Committed: https://crrev.com/893f3dcf906b25e13e9d4951a52aa30843875ef5 Cr-Commit-Position: refs/heads/master@{#434020} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/893f3dcf906b25e13e9d4951a52aa30843875ef5 Cr-Commit-Position: refs/heads/master@{#434020} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
