Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2764743004:
Add DocumentMarker::createGrammarMarker() and createSpellingMarker() (Closed)
DescriptionAdd DocumentMarker::createGrammarMarker() and createSpellingMarker()
DocumentMarkerController refactor part 5: replace DocumentMarker's
Grammar/Spelling-specific constructor with clearly-named createGrammarMarker()
and createSpellingMarker() methods, and update the Grammar/Spelling-specific DocumentMarkerController:addMarker() method to use these new methods.
BUG=
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move test case changes into createTextMatchMarker() CL #
Total comments: 6
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (12 generated)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
rlanday@chromium.org changed required reviewers: + yosin@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2764743004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2764743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:102: const String& description = emptyString); No need to introduce default parameter. Btw, it seems simpler to just renaming the existing ctor to DM::createSpellCheckMarker(type, startOffset, endOffset, description). It also reduces code duplication.
https://codereview.chromium.org/2764743004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2764743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:102: const String& description = emptyString); On 2017/03/22 at 01:42:51, Xiaocheng wrote: > No need to introduce default parameter. Oh oops, I think the default param is supposed to be in the DocumentMarkerController method for adding a marker. > Btw, it seems simpler to just renaming the existing ctor to DM::createSpellCheckMarker(type, startOffset, endOffset, description). It also reduces code duplication. That's an option, but then we lose the 1-to-1 mapping between MarkerType and the create method, and we have to worry about how to handle callers that pass a MarkerType other than Grammar or Spelling (we could use a bool but I don't like that either)
https://codereview.chromium.org/2764743004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2764743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:102: const String& description = emptyString); On 2017/03/22 at 01:51:30, rlanday wrote: > On 2017/03/22 at 01:42:51, Xiaocheng wrote: > > No need to introduce default parameter. > > Oh oops, I think the default param is supposed to be in the DocumentMarkerController method for adding a marker. > > > Btw, it seems simpler to just renaming the existing ctor to DM::createSpellCheckMarker(type, startOffset, endOffset, description). It also reduces code duplication. > > That's an option, but then we lose the 1-to-1 mapping between MarkerType and the create method, and we have to worry about how to handle callers that pass a MarkerType other than Grammar or Spelling (we could use a bool but I don't like that either) I'm fine with having two ctors.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
https://codereview.chromium.org/2764743004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2764743004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1467: document().markers().addTextMatchMarker(markerRange, false); Please exclude addTextMatchmarker() changes.
The CQ bit was checked by rlanday@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...
https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:142: static DocumentMarker* createGrammarMarker( I think we should pick whether we want to order these methods alphabetically or in order of the MarkerType enum, and use the same ordering here and in DocumentMarkerController for the add*Marker() methods
lgtm Please exclude "and fix some test cases..." from patch description.
On 2017/03/22 at 21:07:00, rlanday wrote: > https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): > > https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:142: static DocumentMarker* createGrammarMarker( > I think we should pick whether we want to order these methods alphabetically or in order of the MarkerType enum, and use the same ordering here and in DocumentMarkerController for the add*Marker() methods Does the ordering matter? For .h files, I'm fine as long as these functions are declared together. For .cpp files, nobody cares about ordering (at least I don't)...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add DocumentMarker::createGrammarMarker() and createSpellingMarker() DocumentMarkerController refactor part 5: replace DocumentMarker's Grammar/Spelling-specific constructor with clearly-named createGrammarMarker() and createSpellingMarker() methods. Update the Grammar/Spelling-specific DocumentMarkerController:addMarker() method to use these new methods, and fix some test cases using this method to create TextMatch markers. BUG= ========== to ========== Add DocumentMarker::createGrammarMarker() and createSpellingMarker() DocumentMarkerController refactor part 5: replace DocumentMarker's Grammar/Spelling-specific constructor with clearly-named createGrammarMarker() and createSpellingMarker() methods. Update the Grammar/Spelling-specific DocumentMarkerController:addMarker() method to use these new methods BUG= ==========
Description was changed from ========== Add DocumentMarker::createGrammarMarker() and createSpellingMarker() DocumentMarkerController refactor part 5: replace DocumentMarker's Grammar/Spelling-specific constructor with clearly-named createGrammarMarker() and createSpellingMarker() methods. Update the Grammar/Spelling-specific DocumentMarkerController:addMarker() method to use these new methods BUG= ========== to ========== Add DocumentMarker::createGrammarMarker() and createSpellingMarker() DocumentMarkerController refactor part 5: replace DocumentMarker's Grammar/Spelling-specific constructor with clearly-named createGrammarMarker() and createSpellingMarker() methods, and update the Grammar/Spelling-specific DocumentMarkerController:addMarker() method to use these new methods. BUG= ==========
lgtm w/ nits https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:154: return new DocumentMarker(Grammar, startOffset, endOffset, Could you add TODO that we should have DCHECK_LT(startOffset, endOffset)? https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:162: return new DocumentMarker(Spelling, startOffset, endOffset, Could you add TODO that we should have DCHECK_LT(startOffset, endOffset)? https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:142: static DocumentMarker* createGrammarMarker( On 2017/03/22 at 21:07:00, rlanday wrote: > I think we should pick whether we want to order these methods alphabetically or in order of the MarkerType enum, and use the same ordering here and in DocumentMarkerController for the add*Marker() methods My personal preference is alphabetical order, since we can do without context. But, there are many opinions. Please do reordering if you want in another CL. https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:73: if (type == DocumentMarker::Grammar) { Could you add TODO for introducing add{Grammaer,Spelling}Marker()? We want to expose DocumentMarker::MarkerType.
https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:154: return new DocumentMarker(Grammar, startOffset, endOffset, On 2017/03/23 at 04:01:27, yosin_UTC9 wrote: > Could you add TODO that we should have DCHECK_LT(startOffset, endOffset)? Just to clarify, why do you want me to add a TODO instead of just adding the DCHECK?
On 2017/03/23 at 04:38:21, rlanday wrote: > https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right): > > https://codereview.chromium.org/2764743004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:154: return new DocumentMarker(Grammar, startOffset, endOffset, > On 2017/03/23 at 04:01:27, yosin_UTC9 wrote: > > Could you add TODO that we should have DCHECK_LT(startOffset, endOffset)? > > Just to clarify, why do you want me to add a TODO instead of just adding the DCHECK? TL:DR; Some nasty callers may hit DCHECK Adding DCHECK is sanitize API of DMC, it is not goal of this patch. Having many goals is OK, but if these goals can be exclusive, we can make patch smaller. Imagine if some layout tests failed with DCHECK, we need to change callers then this patch becomes bigger to include these fixes. However, these fixes aren't related to introduce this API. When we have a patch introducing DCHECK, the patch explains nasty callers. Let's remember the reasons we want to have small patch are: 1. Ease of review 2. Easy to bisect 3. Reduce number of reverts; if there are dependent patches, we also need revert them
Obsoleted by https://codereview.chromium.org/2723663002, closing |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
