Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792233003:
Split SpellCheckPanel off SpellCheckProvider (Closed)
DescriptionSplit SpellCheckPanel off SpellCheckProvider
This is Patch 3 of 6 for making SpellCheckProvider a RenderFrameObserver, so
that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk
This patch splits SpellCheckProvider into two classes: functions related to
spelling panel are moved to a new class SpellCheckPanel, and the remaining
functions remain in SpellCheckProvider.
This patch is a preparation for changing SpellCheckProvider into a
RenderFrameObserver.
BUG=638361
Review-Url: https://codereview.chromium.org/2792233003
Cr-Commit-Position: refs/heads/master@{#462335}
Committed: https://chromium.googlesource.com/chromium/src/+/dd47f6d8f5b234da6ec4bf952d44ff73c01bcc36
Patch Set 1 #Patch Set 2 : Tue Apr 4 15:05:22 PDT 2017 #
Total comments: 9
Patch Set 3 : Wed Apr 5 11:05:41 PDT 2017 #Patch Set 4 : Fix compile error #Patch Set 5 : Fix further compile error #
Total comments: 11
Patch Set 6 : Wed Apr 5 15:14:11 PDT 2017 #Patch Set 7 : Remove histograms.xml from patch #
Dependent Patchsets: Messages
Total messages: 42 (24 generated)
Description was changed from ========== Split SpellCheckPanel off SpellCheckProvider BUG=638361 ========== to ========== Split SpellCheckPanel off SpellCheckProvider This is Patch 3 of 5 for making SpellCheckProvider a RenderFrameObserver, so that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk This patch splits SpellCheckProvider into two classes: functions related to spelling panel are moved to a new class SpellCheckPanel, and the remaining functions remain in SpellCheckProvider. This patch is a preparation for changing SpellCheckProvider into a RenderFrameObserver. BUG=638361 ==========
xiaochengh@chromium.org changed reviewers: + groby@chromium.org, rouslan@chromium.org
PTAL. One UMA is moved from spellcheck_provider.cc to spellcheck_panel.cc, and triggers a presubmit warning that it's not in histograms.xml. I found that none of SpellCheck.api.* is listed in histograms.xml. Should they be added? https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.cc (right): https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.cc:27: UMA_HISTOGRAM_BOOLEAN("SpellCheck.api.showUI", show); Presubmit warning: Some UMA_HISTOGRAM lines have been modified and the associated histogram name has no match in either tools/metrics/histograms/histograms.xml or the modifications of it: [components/spellcheck/renderer/spellcheck_panel.cc:27] SpellCheck.api.showUI
https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.cc (right): https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.cc:27: UMA_HISTOGRAM_BOOLEAN("SpellCheck.api.showUI", show); On 2017/04/04 22:16:48, Xiaocheng wrote: > Presubmit warning: > > Some UMA_HISTOGRAM lines have been modified and the associated histogram name > has no match in either tools/metrics/histograms/histograms.xml or the > modifications of it: > [components/spellcheck/renderer/spellcheck_panel.cc:27] SpellCheck.api.showUI I guess you need to update histograms.xml. https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.cc:44: bool SpellCheckPanel::OnMessageReceived(const IPC::Message& message) { The parent class has a concrete implementation. Seems reasonable that it should be invoked from the child to avoid losing some functionality, right? https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.h (right): https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.h:19: SpellCheckPanel(content::RenderView* render_view); explicit. https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.h:38: #endif Only Mac has a "spelling UI". USE_BROWSER_SPELLCHECKER is always true on Mac. Can you remove the #if check and make sure that this file is used/compiled only on Mac?
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaochengh@chromium.org changed reviewers: + isherman@chromium.org, thestig@chromium.org
+isherman +thestig Updated. PTAL. https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.cc (right): https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.cc:27: UMA_HISTOGRAM_BOOLEAN("SpellCheck.api.showUI", show); On 2017/04/05 at 15:17:40, ಠ_ಠ wrote: > On 2017/04/04 22:16:48, Xiaocheng wrote: > > Presubmit warning: > > > > Some UMA_HISTOGRAM lines have been modified and the associated histogram name > > has no match in either tools/metrics/histograms/histograms.xml or the > > modifications of it: > > [components/spellcheck/renderer/spellcheck_panel.cc:27] SpellCheck.api.showUI > > I guess you need to update histograms.xml. Done. https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.cc:44: bool SpellCheckPanel::OnMessageReceived(const IPC::Message& message) { On 2017/04/05 at 15:17:40, ಠ_ಠ wrote: > The parent class has a concrete implementation. Seems reasonable that it should be invoked from the child to avoid losing some functionality, right? I think that's how RenderViewObservers are supposed to work. https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.h (right): https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.h:19: SpellCheckPanel(content::RenderView* render_view); On 2017/04/05 at 15:17:40, ಠ_ಠ wrote: > explicit. Done. https://codereview.chromium.org/2792233003/diff/20001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.h:38: #endif On 2017/04/05 at 15:17:40, ಠ_ಠ wrote: > Only Mac has a "spelling UI". USE_BROWSER_SPELLCHECKER is always true on Mac. Can you remove the #if check and make sure that this file is used/compiled only on Mac? In order not to break anything, I'm not going to introduce any behavior change in this patch. I added a TODO, and will do that after fixing the bug.
Description was changed from ========== Split SpellCheckPanel off SpellCheckProvider This is Patch 3 of 5 for making SpellCheckProvider a RenderFrameObserver, so that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk This patch splits SpellCheckProvider into two classes: functions related to spelling panel are moved to a new class SpellCheckPanel, and the remaining functions remain in SpellCheckProvider. This patch is a preparation for changing SpellCheckProvider into a RenderFrameObserver. BUG=638361 ========== to ========== Split SpellCheckPanel off SpellCheckProvider This is Patch 3 of 6 for making SpellCheckProvider a RenderFrameObserver, so that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk This patch splits SpellCheckProvider into two classes: functions related to spelling panel are moved to a new class SpellCheckPanel, and the remaining functions remain in SpellCheckProvider. This patch is a preparation for changing SpellCheckProvider into a RenderFrameObserver. BUG=638361 ==========
lgtm
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for tackling - LGTM (And hopefully, the #ifdef's will be removed soon?) https://codereview.chromium.org/2792233003/diff/80001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2792233003/diff/80001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:577: new SpellCheckPanel(render_view); Since the panel is OSX only, can we add it on OSX only? https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... File components/spellcheck/renderer/BUILD.gn (right): https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... components/spellcheck/renderer/BUILD.gn:19: "spellcheck_panel.cc", Same here - it's OSX specific. https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.cc (right): https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.cc:19: if (render_view) // NULL in unit tests. I don't think that's still true, given we don't have SpellCheckPannel unit tests? https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.h (right): https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.h:16: // used only on Mac. Ah, I see there's a TODO - I assume this will be implemented soonish?
https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:67834: +<histogram name="SpellCheck.api.showUI" enum="BooleanEnabled"> nit: (Just within this file) please use an enum with more custom-tailored labels, like "Native OS platform UI" vs. "Chrome UI" (or whatever labels are appropriate). https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:67834: +<histogram name="SpellCheck.api.showUI" enum="BooleanEnabled"> nit: I have some quibbles with this metric name, but they're subsumed by my comment below. Mostly noting this as a reminder to myself, in case you convince me that this metric's semantics make sense as-is. https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:67838: + The number of times the platform spelling UI was enabled/disabled. What is the intended use of this histogram? This sort of metric tends to be notoriously hard to use, because it biases toward users who flip the state more frequently. A user who chooses a state once and then never modifies it again will be almost invisible. I'd recommend instead sampling this state periodically. For example, you might want to record whether or not the platform UI is enabled each time that a spelling suggestion is offered.
Thanks for reviewing! https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.cc (right): https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.cc:19: if (render_view) // NULL in unit tests. On 2017/04/05 at 21:19:54, groby wrote: > I don't think that's still true, given we don't have SpellCheckPannel unit tests? The nullptr check is removed. https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... File components/spellcheck/renderer/spellcheck_panel.h (right): https://codereview.chromium.org/2792233003/diff/80001/components/spellcheck/r... components/spellcheck/renderer/spellcheck_panel.h:16: // used only on Mac. On 2017/04/05 at 21:19:54, groby wrote: > Ah, I see there's a TODO - I assume this will be implemented soonish? Should be, after fixing this bug :) https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:67838: + The number of times the platform spelling UI was enabled/disabled. On 2017/04/05 at 21:43:54, Ilya Sherman wrote: > What is the intended use of this histogram? This sort of metric tends to be notoriously hard to use, because it biases toward users who flip the state more frequently. A user who chooses a state once and then never modifies it again will be almost invisible. > > I'd recommend instead sampling this state periodically. For example, you might want to record whether or not the platform UI is enabled each time that a spelling suggestion is offered. Thanks for the idea. This patch is for fixing crbug.com/638361. This metric, being irrelevant to the bug fix, is moved from spellcheck_provider.cc to spellcheck_panel.cc as is. I think I should make sure in this patch that the description added here matches exactly what is shown in the UMA dashboard now, without introducing any change. A better design of this metric needs more discussion and is out of the scope of this patch. I filed crbug.com/708791 for it.
https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:67838: + The number of times the platform spelling UI was enabled/disabled. On 2017/04/05 22:17:48, Xiaocheng wrote: > On 2017/04/05 at 21:43:54, Ilya Sherman wrote: > > What is the intended use of this histogram? This sort of metric tends to be > notoriously hard to use, because it biases toward users who flip the state more > frequently. A user who chooses a state once and then never modifies it again > will be almost invisible. > > > > I'd recommend instead sampling this state periodically. For example, you > might want to record whether or not the platform UI is enabled each time that a > spelling suggestion is offered. > > Thanks for the idea. > > This patch is for fixing crbug.com/638361. This metric, being irrelevant to the > bug fix, is moved from spellcheck_provider.cc to spellcheck_panel.cc as is. I > think I should make sure in this patch that the description added here matches > exactly what is shown in the UMA dashboard now, without introducing any change. > > A better design of this metric needs more discussion and is out of the scope of > this patch. I filed crbug.com/708791 for it. I see. Is there a corresponding CL to remove the metric definition from google3?
On 2017/04/05 at 22:54:07, isherman wrote: > https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:67838: + The number of times the platform spelling UI was enabled/disabled. > On 2017/04/05 22:17:48, Xiaocheng wrote: > > On 2017/04/05 at 21:43:54, Ilya Sherman wrote: > > > What is the intended use of this histogram? This sort of metric tends to be > > notoriously hard to use, because it biases toward users who flip the state more > > frequently. A user who chooses a state once and then never modifies it again > > will be almost invisible. > > > > > > I'd recommend instead sampling this state periodically. For example, you > > might want to record whether or not the platform UI is enabled each time that a > > spelling suggestion is offered. > > > > Thanks for the idea. > > > > This patch is for fixing crbug.com/638361. This metric, being irrelevant to the > > bug fix, is moved from spellcheck_provider.cc to spellcheck_panel.cc as is. I > > think I should make sure in this patch that the description added here matches > > exactly what is shown in the UMA dashboard now, without introducing any change. > > > > A better design of this metric needs more discussion and is out of the scope of > > this patch. I filed crbug.com/708791 for it. > > I see. Is there a corresponding CL to remove the metric definition from google3? Not yet... Will do. Should I move all SpellCheck.* metrics from google3 to Chromium? Or just this one? Is there a guideline for where metric definitions should be put? Sorry for having a bunch of question. I haven't touched any UMA before.
On 2017/04/05 23:01:12, Xiaocheng wrote: > On 2017/04/05 at 22:54:07, isherman wrote: > > > https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:67838: + The number of times the > platform spelling UI was enabled/disabled. > > On 2017/04/05 22:17:48, Xiaocheng wrote: > > > On 2017/04/05 at 21:43:54, Ilya Sherman wrote: > > > > What is the intended use of this histogram? This sort of metric tends to > be > > > notoriously hard to use, because it biases toward users who flip the state > more > > > frequently. A user who chooses a state once and then never modifies it > again > > > will be almost invisible. > > > > > > > > I'd recommend instead sampling this state periodically. For example, you > > > might want to record whether or not the platform UI is enabled each time > that a > > > spelling suggestion is offered. > > > > > > Thanks for the idea. > > > > > > This patch is for fixing crbug.com/638361. This metric, being irrelevant to > the > > > bug fix, is moved from spellcheck_provider.cc to spellcheck_panel.cc as is. > I > > > think I should make sure in this patch that the description added here > matches > > > exactly what is shown in the UMA dashboard now, without introducing any > change. > > > > > > A better design of this metric needs more discussion and is out of the scope > of > > > this patch. I filed crbug.com/708791 for it. > > > > I see. Is there a corresponding CL to remove the metric definition from > google3? > > Not yet... Will do. > > Should I move all SpellCheck.* metrics from google3 to Chromium? Or just this > one? > > Is there a guideline for where metric definitions should be put? > > Sorry for having a bunch of question. I haven't touched any UMA before. That's a good question. All histogram descriptions used to be internal, because a few mentioned work that wasn't open sourced. Several years ago we switched to having histogram descriptions be open source by default, but not all histograms were migrated. If the histogram descriptions do not mention anything confidential -- and I'd rather expect the spellcheck histograms not to -- then it's fine to open source all of them. I'd suggest dropping the histograms.xml change from this CL, and open-sourcing all of these metrics in a separate CL. Thanks!
On 2017/04/05 at 23:03:49, isherman wrote: > On 2017/04/05 23:01:12, Xiaocheng wrote: > > On 2017/04/05 at 22:54:07, isherman wrote: > > > > > https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... > > > tools/metrics/histograms/histograms.xml:67838: + The number of times the > > platform spelling UI was enabled/disabled. > > > On 2017/04/05 22:17:48, Xiaocheng wrote: > > > > On 2017/04/05 at 21:43:54, Ilya Sherman wrote: > > > > > What is the intended use of this histogram? This sort of metric tends to > > be > > > > notoriously hard to use, because it biases toward users who flip the state > > more > > > > frequently. A user who chooses a state once and then never modifies it > > again > > > > will be almost invisible. > > > > > > > > > > I'd recommend instead sampling this state periodically. For example, you > > > > might want to record whether or not the platform UI is enabled each time > > that a > > > > spelling suggestion is offered. > > > > > > > > Thanks for the idea. > > > > > > > > This patch is for fixing crbug.com/638361. This metric, being irrelevant to > > the > > > > bug fix, is moved from spellcheck_provider.cc to spellcheck_panel.cc as is. > > I > > > > think I should make sure in this patch that the description added here > > matches > > > > exactly what is shown in the UMA dashboard now, without introducing any > > change. > > > > > > > > A better design of this metric needs more discussion and is out of the scope > > of > > > > this patch. I filed crbug.com/708791 for it. > > > > > > I see. Is there a corresponding CL to remove the metric definition from > > google3? > > > > Not yet... Will do. > > > > Should I move all SpellCheck.* metrics from google3 to Chromium? Or just this > > one? > > > > Is there a guideline for where metric definitions should be put? > > > > Sorry for having a bunch of question. I haven't touched any UMA before. > > That's a good question. All histogram descriptions used to be internal, because a few mentioned work that wasn't open sourced. Several years ago we switched to having histogram descriptions be open source by default, but not all histograms were migrated. If the histogram descriptions do not mention anything confidential -- and I'd rather expect the spellcheck histograms not to -- then it's fine to open source all of them. > > I'd suggest dropping the histograms.xml change from this CL, and open-sourcing all of these metrics in a separate CL. Thanks! I see. In this case, can we land this CL without changing histograms.xml (since there is already internal definition, though there is presubmit warning), and move the definitions separately?
On 2017/04/05 23:14:37, Xiaocheng wrote: > On 2017/04/05 at 23:03:49, isherman wrote: > > On 2017/04/05 23:01:12, Xiaocheng wrote: > > > On 2017/04/05 at 22:54:07, isherman wrote: > > > > > > > > https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > https://codereview.chromium.org/2792233003/diff/80001/tools/metrics/histogram... > > > > tools/metrics/histograms/histograms.xml:67838: + The number of times > the > > > platform spelling UI was enabled/disabled. > > > > On 2017/04/05 22:17:48, Xiaocheng wrote: > > > > > On 2017/04/05 at 21:43:54, Ilya Sherman wrote: > > > > > > What is the intended use of this histogram? This sort of metric tends > to > > > be > > > > > notoriously hard to use, because it biases toward users who flip the > state > > > more > > > > > frequently. A user who chooses a state once and then never modifies it > > > again > > > > > will be almost invisible. > > > > > > > > > > > > I'd recommend instead sampling this state periodically. For example, > you > > > > > might want to record whether or not the platform UI is enabled each time > > > that a > > > > > spelling suggestion is offered. > > > > > > > > > > Thanks for the idea. > > > > > > > > > > This patch is for fixing crbug.com/638361. This metric, being irrelevant > to > > > the > > > > > bug fix, is moved from spellcheck_provider.cc to spellcheck_panel.cc as > is. > > > I > > > > > think I should make sure in this patch that the description added here > > > matches > > > > > exactly what is shown in the UMA dashboard now, without introducing any > > > change. > > > > > > > > > > A better design of this metric needs more discussion and is out of the > scope > > > of > > > > > this patch. I filed crbug.com/708791 for it. > > > > > > > > I see. Is there a corresponding CL to remove the metric definition from > > > google3? > > > > > > Not yet... Will do. > > > > > > Should I move all SpellCheck.* metrics from google3 to Chromium? Or just > this > > > one? > > > > > > Is there a guideline for where metric definitions should be put? > > > > > > Sorry for having a bunch of question. I haven't touched any UMA before. > > > > That's a good question. All histogram descriptions used to be internal, > because a few mentioned work that wasn't open sourced. Several years ago we > switched to having histogram descriptions be open source by default, but not all > histograms were migrated. If the histogram descriptions do not mention anything > confidential -- and I'd rather expect the spellcheck histograms not to -- then > it's fine to open source all of them. > > > > I'd suggest dropping the histograms.xml change from this CL, and open-sourcing > all of these metrics in a separate CL. Thanks! > > I see. > > In this case, can we land this CL without changing histograms.xml (since there > is already internal definition, though there is presubmit warning), and move the > definitions separately? Correct =)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated with histograms.xml removed from patch.
On 2017/04/05 23:27:36, Xiaocheng wrote: > Updated with histograms.xml removed from patch. Thanks. I believe you no longer need my review on this CL, and can go ahead and commit it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaochengh@chromium.org
Thanks for all comments!
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, rouslan@chromium.org, groby@chromium.org Link to the patchset: https://codereview.chromium.org/2792233003/#ps120001 (title: "Remove histograms.xml from patch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491446836183830,
"parent_rev": "647d3778e7f445c6d4ba9ca9a822278cd63bf839", "commit_rev":
"dd47f6d8f5b234da6ec4bf952d44ff73c01bcc36"}
Message was sent while issue was closed.
Description was changed from ========== Split SpellCheckPanel off SpellCheckProvider This is Patch 3 of 6 for making SpellCheckProvider a RenderFrameObserver, so that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk This patch splits SpellCheckProvider into two classes: functions related to spelling panel are moved to a new class SpellCheckPanel, and the remaining functions remain in SpellCheckProvider. This patch is a preparation for changing SpellCheckProvider into a RenderFrameObserver. BUG=638361 ========== to ========== Split SpellCheckPanel off SpellCheckProvider This is Patch 3 of 6 for making SpellCheckProvider a RenderFrameObserver, so that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk This patch splits SpellCheckProvider into two classes: functions related to spelling panel are moved to a new class SpellCheckPanel, and the remaining functions remain in SpellCheckProvider. This patch is a preparation for changing SpellCheckProvider into a RenderFrameObserver. BUG=638361 Review-Url: https://codereview.chromium.org/2792233003 Cr-Commit-Position: refs/heads/master@{#462335} Committed: https://chromium.googlesource.com/chromium/src/+/dd47f6d8f5b234da6ec4bf952d44... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/dd47f6d8f5b234da6ec4bf952d44... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
