Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
419563003:
Adding a word to dictionary should remove spelling markers (Closed)
|
Created:
6 years, 5 months ago by Klemen Forstnerič Modified:
6 years, 4 months ago CC:
dcheng, abarth-chromium, blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, groby+blinkspell_chromium.org, jamesr, rwlbuis, sof, please use gerrit instead, rouslan+spellwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionAdding a word to dictionary should remove spelling markers.
This patch adds code for removing spelling markers under words on
blink's side.
BUG=3506
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180382
Patch Set 1 #Patch Set 2 : Added a test to WebFrameTest #Patch Set 3 : Rebased to a newer master #Patch Set 4 : Tidied the patch a bit #
Total comments: 2
Patch Set 5 : Removed the removeSpellingMarkersUnderWords method from WebRemoteFrameImpl #
Total comments: 12
Patch Set 6 : Addressed comment #
Total comments: 13
Patch Set 7 : Addressed dcheng's comment #
Total comments: 12
Patch Set 8 : Addressed yosin's comments #
Total comments: 4
Patch Set 9 : Addressed yosin's second comment #
Total comments: 1
Patch Set 10 : Addressed my own nit #
Total comments: 19
Patch Set 11 : Addressed tkent's comment #Patch Set 12 : Removed the WebLocalFrameImpl indirection #Patch Set 13 : Rebased to a newer master #
Total comments: 7
Messages
Total messages: 36 (0 generated)
jamesr: PTAL Patch Set 4.
https://codereview.chromium.org/419563003/diff/60001/public/web/WebFrame.h File public/web/WebFrame.h (right): https://codereview.chromium.org/419563003/diff/60001/public/web/WebFrame.h#ne... public/web/WebFrame.h:443: Please consider adding this new member to WebLocalFrame instead, as I don't think it's ever going to be implemented for WebRemoteFrames (as it makes no sense to call on a remote frame).
https://codereview.chromium.org/419563003/diff/60001/public/web/WebFrame.h File public/web/WebFrame.h (right): https://codereview.chromium.org/419563003/diff/60001/public/web/WebFrame.h#ne... public/web/WebFrame.h:443: On 2014/07/30 18:18:50, dcheng (OOO) wrote: > Please consider adding this new member to WebLocalFrame instead, as I don't > think it's ever going to be implemented for WebRemoteFrames (as it makes no > sense to call on a remote frame). Done.
drive-by comments https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:420: DocumentMarkerAndNodeVector DocumentMarkerController::markerNodePairs() This seems to copy a lot of data, depending on the amount of markers. Why not a removeMarker that takes a predicate instead? https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... Source/core/editing/SpellChecker.cpp:828: { You might want to test this for large documents. IIRC, whole-document checking was definitely too slow. It might be that removal is fine, since it doesn't involve any IPC, but it's worth trying out. https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1845: [] (const WebString &webString) { return static_cast<String>(webString); }); Is blink switched to C11? Chrome style guide says we are not there yet - I don't think all of our compilers are ready yet.
https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:420: DocumentMarkerAndNodeVector DocumentMarkerController::markerNodePairs() On 2014/07/30 21:13:27, groby wrote: > This seems to copy a lot of data, depending on the amount of markers. Why not a > removeMarker that takes a predicate instead? That seems like a great idea, I'll add a removeMarkers method with a predicate. https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... Source/core/editing/SpellChecker.cpp:828: { On 2014/07/30 21:13:27, groby wrote: > You might want to test this for large documents. IIRC, whole-document checking > was definitely too slow. It might be that removal is fine, since it doesn't > involve any IPC, but it's worth trying out. Could you point out which piece of code was shown to be the bottleneck? https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1845: [] (const WebString &webString) { return static_cast<String>(webString); }); On 2014/07/30 21:13:27, groby wrote: > Is blink switched to C11? Chrome style guide says we are not there yet - I don't > think all of our compilers are ready yet. Whoops, my bad. I tried it and when it compiled I thought I was allowed to use lambda expressions. Sorry!
https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:420: DocumentMarkerAndNodeVector DocumentMarkerController::markerNodePairs() On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > On 2014/07/30 21:13:27, groby wrote: > > This seems to copy a lot of data, depending on the amount of markers. Why not > a > > removeMarker that takes a predicate instead? > > That seems like a great idea, I'll add a removeMarkers method with a predicate. Acknowledged. https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... Source/core/editing/SpellChecker.cpp:828: { On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > On 2014/07/30 21:13:27, groby wrote: > > You might want to test this for large documents. IIRC, whole-document checking > > was definitely too slow. It might be that removal is fine, since it doesn't > > involve any IPC, but it's worth trying out. > > Could you point out which piece of code was shown to be the bottleneck? The bottleneck for checking was the IPC traffic. You don't have that here, but you still traverse the entire document. I honestly have no idea how much impact that has - I'd try something like taking a giant text in a non-english language (Proust comes to mind :), pasting it into the text field, adding one word to the dictionary, and just checking if there's perceptible lag. I'm not super-worried - it'll likely be fine - but it's worth checking. https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1845: [] (const WebString &webString) { return static_cast<String>(webString); }); On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > On 2014/07/30 21:13:27, groby wrote: > > Is blink switched to C11? Chrome style guide says we are not there yet - I > don't > > think all of our compilers are ready yet. > > Whoops, my bad. I tried it and when it compiled I thought I was allowed to use > lambda expressions. Sorry! Note: I don't know about blink. I would _assume_ it's the same, but ask a blink-person if you're attached to the lambda :)
https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:420: DocumentMarkerAndNodeVector DocumentMarkerController::markerNodePairs() On 2014/08/01 00:13:22, groby wrote: > On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > > On 2014/07/30 21:13:27, groby wrote: > > > This seems to copy a lot of data, depending on the amount of markers. Why > not > > a > > > removeMarker that takes a predicate instead? > > > > That seems like a great idea, I'll add a removeMarkers method with a > predicate. > > Acknowledged. Done. https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... Source/core/editing/SpellChecker.cpp:828: { On 2014/08/01 00:13:22, groby wrote: > On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > > On 2014/07/30 21:13:27, groby wrote: > > > You might want to test this for large documents. IIRC, whole-document > checking > > > was definitely too slow. It might be that removal is fine, since it doesn't > > > involve any IPC, but it's worth trying out. > > > > Could you point out which piece of code was shown to be the bottleneck? > > The bottleneck for checking was the IPC traffic. You don't have that here, but > you still traverse the entire document. I honestly have no idea how much impact > that has - I'd try something like taking a giant text in a non-english language > (Proust comes to mind :), pasting it into the text field, adding one word to the > dictionary, and just checking if there's perceptible lag. > > I'm not super-worried - it'll likely be fine - but it's worth checking. Seems to work fast enough. :-) https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1845: [] (const WebString &webString) { return static_cast<String>(webString); }); On 2014/08/01 00:13:23, groby wrote: > On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > > On 2014/07/30 21:13:27, groby wrote: > > > Is blink switched to C11? Chrome style guide says we are not there yet - I > > don't > > > think all of our compilers are ready yet. > > > > Whoops, my bad. I tried it and when it compiled I thought I was allowed to use > > lambda expressions. Sorry! > > Note: I don't know about blink. I would _assume_ it's the same, but ask a > blink-person if you're attached to the lambda :) No worries, lambda expressions are nice but if they're not allowed in blink yet, I'll stop using them. :-)
On 2014/08/01 21:58:17, Klemen Forstnerič wrote: > https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... > File Source/core/dom/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/419563003/diff/100001/Source/core/dom/Documen... > Source/core/dom/DocumentMarkerController.cpp:420: DocumentMarkerAndNodeVector > DocumentMarkerController::markerNodePairs() > On 2014/08/01 00:13:22, groby wrote: > > On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > > > On 2014/07/30 21:13:27, groby wrote: > > > > This seems to copy a lot of data, depending on the amount of markers. Why > > not > > > a > > > > removeMarker that takes a predicate instead? > > > > > > That seems like a great idea, I'll add a removeMarkers method with a > > predicate. > > > > Acknowledged. > > Done. > > https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... > File Source/core/editing/SpellChecker.cpp (right): > > https://codereview.chromium.org/419563003/diff/100001/Source/core/editing/Spe... > Source/core/editing/SpellChecker.cpp:828: { > On 2014/08/01 00:13:22, groby wrote: > > On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > > > On 2014/07/30 21:13:27, groby wrote: > > > > You might want to test this for large documents. IIRC, whole-document > > checking > > > > was definitely too slow. It might be that removal is fine, since it > doesn't > > > > involve any IPC, but it's worth trying out. > > > > > > Could you point out which piece of code was shown to be the bottleneck? > > > > The bottleneck for checking was the IPC traffic. You don't have that here, but > > you still traverse the entire document. I honestly have no idea how much > impact > > that has - I'd try something like taking a giant text in a non-english > language > > (Proust comes to mind :), pasting it into the text field, adding one word to > the > > dictionary, and just checking if there's perceptible lag. > > > > I'm not super-worried - it'll likely be fine - but it's worth checking. > > Seems to work fast enough. :-) > > https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... > File Source/web/WebLocalFrameImpl.cpp (right): > > https://codereview.chromium.org/419563003/diff/100001/Source/web/WebLocalFram... > Source/web/WebLocalFrameImpl.cpp:1845: [] (const WebString &webString) { return > static_cast<String>(webString); }); > On 2014/08/01 00:13:23, groby wrote: > > On 2014/07/31 17:14:00, Klemen Forstnerič wrote: > > > On 2014/07/30 21:13:27, groby wrote: > > > > Is blink switched to C11? Chrome style guide says we are not there yet - I > > > don't > > > > think all of our compilers are ready yet. > > > > > > Whoops, my bad. I tried it and when it compiled I thought I was allowed to > use > > > lambda expressions. Sorry! > > > > Note: I don't know about blink. I would _assume_ it's the same, but ask a > > blink-person if you're attached to the lambda :) > > No worries, lambda expressions are nice but if they're not allowed in blink yet, > I'll stop using them. :-) dcheng: PTAL Patch Set 6.
I don't have time to take a more in-depth look at the moment, so I'll send out my initial thoughts. Most comments revolve around blink conventions (can't use auto, etc), but I'm also uncertain about the basic approach that MarkerRemoevrPredicate uses. I can't help but wonder if there's a better way than to construct a Range for each marker and extract the text from that for comparison purposes... https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:501: OwnPtrWillBeMember<MarkerList>& list = (*markers)[markerListIndex]; It looks a little weird to have a stack reference to a Oilpan transition type, but I guess this shows up elsewhere too. https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.h (right): https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:41: MarkerRemoverPredicate(Document* document, const Vector<String>& words) This can't ever be passed a non-null document right? Blink convention is to pass by reference in this case. https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:49: auto markerRange = Range::create(*m_document, auto is not allowed in cross-platform code. https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:50: const_cast<Node *>(node), documentMarker.startOffset(), Convention is to have the * and & by the typename, with no space. https://codereview.chromium.org/419563003/diff/140001/Source/core/editing/Spe... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/419563003/diff/140001/Source/core/editing/Spe... Source/core/editing/SpellChecker.cpp:827: void SpellChecker::removeSpellingMarkersUnderWords(const Vector<String> &words) & should be by type (in the declaration as well) https://codereview.chromium.org/419563003/diff/140001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/140001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1840: transformedWords[i] = static_cast<String>(words[i]); Why do you need the static cast? WebString should have a conversion operator to String in the Blink layer. https://codereview.chromium.org/419563003/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/419563003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3390: auto localFrame = static_cast<LocalFrame *>(frame); We can't use auto in Blink.
I'll take a look if Range::create can be replaced with something more efficient :-). https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.h (right): https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:41: MarkerRemoverPredicate(Document* document, const Vector<String>& words) On 2014/08/05 20:30:38, dcheng (OOO) wrote: > This can't ever be passed a non-null document right? Blink convention is to pass > by reference in this case. Done. https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:49: auto markerRange = Range::create(*m_document, On 2014/08/05 20:30:38, dcheng (OOO) wrote: > auto is not allowed in cross-platform code. Done. https://codereview.chromium.org/419563003/diff/140001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:50: const_cast<Node *>(node), documentMarker.startOffset(), On 2014/08/05 20:30:38, dcheng (OOO) wrote: > Convention is to have the * and & by the typename, with no space. Done. https://codereview.chromium.org/419563003/diff/140001/Source/core/editing/Spe... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/419563003/diff/140001/Source/core/editing/Spe... Source/core/editing/SpellChecker.cpp:827: void SpellChecker::removeSpellingMarkersUnderWords(const Vector<String> &words) On 2014/08/05 20:30:38, dcheng (OOO) wrote: > & should be by type (in the declaration as well) Done. https://codereview.chromium.org/419563003/diff/140001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/140001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1840: transformedWords[i] = static_cast<String>(words[i]); On 2014/08/05 20:30:39, dcheng (OOO) wrote: > Why do you need the static cast? WebString should have a conversion operator to > String in the Blink layer. Done. https://codereview.chromium.org/419563003/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/419563003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3390: auto localFrame = static_cast<LocalFrame *>(frame); On 2014/08/05 20:30:39, dcheng (OOO) wrote: > We can't use auto in Blink. Done.
yosin: PTAL Patch Set 7.
On 2014/08/08 20:23:20, Klemen Forstnerič wrote: > yosin: PTAL Patch Set 7. yosin: ping :-)
https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.h (right): https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:48: nit: Please remove an extra blank line. https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:49: RefPtr<Range> markerRange = Range::create(m_document, Please move implementation of this function to CPP file. See for explanation: http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:54: ? true : false; nit: We can use implicit |bool| casting, no need to use "?" operator. https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:59: Vector<String> m_words; Let's use WTF::HashSet<String> rather than WTF::Vector<String>. Is it better to pass around HashSet from WebFrameImpl? https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:118: Accidental insertion of a blank line? https://codereview.chromium.org/419563003/diff/180001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/180001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1838: Vector<String> transformedWords; nit: Since we know size of transformedWords, it is better to write: Vector<String> transformedWords(words.size()); transformedWords.resize(0);
yosin: PTAL Patch Set 8. https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.h (right): https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:48: On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > nit: Please remove an extra blank line. Done. https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:49: RefPtr<Range> markerRange = Range::create(m_document, On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > Please move implementation of this function to CPP file. > See for explanation: > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... Done. https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:54: ? true : false; On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > nit: We can use implicit |bool| casting, no need to use "?" operator. Done. https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:59: Vector<String> m_words; On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > Let's use WTF::HashSet<String> rather than WTF::Vector<String>. > Is it better to pass around HashSet from WebFrameImpl? m_words is likely to be very small, so even though Vector<String>::contains is a O(n) operation it should be very fast. Could you explain why a WTF::HashSet<String> would be a better container? https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:118: On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > Accidental insertion of a blank line? Whoops. Removed it. https://codereview.chromium.org/419563003/diff/180001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/180001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1838: Vector<String> transformedWords; On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > nit: Since we know size of transformedWords, it is better to write: > > Vector<String> transformedWords(words.size()); > transformedWords.resize(0); Thats a good idea and to better state my intentions I've replaced the for loop with a std::copy().
yosin: please also see my question inline. On 2014/08/11 16:24:49, Klemen Forstnerič wrote: > yosin: PTAL Patch Set 8. > > https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... > File Source/core/dom/DocumentMarkerController.h (right): > > https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... > Source/core/dom/DocumentMarkerController.h:48: > On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > > nit: Please remove an extra blank line. > > Done. > > https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... > Source/core/dom/DocumentMarkerController.h:49: RefPtr<Range> markerRange = > Range::create(m_document, > On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > > Please move implementation of this function to CPP file. > > See for explanation: > > > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... > > Done. > > https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... > Source/core/dom/DocumentMarkerController.h:54: ? true : false; > On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > > nit: We can use implicit |bool| casting, no need to use "?" operator. > > Done. > > https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... > Source/core/dom/DocumentMarkerController.h:59: Vector<String> m_words; > On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > > Let's use WTF::HashSet<String> rather than WTF::Vector<String>. > > Is it better to pass around HashSet from WebFrameImpl? > > m_words is likely to be very small, so even though Vector<String>::contains is a > O(n) operation it should be very fast. Could you explain why a > WTF::HashSet<String> would be a better container? > > https://codereview.chromium.org/419563003/diff/180001/Source/core/dom/Documen... > Source/core/dom/DocumentMarkerController.h:118: > On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > > Accidental insertion of a blank line? > > Whoops. Removed it. > > https://codereview.chromium.org/419563003/diff/180001/Source/web/WebLocalFram... > File Source/web/WebLocalFrameImpl.cpp (right): > > https://codereview.chromium.org/419563003/diff/180001/Source/web/WebLocalFram... > Source/web/WebLocalFrameImpl.cpp:1838: Vector<String> transformedWords; > On 2014/08/11 01:01:59, Yosi_UTC9 wrote: > > nit: Since we know size of transformedWords, it is better to write: > > > > Vector<String> transformedWords(words.size()); > > transformedWords.resize(0); > > Thats a good idea and to better state my intentions I've replaced the for loop > with a std::copy().
https://codereview.chromium.org/419563003/diff/200001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/200001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:51: RefPtr<Range> markerRange = Range::create(m_document, nit: s/RefPtr/RefPtrWIllBeRawPtr/ https://codereview.chromium.org/419563003/diff/200001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:51: RefPtr<Range> markerRange = Range::create(m_document, Let's avoid to use temporary Range object for Oilpan. In Oilpan, Range object isn't destructed when we return from this function. This means when we migrate Oilpan, DOM mutation is slower because of we need to iterate over live Range objects. Since, |node| is |Text|, we can use CharacterData::data(). So, let's make |operator()| to take |const Text*| instead of |const Node*|.
yosin: PTAL Patch Set 9. I noticed one thing about WTF::Vector: if you construct it by giving it a size and then immediately call resize(0), it's size becomes 0, therefore I removed the rezize(0). Oh and, could you please answer my previous question about WTF::HashSet vs WTF::Vector? Thanks! :-) https://codereview.chromium.org/419563003/diff/200001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/200001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:51: RefPtr<Range> markerRange = Range::create(m_document, On 2014/08/12 01:18:08, Yosi_UTC9 wrote: > Let's avoid to use temporary Range object for Oilpan. > In Oilpan, Range object isn't destructed when we return from this function. > This means when we migrate Oilpan, DOM mutation is slower because of we need to > iterate over live Range objects. > > Since, |node| is |Text|, we can use CharacterData::data(). > So, let's make |operator()| to take |const Text*| instead of |const Node*|. Done. https://codereview.chromium.org/419563003/diff/200001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:51: RefPtr<Range> markerRange = Range::create(m_document, On 2014/08/12 01:18:08, Yosi_UTC9 wrote: > nit: s/RefPtr/RefPtrWIllBeRawPtr/ I removed the Range altogether, so no need to this fix anymore :-)
LGTM +tkent@ for OWNERS review. https://codereview.chromium.org/419563003/diff/240001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.h (right): https://codereview.chromium.org/419563003/diff/240001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:51: Vector<String> m_words; > m_words is likely to be very small, so even though Vector<String>::contains is > O(n) operation it should be very fast. Could you explain why a > WTF::HashSet<String> would be a better container? My reasoning was |MarkRemovePredicate| is general term and when we use this for using other purpose other than custom dictionary update. However, it is too nervous. When we'll use |MarkerRemoverPredicate| with thousands of words, we'll change it to |HashSet| or another for efficient look up data structure.
I changed the |const Text*| to a |const Text&| in the method |MarkerRemoverPredicate::operator()|, since as mentioned previously by dcheng, blink convention is to pass by reference when the argument can't be nullptr.
https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:519: Vector<RenderedDocumentMarker *> markersToBeRemoved; |Vector<RenderedDocumentMarker *>| should be |WillBeHeapVector<RawPtrWillBeMember<RenderedDocumentMarker> >|. https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.h (right): https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:45: class MarkerRemoverPredicate { should have |FINAL| https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:47: MarkerRemoverPredicate(const Vector<String>& words); add |explicit| https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:48: bool operator()(const DocumentMarker& documentMarker, Remove argument names. They don't have additional information. https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:50: private: Add a blank line before |private:|. https://codereview.chromium.org/419563003/diff/260001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/260001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1838: WTF::Vector<String> convertedWords(words.size()); |WTF::Vector| should be |Vector|. https://codereview.chromium.org/419563003/diff/260001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1839: std::copy(words.data(), words.data() + words.size(), std::begin(convertedWords)); This works and is ok. However, convertedWords.append(words.data(), words.size()); is simpler. https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3384: WebLocalFrameImpl::fromFrame(localFrame); You don't need to wrap this code.
tkent: PTAL Patch Set 10. Could you also clarify what you meant with your comment in WebViewImpl.cpp:3384? Thanks! https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:519: Vector<RenderedDocumentMarker *> markersToBeRemoved; On 2014/08/14 01:17:14, tkent wrote: > |Vector<RenderedDocumentMarker *>| should be > |WillBeHeapVector<RawPtrWillBeMember<RenderedDocumentMarker> >|. Done. https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.h (right): https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:45: class MarkerRemoverPredicate { On 2014/08/14 01:17:14, tkent wrote: > should have |FINAL| Done. https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:47: MarkerRemoverPredicate(const Vector<String>& words); On 2014/08/14 01:17:14, tkent wrote: > add |explicit| Done. https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:48: bool operator()(const DocumentMarker& documentMarker, On 2014/08/14 01:17:14, tkent wrote: > Remove argument names. They don't have additional information. Done. https://codereview.chromium.org/419563003/diff/260001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.h:50: private: On 2014/08/14 01:17:14, tkent wrote: > Add a blank line before |private:|. Done. https://codereview.chromium.org/419563003/diff/260001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/419563003/diff/260001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1838: WTF::Vector<String> convertedWords(words.size()); On 2014/08/14 01:17:14, tkent wrote: > |WTF::Vector| should be |Vector|. Done. https://codereview.chromium.org/419563003/diff/260001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:1839: std::copy(words.data(), words.data() + words.size(), std::begin(convertedWords)); On 2014/08/14 01:17:15, tkent wrote: > This works and is ok. However, > convertedWords.append(words.data(), words.size()); > is simpler. You're right, it seems easier to understand. I've changed it. https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3384: WebLocalFrameImpl::fromFrame(localFrame); On 2014/08/14 01:17:15, tkent wrote: > You don't need to wrap this code. I don't understand this comment. Could you please clarify it?
https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3382: LocalFrame* localFrame = static_cast<LocalFrame*>(frame); Please use toLocalFrame() instead of static_cast<LocalFrame*>. https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3384: WebLocalFrameImpl::fromFrame(localFrame); On 2014/08/14 16:42:54, Klemen Forstnerič wrote: > On 2014/08/14 01:17:15, tkent wrote: > > You don't need to wrap this code. > > I don't understand this comment. Could you please clarify it? I think he means that you can just directly call the LocalFrame function rather than indirecting through WebLocalFrame.
On 2014/08/14 17:34:28, dcheng (OOO) wrote: > https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:3382: LocalFrame* localFrame = > static_cast<LocalFrame*>(frame); > Please use toLocalFrame() instead of static_cast<LocalFrame*>. > > https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:3384: WebLocalFrameImpl::fromFrame(localFrame); > On 2014/08/14 16:42:54, Klemen Forstnerič wrote: > > On 2014/08/14 01:17:15, tkent wrote: > > > You don't need to wrap this code. > > > > I don't understand this comment. Could you please clarify it? > > I think he means that you can just directly call the LocalFrame function rather > than indirecting through WebLocalFrame. Thanks for clarification dcheng! :-) I've made the change already. I'll upload the change as soon as I've compiled and tested it.
tkent: PTAL Patch Set 12.
The CQ bit was checked by tkent@chromium.org
lgtm https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/419563003/diff/260001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3384: WebLocalFrameImpl::fromFrame(localFrame); On 2014/08/14 16:42:54, Klemen Forstnerič wrote: > On 2014/08/14 01:17:15, tkent wrote: > > You don't need to wrap this code. > > I don't understand this comment. Could you please clarify it? I meant WebLocalFrameImpl* webLocalFrame = WebLocalFrameImpl::fromFrame(localFrame); can be WebLocalFrameImpl* webLocalFrame = WebLocalFrameImpl::fromFrame(localFrame); Blink has no 80 column limit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/klemen.forstneric@gmail.com/419563003/...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...) android_blink_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...) android_chromium_gn_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...) blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...) linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...) linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19893) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22843) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/41995) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47208)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19897) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42020)
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/klemen.forstneric@gmail.com/419563003/...
Message was sent while issue was closed.
Committed patchset #13 (320001) as 180382
Message was sent while issue was closed.
Klemen: Thank you very much for the patch! Could you please address the following comments in a follow up? https://codereview.chromium.org/419563003/diff/320001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/320001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:50: const Text& textNode) const { Please fix the indentation of parameters on line 49 and 50 to be consistent with the rest of the code in this file: bool class::method(const type& arg1, const type& arg2) { https://codereview.chromium.org/419563003/diff/320001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:526: list->remove(list->find(markersToBeRemoved[j].get())); Running list->find for every element in markersToBeRemoved may be slow (on the order of size of |list| times size of |markersToBeRemoved|). Instead of keeping track of makers to be removed, I recommend keeping track of markers to keep and then swapping that with |list|. That is: ------------------------------- WillBeHeapVector<RawPtrWillBeMember<RenderedDocumentMarker> > markersToKeep; for (size_t j = 0; list.get() && j < list->size(); ++j) { if (!i->key->isTextNode() || !shouldRemoveMarker(*list->at(j).get(), static_cast<const Text&>(*i->key))) markersToKeep.append(list->at(j).get()); } swap(*list, markersToKeep); ------------------------------- I hope that compiles ;-). https://codereview.chromium.org/419563003/diff/320001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/419563003/diff/320001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3431: convertedWords.append(words.data(), words.size()); Although the number of frames may be slow, it's best to avoid creating the same vector inside of a loop. (Note that frame->removeSpellingMarkersUnderWords() does not alter this vector.) Please move the vector creation outside of the loop.
Message was sent while issue was closed.
https://codereview.chromium.org/419563003/diff/320001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/320001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:50: const Text& textNode) const { On 2014/08/15 19:08:07, Rouslan Solomakhin wrote: > > Please fix the indentation of parameters on line 49 and 50 to be consistent with > the rest of the code in this file: > > bool class::method(const type& arg1, const type& arg2) { Done. https://codereview.chromium.org/419563003/diff/320001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:526: list->remove(list->find(markersToBeRemoved[j].get())); On 2014/08/15 19:08:07, Rouslan Solomakhin wrote: > > Running list->find for every element in markersToBeRemoved may be slow (on the > order of size of |list| times size of |markersToBeRemoved|). Instead of keeping > track of makers to be removed, I recommend keeping track of markers to keep and > then swapping that with |list|. That is: > > ------------------------------- > > WillBeHeapVector<RawPtrWillBeMember<RenderedDocumentMarker> > markersToKeep; > for (size_t j = 0; list.get() && j < list->size(); ++j) { > if (!i->key->isTextNode() || !shouldRemoveMarker(*list->at(j).get(), > static_cast<const Text&>(*i->key))) > markersToKeep.append(list->at(j).get()); > } > > swap(*list, markersToKeep); > > ------------------------------- > > I hope that compiles ;-). I was able to make the code compile by making markersToKeep of type MarkerList and then transfering ownership of the markers I wanted to keep to that MarkerList. Unfortunately though, the code crashes the tab process. This is the code I had: MarkerList markersToKeep; for (size_t j = 0; list.get() && j < list->size(); ++j) { if (!i->key->isTextNode() || !shouldRemoveMarker(*list->at(j).get(), static_cast<const Text&>(*i->key))) markersToKeep.append(list->at(j).release()); } swap(*list, markersToKeep); Thoughts? :-) https://codereview.chromium.org/419563003/diff/320001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/419563003/diff/320001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:3431: convertedWords.append(words.data(), words.size()); On 2014/08/15 19:08:07, Rouslan Solomakhin wrote: > > Although the number of frames may be slow, it's best to avoid creating the same > vector inside of a loop. (Note that frame->removeSpellingMarkersUnderWords() > does not alter this vector.) Please move the vector creation outside of the > loop. Done.
Message was sent while issue was closed.
https://codereview.chromium.org/419563003/diff/320001/Source/core/dom/Documen... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/419563003/diff/320001/Source/core/dom/Documen... Source/core/dom/DocumentMarkerController.cpp:526: list->remove(list->find(markersToBeRemoved[j].get())); On 2014/08/16 15:45:50, Klemen Forstnerič wrote: > On 2014/08/15 19:08:07, Rouslan Solomakhin wrote: > > > > Running list->find for every element in markersToBeRemoved may be slow (on the > > order of size of |list| times size of |markersToBeRemoved|). Instead of > keeping > > track of makers to be removed, I recommend keeping track of markers to keep > and > > then swapping that with |list|. That is: > > > > ------------------------------- > > > > WillBeHeapVector<RawPtrWillBeMember<RenderedDocumentMarker> > markersToKeep; > > for (size_t j = 0; list.get() && j < list->size(); ++j) { > > if (!i->key->isTextNode() || !shouldRemoveMarker(*list->at(j).get(), > > static_cast<const Text&>(*i->key))) > > markersToKeep.append(list->at(j).get()); > > } > > > > swap(*list, markersToKeep); > > > > ------------------------------- > > > > I hope that compiles ;-). > > I was able to make the code compile by making markersToKeep of type MarkerList > and then transfering ownership of the markers I wanted to keep to that > MarkerList. Unfortunately though, the code crashes the tab process. This is the > code I had: > > MarkerList markersToKeep; > for (size_t j = 0; list.get() && j < list->size(); ++j) { > if (!i->key->isTextNode() || !shouldRemoveMarker(*list->at(j).get(), > static_cast<const Text&>(*i->key))) > markersToKeep.append(list->at(j).release()); > } > > swap(*list, markersToKeep); > > Thoughts? :-) As discussed in IRC, you can discard my suggestion here, because the increased complexity of memory management may not be worth the performance improvement. If you're still interested in investigating the crash in your patch, then use gdb and/or address sanitizer (http://www.chromium.org/developers/testing/addresssanitizer). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
