| Summary: | [Chromium] Should implement EditorClientImpl::requestCheckingOfString() | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||||||||||||||||||||
| Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | eric, fabrizio.machado, fishd, hbono, levin, ojan, rniwa, tkent, tony | ||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Hajime Morrita
2010-12-13 22:05:24 PST
Created attachment 76526 [details]
Patch
CC levin@ because this has an WebKit API change. (In reply to comment #2) > CC levin@ because this has an WebKit API change. Actually that should be Darin Fisher (added). Comment on attachment 76526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76526&action=review Here's a few things that I noticed when I looked at this change quickly. > WebKit/chromium/public/WebTextCheckingResponse.h:40 > +// Note that type() is opaque from the client This looks like it should be a comment in the class (near the type() method). > WebKit/chromium/public/WebTextCheckingResponse.h:43 > +class WebTextCheckingResult { I thought we did one class per header in public and the file would be named the same as the class but Darin would know absolutely. (If so, this class should move into its own header file.) > WebKit/chromium/public/WebTextCheckingResponse.h:46 > + static WebTextCheckingResult makeBadGrammar(int location, int length); It isn't clear to me what these methods do. Perhaps that could be fixed by better methods names. (The method names don't make sense right now.) > WebKit/chromium/public/WebTextCheckingResponse.h:68 > + explicit WebTextCheckingResponse(int sequence, const WebVector<WebTextCheckingResult>& results) explicit shouldn't be here. > WebKit/chromium/public/WebViewClient.h:176 > + // Requests asyncronous spell and grammar checking. The result should be returned by misspelled: asyncronous > WebKit/chromium/src/EditorClientImpl.cpp:892 > + coreResults.append(SpellCheckingResult(static_cast<DocumentMarker::MarkerType>(results[i].type()), results[i].location(), results[0].length())); Shouldn't results[0].length() be results[i].length()? > WebKitTools/ChangeLog:47 > + There are a lot of things wrong here. I think you'll see them when you look at it. You may find it helpful if you review the diff of your changes before getting others to do it. > WebKitTools/DumpRenderTree/chromium/WebViewHost.h:213 > + // Spellcheck realted helper APIs spelling: realted Created attachment 76603 [details]
Patch
Hi Levin, I'm sorry for the bad CC ... and thank you for reviewing anyway! > Here's a few things that I noticed when I looked at this change quickly. > > > WebKit/chromium/public/WebTextCheckingResponse.h:40 > > +// Note that type() is opaque from the client Done. > > This looks like it should be a comment in the class (near the type() method). > > > WebKit/chromium/public/WebTextCheckingResponse.h:43 > > +class WebTextCheckingResult { > > I thought we did one class per header in public and the file would be named the same as the class but Darin would know absolutely. (If so, this class should move into its own header file.) Gave his own header and source files. > > > WebKit/chromium/public/WebTextCheckingResponse.h:46 > > + static WebTextCheckingResult makeBadGrammar(int location, int length); > > It isn't clear to me what these methods do. Perhaps that could be fixed by better methods names. (The method names don't make sense right now.) Tried another name. I'm not sure even the new name is sufficient though... > > > WebKit/chromium/public/WebTextCheckingResponse.h:68 > > + explicit WebTextCheckingResponse(int sequence, const WebVector<WebTextCheckingResult>& results) > > explicit shouldn't be here. Removed. > > > WebKit/chromium/public/WebViewClient.h:176 > > + // Requests asyncronous spell and grammar checking. The result should be returned by > > misspelled: asyncronous Done. > > > WebKit/chromium/src/EditorClientImpl.cpp:892 > > + coreResults.append(SpellCheckingResult(static_cast<DocumentMarker::MarkerType>(results[i].type()), results[i].location(), results[0].length())); > > Shouldn't results[0].length() be results[i].length()? Oops. Done. > > > WebKitTools/ChangeLog:47 > > + > > There are a lot of things wrong here. I think you'll see them when you look at it. You may find it helpful if you review the diff of your changes before getting others to do it. I totally missed about this file...Thank you for pointing this. Fixed. > > > WebKitTools/DumpRenderTree/chromium/WebViewHost.h:213 > > + // Spellcheck realted helper APIs > > spelling: realted Done. Created attachment 76627 [details]
rebased to ToT
Comment on attachment 76627 [details] rebased to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=76627&action=review > WebKit/chromium/public/WebViewClient.h:178 > + // Requests asynchronous spell and grammar checking. The result should be returned by > + // WebView::respondCheckingOfString() > + virtual void requestTextCheck(int sequence, const WebString&) { } nit: find-in-page APIs are somewhat similar in that there are methods on WebFrame and WebFrameClient that work asynchronously in unison. in that API, we use the term "identifier" instead of "sequence", so for consistency it would be better to do the same thing here. another idea though is to follow the model used by WebViewClient::runFileChooser. it defines a WebFileChooserCompletion interface that gets invoked by the embedder when the embedder wishes to push results back into WebKit. this model allows you to avoid the concept of "identifier" altogether. (it actually ends up pushing it up to the Chromium layer where we generate IPCs, but that isn't a bad thing.) so, going with that model, you'd end up with an API that might look like: WebViewClient::requestTextCheck(const WebString&, WebTextCheckCompletion*) then, class WebTextCheckCompletion { public: virtual void didCheckText(const WebVector<WebTextCheckResult>&) = 0; protected: virtual ~WebTextCheckCompletion() {} }; the WebTextCheckCompletion object would be deleted by the didCheckText method. one more comment: i find the term "text check" to be quite awkward. i think we can do better. maybe "languageCheck"? Comment on attachment 76627 [details] rebased to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=76627&action=review > WebKit/chromium/public/WebTextCheckingResult.h:43 > + static WebTextCheckingResult misspellingAt(int location, int length); > + static WebTextCheckingResult badGrammarAt(int location, int length); these static methods need the WEBKIT_API prefix so that they can be exported from WebKit when it is built as a DLL. please review: http://www.mail-archive.com/webkit-dev@lists.webkit.org/msg13648.html > WebKit/chromium/public/WebTextCheckingResult.h:46 > + int type() const { return m_type; } it is generally better to define an enum for things like this. then add code to AssertMatchingEnums.cpp to ensure that the API defined enums do not get out of sync with the WebCore defined ones. > WebKit/chromium/public/WebTextCheckingResult.h:47 > + int location() const { return m_location; } nit: i think it is more common to use the term "position" or "offset" in cases like this. > WebKit/chromium/public/WebViewClient.h:177 > + // WebView::respondCheckingOfString() nit: this comment is incorrect Created attachment 76733 [details]
Patch
Hi Darin, thank you for reviewing! I updated the patch. Could you take another look? > > WebKit/chromium/public/WebTextCheckingResult.h:43 > > + static WebTextCheckingResult misspellingAt(int location, int length); > > + static WebTextCheckingResult badGrammarAt(int location, int length); > > these static methods need the WEBKIT_API prefix so that they can be exported from WebKit when it is built as a DLL. Remove thsese, defining an enum instead. > > WebKit/chromium/public/WebTextCheckingResult.h:46 > > + int type() const { return m_type; } > > it is generally better to define an enum for things like this. then add code to > AssertMatchingEnums.cpp to ensure that the API defined enums do not get out of sync > with the WebCore defined ones. Defined an enum. I didn't noticed AssertMatchingEnums.cpp. Thank you for pointing this. > > > WebKit/chromium/public/WebTextCheckingResult.h:47 > > + int location() const { return m_location; } > > nit: i think it is more common to use the term "position" or "offset" in cases like this. Renamed to position() > > > WebKit/chromium/public/WebViewClient.h:177 > > + // WebView::respondCheckingOfString() > > nit: this comment is incorrect Fixed. Created attachment 77094 [details]
update to ToT
Created attachment 77967 [details]
Updated ToT.
Created attachment 78649 [details]
updated ToT
Created attachment 78656 [details]
addressed some feedback that is missed at the last patch
(In reply to comment #8) The last patch missed this feedback. I updated the patch to handle this. > (From update of attachment 76627 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76627&action=review > > > WebKit/chromium/public/WebViewClient.h:178 > > + // Requests asynchronous spell and grammar checking. The result should be returned by > > + // WebView::respondCheckingOfString() > > + virtual void requestTextCheck(int sequence, const WebString&) { } > > nit: find-in-page APIs are somewhat similar in that there are methods on WebFrame > and WebFrameClient that work asynchronously in unison. in that API, we use the > term "identifier" instead of "sequence", so for consistency it would be better to > do the same thing here. Renamed "sequence" to "identifier" > > another idea though is to follow the model used by WebViewClient::runFileChooser. > it defines a WebFileChooserCompletion interface that gets invoked by the embedder > when the embedder wishes to push results back into WebKit. this model allows you > to avoid the concept of "identifier" altogether. (it actually ends up pushing it > up to the Chromium layer where we generate IPCs, but that isn't a bad thing.) > > so, going with that model, you'd end up with an API that might look like: > > WebViewClient::requestTextCheck(const WebString&, WebTextCheckCompletion*) This makes sense. But in this case, async spellcheck will cross the process boundary and the pointer-as-an-identifier approach doesn't look better than the plain identifier. I'd like to keep current design. Created attachment 79153 [details]
Patch
Darin, I know you are busy, but would you able to take another look at this? ping? Created attachment 80175 [details]
updated to ToT
Darin? (In reply to comment #16) > > so, going with that model, you'd end up with an API that might look like: > > > > WebViewClient::requestTextCheck(const WebString&, WebTextCheckCompletion*) > This makes sense. But in this case, async spellcheck will cross the process boundary > and the pointer-as-an-identifier approach doesn't look better than the plain identifier. > I'd like to keep current design. Well, I think that our WebKit API should not have baked in assumptions about being used in a multi-process browser. It seems like you could just as easily put the map from identifier to WebTextCheckCompletion* outside in Chromium instead of within WebKit. Created attachment 81048 [details]
Patch
Hi Darin, thank you for your feedback!
I updated the patch to address it.
> Well, I think that our WebKit API should not have baked in assumptions about being used in a multi-process browser. It seems like you could just as easily put the map from identifier to WebTextCheckCompletion* outside in Chromium instead of within WebKit.
Sounds making sense. So I switched to take the completion callback pattern.
Comment on attachment 81048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81048&action=review > Source/WebKit/chromium/public/WebTextCheckCompletion.h:43 > +class WebTextCheckCompletion { nit: how about WebTextCheckingCompletion for consistency with the other interfaces, whose names begin with WebTextChecking? > Source/WebKit/chromium/public/WebTextCheckCompletion.h:45 > + virtual int identifier() const = 0; it seems like identifier should just be a parameter of requestCheckingOfText just as it is a parameter to requestCheckingOfString. or, can you tell me why you need to expose this identifier to Chromium? you could also put WebTextCheckingCompletion objects in a base::IDMap within Chromium, and use the ID vended from that for IPC purposes. It seems like the internal identifier used by the editing code could be replaced with an interface like WebTextCheckingCompletion instead. > Source/WebKit/chromium/public/WebTextCheckCompletion.h:46 > + virtual void didRespondTextCheck(const WebVector<WebTextCheckingResult>&) = 0; nit: how about didFinishCheckingText? > Source/WebKit/chromium/public/WebTextCheckingResult.h:43 > + Misspelling = 1 << 0, nit: how about a more specific enum name, like "Error"? then you can have: enum Error { ErrorSpelling = 1 << 0, ErrorGrammar = 1 << 1 }; And the field can be: Error error() const { return m_error; } > Source/WebKit/chromium/public/WebViewClient.h:182 > + // Requests asynchronous spell and grammar checking. The result should be returned by nit: "spelling and grammar checking" > Source/WebKit/chromium/public/WebViewClient.h:183 > + // WebView::respondTextCheck() i think this comment is now wrong. there is no respondTextCheck method. > Source/WebKit/chromium/public/WebViewClient.h:184 > + virtual void requestTextCheck(const WebString&, WebTextCheckCompletion*) { } requestTextCheck -> requestCheckingOfText perhaps? > Source/WebKit/chromium/src/EditorClientImpl.h:194 > + nit: no extra line here > Tools/DumpRenderTree/chromium/WebViewHost.cpp:414 > +static void invokeRespondLastTextCheck(void* context) please put static helper methods at the top of the file, so that you don't interrupt the flow of member function implementations. > Tools/DumpRenderTree/chromium/WebViewHost.cpp:436 > + m_spellcheck.spellCheckWord(WebString(text.characters(), text.length()), &misspelledPosition, &misspelledLength); I think you should be able to just pass a String here and the conversion to WebString will happen automatically. Created attachment 81182 [details]
Patch
Hi Darin, Thank you for reviewing again. I addressed the points. > > > Source/WebKit/chromium/public/WebTextCheckCompletion.h:43 > > +class WebTextCheckCompletion { > > nit: how about WebTextCheckingCompletion for consistency with the other interfaces, whose names begin with WebTextChecking? Sure. Renamed > > > Source/WebKit/chromium/public/WebTextCheckCompletion.h:45 > > + virtual int identifier() const = 0; > > it seems like identifier should just be a parameter of requestCheckingOfText just as it > is a parameter to requestCheckingOfString. > > or, can you tell me why you need to expose this identifier to Chromium? you could also > put WebTextCheckingCompletion objects in a base::IDMap within Chromium, and use the ID > vended from that for IPC purposes. It seems like the internal identifier used by the > editing code could be replaced with an interface like WebTextCheckingCompletion instead. Good point. I thought re-numbering ID is a bit redundant. But IDMap class helps. Thank you for pointing this. I didn't know that. Now removed identifier() method. > > > Source/WebKit/chromium/public/WebTextCheckCompletion.h:46 > > + virtual void didRespondTextCheck(const WebVector<WebTextCheckingResult>&) = 0; > > nit: how about didFinishCheckingText? > Renamed. > > Source/WebKit/chromium/public/WebTextCheckingResult.h:43 > > + Misspelling = 1 << 0, > > nit: how about a more specific enum name, like "Error"? > > then you can have: > > enum Error { > ErrorSpelling = 1 << 0, > ErrorGrammar = 1 << 1 > }; > > And the field can be: > > Error error() const { return m_error; } Done. > > > Source/WebKit/chromium/public/WebViewClient.h:182 > > + // Requests asynchronous spell and grammar checking. The result should be returned by > > nit: "spelling and grammar checking" Fixed. > > > Source/WebKit/chromium/public/WebViewClient.h:183 > > + // WebView::respondTextCheck() > > i think this comment is now wrong. there is no respondTextCheck method. Sure. removed. > > > Source/WebKit/chromium/public/WebViewClient.h:184 > > + virtual void requestTextCheck(const WebString&, WebTextCheckCompletion*) { } > > requestTextCheck -> requestCheckingOfText perhaps? Done. > > > Source/WebKit/chromium/src/EditorClientImpl.h:194 > > + > > nit: no extra line here Done. > > > Tools/DumpRenderTree/chromium/WebViewHost.cpp:414 > > +static void invokeRespondLastTextCheck(void* context) > > please put static helper methods at the top of the file, so that you don't > interrupt the flow of member function implementations. I'm sorry for that. Moved. > > > Tools/DumpRenderTree/chromium/WebViewHost.cpp:436 > > + m_spellcheck.spellCheckWord(WebString(text.characters(), text.length()), &misspelledPosition, &misspelledLength); > > I think you should be able to just pass a String here and the conversion to > WebString will happen automatically. I tried, but it didn't happen. It looks that WEKIT_IMPLEMENTATION is not defined in DumpRenderTree. > > I think you should be able to just pass a String here and the conversion to
> > WebString will happen automatically.
> I tried, but it didn't happen. It looks that WEKIT_IMPLEMENTATION is not defined in DumpRenderTree.
Ah, that's right! Of course it won't work then. Thanks for trying.
Comment on attachment 81182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81182&action=review > Source/WebKit/chromium/public/WebTextCheckingCompletion.h:35 > +#include "WebTextCheckingResult.h" nit: i think you can just forward declare WebTextCheckingResult and WebVector here instead. > Source/WebKit/chromium/public/WebTextCheckingResult.h:35 > +#include "WebVector.h" nit: no need for WebVector include here > Tools/DumpRenderTree/chromium/WebViewHost.cpp:232 > + WebViewHost* wvh = static_cast<WebViewHost*>(context); how do you know that the WebViewHost hasn't been deleted by the time this function is called? Created attachment 81444 [details]
Addressed the feedback
Darin, thanks for your review again. I addressed the points. > (From update of attachment 81182 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81182&action=review > > > Source/WebKit/chromium/public/WebTextCheckingCompletion.h:35 > > +#include "WebTextCheckingResult.h" > > nit: i think you can just forward declare WebTextCheckingResult and WebVector here instead. Sure. Fixed. > > > Source/WebKit/chromium/public/WebTextCheckingResult.h:35 > > +#include "WebVector.h" > > nit: no need for WebVector include here Done. > > > Tools/DumpRenderTree/chromium/WebViewHost.cpp:232 > > + WebViewHost* wvh = static_cast<WebViewHost*>(context); > > how do you know that the WebViewHost hasn't been deleted by the time this function is called? No. I found there are some other place use similar trick in WebViewHost. So I filed Bug 53899 to address them separately. My plan is to track live WebViewHost instances and verify passed one's liveness before using it. (In reply to comment #31) > > how do you know that the WebViewHost hasn't been deleted by the time this function is called? > No. > I found there are some other place use similar trick in WebViewHost. > So I filed Bug 53899 to address them separately. > My plan is to track live WebViewHost instances and verify passed one's liveness before using it. OK (In reply to comment #32) > (In reply to comment #31) > > > how do you know that the WebViewHost hasn't been deleted by the time this function is called? > > No. > > I found there are some other place use similar trick in WebViewHost. > > So I filed Bug 53899 to address them separately. > > My plan is to track live WebViewHost instances and verify passed one's liveness before using it. > > OK Thanks! will land shortly. Committed r77875: <http://trac.webkit.org/changeset/77875> |