Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1003143002:
Send autocapitalize flags to the Android virtual keyboard based on Blink hints. (Closed)
|
Created:
5 years, 9 months ago by mlamouri (slow - plz ping) Modified:
5 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, penghuang+watch_chromium.org, James Su, yukishiino+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend autocapitalize flags to the Android virtual keyboard based on Blink hints.
This changes the behaviour of AdapterInputConnection to pass a
InputType.TYPE_TEXT_FLAG_CAP_* based on the
WebTextInputFlags.Autocapitalize* instead of doing it purely based on the
element's type.
This CL requires the following Blink CL:
https://codereview.chromium.org/995363002
Also the following build fix CL:
https://codereview.chromium.org/1040643003
BUG=466930
Committed: https://crrev.com/d1421a5faf9dc2d3b3cad10640576b24a092d9ba
Cr-Commit-Position: refs/heads/master@{#322414}
Committed: https://crrev.com/cf9d60e3d3d905de5ac5b1fc068ab3e61db41db5
Cr-Commit-Position: refs/heads/master@{#322718}
Patch Set 1 #
Total comments: 8
Patch Set 2 : review comments #
Total comments: 8
Patch Set 3 : review comment + no text_input_flags.h update #Patch Set 4 : test #Patch Set 5 : roooooll #Patch Set 6 : rebase #Patch Set 7 : add landmines :( #
Total comments: 1
Patch Set 8 : remove landmines #Messages
Total messages: 40 (13 generated)
mlamouri@chromium.org changed reviewers: + aurimas@chromium.org, nona@chromium.org
aurimas@, PTAL at content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java nona@, PTAL at ui/base/ime/text_input_flags.h
https://chromiumcodereview.appspot.com/1003143002/diff/1/content/public/andro... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://chromiumcodereview.appspot.com/1003143002/diff/1/content/public/andro... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:84: } else if (inputType == TextInputType.TEXT_AREA Can we split this check into to since we are doing different things for text area and content editable anyway? https://chromiumcodereview.appspot.com/1003143002/diff/1/content/public/andro... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:90: outAttrs.inputType |= InputType.TYPE_TEXT_FLAG_CAP_SENTENCES; Java requires to add {} for if statements that do not all fit in one line. https://chromiumcodereview.appspot.com/1003143002/diff/1/content/public/andro... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:131: // The virtual keyboard will filter out unexpected associations like That seems like a big assumption. Is there documentation somewhere about virtual keyboards doing that? https://chromiumcodereview.appspot.com/1003143002/diff/1/content/public/andro... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:134: outAttrs.inputType |= InputType.TYPE_TEXT_FLAG_CAP_CHARACTERS; Curly braces everywhere :)
PTAL https://codereview.chromium.org/1003143002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1003143002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:84: } else if (inputType == TextInputType.TEXT_AREA On 2015/03/18 at 18:57:11, aurimas wrote: > Can we split this check into to since we are doing different things for text area and content editable anyway? Instead of doing that, I moved the autocapitalization code for content editable with the rest of the others. That way, we can keep this common for both. https://codereview.chromium.org/1003143002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:90: outAttrs.inputType |= InputType.TYPE_TEXT_FLAG_CAP_SENTENCES; On 2015/03/18 at 18:57:11, aurimas wrote: > Java requires to add {} for if statements that do not all fit in one line. Done. https://codereview.chromium.org/1003143002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:131: // The virtual keyboard will filter out unexpected associations like On 2015/03/18 at 18:57:11, aurimas wrote: > That seems like a big assumption. Is there documentation somewhere about virtual keyboards doing that? Hmm, I did not really carry what I meant to say. The idea is really that the virtual keyboard should feel free to ignore values and given that they can, Chrome has no reason to step in between the web page and the virtual keyboard and decide what's the right decision. We can always revise that decision later if this is something that sounds wrong but I think it will be fine. https://codereview.chromium.org/1003143002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:134: outAttrs.inputType |= InputType.TYPE_TEXT_FLAG_CAP_CHARACTERS; On 2015/03/18 at 18:57:11, aurimas wrote: > Curly braces everywhere :) \o/ ... sorry }o{
mlamouri@chromium.org changed reviewers: + shuchen@chromium.org - nona@chromium.org
AdapterInputConnection LGTM % nits https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:87: // Textarea autocapitalization is defined by WebTextInputFlags. This comment is out of place. All the capitalization is handled below, so we don't really need it here. https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:125: // Handling of autocapitalize. Blink will send the flag taking into Java line limit is 100 chars. Adjust all the comments to be wider. https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:129: // Autocapitalize is meant as a hint. It is okay for the virtual I am not sure that is comment "It is ok for the virtual keyboard to ignore..." adds much value. We can just say "Autocapitalize is meant as a hint for the virtual keyboard". https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:140: if (inputType == TextInputType.CONTENT_EDITABLE) Missing {}
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org Link to the patchset: https://codereview.chromium.org/1003143002/#ps40001 (title: "review comment + no text_input_flags.h update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1003143002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:87: // Textarea autocapitalization is defined by WebTextInputFlags. On 2015/03/19 at 22:12:17, aurimas wrote: > This comment is out of place. All the capitalization is handled below, so we don't really need it here. Done. https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:125: // Handling of autocapitalize. Blink will send the flag taking into On 2015/03/19 at 22:12:17, aurimas wrote: > Java line limit is 100 chars. Adjust all the comments to be wider. Done. https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:129: // Autocapitalize is meant as a hint. It is okay for the virtual On 2015/03/19 at 22:12:17, aurimas wrote: > I am not sure that is comment "It is ok for the virtual keyboard to ignore..." adds much value. We can just say "Autocapitalize is meant as a hint for the virtual keyboard". Done. https://codereview.chromium.org/1003143002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:140: if (inputType == TextInputType.CONTENT_EDITABLE) On 2015/03/19 at 22:12:17, aurimas wrote: > Missing {} Done.
Looks like no changes of IMF, so removing myself from reviewers.
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org Link to the patchset: https://codereview.chromium.org/1003143002/#ps80001 (title: "roooooll")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1003143002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org Link to the patchset: https://codereview.chromium.org/1003143002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1003143002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d1421a5faf9dc2d3b3cad10640576b24a092d9ba Cr-Commit-Position: refs/heads/master@{#322414}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1008153003/ by scheib@chromium.org. The reason for reverting is: compile fail on http://build.chromium.org/p/chromium.linux/builders/Android%20Arm64%20Builder... ../content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:128:error: cannot find symbol if ((inputFlags & WebTextInputFlags.AutocapitalizeCharacters) != 0) { .
mlamouri@chromium.org changed reviewers: + thakis@chromium.org - shuchen@chromium.org
thakis@, could you have a look at that change? I am sad that I have to use landmines but we are in a situation where try bots seem to be mostly clobbered and fine but not the main bots :( It will be very tedious and painful to try to fix that issue for that CL at that point.
can you fix the java header generation instead of adding a landmine? is it understood what's wrong with the java generation?
https://codereview.chromium.org/1003143002/diff/120001/build/get_landmines.py File build/get_landmines.py (right): https://codereview.chromium.org/1003143002/diff/120001/build/get_landmines.py... build/get_landmines.py:31: # landmine. ^ also see this
On 2015/03/26 at 22:11:46, thakis wrote: > https://codereview.chromium.org/1003143002/diff/120001/build/get_landmines.py > File build/get_landmines.py (right): > > https://codereview.chromium.org/1003143002/diff/120001/build/get_landmines.py... > build/get_landmines.py:31: # landmine. > ^ also see this As said in my chromium-dev email, I have no idea what's wrong. bauerb@ and I looked at it. I spent hours to try to understand what's happening. bauerb@ is on leave for the rest of the week and no one seem particularly interested to help.
Not understanding what's going wrong is not an excuse to land broken stuff. (I'm away until monday; maybe i can help on monday.) cjhopman understands java and build things well, maybe he can help. On Thu, Mar 26, 2015 at 3:20 PM, <mlamouri@chromium.org> wrote: > On 2015/03/26 at 22:11:46, thakis wrote: > >> https://codereview.chromium.org/1003143002/diff/120001/ >> build/get_landmines.py >> File build/get_landmines.py (right): >> > > > https://codereview.chromium.org/1003143002/diff/120001/ > build/get_landmines.py#newcode31 > >> build/get_landmines.py:31: # landmine. >> ^ also see this >> > > As said in my chromium-dev email, I have no idea what's wrong. bauerb@ > and I > looked at it. I spent hours to try to understand what's happening. bauerb@ > is on > leave for the rest of the week and no one seem particularly interested to > help. > > https://codereview.chromium.org/1003143002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Mar 26, 2015 at 3:27 PM, Nico Weber <thakis@chromium.org> wrote: > Not understanding what's going wrong is not an excuse to land broken stuff. > (I understand that this CL is likely just exposing existing brokenness.) > (I'm away until monday; maybe i can help on monday.) > > cjhopman understands java and build things well, maybe he can help. > > On Thu, Mar 26, 2015 at 3:20 PM, <mlamouri@chromium.org> wrote: > >> On 2015/03/26 at 22:11:46, thakis wrote: >> >>> https://codereview.chromium.org/1003143002/diff/120001/ >>> build/get_landmines.py >>> File build/get_landmines.py (right): >>> >> >> >> https://codereview.chromium.org/1003143002/diff/120001/ >> build/get_landmines.py#newcode31 >> >>> build/get_landmines.py:31: # landmine. >>> ^ also see this >>> >> >> As said in my chromium-dev email, I have no idea what's wrong. bauerb@ >> and I >> looked at it. I spent hours to try to understand what's happening. bauerb@ >> is on >> leave for the rest of the week and no one seem particularly interested to >> help. >> >> https://codereview.chromium.org/1003143002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/03/26 at 22:27:27, thakis wrote: > Not understanding what's going wrong is not an excuse to land broken stuff. > (I'm away until monday; maybe i can help on monday.) I am not trying to find excuses here. I spent at least an order of magnitude more time trying to find what the breakage was than writing the actual CL. I can't reproduce this locally and the build dependencies seem fine. I was only able to reproduce by sending to try or having bauerb@ log into bots. I can no longer do any of these. Even if I were to think that I might have found a solution, I couldn't try it except by landing the CL and hoping for the best. At that point, the question is really whether breaking the tree is less important that landing landmines... Unless someone is able to reproduce this locally obviously.
I finally found the cause of my troubles and sent a fix here: https://codereview.chromium.org/1040643003 I wasn't able to diagnose the issue because I was checking whether the .java files were correctly generated (and they were) but the issue was coming from the jar files not being updated actually.
On 2015/03/28 at 20:27:58, Mounir Lamouri wrote: > I finally found the cause of my troubles and sent a fix here: https://codereview.chromium.org/1040643003 > > I wasn't able to diagnose the issue because I was checking whether the .java files were correctly generated (and they were) but the issue was coming from the jar files not being updated actually. FWIW, it seems that it was the cause of a landmine last month: https://codereview.chromium.org/899403002
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org Link to the patchset: https://codereview.chromium.org/1003143002/#ps140001 (title: "remove landmines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1003143002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/cf9d60e3d3d905de5ac5b1fc068ab3e61db41db5 Cr-Commit-Position: refs/heads/master@{#322718}
Message was sent while issue was closed.
Thank you very much for getting to the bottom of this. |
