Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1009703002:
Changing font size with pinch gesture in Reader Mode (Closed)
DescriptionChanging font size with pinch gesture in Reader Mode
When users pinch in Reader Mode, the page would zoom in or out as if it
is a normal web page allowing user-zoom. At the end of pinch gesture, the
page would do text reflow. These pinch-to-zoom and text reflow effects
are not native, but are emulated using CSS and JavaScript.
In order to achieve near-native zooming and panning frame rate, fake 3D
transform is used so that the layer doesn't repaint for each frame.
After the text reflow, the web content shown in the viewport should
roughly be the same paragraph before zooming.
The control point of font size is the html element, so that both "em" and
"rem" are adjusted. Accordingly, font size of body is no longer specified
in CSS in unit of pixel.
Some CSS styles and animations are updated to fix issues specific to
resizing.
BUG=445632
Committed: https://crrev.com/6349c066f2a16007c0fba1684dfe2a18d40a9e20
Cr-Commit-Position: refs/heads/master@{#326945}
Patch Set 1 #
Total comments: 11
Patch Set 2 : multitouch and tests #
Total comments: 12
Patch Set 3 : address comments #
Total comments: 13
Patch Set 4 : test readability and singularity #
Total comments: 6
Patch Set 5 : address comments of jdduke #
Total comments: 10
Patch Set 6 : address tdresser's comment #
Total comments: 6
Patch Set 7 : fix typo #Patch Set 8 : remove symlink #Patch Set 9 : fix isolate #Patch Set 10 : merge master #Patch Set 11 : update pinch_tester.html for changed viewer structure #Patch Set 12 : fix animation conflict with feedback #Patch Set 13 : temporarily merge https://crrev.com/1093993003/ for embedded test server #Patch Set 14 : merge master #Patch Set 15 : revert https://crrev.com/1093993003/, fix copyright, fix embedded_test_server #Patch Set 16 : fix relative url #Patch Set 17 : remove redundant code, use const in js, fix feedback font size #
Total comments: 1
Messages
Total messages: 66 (20 generated)
wychen@chromium.org changed reviewers: + cjhopman@chromium.org
PTAL, or try it out.
cjhopman@chromium.org changed reviewers: + jdduke@chromium.org
jdduke: wdyt of this approach?
How difficult would it be to test this? Simulate a few event sequences and verify it behaves sanely? https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:114: if (e.touches.length != 2) return; How much more complicated is supporting >2 touches? For scale gesture detection on Android, we abstract the notion of "touchDist" to be a span indicating the average distance between touch points through the focal point, i.e., compute the "focal" position by averaging all the point positions, compute the average distance of each point to that focal point, then double that (giving you the diameter of the approximate circle formed by the touch points). We can probably modify the touchDist/Mid functions to do this and it'll just work. https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:139: var fontScale = 1 + ((pinchScale - 1) * FONT_SCALE_MULTIPLIER); We might want to be more careful about the min font scale here, I was pretty casual in the limits I enforced. https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:146: // With 2D transform, the frame rate would be much lower. I can't speak to this logic. https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:156: if (!pinching) return; We'll need to be careful here as well, as we probably want to continue pinching if the user lifts their 4th or 3rd finger (for which we'll see a touchend). https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:162: document.documentElement.style.fontSize = scaleFactor * baseSize + "px"; I'm an HTML newb so we'll want somebody else to review this.
https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:114: if (e.touches.length != 2) return; On 2015/03/16 20:27:52, jdduke wrote: > How much more complicated is supporting >2 touches? > > For scale gesture detection on Android, we abstract the notion of "touchDist" to > be a span indicating the average distance between touch points through the focal > point, i.e., compute the "focal" position by averaging all the point positions, > compute the average distance of each point to that focal point, then double that > (giving you the diameter of the approximate circle formed by the touch points). > > We can probably modify the touchDist/Mid functions to do this and it'll just > work. Good suggestion! I've never used more than two fingers for zooming on a phone, so this doesn't occur to me. Right now, using more than two fingers wouldn't break this feature too much because handleTouchMove() doesn't check number of points, but definitely not as good as it can be. I'll give it a shot. https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:139: var fontScale = 1 + ((pinchScale - 1) * FONT_SCALE_MULTIPLIER); On 2015/03/16 20:27:52, jdduke wrote: > We might want to be more careful about the min font scale here, I was pretty > casual in the limits I enforced. 0.4 might be in the unreadable range, and 0.4~2.5 seems to be broader than necessary, but later on, the font size would be the product of "Android accessibility scaling", "Chrome accessibility scaling", and the font scaling factor defined here. Therefore, broader range leaves users more flexible choices. https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:146: // With 2D transform, the frame rate would be much lower. On 2015/03/16 20:27:53, jdduke wrote: > I can't speak to this logic. The frame rate was enhanced from <30fps to 60fps through. :p Using any kind of null 3D transform is just to trigger the layer composition mechanism without actually changing how it looks. https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:156: if (!pinching) return; On 2015/03/16 20:27:53, jdduke wrote: > We'll need to be careful here as well, as we probably want to continue pinching > if the user lifts their 4th or 3rd finger (for which we'll see a touchend). Indeed. https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:162: document.documentElement.style.fontSize = scaleFactor * baseSize + "px"; On 2015/03/16 20:27:52, jdduke wrote: > I'm an HTML newb so we'll want somebody else to review this. Why would this line in particular concern you? The font size needs to be adjusted at the html element rather than the body element, and measuring the original size in px helps decouple CSS and JS.
https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/1/components/dom_distiller/co... components/dom_distiller/core/javascript/dom_distiller_viewer.js:162: document.documentElement.style.fontSize = scaleFactor * baseSize + "px"; On 2015/03/17 17:36:11, Wei-Yin Chen wrote: > On 2015/03/16 20:27:52, jdduke wrote: > > I'm an HTML newb so we'll want somebody else to review this. > > Why would this line in particular concern you? > > The font size needs to be adjusted at the html element rather than the body > element, and measuring the original size in px helps decouple CSS and JS. Oh it's not that particular line that concerns me, I'm just saying my HTML knowledge is sadly limited so it would be good to have a Blink engineer review this just to make sure there's nothing we're missing and/or a different approach we could take.
Multi-touch (more than 2 fingers) and tests are added. PTAL.
jdduke@chromium.org changed reviewers: + rbyers@chromium.org
This looks very reasonable, thanks! +rbyers as another set of eyes on the pinch code. https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc (right): https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:549: callback_ = run_loop.QuitClosure(); To avoid confusion with the existing quit_closure_, what about just passing the callback as the first arg when binding the |OnJsExecutionDone| callback? https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:116: var baseSize = I assume this size will be reliably determined by the time we hit this code? https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:119: var refresh = function() { Maybe |refreshTransform()|? https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:208: e.preventDefault(); It might be good to sanity check that we have >= 2 touches here. Perhaps have a |endPinchIfNecessary()| function that contains the cleanup code at the end of the |handleTouchEnd()| function? https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:250: pinching = false; We don't need to do any more cleanup here? https://codereview.chromium.org/1009703002/diff/20001/components/test/data/do... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/20001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:2: 'use strict'; Thanks for adding tests!
PTAL https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc (right): https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:549: callback_ = run_loop.QuitClosure(); On 2015/03/23 15:42:06, jdduke wrote: > To avoid confusion with the existing quit_closure_, what about just passing the > callback as the first arg when binding the |OnJsExecutionDone| callback? Done. https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:116: var baseSize = On 2015/03/23 15:42:06, jdduke wrote: > I assume this size will be reliably determined by the time we hit this code? It should be as of now. If things are added to the 'onload' event in the future, then maybe the style could be different from the ones specified in CSS, but at least they would be deterministic. https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:119: var refresh = function() { On 2015/03/23 15:42:06, jdduke wrote: > Maybe |refreshTransform()|? Done. https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:208: e.preventDefault(); On 2015/03/23 15:42:06, jdduke wrote: > It might be good to sanity check that we have >= 2 touches here. Perhaps have a > |endPinchIfNecessary()| function that contains the cleanup code at the end of > the |handleTouchEnd()| function? Done. https://codereview.chromium.org/1009703002/diff/20001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:250: pinching = false; On 2015/03/23 15:42:06, jdduke wrote: > We don't need to do any more cleanup here? Done. https://codereview.chromium.org/1009703002/diff/20001/components/test/data/do... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/20001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:2: 'use strict'; On 2015/03/23 15:42:06, jdduke wrote: > Thanks for adding tests! Writing tests also forced me to reorganize the implementation in a cleaner way!
jdduke@chromium.org changed reviewers: + tdresser@chromium.org - rbyers@chromium.org
-rbyers as he'll be pretty busy this week. Tim, would you mind doing a sanity check on the pinch code?
Can features be implemented as extensions on Android? If so, should this be implemented as an extension? I don't see why we need to reimplement pinch-zoom here. Can't we only listen to touchstart, touchend, and touchcancel, and never call preventDefault()? We only need custom behavior when the pinch is finished. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:4: function assertTrue(condition, message) { It's a shame to have to implement all these trivial assertion functions. I was digging around to see what other features implemented in js do for testing, and the primary testing I found that wasn't specific to webapps/extensions was in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w..., for example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w.... I'm not sure if it would be worth using that kind of structure, as it feels a bit heavy, but it would be nice to avoid reinventing the wheel here. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:4: function assertTrue(condition, message) { Also, this method is pretty much equivalent to console.assert, isn't it? https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:57: function makeTouch(e) { You should be able to get access to window.eventSender, which would give you a nice api for synthesizing touches. Otherwise, if you're using a browsertest, you might as well synthesize touches from there, where we already have a nice touch synthesis API. We have had some flake issues with browsertests with touch, so I'd use window.eventSender over sending the events from C++. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:145: pincher.handleTouchStart(e1); I would find this much easier to read if the positions were inline with the tests. To figure out what's happening now, I have to constantly scroll back and forth between the definitions of the events and the tests themselves.
On 2015/03/25 13:45:37, tdresser wrote: > Can features be implemented as extensions on Android? If so, should this be > implemented as an extension? > Nope :( > I don't see why we need to reimplement pinch-zoom here. Can't we only listen to > touchstart, touchend, and touchcancel, and never call preventDefault()? We only > need custom behavior when the pinch is finished. So, what happens when the default, platform scale gesture bumps against the logical page scale constraints? Can JS manipulate those in some fashion? The prototype patch I had for this did live reflow as we scaled, which requires JS touch handling. This version defers that, so I guess it's not quite as necessary. I haven't actually tried this out to see what it looks like as you pinch.
tdresser@chromium.org changed reviewers: + bokan@chromium.org
+bokan@, for insight on pinch behavior. I don't think you can adjust the pinch scale constraints, but I expect the dynamic range of pinch is high enough to represent all the font scales you would want. Can you trick the page into starting with a specific page scale, and go from there?
On 2015/03/25 18:40:08, tdresser wrote: > +bokan@, for insight on pinch behavior. > > I don't think you can adjust the pinch scale constraints, but I expect the > dynamic range of pinch is high enough to represent all the font scales you would > want. > > Can you trick the page into starting with a specific page scale, and go from > there? I think we have a lot of freedom here, as long as the experience is fairly seamless and there's enough JS-visible data that we can always get into the right state when the native pinch gesture ends. We can certainly modify the initial scale and/or font size to give us the right initial conditions.
On 2015/03/25 18:40:08, tdresser wrote: > +bokan@, for insight on pinch behavior. > > I don't think you can adjust the pinch scale constraints, but I expect the > dynamic range of pinch is high enough to represent all the font scales you would > want. > > Can you trick the page into starting with a specific page scale, and go from > there? Page scale isn't explicitly exposed to authors at all today, you can kind of calculate it by comparing innerWidth and outerWidth but you can't set it. You can control the initial scale and limits using the viewport meta tag, but you can't script it AFAIK since that's read and applied early in page load. Though, the limit is 5X on Android (4X on desktop) so I think it's plenty for the purpose here. It seems like the reason we need to reimplement pinch here is that at the end of the pinch we change the font size to match the scale but then we need to reset the page scale to 1 since the font size now reflects the desired scale and there's no way to set page scale, is my understanding correct? Maybe a bit late in the process, but have you thought about going about this the other way around? I.e. Don't prevent browser pinch-zoom, but when the pinch ends, instead of setting the font size and resetting page scale, resize the <div> containing the text so that it's the width of the scaled viewport. window.innerWidth/innerHeight reflect the visible area as the page is pinch-zoomed. Then you just have to move the viewport via window.scrollTo so that the current text stays in the viewport. This way you use the native pinch-zoom machinery with all it's semantics, all you're doing is reflowing the text so that it fits in the smaller viewport.
On 2015/03/25 19:09:30, bokan wrote: > Page scale isn't explicitly exposed to authors at all today, you can kind of > calculate it by comparing innerWidth and outerWidth but you can't set it. You > can control the initial scale and limits using the viewport meta tag, but you > can't script it AFAIK since that's read and applied early in page load. Though, > the limit is 5X on Android (4X on desktop) so I think it's plenty for the > purpose here. > > It seems like the reason we need to reimplement pinch here is that at the end of > the pinch we change the font size to match the scale but then we need to reset > the page scale to 1 since the font size now reflects the desired scale and > there's no way to set page scale, is my understanding correct? > Yes. > Maybe a bit late in the process, but have you thought about going about this the > other way around? I.e. Don't prevent browser pinch-zoom, but when the pinch > ends, instead of setting the font size and resetting page scale, resize the > <div> containing the text so that it's the width of the scaled viewport. > window.innerWidth/innerHeight reflect the visible area as the page is > pinch-zoomed. Then you just have to move the viewport via window.scrollTo so > that the current text stays in the viewport. This way you use the native > pinch-zoom machinery with all it's semantics, all you're doing is reflowing the > text so that it fits in the smaller viewport. Right, this was the idea being discussed. If all the necessary data is exposed to JS, then it's probably worth giving this a try. That is, assuming we're confident we'll never want to zoom via text resize + reflow (or changing document.body.style.zoom + scrollTo, which is similar to browser-style zoom).
> > Maybe a bit late in the process, but have you thought about going about this > the > > other way around? I.e. Don't prevent browser pinch-zoom, but when the pinch > > ends, instead of setting the font size and resetting page scale, resize the > > <div> containing the text so that it's the width of the scaled viewport. > > window.innerWidth/innerHeight reflect the visible area as the page is > > pinch-zoomed. Then you just have to move the viewport via window.scrollTo so > > that the current text stays in the viewport. This way you use the native > > pinch-zoom machinery with all it's semantics, all you're doing is reflowing > the > > text so that it fits in the smaller viewport. > > Right, this was the idea being discussed. If all the necessary data is exposed > to JS, then it's probably worth giving this a try. That is, assuming we're > confident we'll never want to zoom via text resize + reflow (or changing > document.body.style.zoom + scrollTo, which is similar to browser-style zoom). Just resizing the divs to have width == viewport width on scroll end doesn't seem like it would handle images (or other things that shouldn't be zoomed after the adjustment) correctly. If we can, in js, set the zoom back to 1 (and update font size, scroll offset, etc), then letting the browser handle the zoom might work.
Talking offline with aelias@, we both agreed that having custom JS-driven pinch behavior here is fine. The implementation is nice and straightforward, and it gives us some flexibility that we otherwise wouldn't have by relying on native pinch.
https://codereview.chromium.org/1009703002/diff/40001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/40001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:145: var count = e.touches.length; We'll probably want some kind of minimum span threshold to prevent singularities. I suspect just returning Math.max(REASONABLE_MIN, sum/count); would be sufficient, where REASONABLE_MIN is about the distance between your finger pointers when you press them loosely together and touch down (~10-20 or so?). Probably also want a test for that.
On 2015/03/26 at 22:03:41, cjhopman wrote: > Just resizing the divs to have width == viewport width on scroll end doesn't seem like it would handle images (or other things that shouldn't be zoomed after the adjustment) correctly. If we can, in js, set the zoom back to 1 (and update font size, scroll offset, etc), then letting the browser handle the zoom might work. Besides Chris' point, reimplementing pinch gesture in JavaScript also enables finer control over the zooming behavior. The zooming speed is slower than the pinching speed intentionally, in order to prevent sudden changes in scale. The parameter FONT_SCALE_MULTIPLIER tunes how slow it is.
On 2015/03/27 01:38:04, Wei-Yin Chen wrote: > On 2015/03/26 at 22:03:41, cjhopman wrote: > > Just resizing the divs to have width == viewport width on scroll end doesn't > seem like it would handle images (or other things that shouldn't be zoomed after > the adjustment) correctly. If we can, in js, set the zoom back to 1 (and update > font size, scroll offset, etc), then letting the browser handle the zoom might > work. > > Besides Chris' point, reimplementing pinch gesture in JavaScript also enables > finer control over the zooming behavior. The zooming speed is slower than the > pinching speed intentionally, in order to prevent sudden changes in scale. The > parameter FONT_SCALE_MULTIPLIER tunes how slow it is. Using JS driven pinch SGTM.
PTAL. https://codereview.chromium.org/1009703002/diff/40001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/40001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:145: var count = e.touches.length; On 2015/03/27 01:26:25, jdduke wrote: > We'll probably want some kind of minimum span threshold to prevent > singularities. I suspect just returning Math.max(REASONABLE_MIN, sum/count); > would be sufficient, where REASONABLE_MIN is about the distance between your > finger pointers when you press them loosely together and touch down (~10-20 or > so?). Probably also want a test for that. Done. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:4: function assertTrue(condition, message) { On 2015/03/25 13:45:36, tdresser wrote: > Also, this method is pretty much equivalent to console.assert, isn't it? Almost, since I need to throw an exception as well. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:4: function assertTrue(condition, message) { On 2015/03/25 13:45:36, tdresser wrote: > It's a shame to have to implement all these trivial assertion functions. I was > digging around to see what other features implemented in js do for testing, and > the primary testing I found that wasn't specific to webapps/extensions was in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w..., > for example: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w.... > > I'm not sure if it would be worth using that kind of structure, as it feels a > bit heavy, but it would be nice to avoid reinventing the wheel here. > Thanks for the suggestion! I've looked at some JS testing/assertion libraries/frameworks, but they seem a bit too heavy for just this single test in browsertests. We might add more JavaScript-based tests in browsertests for Reader Mode, so I plan to pick a light-weight library when needed. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:57: function makeTouch(e) { On 2015/03/25 13:45:36, tdresser wrote: > You should be able to get access to window.eventSender, which would give you a > nice api for synthesizing touches. > > Otherwise, if you're using a browsertest, you might as well synthesize touches > from there, where we already have a nice touch synthesis API. We have had some > flake issues with browsertests with touch, so I'd use window.eventSender over > sending the events from C++. > The reason I mock touch events in JS is exactly to reduce flakiness. window.eventSender doesn't seem to support multiple touches, and Document.createTouch() seems too heavy. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:145: pincher.handleTouchStart(e1); On 2015/03/25 13:45:36, tdresser wrote: > I would find this much easier to read if the positions were inline with the > tests. To figure out what's happening now, I have to constantly scroll back and > forth between the definitions of the events and the tests themselves. Indeed. They are inlined now.
I'll defer to Tim on the JS test code, but the pinch code itself lgtm with a couple nits. https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc (right): https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:97: const base::Value* js_result_; Shouldn't this be |scoped_ptr<base::Value> js_result_|? https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:555: const base::DictionaryValue* dict; ASSERT_TRUE(js_result_); https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:154: return Math.max(20, sum/count); Let's stick the 20 value as a constant at the op (by FONT_SCALE_MULTIPLIER), maybe call it MIN_SPAN_LENGTH
PTAL. https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc (right): https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:97: const base::Value* js_result_; On 2015/04/02 23:34:22, jdduke wrote: > Shouldn't this be |scoped_ptr<base::Value> js_result_|? Done. https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:555: const base::DictionaryValue* dict; On 2015/04/02 23:34:22, jdduke wrote: > ASSERT_TRUE(js_result_); Done. https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/60001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:154: return Math.max(20, sum/count); On 2015/04/02 23:34:22, jdduke wrote: > Let's stick the 20 value as a constant at the op (by FONT_SCALE_MULTIPLIER), > maybe call it MIN_SPAN_LENGTH Done.
https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:4: function assertTrue(condition, message) { On 2015/04/02 02:05:59, wychen wrote: > On 2015/03/25 13:45:36, tdresser wrote: > > Also, this method is pretty much equivalent to console.assert, isn't it? > > Almost, since I need to throw an exception as well. Acknowledged. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:4: function assertTrue(condition, message) { On 2015/03/25 13:45:36, tdresser wrote: > It's a shame to have to implement all these trivial assertion functions. I was > digging around to see what other features implemented in js do for testing, and > the primary testing I found that wasn't specific to webapps/extensions was in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w..., > for example: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w.... > > I'm not sure if it would be worth using that kind of structure, as it feels a > bit heavy, but it would be nice to avoid reinventing the wheel here. > Acknowledged. https://codereview.chromium.org/1009703002/diff/40001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:57: function makeTouch(e) { On 2015/04/02 02:05:59, wychen wrote: > On 2015/03/25 13:45:36, tdresser wrote: > > You should be able to get access to window.eventSender, which would give you a > > nice api for synthesizing touches. > > > > Otherwise, if you're using a browsertest, you might as well synthesize touches > > from there, where we already have a nice touch synthesis API. We have had some > > flake issues with browsertests with touch, so I'd use window.eventSender over > > sending the events from C++. > > > > The reason I mock touch events in JS is exactly to reduce flakiness. > window.eventSender doesn't seem to support multiple touches, and > Document.createTouch() seems too heavy. See https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... for an example of using eventSender with multiple touches. https://codereview.chromium.org/1009703002/diff/80001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/80001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:169: function touchPageMid(e) { Would it be simpler to implement touchPageMid in terms of touchClientMid (or vice-versa)? https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:56: function makeTouch(e, offset) { I find this somewhat hard to read, with the type of e being so flexible; however, I'm hopeful that you'll be able to use eventSender, which makes this comment irrelevant. https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:106: assertTrue(pincher.status().clampedScale < 0.9); I'd prefer this assertion be directly after the touchmove. https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:126: assertTrue(pincher.status().clampedScale > 1.1); I'd prefer this assertion be directly after the touchmove. https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:153: pincher.handleTouchEnd(makeTouch([30])); Where did this 30 come from? Shouldn't it be 10?
Tim, PTAL. Thanks! https://codereview.chromium.org/1009703002/diff/80001/components/dom_distille... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/80001/components/dom_distille... components/dom_distiller/core/javascript/dom_distiller_viewer.js:169: function touchPageMid(e) { On 2015/04/07 13:56:15, tdresser wrote: > Would it be simpler to implement touchPageMid in terms of touchClientMid (or > vice-versa)? Done. https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:56: function makeTouch(e, offset) { On 2015/04/07 13:56:15, tdresser wrote: > I find this somewhat hard to read, with the type of e being so flexible; > however, I'm hopeful that you'll be able to use eventSender, which makes this > comment irrelevant. The interface of eventSender looks nice, but unfortunately it is only available in DRT mode, which cannot be turned on in component_browsertests. I've replaced this with a minimal mock of eventSender API, so the readability should be better now. https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:106: assertTrue(pincher.status().clampedScale < 0.9); On 2015/04/07 13:56:15, tdresser wrote: > I'd prefer this assertion be directly after the touchmove. Done. https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:126: assertTrue(pincher.status().clampedScale > 1.1); On 2015/04/07 13:56:15, tdresser wrote: > I'd prefer this assertion be directly after the touchmove. Done. https://codereview.chromium.org/1009703002/diff/80001/components/test/data/do... components/test/data/dom_distiller/pinch_tester.js:153: pincher.handleTouchEnd(makeTouch([30])); On 2015/04/07 13:56:15, tdresser wrote: > Where did this 30 come from? Shouldn't it be 10? Done.
Looks great, thanks. LGTM with nit. https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... components/test/data/dom_distiller/pinch_tester.js:56: var touch = (function() { It might be worth factoring this out into a different file, but I don't feel particularly strongly about it. https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... components/test/data/dom_distiller/pinch_tester.js:116: pincher.handleTouchStart(t.events()); I'm not clear on what's going on here. The comment mentions extra move events, but you're calling handleTouchStart for touches which are already down.
https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... File components/test/data/dom_distiller/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... components/test/data/dom_distiller/dom_distiller_viewer.js:1: ../../../dom_distiller/core/javascript/dom_distiller_viewer.js Is this a symlink? We shouldn't be using symlinks.
https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... File components/test/data/dom_distiller/pinch_tester.js (right): https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... components/test/data/dom_distiller/pinch_tester.js:56: var touch = (function() { On 2015/04/21 12:28:45, tdresser wrote: > It might be worth factoring this out into a different file, but I don't feel > particularly strongly about it. Acknowledged. https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... components/test/data/dom_distiller/pinch_tester.js:116: pincher.handleTouchStart(t.events()); On 2015/04/21 12:28:45, tdresser wrote: > I'm not clear on what's going on here. > The comment mentions extra move events, but you're calling handleTouchStart for > touches which are already down. Good catch! It was a typo. Fixed!
https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... File components/test/data/dom_distiller/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... components/test/data/dom_distiller/dom_distiller_viewer.js:1: ../../../dom_distiller/core/javascript/dom_distiller_viewer.js On 2015/04/22 00:41:28, cjhopman wrote: > Is this a symlink? We shouldn't be using symlinks. Fixed, depending on this supporting CL: https://codereview.chromium.org/1093993003/
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1009703002/#ps240001 (title: "temporarily merge https://crrev.com/1093993003/ for embedded test server")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009703002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1009703002/#ps260001 (title: "merge master")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009703002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1009703002/#ps280001 (title: "revert https://crrev.com/1093993003/, fix copyright, fix embedded_test_server")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009703002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
wychen@chromium.org changed reviewers: + csharp@chromium.org
+csharp as owner of components/components_browsertests.isolate
components/components_browsertests.isolate lgtm
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, tdresser@chromium.org, csharp@chromium.org Link to the patchset: https://codereview.chromium.org/1009703002/#ps320001 (title: "remove redundant code, use const in js, fix feedback font size")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009703002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/04/22 04:31:33, wychen wrote: > https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... > File components/test/data/dom_distiller/dom_distiller_viewer.js (right): > > https://codereview.chromium.org/1009703002/diff/100001/components/test/data/d... > components/test/data/dom_distiller/dom_distiller_viewer.js:1: > ../../../dom_distiller/core/javascript/dom_distiller_viewer.js > On 2015/04/22 00:41:28, cjhopman wrote: > > Is this a symlink? We shouldn't be using symlinks. > > Fixed, depending on this supporting CL: > https://codereview.chromium.org/1093993003/ That CL turned out to be not necessary. Chris, PTAL. Thanks!
lgtm
The CQ bit was checked by wychen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009703002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/6349c066f2a16007c0fba1684dfe2a18d40a9e20 Cr-Commit-Position: refs/heads/master@{#326945}
Message was sent while issue was closed.
mdjones@chromium.org changed reviewers: + mdjones@chromium.org
Message was sent while issue was closed.
This change causes a regression in the distiller HTML feedback form positioning; it should remain at the bottom of the page regardless of height. Fix: https://codereview.chromium.org/1117123002
Message was sent while issue was closed.
https://codereview.chromium.org/1009703002/diff/320001/components/dom_distill... File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right): https://codereview.chromium.org/1009703002/diff/320001/components/dom_distill... components/dom_distiller/core/javascript/dom_distiller_viewer.js:207: const FONT_SCALE_MULTIPLIER = 0.5; The use of the "const" keyword breaks distiller on iOS. Fix: https://codereview.chromium.org/1148083003 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
