Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
262053005:
Allow zoom/restore in a zoomed document. It is a regression from an earlier fix. (Closed)
|
Created:
6 years, 7 months ago by Srini Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink, Elliot Glaysher, jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFix for magnifying glass to magnify/shrink images when the zoom is not at 100%.
Allow zoom/restore in a zoomed image document. It is a regression from the earlier fix:
http://src.chromium.org/viewvc/blink?revision=156402&view=revision
BUG=324086
R=levi, tony
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176023
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use enums instead of bool. Add layout test to check this condition. #
Total comments: 6
Patch Set 3 : Fix comments and move to requestAnimationFrame #Patch Set 4 : Fixed a typo in the previous commit. #Patch Set 5 : Rebased code #
Messages
Total messages: 31 (0 generated)
Hi Tony, I added a patch for allowing zoom/restore in a zoomed image document. Can you check this out? I can rework if you want me do this in a different way. Thanks, -Srini.
This should have a layout test. I'm adding Elliott as a reviewer. I'm not working on Chromium/Blink right now. Elliott can either take over the review or redirect to someone who would be appropriate. https://codereview.chromium.org/262053005/diff/1/Source/core/html/ImageDocume... File Source/core/html/ImageDocument.h (right): https://codereview.chromium.org/262053005/diff/1/Source/core/html/ImageDocume... Source/core/html/ImageDocument.h:46: void windowSizeChanged(bool resizeZoomedDocument); Can you make this an enum with 2 values instead of a bool? You can see other examples of this in the code.
On 2014/05/05 16:38:13, tony wrote: > This should have a layout test. > Added. > I'm adding Elliott as a reviewer. I'm not working on Chromium/Blink right now. > Elliott can either take over the review or redirect to someone who would be > appropriate. > > https://codereview.chromium.org/262053005/diff/1/Source/core/html/ImageDocume... > File Source/core/html/ImageDocument.h (right): > > https://codereview.chromium.org/262053005/diff/1/Source/core/html/ImageDocume... > Source/core/html/ImageDocument.h:46: void windowSizeChanged(bool > resizeZoomedDocument); > Can you make this an enum with 2 values instead of a bool? You can see other > examples of this in the code. Fixed it. Elliott can you check this up. I can rework if there are any comments. Thanks, -Srini.
Maybe levi can review.
What's the earlier fix that caused the regression (please add a link)? You say "a zoomed document" but your change is only for ImageDocuments, right? Add the subject line to your description as well.
Updated the subject and added the revision url.
ping reviewers
On 2014/05/09 18:32:25, tony wrote: > ping reviewers Hi, can anyone check this patch? Thanks, -Srini.
+More reviewers. It's been over a week since this patch was uploaded.
On 2014/05/13 16:27:56, tony wrote: > +More reviewers. It's been over a week since this patch was uploaded. It is nearly 2 weeks. Can someone just give it a try. I have the patch and a test case for this bug.
Looks like jochen reviewed the last one of these? I also lean on Rick for Zoom-related things these days, but I'm not sure he has strong opinions about the image viewer. Does this new behavior match FF/IE?
https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... File LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html (right): https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:35: // On GTK+, sometimes the resize callback fires before the GTK This seems like a bug...
On 2014/05/19 19:51:37, eseidel wrote: > Looks like jochen reviewed the last one of these? > > I also lean on Rick for Zoom-related things these days, but I'm not sure he has > strong opinions about the image viewer. > > Does this new behavior match FF/IE? I dont have an IE, but this is similar to FF behavior.
On 2014/05/19 19:52:48, eseidel wrote: > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > File LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html > (right): > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:35: // On > GTK+, sometimes the resize callback fires before the GTK > This seems like a bug... This is an existing comment on a LayoutTests/fast/images/image-zoom-to-25.html and I derived this test from that. It was added in https://chromiumcodereview.appspot.com/23242015 by Tony.
On 2014/05/19 19:51:37, eseidel wrote: > Looks like jochen reviewed the last one of these? > > I also lean on Rick for Zoom-related things these days, but I'm not sure he has > strong opinions about the image viewer. Yeah sorry I don't have any context at all on the image viewer. > Does this new behavior match FF/IE?
https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... File LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html (right): https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:35: // On GTK+, sometimes the resize callback fires before the GTK On 2014/05/19 19:52:49, eseidel wrote: > This seems like a bug... Now that Linux is on views, it probably doesn't apply anymore. The history is that resizing a window was a sync call on Windows so the API was written in a way that assumed it would happen sync. GTK+/X does window resizes async, but rather than making the API async, we implemented the sync API.
On 2014/05/20 16:33:43, tony wrote: > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > File LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html > (right): > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:35: // On > GTK+, sometimes the resize callback fires before the GTK > On 2014/05/19 19:52:49, eseidel wrote: > > This seems like a bug... > > Now that Linux is on views, it probably doesn't apply anymore. So, I can remove the comment and the loop in this & other related cases Tony? > > The history is that resizing a window was a sync call on Windows so the API was > written in a way that assumed it would happen sync. GTK+/X does window resizes > async, but rather than making the API async, we implemented the sync API.
On 2014/05/20 17:15:08, Srini wrote: > On 2014/05/20 16:33:43, tony wrote: > > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > > File LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html > > (right): > > > > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > > LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:35: // On > > GTK+, sometimes the resize callback fires before the GTK > > On 2014/05/19 19:52:49, eseidel wrote: > > > This seems like a bug... > > > > Now that Linux is on views, it probably doesn't apply anymore. > > So, I can remove the comment and the loop in this & other related cases Tony? Have you tested and verified that it doesn't apply anymore? I'm only guessing. I would probably do it in a separate change as not to delay this change any longer.
On 2014/05/20 17:20:48, tony wrote: > On 2014/05/20 17:15:08, Srini wrote: > > On 2014/05/20 16:33:43, tony wrote: > > > > > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > > > File LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html > > > (right): > > > > > > > > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > > > LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:35: // > On > > > GTK+, sometimes the resize callback fires before the GTK > > > On 2014/05/19 19:52:49, eseidel wrote: > > > > This seems like a bug... > > > > > > Now that Linux is on views, it probably doesn't apply anymore. > > > > So, I can remove the comment and the loop in this & other related cases Tony? > > Have you tested and verified that it doesn't apply anymore? I'm only guessing. > > I would probably do it in a separate change as not to delay this change any > longer. Im sorry, I wasn't clear. I would test this separately and create another CL to remove this if required. Im still looking for an approval :(
We're still looking for someone with zooming context (jochen?) to approve this patch. I am not that person.
I can review this https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... File LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html (right): https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:2: <html> We usually leave off the <html>, <head> and <body> elements. https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:25: function zoomPage() onload = function() https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:40: }, 0); We shouldn't add this gtk hack. Maybe you want requestAnimationFrame() too? setTimeout is really suspect and means flake. https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:53: } else if (window.eventSender && clicked){ put the eventSender guards right around the eventSender code below, this if should be if (clicked) { } else { }
On 2014/05/29 00:12:02, esprehn wrote: > I can review this > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > File LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html > (right): > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:2: <html> > We usually leave off the <html>, <head> and <body> elements. Fixed it. > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:25: function > zoomPage() > onload = function() fixed it. > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:40: }, 0); > We shouldn't add this gtk hack. Maybe you want requestAnimationFrame() too? > setTimeout is really suspect and means flake. I moved this setTimeout to requestAnimationFrame(). Independently I'll take a different CL to see if even if this holds true anymore. if not I'll remove it completely from other cases too. > > https://codereview.chromium.org/262053005/diff/20001/LayoutTests/fast/images/... > LayoutTests/fast/images/image-click-scale-restore-zoomed-image.html:53: } else > if (window.eventSender && clicked){ > put the eventSender guards right around the eventSender code below, this if > should be > > if (clicked) { > > } else { > > } Fixed it. Thanks, -Srini.
lgtm
The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6973) linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13167)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6985)
The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....
Message was sent while issue was closed.
Change committed as 176023 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
