Implement chrome.languageSettingsPrivate custom spell check functions

Implement chrome.languageSettingsPrivate custom spell check functions

Implement functions relating to the custom spell check dictionary. These
functions will be used by the Material Design Settings page.

BUG=479043
Committed: https://crrev.com/3c03efbea3549d485ace8ef14859833163ec3972
Cr-Commit-Position: refs/heads/master@{#355910}

[michaelpg, 2015-09-28 22:31:58]

Patchset #2 (id:20001) has been deleted

[michaelpg, 2015-09-28 22:35:16]

michaelpg@chromium.org changed reviewers:
+ asvitkine@google.com, finnur@chromium.org, stevenjb@chromium.org

[michaelpg, 2015-09-28 22:35:16]

michaelpg@chromium.org changed required reviewers:
+ asvitkine@google.com, finnur@chromium.org

[michaelpg, 2015-09-28 22:35:17]

PTAL, thanks.

finnur@ for extensions
asvkitkine@ for metrics

[michaelpg, 2015-09-28 22:35:40]

michaelpg@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[michaelpg, 2015-09-28 22:35:40]

michaelpg@chromium.org changed required reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[michaelpg, 2015-09-28 23:57:04]

michaelpg@chromium.org changed reviewers:
+ dbeam@chromium.org

[michaelpg, 2015-09-28 23:57:05]

michaelpg@chromium.org changed required reviewers:
+ dbeam@chromium.org

[michaelpg, 2015-09-28 23:57:05]

+dbeam for externs

[stevenjb, 2015-09-28 23:58:11]

lgtm

https://codereview.chromium.org/1373073003/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
(right):

https://codereview.chromium.org/1373073003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:205:
const SpellcheckCustomDictionary::Change& dictionary_change) {
nit: NOTREACHED() << "SpellcheckCustomDictionary Changed() called before
Loaded()";
(mostly to help document)

[Dan Beam, 2015-09-29 00:04:24]

lgtm

[michaelpg, 2015-09-29 00:27:24]

https://codereview.chromium.org/1373073003/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
(right):

https://codereview.chromium.org/1373073003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:205:
const SpellcheckCustomDictionary::Change& dictionary_change) {
On 2015/09/28 23:58:10, stevenjb wrote:
> nit: NOTREACHED() << "SpellcheckCustomDictionary Changed() called before
> Loaded()";
> (mostly to help document)

Done.

[Finnur, 2015-09-29 11:00:25]

General comment before I dive in...
BUG= seems to point to wrong bug, no?

[Finnur, 2015-09-29 11:04:47]

Apart from my general comment, this LGTM.

[Alexei Svitkine (slow), 2015-09-29 14:24:20]

lgtm

[michaelpg, 2015-09-29 20:51:32]

On 2015/09/29 11:00:25, Finnur wrote:
> General comment before I dive in...
> BUG= seems to point to wrong bug, no?

Good catch, thanks!

[michaelpg, 2015-09-29 20:51:50]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-09-29 20:51:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1373073003/#ps60001
(title: "add notreached")

[commit-bot: I haz the power, 2015-09-29 20:54:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373073003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373073003/60001

[commit-bot: I haz the power, 2015-09-29 20:57:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-29 20:57:14]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[michaelpg, 2015-09-30 23:17:30]

Patchset #5 (id:100001) has been deleted

[michaelpg, 2015-09-30 23:18:49]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-09-30 23:18:49]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
stevenjb@chromium.org, asvitkine@chromium.org, dbeam@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1373073003/#ps120001 (title: "branch off
master")

[commit-bot: I haz the power, 2015-09-30 23:20:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373073003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373073003/120001

[commit-bot: I haz the power, 2015-10-01 02:07:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 02:07:44]

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...)

[michaelpg, 2015-10-01 17:28:27]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2015-10-01 17:29:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373073003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373073003/120001

[michaelpg, 2015-10-01 18:04:37]

Patchset #6 (id:140001) has been deleted

[michaelpg, 2015-10-21 04:00:00]

isherman/asvitkine: PTAL at the histograms.xml question.

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:59072: +  <int value="1073"
label="FILEMANAGERPRIVATE_CANCELALLFILETRANSFERS"/>
These entries were shifted as a result of a rebase; is that bad?

The actual rebase conflict only showed an issue with the last 3 entries
(TABCAPTURE_CAPTUREOFFSCREENTAB was added, which conflicted with my addition of
the last two LANGUAGESETTINGSPRIVATE items). So I could have fixed it manually.
But since that caused problems last time[1], I ran
update_extension_histograms.py this time, and this was the result.

How do I fix this?

[1]
https://codereview.chromium.org/1274753006/diff/100001/tools/metrics/histogra...

[Alexei Svitkine (slow), 2015-10-21 15:45:30]

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:59058: +  <int value="1060"
label="FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS"/>
So the issue was that GETENTRYACTIONS was added smack in the middle of the enum
without anyone noticing? How long ago was this / did this make it into a branch
cut yet? If not, I suggest renumbering it to the end. Then, the bad numbering
will be limited to only a few versions.

[stevenjb, 2015-10-21 16:58:29]

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:59058: +  <int value="1060"
label="FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS"/>
On 2015/10/21 15:45:30, Alexei Svitkine (slow) wrote:
> So the issue was that GETENTRYACTIONS was added smack in the middle of the
enum
> without anyone noticing? How long ago was this / did this make it into a
branch
> cut yet? If not, I suggest renumbering it to the end. Then, the bad numbering
> will be limited to only a few versions.

FWIW, I ran into something like this when I ran 'python
tools/metrics/histograms/update_extension_histograms.py' on another CL. Whatever
we do we should make sure that script produces correct output moving forward.

[Alexei Svitkine (slow), 2015-10-21 17:09:57]

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:59058: +  <int value="1060"
label="FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS"/>
On 2015/10/21 16:58:29, stevenjb wrote:
> On 2015/10/21 15:45:30, Alexei Svitkine (slow) wrote:
> > So the issue was that GETENTRYACTIONS was added smack in the middle of the
> enum
> > without anyone noticing? How long ago was this / did this make it into a
> branch
> > cut yet? If not, I suggest renumbering it to the end. Then, the bad
numbering
> > will be limited to only a few versions.
> 
> FWIW, I ran into something like this when I ran 'python
> tools/metrics/histograms/update_extension_histograms.py' on another CL.
Whatever
> we do we should make sure that script produces correct output moving forward.

To follow-up, this is the bad CL:

https://codereview.chromium.org/1239043002

Looks like it meant to rename FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS and 
FILEMANAGERPRIVATEINTERNAL_EXECUTEENTRYACTION, but instead ended up adding the
new ones without deleting the other ones.

I think the correct fix is to remove those ones. Can you do it as part of this
CL? I think when you do that, the diff will look much better and will also fix
the issue with the data (i.e. since that CL landed, all the values have been
mislabeled in UMA, but it's only been 4 days).

[stevenjb, 2015-10-21 17:21:29]

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:59058: +  <int value="1060"
label="FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS"/>
On 2015/10/21 17:09:57, Alexei Svitkine (slow) wrote:
> On 2015/10/21 16:58:29, stevenjb wrote:
> > On 2015/10/21 15:45:30, Alexei Svitkine (slow) wrote:
> > > So the issue was that GETENTRYACTIONS was added smack in the middle of the
> > enum
> > > without anyone noticing? How long ago was this / did this make it into a
> > branch
> > > cut yet? If not, I suggest renumbering it to the end. Then, the bad
> numbering
> > > will be limited to only a few versions.
> > 
> > FWIW, I ran into something like this when I ran 'python
> > tools/metrics/histograms/update_extension_histograms.py' on another CL.
> Whatever
> > we do we should make sure that script produces correct output moving
forward.
> 
> To follow-up, this is the bad CL:
> 
> https://codereview.chromium.org/1239043002
> 
> Looks like it meant to rename FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS and 
> FILEMANAGERPRIVATEINTERNAL_EXECUTEENTRYACTION, but instead ended up adding the
> new ones without deleting the other ones.
> 
> I think the correct fix is to remove those ones. Can you do it as part of this
> CL? I think when you do that, the diff will look much better and will also fix
> the issue with the data (i.e. since that CL landed, all the values have been
> mislabeled in UMA, but it's only been 4 days).

I would suggest doing the fix in a separate CL.

[Alexei Svitkine (slow), 2015-10-21 17:30:35]

Separate CL SGTM too. Whatever it is, please do it ASAP before more people
run into this problem. (I guess another alternative is to revert the bad
CL, but I don't know if any dependent things landed already.)

On Wed, Oct 21, 2015 at 1:21 PM, <stevenjb@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
>
>
>
https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:59058: +  <int value="1060"
> label="FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS"/>
> On 2015/10/21 17:09:57, Alexei Svitkine (slow) wrote:
>
>> On 2015/10/21 16:58:29, stevenjb wrote:
>> > On 2015/10/21 15:45:30, Alexei Svitkine (slow) wrote:
>> > > So the issue was that GETENTRYACTIONS was added smack in the
>>
> middle of the
>
>> > enum
>> > > without anyone noticing? How long ago was this / did this make it
>>
> into a
>
>> > branch
>> > > cut yet? If not, I suggest renumbering it to the end. Then, the
>>
> bad
>
>> numbering
>> > > will be limited to only a few versions.
>> >
>> > FWIW, I ran into something like this when I ran 'python
>> > tools/metrics/histograms/update_extension_histograms.py' on another
>>
> CL.
>
>> Whatever
>> > we do we should make sure that script produces correct output moving
>>
> forward.
>
> To follow-up, this is the bad CL:
>>
>
> https://codereview.chromium.org/1239043002
>>
>
> Looks like it meant to rename
>>
> FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS and
>
>> FILEMANAGERPRIVATEINTERNAL_EXECUTEENTRYACTION, but instead ended up
>>
> adding the
>
>> new ones without deleting the other ones.
>>
>
> I think the correct fix is to remove those ones. Can you do it as part
>>
> of this
>
>> CL? I think when you do that, the diff will look much better and will
>>
> also fix
>
>> the issue with the data (i.e. since that CL landed, all the values
>>
> have been
>
>> mislabeled in UMA, but it's only been 4 days).
>>
>
> I would suggest doing the fix in a separate CL.
>
> https://codereview.chromium.org/1373073003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[michaelpg, 2015-10-21 18:04:39]

Patchset #7 (id:180001) has been deleted

[michaelpg, 2015-10-21 18:06:30]

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1373073003/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:59058: +  <int value="1060"
label="FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS"/>
On 2015/10/21 17:21:28, stevenjb wrote:
> On 2015/10/21 17:09:57, Alexei Svitkine (slow) wrote:
> > On 2015/10/21 16:58:29, stevenjb wrote:
> > > On 2015/10/21 15:45:30, Alexei Svitkine (slow) wrote:
> > > > So the issue was that GETENTRYACTIONS was added smack in the middle of
the
> > > enum
> > > > without anyone noticing? How long ago was this / did this make it into a
> > > branch
> > > > cut yet? If not, I suggest renumbering it to the end. Then, the bad
> > numbering
> > > > will be limited to only a few versions.
> > > 
> > > FWIW, I ran into something like this when I ran 'python
> > > tools/metrics/histograms/update_extension_histograms.py' on another CL.
> > Whatever
> > > we do we should make sure that script produces correct output moving
> forward.
> > 
> > To follow-up, this is the bad CL:
> > 
> > https://codereview.chromium.org/1239043002
> > 
> > Looks like it meant to rename FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS and

> > FILEMANAGERPRIVATEINTERNAL_EXECUTEENTRYACTION, but instead ended up adding
the
> > new ones without deleting the other ones.
> > 
> > I think the correct fix is to remove those ones. Can you do it as part of
this
> > CL? I think when you do that, the diff will look much better and will also
fix
> > the issue with the data (i.e. since that CL landed, all the values have been
> > mislabeled in UMA, but it's only been 4 days).
> 
> I would suggest doing the fix in a separate CL.

Done: https://codereview.chromium.org/1414263003/

[michaelpg, 2015-10-23 20:48:10]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-10-23 20:48:10]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
stevenjb@chromium.org, asvitkine@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1373073003/#ps200001
(title: "rebase on histogram fixes")

[commit-bot: I haz the power, 2015-10-23 20:49:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373073003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373073003/200001

[commit-bot: I haz the power, 2015-10-23 22:39:19]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-10-23 22:40:07]

Patchset 7 (id:??) landed as
https://crrev.com/3c03efbea3549d485ace8ef14859833163ec3972
Cr-Commit-Position: refs/heads/master@{#355910}