Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1096773002:
Switching Clank to use direction-based selection granularity strategy and adding a chrome flag. (Closed)
|
Created:
5 years, 8 months ago by mfomitchev Modified:
5 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@unified_ALL_unchanged Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitching Clank to use direction-based selection granularity strategy and adding a chrome flag.
This will also be enabled on Aura when Aura and Android touch selection flows
finally unified. I also plan to have at least one more experimental strategy
implmenented.
Prerequisite CL: https://codereview.chromium.org/988023005/
BUG=451255
Committed: https://crrev.com/5ad034ec6cf4c0aef10e31b6b106ae45ff5a034d
Cr-Commit-Position: refs/heads/master@{#326896}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressing feedback. #Patch Set 3 : Adding the new flag to LoginCustomFlags enum #
Messages
Total messages: 38 (16 generated)
mfomitchev@chromium.org changed reviewers: + jdduke@chromium.org
mfomitchev@chromium.org changed reviewers: + sievers@chromium.org
sievers@chromium.org: Please review changes in chrome/ sievers@chromium.org: Please review changes in content/
mfomitchev@chromium.org changed reviewers: + sgurun@chromium.org
https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1213: "touch-selection-strategy", "touch-selection-strategy" values of 0/1 don't really tell us anything. Can we just call this |kEnableSmartTextSelection| with an appropriate flag description, and use that to determine the right strategy/granularity?
On 2015/04/17 17:05:16, jdduke wrote: > https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.cc > File chrome/browser/about_flags.cc (right): > > https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.... > chrome/browser/about_flags.cc:1213: "touch-selection-strategy", > "touch-selection-strategy" values of 0/1 don't really tell us anything. Can we > just call this |kEnableSmartTextSelection| with an appropriate flag description, > and use that to determine the right strategy/granularity? Hmm, I guess if it's on by default on Android that would be kDisableSmartTextSelection.
https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1213: "touch-selection-strategy", Eventually I'd like to add another strategy: shrink by character only until the word boundary and then switch to word granularity if you continue shrinking. Then we will have three strategies, and enable/disable won't work.
https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:1213: "touch-selection-strategy", On 2015/04/17 17:14:37, mfomitchev wrote: > Eventually I'd like to add another strategy: shrink by character only until the > word boundary and then switch to word granularity if you continue shrinking. > Then we will have three strategies, and enable/disable won't work. Acknowledged.
mfomitchev@chromium.org changed reviewers: - sgurun@chromium.org, sievers@chromium.org
mfomitchev@chromium.org changed reviewers: + sgurun@chromium.org, sievers@chromium.org - jdduke@chromium.org
mfomitchev@chromium.org changed reviewers: + jdduke@chromium.org
sivers: *ping*
https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:184: switches::kTouchTextSelectionStrategy, I can't say I love the use of ordinals to describe the strategy. People wanting to "experiment" will have to look at the soure before they're able to make a change. Any reason to use integers instead of string constants?
https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:184: switches::kTouchTextSelectionStrategy, On 2015/04/21 14:45:08, jdduke wrote: > I can't say I love the use of ordinals to describe the strategy. People wanting > to "experiment" will have to look at the soure before they're able to make a > change. Any reason to use integers instead of string constants? These will shown as strings in the about://flags UI: Default, Character, and Direction (see generated_resources.grd). I suppose I could turn "0" and "1" into "character" and "direction" as well. I don't have much of an opinion either way - I just copied kOverscrollHistoryNavigationChoices figuring it's better to be consistent.
https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1096773002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:184: switches::kTouchTextSelectionStrategy, On 2015/04/21 15:02:37, mfomitchev wrote: > On 2015/04/21 14:45:08, jdduke wrote: > > I can't say I love the use of ordinals to describe the strategy. People > wanting > > to "experiment" will have to look at the soure before they're able to make a > > change. Any reason to use integers instead of string constants? > > These will shown as strings in the about://flags UI: Default, Character, and > Direction (see generated_resources.grd). I suppose I could turn "0" and "1" into > "character" and "direction" as well. I don't have much of an opinion either way > - I just copied kOverscrollHistoryNavigationChoices figuring it's better to be > consistent. Ah, I didn't realize the names already get pretty printed, definitely fine as-is for me!
mfomitchev@chromium.org changed reviewers: - sievers@chromium.org
mfomitchev@chromium.org changed reviewers: + nasko@chromium.org
nasko@chromium.org: Please review changes in content/
mfomitchev@chromium.org changed reviewers: - nasko@chromium.org
On 2015/04/22 17:11:16, mfomitchev wrote: > mailto:nasko@chromium.org: Please review changes in > > content/ nevermind (just saw the alias change)
mfomitchev@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in content/
LGTM with nits. Side note: we're generally avoiding new code in RenderViewImpl, but webview()->settings() hasn't been moved out yet, so this is ok for now. Deprecation notice is in the .h file: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... https://codereview.chromium.org/1096773002/diff/1/content/public/common/conte... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/1096773002/diff/1/content/public/common/conte... content/public/common/content_switches.cc:793: // handles are dragged. Can you say something about the potential values (similar to kTabCaptureUpscaleQuality above)? https://codereview.chromium.org/1096773002/diff/1/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1096773002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:777: if (selection_strategy_str == "0") From the earlier discussion, I think "character" or "direction" do seem like better values than the strings "0" or "1". That's more sensible to pass in from the command line, and it makes this easier to read.
https://codereview.chromium.org/1096773002/diff/1/content/public/common/conte... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/1096773002/diff/1/content/public/common/conte... content/public/common/content_switches.cc:793: // handles are dragged. On 2015/04/22 19:04:59, Charlie Reis wrote: > Can you say something about the potential values (similar to > kTabCaptureUpscaleQuality above)? Done. https://codereview.chromium.org/1096773002/diff/1/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1096773002/diff/1/content/renderer/render_vie... content/renderer/render_view_impl.cc:777: if (selection_strategy_str == "0") On 2015/04/22 19:04:59, Charlie Reis wrote: > From the earlier discussion, I think "character" or "direction" do seem like > better values than the strings "0" or "1". That's more sensible to pass in from > the command line, and it makes this easier to read. Done.
The CQ bit was checked by mfomitchev@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1096773002/#ps20001 (title: "Addressing feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096773002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mfomitchev@chromium.org changed reviewers: + isherman@chromium.org
isherman@chromium.org: Please review changes in histograms.xml
histograms.xml lgtm
The CQ bit was checked by mfomitchev@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1096773002/#ps40001 (title: "Adding the new flag to LoginCustomFlags enum")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096773002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5ad034ec6cf4c0aef10e31b6b106ae45ff5a034d Cr-Commit-Position: refs/heads/master@{#326896} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
